From 3b00f9f893fededc53044adbcd1b5c0b57aa5c54 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 8 Sep 2026 23:36:00 +0200 Subject: [PATCH] refactor(subgroups): lift Marmot room creation out of the ritual view model Phase 6 of docs/subgroups.md. `DkgRitualViewModel.createAdminGroup` was 120 lines of room creation living in a view model, and a subgroup needs all of it with four different values. It moves to `managers/MarmotGroupCreation`, unchanged in behaviour, and the view model shrinks by 154 lines to the four values and a `when` over the result. The rules that were already right stay right for the subgroup for free, which is the whole reason to move rather than to write a second one: **Every key package before anything exists.** The id is derived, so there is exactly one room per key at a path; a half-created one occupies that address permanently and there is no second id to retry with. Better to create nothing and name who is missing. (Phase 7 will check this at the picker instead, so a subgroup does not discover it after three ceremonies -- this stays as the backstop.) **`adminPubkeys` baked into the epoch-0 group context** rather than added by a later commit, so a member welcomed afterwards gets a populated group instead of chasing a bootstrap commit that predates their membership. That is why `MarmotGroupData` is built by hand rather than through `MarmotGroupData.bootstrap`, which hardcodes a single admin. **`adopt` before the members are added.** Filing the key state is local and certain; adding members is a relay round trip that can partly fail. The room comes into existence already knowing what it signs with, whatever happens next. **Derived ids make every step reachable twice**, so `Existing` is a success rather than a refusal -- a second tap or another member getting there first should join what exists rather than mint a rival group on one address. Four outcomes instead of four scattered early returns: `Created` with the members who could not be added, `Existing`, `BlockedOn` with the missing key packages, and `Failed`. The view model keeps the one thing that was genuinely its own -- turning missing public keys into names, since "no key package for 3 people" is not actionable and "Bob needs to publish a key package" is. **It is reached through `ChatRepository.createMarmotGroup`, not called directly.** View models in this app talk to repositories and managers take the database; the first cut had the view model reaching for `dkgRepository.database`, which does not exist on the interface and should not. The repository method is three lines of delegation and keeps the boundary where the rest of the app has it. `parentChatRoomId` is the one genuinely new parameter, written onto the room after `getOrCreateChatRoom` returns rather than passed into it -- that call is shared with every other way a room appears and none of them has a parent to hand it. Nine imports the extraction made dead are dropped from the view model. 397 common tests, 701 jvm tests, `m3Audit` meets every budget. Co-Authored-By: Claude Opus 5 --- .../repository/DatabaseChatRepository.kt | 23 ++ .../compose/managers/MarmotGroupCreation.kt | 211 ++++++++++++++++++ .../compose/repository/ChatRepository.kt | 39 ++++ .../ui/view/model/DkgRitualViewModel.kt | 196 ++++------------ 4 files changed, 315 insertions(+), 154 deletions(-) create mode 100644 composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt index a367f6ba..6ee41cc7 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/database/repository/DatabaseChatRepository.kt @@ -6,6 +6,7 @@ import press.mantra.compose.database.model.GroupKeyState import press.mantra.compose.database.model.GroupSignedEvent import press.mantra.compose.managers.ChronicleManager import press.mantra.compose.managers.GroupKeyStateManager +import press.mantra.compose.managers.MarmotGroupCreation import press.mantra.compose.database.model.ChatMessage import press.mantra.compose.database.model.ChatRoom import press.mantra.compose.database.model.GiftWrapPayload @@ -43,6 +44,28 @@ class DatabaseChatRepository( private val scope: CoroutineScope ): press.mantra.compose.repository.ChatRepository { val logger = Logger.withTag(TAG) + + override suspend fun createMarmotGroup( + groupId: String, + name: String, + purpose: String, + adminPublicKeys: Set, + userPublicKey: HexKey, + keyPair: KeyPair, + path: List, + parentChatRoomId: HexKey?, + ): MarmotGroupCreation.Outcome = MarmotGroupCreation.create( + database = database, + chatRepository = this, + groupId = groupId, + name = name, + purpose = purpose, + adminPublicKeys = adminPublicKeys, + userPublicKey = userPublicKey, + keyPair = keyPair, + path = path, + parentChatRoomId = parentChatRoomId, + ) override suspend fun observeChatRoomListByPublicKey(publicKey: String): Flow> { return database.chatRoomDao().observeChatRoomListByUserPublicKey(publicKey) } diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt new file mode 100644 index 00000000..ca00f401 --- /dev/null +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/managers/MarmotGroupCreation.kt @@ -0,0 +1,211 @@ +package press.mantra.compose.managers + +import co.touchlab.kermit.Logger +import com.vitorpamplona.quartz.marmot.mip01Groups.MarmotGroupData +import com.vitorpamplona.quartz.marmot.mls.crypto.Ed25519 +import com.vitorpamplona.quartz.marmot.mls.group.MlsGroup +import com.vitorpamplona.quartz.nip01Core.core.HexKey +import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.flow.filterNotNull +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withTimeoutOrNull +import press.mantra.compose.database.MantraDatabase +import press.mantra.compose.database.model.MarmotKeyPackage +import press.mantra.compose.database.model.intermdiate.LocalChatRoom +import press.mantra.compose.extensions.toHex +import press.mantra.compose.nostr.Relays +import press.mantra.compose.repository.ChatRepository + +/** + * Creating a Marmot room at an id the group's key derives, once, for every flow + * that needs one. + * + * Two do: the `#admins` room a group opens after its own ceremony, and a + * subgroup, which is the same act with four different values. This was 120 lines + * inside `DkgRitualViewModel` and is here unchanged in behaviour, because the + * rules it already gets right are not rules worth deriving twice. + * + * ### The three that are not obvious + * + * **Every key package before anything exists.** The id is derived, so there is + * exactly one room per key at a path; a half-created one occupies that address + * permanently and there is no second id to retry with. Better to create nothing + * and name who is missing -- and better still to have checked at the picker, so + * that a subgroup does not discover it after three ceremonies. See + * `docs/subgroups.md`. + * + * **`adminPubkeys` baked into the epoch-0 group context**, rather than added by a + * later commit, so a member welcomed afterwards gets a populated group instead of + * chasing a bootstrap commit that predates their membership. That is why + * `MarmotGroupData` is built here rather than through `MarmotGroupData.bootstrap`, + * which hardcodes a single admin. + * + * **[GroupKeyStateManager.adopt] before the members are added.** Filing the key + * state is local and certain; adding members is a relay round trip that can + * partly fail. Doing it first means the room comes into existence already knowing + * what it signs with, whatever happens next. + * + * ### Derived ids make every step reachable twice + * + * A second tap, or another member having got there first, lands on the same id. + * Joining what exists beats minting a rival group on one address, so [Existing] + * is a success rather than a refusal. + */ +object MarmotGroupCreation { + private const val TAG = "MarmotGroupCreation" + + private val logger = Logger.withTag(TAG) + + /** Matches `SelectChatRoomTypeViewModel`: relays are not always prompt. */ + const val KEY_PACKAGE_LOOKUP_TIMEOUT: Long = 10_000L + + sealed interface Outcome { + /** + * The room was created. [notAdded] are the members whose invite failed + * after it existed -- a room with most of the group in it is more useful + * than no room, and the rest can be invited again. + */ + data class Created(val room: LocalChatRoom, val notAdded: List) : Outcome + + /** The room was already here. Every path into one ends the same way. */ + data class Existing(val room: LocalChatRoom) : Outcome + + /** + * Nothing was created, because [missing] have no published key package. + * + * The one refusal that is not a failure: creating the room anyway would + * list an admin who is not in the MLS tree, in a group that disagrees with + * itself from its first epoch, at an address nothing can replace. + */ + data class BlockedOn(val missing: List) : Outcome + + data class Failed(val reason: String) : Outcome + } + + /** + * Creates the room [groupId] with [adminPublicKeys] as its admins. + * + * [purpose] is the human half of the description; the derivation path is + * appended to it, because MIP-01 has no field for one and the path is what + * rebuilds the `TweakCache` a signing session needs. + * + * [parentChatRoomId] is written onto the room when this is a subgroup. It is + * the verified value off the child's own key state -- never a claim off a + * proposal. + */ + suspend fun create( + database: MantraDatabase, + chatRepository: ChatRepository, + groupId: String, + name: String, + purpose: String, + adminPublicKeys: Set, + userPublicKey: HexKey, + keyPair: KeyPair, + path: List = SharedKeyDerivation.MARMOT_ADMIN_GROUP_PATH, + parentChatRoomId: HexKey? = null, + keyPackageLookupTimeout: Long = KEY_PACKAGE_LOOKUP_TIMEOUT + ): Outcome { + chatRepository.getChatRoomByIdentifier(groupId)?.let { return Outcome.Existing(it) } + + val peers = adminPublicKeys.filterNot { it == userPublicKey } + val keyPackages = keyPackagesFor(chatRepository, peers, keyPackageLookupTimeout) + + val missing = keyPackages.filter { it.second == null }.map { it.first } + if (missing.isNotEmpty()) { + logger.w("Not creating $groupId: no key package for $missing") + return Outcome.BlockedOn(missing) + } + + val addable = keyPackages.mapNotNull { (publicKey, keyPackage) -> + keyPackage?.let { publicKey to it } + } + + val description = SharedKeyDerivation.describe(purpose = purpose, path = path) + + // Built directly rather than through MarmotGroupData.bootstrap, which + // hardcodes a single admin. + val metadata = MarmotGroupData( + nostrGroupId = groupId, + name = name, + description = description, + adminPubkeys = adminPublicKeys.toList(), + relays = Relays.DefaultDMRelayList.map { it.url } + ) + + val signingKeyPair = Ed25519.generateKeyPair() + val group = MlsGroup.create( + keyPair.pubKey, + signingKeyPair.privateKey, + listOf(metadata.toExtension()) + ) + + // Keyed on the Marmot nostrGroupId, not MlsGroup's own groupId: they are + // unrelated 32-byte values, and every inbound path resolves rooms by the + // former. + val localChatRoom = chatRepository.getOrCreateChatRoom( + chatRoomId = groupId, + activeUserPublicKey = userPublicKey, + relayHint = null, + defaultSubject = name, + description = description, + mlsGroupState = group.saveState().encodeTls().toHex() + ) ?: return Outcome.Failed("Couldn't create the group. Please try again.") + + if (parentChatRoomId != null) { + // Written after the room exists rather than passed into its creation, + // because `getOrCreateChatRoom` is shared with every other way a room + // appears and none of them has a parent to hand it. + database.chatRoomDao().upsert( + localChatRoom.chatRoom.copy(parentChatRoomId = parentChatRoomId) + ) + } + + // The room comes into existence already knowing what it signs with. The + // group agreed that before any of this ran and this device has been + // holding the signed statement since; this is simply the first moment + // there is a row to file it against. + GroupKeyStateManager.adopt(database, groupId) + + val notAdded = runCatching { + chatRepository.addMembers(localChatRoom = localChatRoom, peers = addable) + }.onFailure { + logger.e("Failed to add members to $groupId", it) + }.getOrElse { addable.map { (publicKey, _) -> publicKey } } + + if (notAdded.isNotEmpty()) { + logger.w("$groupId created without ${notAdded.size} member(s): $notAdded") + } + + return Outcome.Created( + room = chatRepository.getChatRoomByIdentifier(groupId) ?: localChatRoom, + notAdded = notAdded + ) + } + + /** + * Every peer's published key package, or null where there is none. + * + * Concurrently, so the whole set costs one relay round trip rather than one + * each, and with a timeout because a member who has never published one would + * otherwise hold the flow open forever. + */ + suspend fun keyPackagesFor( + chatRepository: ChatRepository, + publicKeys: List, + timeout: Long = KEY_PACKAGE_LOOKUP_TIMEOUT + ): List> = coroutineScope { + publicKeys.map { publicKey -> + async { + publicKey to withTimeoutOrNull(timeout) { + chatRepository.observeMarmotKeyPackageForPublicKey(publicKey) + .filterNotNull() + .first() + } + } + }.awaitAll() + } +} diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt index 4be1d1e5..150260b9 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/repository/ChatRepository.kt @@ -4,7 +4,10 @@ import press.mantra.compose.database.model.ChatRoom import press.mantra.compose.database.model.GiftWrapPayload import press.mantra.compose.database.model.GroupKeyState import press.mantra.compose.database.model.GroupSignedEvent +import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair import press.mantra.compose.database.model.MarmotKeyPackage +import press.mantra.compose.managers.SharedKeyDerivation +import press.mantra.compose.managers.MarmotGroupCreation import press.mantra.compose.database.model.Participant import press.mantra.compose.database.model.intermdiate.LocalChatMessage import press.mantra.compose.database.model.intermdiate.LocalChatRoom @@ -113,6 +116,30 @@ interface ChatRepository { peers: List>, ): List + /** + * Creates a Marmot room at an id the group's key derives, with + * [adminPublicKeys] as its admins, and welcomes them into it. + * + * Two flows want exactly this: the `#admins` room a group opens after its + * ceremony, and a subgroup. See `MarmotGroupCreation`, which holds the rules + * -- every key package before anything exists, the admins baked into epoch 0, + * and the key state filed before the members are added. + * + * Refuses rather than half-creates: the id is derived, so a partly created + * room occupies that address permanently and there is no second id to retry + * with. + */ + suspend fun createMarmotGroup( + groupId: String, + name: String, + purpose: String, + adminPublicKeys: Set, + userPublicKey: HexKey, + keyPair: KeyPair, + path: List = SharedKeyDerivation.MARMOT_ADMIN_GROUP_PATH, + parentChatRoomId: HexKey? = null, + ): MarmotGroupCreation.Outcome + /** * Sends [text] to the room, or -- when [directMessageRecipientPublicKey] is given -- * to that one member of it. @@ -263,6 +290,18 @@ interface ChatRepository { peers: List> ): List = peers.map { it.first } + override suspend fun createMarmotGroup( + groupId: String, + name: String, + purpose: String, + adminPublicKeys: Set, + userPublicKey: HexKey, + keyPair: KeyPair, + path: List, + parentChatRoomId: HexKey?, + ): MarmotGroupCreation.Outcome = + MarmotGroupCreation.Outcome.Failed("No database") + override suspend fun sendChatMessage( text: String, 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 23979036..677dcc8c 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 @@ -14,22 +14,13 @@ import press.mantra.compose.database.model.types.DkgRitualStage import press.mantra.compose.database.model.types.FrostSigningStage import press.mantra.compose.database.model.types.ChatRoomType import press.mantra.compose.managers.ChillDkgRitualManager -import press.mantra.compose.extensions.toHex +import press.mantra.compose.managers.MarmotGroupCreation import press.mantra.compose.managers.SharedKeyDerivation -import press.mantra.compose.nostr.Relays import press.mantra.compose.ui.composable.navigation.routes.ChatRoomMessagingRoute import press.mantra.compose.ui.composable.navigation.routes.Route -import com.vitorpamplona.quartz.marmot.mip01Groups.MarmotGroupData -import com.vitorpamplona.quartz.marmot.mls.crypto.Ed25519 -import com.vitorpamplona.quartz.marmot.mls.group.MlsGroup import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair -import kotlinx.coroutines.async -import kotlinx.coroutines.awaitAll -import kotlinx.coroutines.coroutineScope -import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext -import kotlinx.coroutines.withTimeoutOrNull import kotlinx.coroutines.Dispatchers.Main import press.mantra.compose.nostr.dkg.DkgRitualEvents import press.mantra.compose.nostr.frost.GroupKeyStateEvent @@ -382,163 +373,60 @@ class DkgRitualViewModel( dkgRitualUIState = loaded.copy(adminGroupBlockedOn = emptyList()) viewModelScope.launch(Dispatchers.IO) { - // Derived ids make this reachable twice -- a second tap, or another - // member having got there first. Joining what exists beats minting a - // rival group on the same id. - val existing = chatRepository.getChatRoomByIdentifier(groupId) - if (existing != null) { - isActionPending.value = false - withContext(Main) { + val outcome = chatRepository.createMarmotGroup( + groupId = groupId, + name = name, + purpose = "Admins of ${loaded.localChatRoom.chatRoom.subject ?: "the group"}.", + adminPublicKeys = members, + userPublicKey = activeUserPublicKey, + keyPair = keyPair + ) + + isActionPending.value = false + + when (outcome) { + is MarmotGroupCreation.Outcome.BlockedOn -> { + // 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) + } + ) + } + + is MarmotGroupCreation.Outcome.Failed -> { + dkgRitualUIState = DkgRitualUIState.Error(outcome.reason) + } + + is MarmotGroupCreation.Outcome.Created -> withContext(Main) { onNavigateToRoute( ChatRoomMessagingRoute( activeUserPublicKey = activeUserPublicKey, - chatRoomId = existing.chatRoom.id, + chatRoomId = outcome.room.chatRoom.id, relayHint = null ) ) } - return@launch - } - // Every key package, before anything exists. Two reasons this is a - // precondition rather than a best effort: - // - // MarmotGroupData.adminPubkeys is baked into the epoch-0 GroupContext - // and names every member, so a room created without one of them lists an - // admin who is not in the MLS tree -- a group that disagrees with itself - // from its first epoch. - // - // And the id is derived, so there is exactly one room per group at this - // path. A half-created one occupies that address permanently; there is no - // second id to retry with. Better to create nothing and say who is - // missing. - val peers = members.filterNot { it == activeUserPublicKey } - val keyPackages = coroutineScope { - peers.map { publicKey -> - async { - publicKey to withTimeoutOrNull(KEY_PACKAGE_LOOKUP_TIMEOUT) { - chatRepository.observeMarmotKeyPackageForPublicKey(publicKey) - .filterNotNull() - .first() - } - } - }.awaitAll() - } - - val missing = keyPackages.filter { it.second == null }.map { it.first } - if (missing.isNotEmpty()) { - logger.w("Not creating admin group $groupId: no key package for $missing") - - isActionPending.value = false - dkgRitualUIState = loaded.copy( - adminGroupBlockedOn = missing.map { publicKey -> - loaded.ritualMembers - .firstOrNull { it.participant.participantPublicKey == publicKey } - ?.profile - ?.humanReadableNameOrPubkey() - ?: publicKey.take(12) - } - ) - return@launch - } - - val addable = keyPackages.mapNotNull { (publicKey, keyPackage) -> - keyPackage?.let { publicKey to it } - } - - val relays = Relays.DefaultDMRelayList.map { it.url } - - // Built directly rather than through MarmotGroupData.bootstrap, which - // hardcodes a single admin. Baked into the epoch-0 GroupContext so later - // invitees get a populated group from their welcome instead of chasing a - // bootstrap commit that predates their membership. - val metadata = MarmotGroupData( - nostrGroupId = groupId, - name = name, - // Carries the derivation path. MIP-01 has no field for it, and the - // path is what rebuilds the TweakCache a signing session needs -- - // recomputable from the constant only for as long as the constant - // never changes. - description = SharedKeyDerivation.describe( - purpose = "Admins of ${loaded.localChatRoom.chatRoom.subject ?: "the group"}." - ), - adminPubkeys = members.toList(), - relays = relays - ) - - val signingKeyPair = Ed25519.generateKeyPair() - val group = MlsGroup.create(keyPair.pubKey, signingKeyPair.privateKey, listOf(metadata.toExtension())) - - // Keyed on the Marmot nostrGroupId, not MlsGroup's own groupId: they are - // unrelated 32-byte values, and every inbound path resolves rooms by the - // former. - val localChatRoom = chatRepository.getOrCreateChatRoom( - chatRoomId = groupId, - activeUserPublicKey = activeUserPublicKey, - relayHint = null, - defaultSubject = name, - description = metadata.description, - mlsGroupState = group.saveState().encodeTls().toHex() - ) - - if (localChatRoom == null) { - isActionPending.value = false - dkgRitualUIState = DkgRitualUIState.Error("Couldn't create the admin group. Please try again.") - return@launch - } - - // The room comes into existence already knowing what it signs with. - // The group agreed that before any of this ran, in the NIP-17 room - // the ceremony was held in, and this device has been holding the - // signed statement since -- the line above is simply the first - // moment there is a room row to file it against. - // - // Before the members are added rather than after, because nothing - // here goes on the wire: filing it is local, and doing it while the - // room is certain to exist beats doing it after a step that can - // partly fail. - // - // Every other member does the same on their Welcome, in NostrDao, - // off the same event. A member invited later holds no such event, - // and no share either, so they have nothing to pick wrongly between - // -- `FrostSigningManager.completedKey` rederives its way to the - // same key for them. - dkgRepository.adoptGroupKeyState(localChatRoom.chatRoom.id) - - val notAdded = runCatching { - chatRepository.addMembers(localChatRoom = localChatRoom, peers = addable) - }.onFailure { - logger.e("Failed to add members to admin group $groupId", it) - }.getOrElse { addable.map { (publicKey, _) -> publicKey } } - - isActionPending.value = false - - if (notAdded.isNotEmpty()) { - logger.w("Admin group $groupId created without ${notAdded.size} member(s): $notAdded") - } - - withContext(Main) { - onNavigateToRoute( - ChatRoomMessagingRoute( - activeUserPublicKey = activeUserPublicKey, - chatRoomId = localChatRoom.chatRoom.id, - relayHint = null + is MarmotGroupCreation.Outcome.Existing -> withContext(Main) { + onNavigateToRoute( + ChatRoomMessagingRoute( + activeUserPublicKey = activeUserPublicKey, + chatRoomId = outcome.room.chatRoom.id, + relayHint = null + ) ) - ) + } } } } - /** - * Adds each member to the freshly created admin room, returning those who could - * not be added. - * - * A Marmot invite needs the invitee's published key package, so a member who has - * never published one cannot be added here and has to be invited later. That is - * reported rather than treated as failure: a room with most of the group in it - * is more useful than no room. - */ - override fun onCleared() { messageObserver?.cancel() keyStateObserver?.cancel()