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 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
2026-10-02 19:45:28 +02:00
parent 7a28b227d9
commit 4e6ca0a080
4 changed files with 428 additions and 100 deletions

View File

@@ -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

View File

@@ -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 {

View File

@@ -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> = _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) {