From 4e6ca0a08032b0405fea7ca87a54420557476a09 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Fri, 2 Oct 2026 19:45:28 +0200 Subject: [PATCH] fix(dkg): stop the key ceremony screen offering to start the ceremony it is showing "Start key ceremony" is not a button, it is the whole no-ceremony screen -- the explainer, the quorum picker and the FAB together -- so anything that leaves `DkgRitualUIState.Loaded` saying there is no session puts that offer in front of a member watching one run. Two unrelated things did, and they fail differently: one flashes, one sticks. **The state went up before the rows were read.** `initiate()` published a bare `Loaded(localChatRoom)` -- session null, `roomHasCeremony` false -- and *then* started the two flows that answer both questions. A cold open is the only time those queries are slow, and it is exactly when a member arrives from the transcript's notice, so the offer was drawn over a running ceremony for as long as they took. Both answers are a row lookup, so they are now read with `first()` before anything is published and the state goes up already correct. The flows still run; they only keep it current. **Seven flows were folding into one `var ... by mutableStateOf`.** The ceremony, the room's other ceremonies, the ceremony's messages, the key state, whether the key state is signed, the certificate and the certificate's signing -- each of them a read, a `copy` and an assignment, every one emitting on Room's own executor. Two emissions arriving together silently dropped one: whichever read first wrote last, over the top of the other's change. The field that went missing most visibly was `session`, copied back to null by a watcher that had read the state before the ceremony landed in it. That is not a flash. The session flow only re-emits when the `DkgSession` table changes, and a ceremony waiting on a member who has not opened the app changes nothing, so the screen sat there offering to start the ceremony it was showing. `MutableStateFlow` with every write through `update { }` -- a compare-and-set that retries on a loss, so a change can only ever be folded into the state that is current. The one rule it imposes is that the block re-runs, so it must be pure: `pendingApproval` is a suspending database lookup that used to sit *inside* the `copy`, and it now happens above the call. `updateLoaded` carries the `as? Loaded ?: return` that all twenty call sites used to write by hand. **Two knock-ons.** `canStartRitual()` and `quorumRange()` were read from the composable and are now passed the state, because reading a flow's `.value` inside composition observes nothing -- the offer would have been decided once and never revised. Passing the rendered state is also more correct while `ScreenStateTransition` still holds the outgoing one. `DkgRitualScreen` and `DkgApprovalScaffold` collect with `collectAsState()`. **The button itself is right and stays.** Creating a robust group already opens the ceremony -- `SelectChatRoomTypeViewModel.createNip17ChatRoom` calls `proposeRitual` immediately after `createNip17ChatRoom` -- and this screen's offer is the deliberate fallback for the case where that failed (`keyCeremonyNotStarted`) and for rooms created before it existed. What was broken is when it appeared, not that it exists. **Tests.** `DkgRitualStartOfferJvmTest`, two cases, and each was checked against its own regression reinstated rather than only against the fix. The first asserts the *first* `Loaded` already knows the room is running a ceremony, with a deliberately slow query so the window is proved gone rather than hidden. The second forces the lost update to happen rather than racing into it: the session watcher is parked inside `pendingApproval` while the key-state watcher writes, and the write that follows must not carry the stale copy over it. **Not done.** `HomeViewModel`, `ChatMessageListViewModel`, `ProposalListViewModel` and others still declare `by mutableStateOf` in a view model constructor. They are only at risk where more than one flow folds into one state object; none was audited here. 964 jvm tests, 454 common tests, `m3Audit` meeting every budget. Co-Authored-By: Claude Opus 5 --- .../ui/composable/DkgApprovalScaffold.kt | 3 +- .../compose/ui/composable/DkgRitualScreen.kt | 7 +- .../ui/view/model/DkgRitualViewModel.kt | 271 +++++++++++------- .../view/model/DkgRitualStartOfferJvmTest.kt | 247 ++++++++++++++++ 4 files changed, 428 insertions(+), 100 deletions(-) create mode 100644 composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/DkgRitualStartOfferJvmTest.kt diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgApprovalScaffold.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgApprovalScaffold.kt index cc358aa5..d7513e87 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgApprovalScaffold.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgApprovalScaffold.kt @@ -22,6 +22,7 @@ import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable +import androidx.compose.runtime.collectAsState import androidx.compose.runtime.LaunchedEffect import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -100,7 +101,7 @@ internal fun DkgApprovalScaffold( ) } ) { padding -> - when (val state = dkgRitualViewModel.dkgRitualUIState) { + when (val state = dkgRitualViewModel.dkgRitualUIState.collectAsState().value) { is DkgRitualUIState.Loading -> Column( modifier = Modifier.fillMaxWidth().padding(padding).readableContent().padding(MaterialTheme.spacing.space250), horizontalAlignment = Alignment.CenterHorizontally diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgRitualScreen.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgRitualScreen.kt index f669285c..3ea26e01 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgRitualScreen.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/DkgRitualScreen.kt @@ -37,6 +37,7 @@ import androidx.compose.material3.Surface import androidx.compose.material3.Text import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable +import androidx.compose.runtime.collectAsState import androidx.compose.runtime.LaunchedEffect import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -154,7 +155,7 @@ fun DkgRitualScreen( ) ) - ScreenStateTransition(dkgRitualViewModel.dkgRitualUIState) { uiState -> + ScreenStateTransition(dkgRitualViewModel.dkgRitualUIState.collectAsState().value) { uiState -> when (val dkgRitualUIState = uiState) { is DkgRitualUIState.Error -> { Column( @@ -168,7 +169,7 @@ fun DkgRitualScreen( is DkgRitualUIState.Loaded -> { val session = dkgRitualUIState.session val participantCount = dkgRitualUIState.participantCount - val canStartRitual = dkgRitualViewModel.canStartRitual() + val canStartRitual = dkgRitualViewModel.canStartRitual(dkgRitualUIState) val isActionPending = dkgRitualViewModel.isActionPending.value Scaffold( @@ -286,7 +287,7 @@ fun DkgRitualScreen( QuorumStepper( threshold = dkgRitualViewModel.threshold.value, participantCount = participantCount, - quorumRange = dkgRitualViewModel.quorumRange(), + quorumRange = dkgRitualViewModel.quorumRange(dkgRitualUIState), onThresholdChange = dkgRitualViewModel::setThreshold ) } else { diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt index 76d61b2b..feb4316f 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/DkgRitualViewModel.kt @@ -1,9 +1,7 @@ package press.mantra.compose.ui.view.model import androidx.compose.runtime.MutableState -import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.setValue import androidx.lifecycle.ViewModel import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.viewModelScope @@ -36,7 +34,10 @@ import press.mantra.compose.identity.Identity import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.IO import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch /** @@ -60,8 +61,51 @@ class DkgRitualViewModel( val dkgRepository: DkgRepository, ): ViewModel() { - var dkgRitualUIState: DkgRitualUIState by mutableStateOf(initialDkgRitualUIState) - private set + private val _dkgRitualUIState = MutableStateFlow(initialDkgRitualUIState) + + /** + * What the screen draws -- a flow rather than a `mutableStateOf`, because + * seven flows write it. + * + * The ceremony, the room's other ceremonies, the ceremony's messages, the + * key state, whether the key state is signed, the certificate and the + * certificate's signing all fold into this one object, and every one of + * them emits on Room's own executor. Held + * as a `var ... by mutableStateOf`, each of those was a read, a `copy` and + * an assignment with a suspension point in the middle, so two emissions + * arriving together silently dropped one: whichever read first wrote last, + * over the top of the other's change. + * + * The change that went missing most visibly was [DkgRitualUIState.Loaded.session], + * copied back to null by a watcher that had read the state before the + * ceremony landed in it -- and a screen with no session is the screen that + * offers to start one. It stayed that way until some other table changed and + * the session flow emitted again, which for a ceremony waiting on an absent + * member is never. + * + * [MutableStateFlow.update] retries on a losing compare-and-set, so a change + * can only be folded into the state that is actually current. Every write + * below goes through it -- see [updateLoaded]. It is the same rule the + * wallet view models already keep, for the same reason. + */ + val dkgRitualUIState: StateFlow = _dkgRitualUIState.asStateFlow() + + /** + * Folds a change into the loaded state, atomically, and does nothing when + * the screen is not in it -- the `as? Loaded ?: return` that every one of + * these call sites used to write by hand. + * + * [block] is re-run when it loses the race, so it has to be a pure function + * of the state it is handed: read nothing else out of the view model inside + * it, and do the suspending work above the call. + */ + private inline fun updateLoaded( + block: (DkgRitualUIState.Loaded) -> DkgRitualUIState.Loaded + ) { + _dkgRitualUIState.update { current -> + if (current is DkgRitualUIState.Loaded) block(current) else current + } + } private val logger = Logger.withTag(TAG) @@ -101,9 +145,15 @@ class DkgRitualViewModel( * Any member may: ChillDKG gives the coordinator no say in the outcome, and a * NIP-17 room records no creator to reserve the job for. What rules it out is * a ceremony already running, or a group too small to split a key across. + * + * [state] is passed in by the screen rather than read off the view model, + * because the state is a flow now and reading `.value` inside composition + * observes nothing -- the offer would be decided once and never revised. The + * screen hands in the state it is drawing, which is also the only state the + * answer should be about while a transition still holds the outgoing one. */ - fun canStartRitual(): Boolean { - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return false + fun canStartRitual(state: DkgRitualUIState = dkgRitualUIState.value): Boolean { + val loaded = state as? DkgRitualUIState.Loaded ?: return false if (loaded.session != null && loaded.session.stage != DkgRitualStage.FAILED) return false @@ -116,8 +166,8 @@ class DkgRitualViewModel( return ChillDkgRitualManager.canRunRitual(loaded.localChatRoom) } - fun quorumRange(): IntRange { - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded + fun quorumRange(state: DkgRitualUIState = dkgRitualUIState.value): IntRange { + val loaded = state as? DkgRitualUIState.Loaded ?: return ChatRoomType.quorumRange(ChatRoomType.MINIMUM_QUORUM) // Below the minimum the range runs backwards and is empty, which `coerceIn` @@ -137,17 +187,10 @@ class DkgRitualViewModel( viewModelScope.launch(Dispatchers.IO) { val localChatRoom = chatRepository.getChatRoomByIdentifier(chatRoomId) if (localChatRoom == null) { - dkgRitualUIState = DkgRitualUIState.Error("Couldn't load this chat") + _dkgRitualUIState.value = DkgRitualUIState.Error("Couldn't load this chat") return@launch } - dkgRitualUIState = DkgRitualUIState.Loaded(localChatRoom = localChatRoom) - threshold.value = ChatRoomType - .defaultQuorum(ChillDkgRitualManager.memberPublicKeys(localChatRoom).size) - .coerceIn(quorumRange()) - - observeKeyState() - // Room-scoped, deliberately. Only the member who opened a subgroup // arrives here knowing it is one -- everybody else reaches this screen // from the room, through the transcript's ritual notice or the group's @@ -171,30 +214,65 @@ class DkgRitualViewModel( // 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 + val roomSessions = dkgRepository.observeLatestSessionForChatRoom(chatRoomId) - dkgRitualUIState = current.copy( - roomHasCeremony = newest != null && newest.stage != DkgRitualStage.FAILED - ) + // Both answers are read here, before anything is published, rather + // than awaited after a bare `Loaded` has gone up. A `Loaded` with no + // session and no ceremony *is* the screen that offers to start one, + // so publishing it first flashed "start key ceremony" over a ceremony + // already running -- for as long as the two queries took, which is + // long enough to read and long enough to tap. The flows below keep + // both current from here on; this only settles what the first frame + // says. + val shownSession = sessions.first() + val newestSession = roomSessions.first() + + _dkgRitualUIState.value = DkgRitualUIState.Loaded( + localChatRoom = localChatRoom, + session = shownSession, + parentChatRoomId = shownSession?.parentChatRoomId, + pendingApproval = shownSession?.let { dkgRepository.pendingApproval(it) }, + roomHasCeremony = newestSession != null && + newestSession.stage != DkgRitualStage.FAILED + ) + + threshold.value = ChatRoomType + .defaultQuorum(ChillDkgRitualManager.memberPublicKeys(localChatRoom).size) + .coerceIn(quorumRange()) + + observeKeyState() + + launch { + roomSessions.collect { newest -> + updateLoaded { current -> + current.copy( + roomHasCeremony = newest != null && + newest.stage != DkgRitualStage.FAILED + ) + } } } sessions.collect { session -> - val loaded = (dkgRitualUIState as? DkgRitualUIState.Loaded) - ?: DkgRitualUIState.Loaded(localChatRoom = localChatRoom) - // 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, - parentChatRoomId = parent, - pendingApproval = session?.let { dkgRepository.pendingApproval(it) } - ) + // Above the update, not inside it: `update` re-runs its block when + // it loses the race, and a block that re-runs cannot suspend. + val pending = session?.let { dkgRepository.pendingApproval(it) } + + _dkgRitualUIState.update { current -> + val loaded = current as? DkgRitualUIState.Loaded + ?: DkgRitualUIState.Loaded(localChatRoom = localChatRoom) + + loaded.copy( + session = session, + parentChatRoomId = parent, + pendingApproval = pending + ) + } // Re-point the per-ceremony watchers whenever the ceremony or its // purpose changes, which for a member arriving cold is once. @@ -212,23 +290,32 @@ class DkgRitualViewModel( if (session != null) { messageObserver = viewModelScope.launch(Dispatchers.IO) { dkgRepository.observeMessages(session.id).collect { messages -> - val current = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return@collect - fun senders(kind: Kind) = messages .filter { it.kind == kind } .map { it.participantPublicKey } .toSet() - dkgRitualUIState = current.copy( - hostKeyParticipants = senders(DkgRitualEvents.HOST_KEY), - round1Participants = senders(DkgRitualEvents.ROUND_1), - round2Participants = senders(DkgRitualEvents.ROUND_2), - // Recomputed here as well as on the session, because - // whether round 1 can be asked for depends on how many - // host keys have arrived, which only this flow sees. - pendingApproval = current.session - ?.let { dkgRepository.pendingApproval(it) } - ) + // Recomputed here as well as on the session, because + // whether round 1 can be asked for depends on how many + // host keys have arrived, which only this flow sees. + val pending = dkgRepository.pendingApproval(session) + + updateLoaded { current -> + // This watcher belongs to one ceremony, and + // cancelling it does not reach into a body already + // running -- so say which ceremony these counts are + // for rather than letting them land on the next. + if (current.session?.id != session.id) { + return@updateLoaded current + } + + current.copy( + hostKeyParticipants = senders(DkgRitualEvents.HOST_KEY), + round1Participants = senders(DkgRitualEvents.ROUND_1), + round2Participants = senders(DkgRitualEvents.ROUND_2), + pendingApproval = pending + ) + } } } } @@ -257,17 +344,15 @@ class DkgRitualViewModel( // before there is a room -- which is the whole point of asking it. launch { dkgRepository.observeSigningSessions(chatRoomId).collect { sessions -> - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return@collect + val keyState = sessions.firstOrNull { local -> + local.items.any { item -> + runCatching { + Event.fromJson(item.unsignedEventJson).kind + }.getOrNull() == GroupKeyStateEvent.KIND + } + }?.session - dkgRitualUIState = loaded.copy( - keyStateSession = sessions.firstOrNull { local -> - local.items.any { item -> - runCatching { - Event.fromJson(item.unsignedEventJson).kind - }.getOrNull() == GroupKeyStateEvent.KIND - } - }?.session - ) + updateLoaded { it.copy(keyStateSession = keyState) } } } @@ -293,9 +378,9 @@ class DkgRitualViewModel( certificateObserver?.cancel() if (dkgSessionId == null) { - dkgRitualUIState = (dkgRitualUIState as? DkgRitualUIState.Loaded) - ?.copy(isKeyStateSigned = false, isCertified = false, certificateSession = null) - ?: dkgRitualUIState + updateLoaded { + it.copy(isKeyStateSigned = false, isCertified = false, certificateSession = null) + } return } @@ -306,41 +391,33 @@ class DkgRitualViewModel( // about this one's key. launch { dkgRepository.observeSignedGroupKeyState(dkgSessionId).collect { state -> - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return@collect - - dkgRitualUIState = loaded.copy(isKeyStateSigned = state != null) + updateLoaded { it.copy(isKeyStateSigned = state != null) } } } if (parentChatRoomId == null) { - dkgRitualUIState = (dkgRitualUIState as? DkgRitualUIState.Loaded) - ?.copy(isCertified = false, certificateSession = null) - ?: dkgRitualUIState + updateLoaded { it.copy(isCertified = false, certificateSession = null) } return@launch } launch { dkgRepository.observeBirthCertificate(dkgSessionId, parentChatRoomId) .collect { certificate -> - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return@collect - - dkgRitualUIState = loaded.copy(isCertified = certificate != null) + updateLoaded { it.copy(isCertified = certificate != null) } } } launch { dkgRepository.observeSigningSessions(parentChatRoomId).collect { sessions -> - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return@collect + val certificate = sessions.firstOrNull { local -> + local.items.any { item -> + runCatching { + Event.fromJson(item.unsignedEventJson).kind + }.getOrNull() == SubgroupBirthCertificateEvent.KIND + } + }?.session - dkgRitualUIState = loaded.copy( - certificateSession = sessions.firstOrNull { local -> - local.items.any { item -> - runCatching { - Event.fromJson(item.unsignedEventJson).kind - }.getOrNull() == SubgroupBirthCertificateEvent.KIND - } - }?.session - ) + updateLoaded { it.copy(certificateSession = certificate) } } } } @@ -359,7 +436,7 @@ class DkgRitualViewModel( fun proposeKeyState() { if (isActionPending.value) return - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return + val loaded = dkgRitualUIState.value as? DkgRitualUIState.Loaded ?: return val session = loaded.session ?: return if (session.thresholdPublicKey == null) return @@ -398,7 +475,7 @@ class DkgRitualViewModel( if (signing == null) { logger.e("Failed to propose the key state for $chatRoomId") - dkgRitualUIState = DkgRitualUIState.Error( + _dkgRitualUIState.value = DkgRitualUIState.Error( "Couldn't ask the group to agree its signing key. Please try again." ) } @@ -421,7 +498,7 @@ class DkgRitualViewModel( fun proposeBirthCertificate() { if (isActionPending.value) return - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return + val loaded = dkgRitualUIState.value as? DkgRitualUIState.Loaded ?: return // The state's, not the route's. Every step after the ceremony is open to // any of the subgroup's admins -- see `SubgroupManager` -- and they reach // this screen from the room, with no route to have told them anything. @@ -459,7 +536,7 @@ class DkgRitualViewModel( if (signing == null) { logger.e("Failed to propose a birth certificate in $parent") - dkgRitualUIState = DkgRitualUIState.Error( + _dkgRitualUIState.value = DkgRitualUIState.Error( "Couldn't ask the group to certify this subgroup. Please try again." ) } @@ -470,11 +547,11 @@ class DkgRitualViewModel( fun startRitual() { if (isActionPending.value) return - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return + val loaded = dkgRitualUIState.value as? DkgRitualUIState.Loaded ?: return val nostrPrivateKey = activeIdentityStateFlow.value?.nostrPrivateKey if (nostrPrivateKey == null) { - dkgRitualUIState = DkgRitualUIState.Error("Couldn't read your keys. Please try again.") + _dkgRitualUIState.value = DkgRitualUIState.Error("Couldn't read your keys. Please try again.") return } @@ -492,7 +569,7 @@ class DkgRitualViewModel( if (session == null) { logger.e("Failed to start DKG ritual for $chatRoomId") - dkgRitualUIState = DkgRitualUIState.Error("Couldn't start the key ceremony. Please try again.") + _dkgRitualUIState.value = DkgRitualUIState.Error("Couldn't start the key ceremony. Please try again.") } // On success the session flow above delivers the new row; no need to // set it here and risk racing the observer. @@ -509,12 +586,12 @@ class DkgRitualViewModel( fun approve(step: DkgApprovalStep) { if (isActionPending.value) return - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return + val loaded = dkgRitualUIState.value as? DkgRitualUIState.Loaded ?: return val session = loaded.session ?: return val nostrPrivateKey = activeIdentityStateFlow.value?.nostrPrivateKey if (nostrPrivateKey == null) { - dkgRitualUIState = DkgRitualUIState.Error("Couldn't read your keys. Please try again.") + _dkgRitualUIState.value = DkgRitualUIState.Error("Couldn't read your keys. Please try again.") return } @@ -558,7 +635,7 @@ class DkgRitualViewModel( fun createAdminGroup(onNavigateToRoute: (Route) -> Unit) { if (isActionPending.value) return - val loaded = dkgRitualUIState as? DkgRitualUIState.Loaded ?: return + val loaded = dkgRitualUIState.value as? DkgRitualUIState.Loaded ?: return val session = loaded.session ?: return val thresholdPublicKey = session.thresholdPublicKey ?: return @@ -568,7 +645,7 @@ class DkgRitualViewModel( val nostrPrivateKey = activeIdentityStateFlow.value?.nostrPrivateKey if (nostrPrivateKey == null) { - dkgRitualUIState = DkgRitualUIState.Error("Couldn't read your keys. Please try again.") + _dkgRitualUIState.value = DkgRitualUIState.Error("Couldn't read your keys. Please try again.") return } @@ -594,7 +671,7 @@ class DkgRitualViewModel( ?: if (subgroupParent != null) "Subgroup" else "Group" isActionPending.value = true - dkgRitualUIState = loaded.copy(adminGroupBlockedOn = emptyList()) + updateLoaded { it.copy(adminGroupBlockedOn = emptyList()) } viewModelScope.launch(Dispatchers.IO) { val outcome = chatRepository.createMarmotGroup( @@ -617,19 +694,21 @@ class DkgRitualViewModel( // Named rather than counted. A member cannot publish a key // package on somebody else's behalf, so the only useful thing // to say is whose door to knock on. - dkgRitualUIState = loaded.copy( - adminGroupBlockedOn = outcome.missing.map { publicKey -> - loaded.ritualMembers - .firstOrNull { it.participant.participantPublicKey == publicKey } - ?.profile - ?.humanReadableNameOrPubkey() - ?: publicKey.take(12) - } - ) + updateLoaded { current -> + current.copy( + adminGroupBlockedOn = outcome.missing.map { publicKey -> + current.ritualMembers + .firstOrNull { it.participant.participantPublicKey == publicKey } + ?.profile + ?.humanReadableNameOrPubkey() + ?: publicKey.take(12) + } + ) + } } is MarmotGroupCreation.Outcome.Failed -> { - dkgRitualUIState = DkgRitualUIState.Error(outcome.reason) + _dkgRitualUIState.value = DkgRitualUIState.Error(outcome.reason) } is MarmotGroupCreation.Outcome.Created -> withContext(Main) { diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/DkgRitualStartOfferJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/DkgRitualStartOfferJvmTest.kt new file mode 100644 index 00000000..e82b816e --- /dev/null +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/DkgRitualStartOfferJvmTest.kt @@ -0,0 +1,247 @@ +package press.mantra.compose.ui.view.model + +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.setMain +import kotlinx.coroutines.withTimeout +import press.mantra.compose.database.model.ChatRoom +import press.mantra.compose.database.model.DkgSession +import press.mantra.compose.database.model.FrostSigningItem +import press.mantra.compose.database.model.FrostSigningSession +import press.mantra.compose.database.model.Participant +import press.mantra.compose.database.model.intermdiate.LocalChatRoom +import press.mantra.compose.database.model.intermdiate.LocalFrostSigningSession +import press.mantra.compose.database.model.intermdiate.LocalParticipant +import press.mantra.compose.database.model.types.DkgApprovalStep +import press.mantra.compose.database.model.types.DkgRitualStage +import press.mantra.compose.repository.ChatRepository +import press.mantra.compose.repository.DkgRepository +import press.mantra.compose.ui.view.state.DkgRitualUIState +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNotNull + +/** + * The two ways the key-ceremony screen came to offer to start the ceremony it + * was already showing. + * + * "Start key ceremony" is the whole no-ceremony screen -- the explainer, the + * quorum picker and the button -- so anything that leaves the state saying + * there is no ceremony puts that offer in front of a member watching one run. + * Both of these did, and both are invisible to a test that only asks the view + * model what it settles on: the state was right before and right after, and + * wrong in between for long enough to read and to tap. + * + * Neither is about ChillDKG. They are about one `DkgRitualUIState` being + * assembled by seven flows at once, which is why they are tested here rather + * than against the ritual. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class DkgRitualStartOfferJvmTest { + private val chatRoomId = "a1b2c3d4".repeat(8) + private val me = "11".repeat(32) + private val peer = "22".repeat(32) + + @BeforeTest + fun setUp() { + Dispatchers.setMain(UnconfinedTestDispatcher()) + } + + @AfterTest + fun tearDown() { + Dispatchers.resetMain() + } + + /** + * The first loaded state already knows the room is running a ceremony. + * + * The screen used to publish a bare `Loaded` and then wait for the rows, + * and a `Loaded` with no session and no ceremony *is* the offer to start + * one. On a cold open -- the only time the queries are slow -- that is + * exactly when it was shown. + */ + @Test + fun `the offer to start is not published while the rows are still being read`() = runBlocking { + val session = session(DkgRitualStage.COLLECTING_HOST_KEYS) + + val viewModel = viewModel( + object : DkgRepository by DkgRepository.NO_OP_DKG_REPOSITORY { + // Slow on purpose: a Room query answers in microseconds and + // would hide the window rather than prove it is gone. + override fun observeLatestSessionForChatRoom( + chatRoomId: String, + parentChatRoomId: String?, + ): Flow = flow { + delay(50) + emit(session) + } + } + ) + + viewModel.initiate() + + val firstLoaded = withTimeout(5_000) { + viewModel.dkgRitualUIState.first { it is DkgRitualUIState.Loaded } + } as DkgRitualUIState.Loaded + + assertEquals(session.id, firstLoaded.session?.id, "the first loaded state missed the ceremony") + assertFalse( + viewModel.canStartRitual(firstLoaded), + "offered to start a ceremony on a room already running one" + ) + } + + /** + * A write that suspends between reading the state and writing it back does + * not carry the stale copy over somebody else's change. + * + * Every flow folding into this state used to read it, build a `copy` and + * assign -- and the session watcher asks the database what this member owes + * in the middle of that. The key-state watcher writing during that pause had + * its change thrown away by the assignment that followed. The same pause in + * the other direction is what put `session` back to null and offered to + * start the ceremony on screen; this is the half of it that can be forced to + * happen rather than raced into. + */ + @Test + fun `a change written during a suspension survives the write that follows it`() = runBlocking { + val session = session(DkgRitualStage.COLLECTING_ROUND_1) + val signingSessions = MutableStateFlow>(emptyList()) + + // Held open until the test has had the key-state watcher write. The + // seeding call runs before anything else is collecting, so it is let + // through; the session watcher's call is the one that waits. + val gate = CompletableDeferred() + var calls = 0 + + val viewModel = viewModel( + object : DkgRepository by DkgRepository.NO_OP_DKG_REPOSITORY { + override fun observeLatestSessionForChatRoom( + chatRoomId: String, + parentChatRoomId: String?, + ): Flow = flowOf(session) + + override fun observeSigningSessions( + chatRoomId: String + ): Flow> = signingSessions + + override suspend fun pendingApproval(session: DkgSession): DkgApprovalStep? { + calls += 1 + if (calls == 1) return null + + gate.await() + return DkgApprovalStep.ROUND_1 + } + } + ) + + viewModel.initiate() + + withTimeout(5_000) { viewModel.dkgRitualUIState.first { it is DkgRitualUIState.Loaded } } + + // The watcher is now parked inside `pendingApproval`, holding a copy of + // the state as it was before this line. + signingSessions.value = listOf(keyStateSigningSession()) + + withTimeout(5_000) { + viewModel.dkgRitualUIState.first { + (it as? DkgRitualUIState.Loaded)?.keyStateSession != null + } + } + + gate.complete(Unit) + + val settled = withTimeout(5_000) { + viewModel.dkgRitualUIState.first { + (it as? DkgRitualUIState.Loaded)?.pendingApproval == DkgApprovalStep.ROUND_1 + } + } as DkgRitualUIState.Loaded + + assertNotNull( + settled.keyStateSession, + "the session watcher wrote back a copy taken before the key state arrived" + ) + assertEquals(session.id, settled.session?.id) + } + + private fun viewModel(dkgRepository: DkgRepository) = DkgRitualViewModel( + chatRoomId = chatRoomId, + activeUserPublicKey = me, + initialDkgRitualUIState = DkgRitualUIState.Loading, + activeIdentityStateFlow = MutableStateFlow(null), + chatRepository = object : ChatRepository by ChatRepository.NO_OP_CHAT_REPOSITORY { + override suspend fun getChatRoomByIdentifier(id: HexKey): LocalChatRoom? = room() + }, + dkgRepository = dkgRepository + ) + + private fun room() = LocalChatRoom( + chatRoom = ChatRoom( + id = chatRoomId, + userPublicKey = me, + subject = "Ekklesia", + description = null, + mlsGroupState = null, + ), + localParticipants = listOf(me, peer).map { publicKey -> + LocalParticipant( + participant = Participant( + participantPublicKey = publicKey, + chatRoomId = chatRoomId, + relayHint = null, + ), + profile = null, + ) + }, + ) + + private fun session(stage: DkgRitualStage) = DkgSession( + id = "ceremony", + chatRoomId = chatRoomId, + coordinatorPublicKey = me, + userPublicKey = me, + threshold = 2, + participantCount = 2, + hostPublicKey = me, + round1Random = "66".repeat(32), + round2AuxRandom = "77".repeat(32), + stage = stage, + ) + + private fun keyStateSigningSession() = LocalFrostSigningSession( + session = FrostSigningSession( + id = "signing", + chatRoomId = chatRoomId, + coordinatorPublicKey = me, + userPublicKey = me, + dkgSessionId = "ceremony", + threshold = 2, + participantCount = 2, + signerId = 0, + ), + unorderedItems = listOf( + FrostSigningItem( + sessionId = "signing", + itemIndex = 0, + unsignedEventJson = """{"id":"${"33".repeat(32)}","pubkey":"$me",""" + + """"created_at":0,"kind":30326,"tags":[],"content":"","sig":"${"44".repeat(64)}"}""", + eventId = "33".repeat(32), + nonceRandom = "55".repeat(32), + ) + ), + ) +}