chore: bump de-mls and adapt group_v2 to its new API - #232
Conversation
…rface # Conflicts: # core/conversations/Cargo.toml
| &group_config, | ||
| group_config, |
There was a problem hiding this comment.
[Pebble] Passing a partial builder to a constructor, has some sharp edges.
- The group_config is no longer Idempotent - restoring a conversation for persistent storage, could result in a different configuration causing state desync. Subsequent calls are not guaranteed to produce the same config.
- Validation occurs across a API boundary, the caller does not know if the configuration is valid for the DeMLS usecase.
- Any configuration set could get clobbered by DeMLS leading to difficulty debugging.
A preferred approach from my perspective would be to wrap MlsGroupCreateConfigBuilder and create a dedicated Builder that does not expose the features the DeMLS must control.
Callsites get consistent results, theres a clear separation of concerns with no conflicts over ownership, and its validated immediately which reduces error handling infra.
Always worth making the contract between two separate components well defined, and fool proof
| encoded_credential: sender.credential.serialized_content().to_vec(), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Sand] The lexical nesting is getting quite deep ( Func, For, match, match, match-arm, if-let, ....
Use dedicated handler functions to keep this readable.
fn process_event_obligations(.. , obligation: conversations::Obligation, ...)
fn process_event_info(.. , event_info: conversations::Info , ...)
note: qualified imports are preferred for generically named types such as Info which will not be meaningful to readers of this code
| base64 = "0.22" | ||
| chat-proto = { workspace = true } | ||
| de-mls = { git = "https://github.com/vacp2p/de-mls", rev = "4d85a02" } | ||
| de-mls = { git = "https://github.com/vacp2p/de-mls", rev = "dad6506" } |
There was a problem hiding this comment.
[Dust] Either pinning to the dev branch, or putting a comment of where to go look upstream would be helpful for reviewers.
Considering the rev will not survive being merged, it needs to be updated anyways
| MlsGroupCreateConfig::builder() | ||
| .ciphersuite(crate::inbox_v2::CIPHER_SUITE) | ||
| .capabilities(capabilities_with_group_metadata()) | ||
| .use_ratchet_tree_extension(true) // Embed the ratchet tree in the Welcome so joiners can build the group |
There was a problem hiding this comment.
[?] Where does the ratchet tree go? Is that something DeMLS solves?
2 participants
Bumps de-mls past its output-surface rework and adapts
group_v2to it.Events now come in three kinds.
Obligationis something only we can do — deliver a message, route a welcome, drop the conversation.Requestis a decision de-mls is asking for, each carrying its own fallback if we ignore it.Infois state we could equally well query. The match follows that order, and obligations are matched exhaustively, so a new one from de-mls breaks the build instead of being silently ignored. Teardown moved in there too, which means it now fires whether our removal arrives on a frame or on a timer; before, only the timer path was handled.The MLS group is available directly through
mls_group(), so every OpenMLS read — members, extensions, epoch — comes from OpenMLS rather than needing a de-mls accessor added per capability.Driving calls all return
Result<(), ConversationError>now. A conversation produces events and bytes to publish and nothing else, so there is no third kind of return value to reason about: drain both, and readnext_wakeup_in()when scheduling.The bump now lands on de-mls's rework around openmls branch (vacp2p/de-mls#153, which should merge first): pinned MLS settings —
max_past_epochs=3, a 64-message reorder window, a decrypt gate floored at the member's join epoch — and commits built with inline proposals, so a minting steward is no longer mute for the whole commit round. On our side that meansgroup_config()hands de-mls a builder instead of a finished config (de-mls stamps its settings on top; ciphersuite, capabilities and the group-metadata extension stay ours) andConversation::jointakes a join-config builder the same way.Where this configuration lives is not final and still discussable — the settings are local per member and most of them degrade gracefully when they differ — but the fields that help resolve the fork are now set up and every joiner runs them, not just the group's creator.