fix(subgroups): land the coordinator in the transcript, not on a screen offering another ceremony
Pressing "Start the key ceremony" in the picker opened the ceremony and then navigated to the shared-key screen -- whose whole job is to *offer* a ceremony to a room that has none. So the coordinator arrived at a "Start key ceremony" button seconds after starting one, and pressing it opened a second. The second one is not a duplicate as far as the protocol is concerned, which is why nothing refused it. `proposeRitual`'s guard is scoped by purpose, deliberately: a room has to be able to hold the group's own ceremony and a subgroup's, since a subgroup whose admins are the whole group runs in the room the group's own ceremony ran in. A room holding a subgroup's ceremony will therefore take one for *no* subgroup without complaint -- and then "the room's newest ceremony" is the new empty one, and the subgroup's is buried under it. That is what happened to 2.0 and 2.1. **The coordinator goes to the transcript.** The ceremony is already open and its first line is already in that room; what they need is to watch it and answer the requests as they arrive, through the same ritual notices every other member uses -- which since `0b65d702` name their own ceremony and so open the right one. The shared-key screen has nothing to offer a room that already has a ceremony, and should not have been the destination. **And the screen stops offering one.** `canStartRitual` asked only about the ceremony being *shown*; it now also asks whether the room holds any that has not failed, which is a different question and the one that matters here. `DkgRitualUIState.roomHasCeremony` is observed separately for exactly that reason. **The route's `parentChatRoomId` goes.** Nothing passed it any more, and it was the shape of two of these bugs: only the member who opened a subgroup could ever fill it in, so a route that claimed to know the purpose was right for one caller and silently wrong for everybody else. The purpose is read off `DkgSession.parentChatRoomId` and nowhere else now, which makes that rule structural rather than conventional. One test in `RobustRoomKeyCeremonyTest` pinning the thing that is *not* a bug: a room holding a subgroup's ceremony still accepts one for another purpose, because two purposes are two ceremonies -- so the refusal has to live in what the UI offers, not in the guard. 397 common tests, 718 jvm tests, `m3Audit` meets every budget. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -132,11 +132,6 @@ import press.mantra.compose.ui.theme.ConformancePreviews
|
||||
fun DkgRitualScreen(
|
||||
activeUserPublicKey: HexKey,
|
||||
chatRoomId: String,
|
||||
/**
|
||||
* Set when this ceremony is making a subgroup, which grows the ladder by one
|
||||
* rung: the parent's birth certificate, between the key and the key state.
|
||||
*/
|
||||
parentChatRoomId: String? = null,
|
||||
/** Which of the room's ceremonies to show; null takes the newest. */
|
||||
dkgSessionId: String? = null,
|
||||
initialDkgRitualUIState: DkgRitualUIState = DkgRitualUIState.Loading,
|
||||
@@ -149,7 +144,6 @@ fun DkgRitualScreen(
|
||||
factory = DkgRitualViewModel.factory(
|
||||
chatRoomId = chatRoomId,
|
||||
activeUserPublicKey = activeUserPublicKey,
|
||||
parentChatRoomId = parentChatRoomId,
|
||||
dkgSessionId = dkgSessionId,
|
||||
initialDkgRitualUIState = initialDkgRitualUIState,
|
||||
activeWalletStateFlow = activeWalletStateFlow,
|
||||
|
||||
@@ -552,7 +552,6 @@ fun MantraNavHost(
|
||||
DkgRitualScreen(
|
||||
activeUserPublicKey = route.activeUserPublicKey,
|
||||
chatRoomId = route.chatRoomId,
|
||||
parentChatRoomId = route.parentChatRoomId,
|
||||
dkgSessionId = route.dkgSessionId,
|
||||
activeWalletStateFlow = sovereignWalletViewModel.activeWalletInUI,
|
||||
chatRepository = databaseChatRepository,
|
||||
|
||||
@@ -5,17 +5,17 @@ import kotlinx.serialization.Serializable
|
||||
/**
|
||||
* The shared-key ceremony for one NIP-17 group.
|
||||
*
|
||||
* [parentChatRoomId] is set when the ceremony is making a subgroup, and turns the
|
||||
* screen from a three-step ladder into a four-step one -- the extra rung being the
|
||||
* parent's birth certificate. It is a claim rather than evidence, in the route as
|
||||
* on the wire: what makes a lineage real is the certificate the parent's quorum
|
||||
* signs, two steps later. See `docs/subgroups.md`.
|
||||
* Deliberately says nothing about *what the ceremony is for*. A subgroup's
|
||||
* ceremony grows the ladder by a rung -- the parent's birth certificate -- and
|
||||
* that is read off `DkgSession.parentChatRoomId`, never off the route. Only the
|
||||
* member who opened a subgroup could ever have put it here, and every other way
|
||||
* into this screen is from a room; a route that knows more than the room does is
|
||||
* a route that is right for one caller and silently wrong for the rest.
|
||||
*/
|
||||
@Serializable
|
||||
data class DkgRitualRoute(
|
||||
val activeUserPublicKey: String,
|
||||
val chatRoomId: String,
|
||||
val parentChatRoomId: String? = null,
|
||||
/**
|
||||
* Which of the room's ceremonies to show, when the caller knows.
|
||||
*
|
||||
|
||||
@@ -50,11 +50,6 @@ import kotlinx.coroutines.launch
|
||||
class DkgRitualViewModel(
|
||||
val chatRoomId: String,
|
||||
val activeUserPublicKey: HexKey,
|
||||
/**
|
||||
* The group this ceremony is making a subgroup of, or null for an ordinary
|
||||
* one. Off the route, which got it off the proposal -- a claim either way.
|
||||
*/
|
||||
val parentChatRoomId: HexKey? = null,
|
||||
/**
|
||||
* The ceremony to show, when the caller knew which. Null falls back to the
|
||||
* room's newest -- see [DkgRitualRoute.dkgSessionId].
|
||||
@@ -113,6 +108,12 @@ class DkgRitualViewModel(
|
||||
|
||||
if (loaded.session != null && loaded.session.stage != DkgRitualStage.FAILED) return false
|
||||
|
||||
// Not just the ceremony on screen. A room can hold more than one, and this
|
||||
// screen exists to offer one to a room that has none -- so a room with a
|
||||
// ceremony running under a different purpose must not be offered another,
|
||||
// whatever this screen happens to be showing.
|
||||
if (loaded.roomHasCeremony) return false
|
||||
|
||||
return ChillDkgRitualManager.canRunRitual(loaded.localChatRoom)
|
||||
}
|
||||
|
||||
@@ -141,10 +142,7 @@ class DkgRitualViewModel(
|
||||
return@launch
|
||||
}
|
||||
|
||||
dkgRitualUIState = DkgRitualUIState.Loaded(
|
||||
localChatRoom = localChatRoom,
|
||||
parentChatRoomId = parentChatRoomId
|
||||
)
|
||||
dkgRitualUIState = DkgRitualUIState.Loaded(localChatRoom = localChatRoom)
|
||||
threshold.value = ChatRoomType
|
||||
.defaultQuorum(ChillDkgRitualManager.memberPublicKeys(localChatRoom).size)
|
||||
.coerceIn(quorumRange())
|
||||
@@ -171,17 +169,27 @@ class DkgRitualViewModel(
|
||||
?.let { dkgRepository.observeSessionById(it) }
|
||||
?: dkgRepository.observeLatestSessionForChatRoom(chatRoomId)
|
||||
|
||||
// Whether the room has any ceremony at all, which is a different
|
||||
// question from which one is being shown -- see
|
||||
// `DkgRitualUIState.roomHasCeremony`.
|
||||
launch {
|
||||
dkgRepository.observeLatestSessionForChatRoom(chatRoomId).collect { newest ->
|
||||
val current = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return@collect
|
||||
|
||||
dkgRitualUIState = current.copy(
|
||||
roomHasCeremony = newest != null && newest.stage != DkgRitualStage.FAILED
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
sessions.collect { session ->
|
||||
val loaded = (dkgRitualUIState as? DkgRitualUIState.Loaded)
|
||||
?: DkgRitualUIState.Loaded(
|
||||
localChatRoom = localChatRoom,
|
||||
parentChatRoomId = parentChatRoomId
|
||||
)
|
||||
?: DkgRitualUIState.Loaded(localChatRoom = localChatRoom)
|
||||
|
||||
// The session's own claim wins over the route's. They agree for the
|
||||
// coordinator; for everybody else the route has nothing and the
|
||||
// session is the only thing that knows.
|
||||
val parent = session?.parentChatRoomId ?: parentChatRoomId
|
||||
// The session is the only thing that knows what the ceremony is
|
||||
// for. The route used to carry it too, which was right for the one
|
||||
// member who opened the subgroup and null for everybody else.
|
||||
val parent = session?.parentChatRoomId
|
||||
|
||||
dkgRitualUIState = loaded.copy(
|
||||
session = session,
|
||||
@@ -661,7 +669,6 @@ class DkgRitualViewModel(
|
||||
fun factory(
|
||||
chatRoomId: String,
|
||||
activeUserPublicKey: HexKey,
|
||||
parentChatRoomId: HexKey? = null,
|
||||
dkgSessionId: String? = null,
|
||||
initialDkgRitualUIState: DkgRitualUIState = DkgRitualUIState.Loading,
|
||||
activeWalletStateFlow: StateFlow<ActiveWallet?>,
|
||||
@@ -672,7 +679,6 @@ class DkgRitualViewModel(
|
||||
DkgRitualViewModel(
|
||||
chatRoomId = chatRoomId,
|
||||
activeUserPublicKey = activeUserPublicKey,
|
||||
parentChatRoomId = parentChatRoomId,
|
||||
dkgSessionId = dkgSessionId,
|
||||
initialDkgRitualUIState = initialDkgRitualUIState,
|
||||
activeWalletStateFlow = activeWalletStateFlow,
|
||||
|
||||
@@ -24,7 +24,7 @@ import press.mantra.compose.managers.ChillDkgRitualManager
|
||||
import press.mantra.compose.managers.MarmotGroupCreation
|
||||
import press.mantra.compose.repository.ChatRepository
|
||||
import press.mantra.compose.repository.DkgRepository
|
||||
import press.mantra.compose.ui.composable.navigation.routes.DkgRitualRoute
|
||||
import press.mantra.compose.ui.composable.navigation.routes.ChatRoomMessagingRoute
|
||||
import press.mantra.compose.ui.composable.navigation.routes.Route
|
||||
import press.mantra.compose.ui.view.state.SelectSubgroupAdminsUIState
|
||||
|
||||
@@ -248,11 +248,24 @@ class SelectSubgroupAdminsViewModel(
|
||||
}
|
||||
|
||||
withContext(Dispatchers.Main) {
|
||||
// The transcript, not the shared-key screen. The ceremony is
|
||||
// already open and its first line is already in this room, so what
|
||||
// the coordinator needs is to watch it and answer the requests as
|
||||
// they arrive -- which is the same path every other member takes,
|
||||
// through the ritual notices that name their own ceremony.
|
||||
//
|
||||
// Sending them to the shared-key screen instead was actively
|
||||
// harmful: that screen's job is to *offer* a ceremony to a room
|
||||
// that has none, so the coordinator arrived at a "Start key
|
||||
// ceremony" button seconds after starting one, and pressing it
|
||||
// opened a second -- for no subgroup, so `proposeRitual`'s guard
|
||||
// did not recognise it as a duplicate -- which then buried the
|
||||
// subgroup's under the room's newest.
|
||||
onNavigateToRoute(
|
||||
DkgRitualRoute(
|
||||
ChatRoomMessagingRoute(
|
||||
activeUserPublicKey = activeUserPublicKey,
|
||||
chatRoomId = ceremonyRoom.chatRoom.id,
|
||||
parentChatRoomId = parentChatRoomId
|
||||
relayHint = null
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
@@ -63,6 +63,18 @@ sealed interface DkgRitualUIState {
|
||||
* signed first. It is what gates creating the room.
|
||||
*/
|
||||
val isKeyStateSigned: Boolean = false,
|
||||
/**
|
||||
* Whether this room already holds a ceremony that has not failed --
|
||||
* whichever one [session] happens to be showing.
|
||||
*
|
||||
* Two different questions, and conflating them opened a second ceremony
|
||||
* in a room that already had one. [session] is the ceremony being looked
|
||||
* at; this is whether the room has any. `proposeRitual`'s guard is scoped
|
||||
* by purpose, so a room holding a subgroup's ceremony would happily take
|
||||
* a second one for no subgroup -- and then "the room's newest ceremony"
|
||||
* is the new empty one and the subgroup's is buried under it.
|
||||
*/
|
||||
val roomHasCeremony: Boolean = false,
|
||||
/**
|
||||
* The group this ceremony is making a subgroup of, or null for an
|
||||
* ordinary one.
|
||||
|
||||
@@ -421,4 +421,38 @@ class RobustRoomKeyCeremonyTest {
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The second ceremony a coordinator was able to open seconds after the first.
|
||||
*
|
||||
* `proposeRitual`'s guard is scoped by purpose, deliberately -- a room has to
|
||||
* be able to hold the group's own ceremony and a subgroup's. But that means a
|
||||
* room with a subgroup's ceremony in it will take a second one for *no*
|
||||
* subgroup without complaint, and then "the room's newest ceremony" is the new
|
||||
* empty one and the subgroup's is buried under it.
|
||||
*
|
||||
* Nothing on the wire stops that and nothing should: two purposes are two
|
||||
* ceremonies. What stops it is not offering it -- the shared-key screen exists
|
||||
* to give a ceremony to a room that has none, and this room has one.
|
||||
*/
|
||||
@Test
|
||||
fun `a room holding a ceremony will still take one for another purpose`() = runBlocking {
|
||||
val room = robustRoom()
|
||||
|
||||
val subgroup = openCeremony(room, parentChatRoomId = "ab".repeat(32))
|
||||
val own = openCeremony(room)
|
||||
|
||||
// Both exist, which is correct at this level: the guard is about purpose.
|
||||
assertNotEquals(subgroup.id, own.id)
|
||||
|
||||
// So the refusal has to live in what the screen offers, which is why
|
||||
// `DkgRitualUIState.roomHasCeremony` is asked separately from which
|
||||
// ceremony is being shown.
|
||||
assertEquals(
|
||||
2,
|
||||
db.dkgSessionDao().getByParentChatRoomId("ab".repeat(32)).size +
|
||||
(if (db.dkgSessionDao().getLatestSessionFor(room.chatRoom.id, null) != null) 1 else 0),
|
||||
"a room can end up holding two ceremonies, so the UI must not offer a third",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user