fix(multisig-client): keep local state when GUARDIAN nonce lags instead of throwing#344
Conversation
…ad of throwing syncState() threw in ensureSafeToOverwriteLocalState whenever the GUARDIAN's nonce was <= local, and refreshConfigFromAccount ran only after the overwrite branch — so a GUARDIAN that is merely behind (its pushed delta not yet canonicalized) left Multisig.account frozen at the pre-execute snapshot even though the local account was already correct and on-chain-verifiable. Rename the guard to isSafeToOverwriteLocalState: return false (skip overwrite, keep local) when the GUARDIAN is strictly behind, so config is still refreshed from the authoritative local account. Same-nonce/divergent-commitment and incoming-vs-on-chain mismatches still throw.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough
ChangessyncState lag handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GUARDIAN
participant Multisig
participant MidenClientStore
GUARDIAN->>Multisig: provide account state
Multisig->>MidenClientStore: compare incoming and local nonce
MidenClientStore-->>Multisig: keep local or allow overwrite
Multisig->>MidenClientStore: refresh configuration from selected account
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/miden-multisig-client/src/multisig.ts (1)
379-385: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the inline implementation comments.
The surrounding JSDoc already documents both branches.
Suggested cleanup
- // GUARDIAN is behind local: keep the (on-chain-verifiable) local state. if (incomingNonce < localNonce) { return false; } - // Same nonce but divergent state — this method is only reached when the - // commitments already differ — is a genuine fork, not GUARDIAN lag. if (incomingNonce === localNonce) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/miden-multisig-client/src/multisig.ts` around lines 379 - 385, Remove the inline comments surrounding the incomingNonce < localNonce branch and the same-nonce divergent-state path in the relevant multisig method, while preserving the existing logic and relying on the surrounding JSDoc.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/miden-multisig-client/src/multisig.ts`:
- Around line 379-385: Remove the inline comments surrounding the incomingNonce
< localNonce branch and the same-nonce divergent-state path in the relevant
multisig method, while preserving the existing logic and relying on the
surrounding JSDoc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e28359e-4770-422e-b324-725b265d5786
📒 Files selected for processing (2)
packages/miden-multisig-client/src/multisig.test.tspackages/miden-multisig-client/src/multisig.ts
…OverwriteLocalState The surrounding JSDoc already documents both the behind-local (skip) and same-nonce-divergence (throw) branches. Addresses CodeRabbit review nit on OpenZeppelin#344.
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks for contribution <3.
LGTM
This PR introduces small divergence with Rust client
crates/miden-multisig-client/src/client/account.rs
We should update Rust client logic to match TS client.
We can open an issue for later if not handled within this PR.
There was a problem hiding this comment.
Pull request overview
This PR fixes a UX-stalling behavior in the TypeScript Miden multisig SDK (packages/miden-multisig-client) where Multisig.syncState() could throw when the GUARDIAN-reported nonce lagged behind local state, preventing Multisig.account (and derived config) from being refreshed and leaving UI consumers stuck on a stale snapshot.
Changes:
- Changed the overwrite guard from a throwing helper (
ensureSafeToOverwriteLocalState) to a boolean decision helper (isSafeToOverwriteLocalState), treating “GUARDIAN behind local” as a non-fatal skip. - Ensured
syncState()always refreshes multisig config from the authoritative account (local or imported), soMultisig.accountadvances even when GUARDIAN lags. - Updated and expanded tests to cover “GUARDIAN behind” non-throw behavior and added a regression test for the frozen-account repro.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/miden-multisig-client/src/multisig.ts | Updates syncState() overwrite decision flow to skip (not throw) when GUARDIAN nonce is behind, while still refreshing config from the chosen authoritative account. |
| packages/miden-multisig-client/src/multisig.test.ts | Adjusts nonce-behind test to expect no throw/no overwrite/no RPC call, and adds a regression test ensuring Multisig.account “unfreezes” when GUARDIAN lags post-execute. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
=======================================
Coverage 78.01% 78.01%
=======================================
Files 167 167
Lines 31065 31065
=======================================
Hits 24236 24236
Misses 6829 6829 Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
What
Multisig.syncState()threw — and leftMultisig.accountfrozen at its previous snapshot — whenever the GUARDIAN's nonce was<=the local nonce, i.e. whenever the GUARDIAN was simply behind local.refreshConfigFromAccount()runs only after the overwrite branch, so the throw inensureSafeToOverwriteLocalState()unwoundsyncState()beforethis.accountwas ever refreshed. The UI (which readsMultisig.account) then stayed frozen on the pre-execute state even though the local account was already correct and independently on-chain-verifiable.This is the normal state during the post-execute canonicalization window (#316) and permanent if the candidate is discarded (#312 / #319).
Change
ensureSafeToOverwriteLocalState→isSafeToOverwriteLocalState, returningbooleaninstead of throwing on the nonce guard.incomingNonce < localNonce): returnfalse→syncState()skips the overwrite, keeps local, and still callsrefreshConfigFromAccount(localAccount)sothis.accountadvances and the UI unfreezes.The guard "don't clobber good local state with a lower-nonce GUARDIAN state" is preserved — it is now a skip, not a fatal error. This is the client-side complement to #316: #316 removes the server-side lag; this stops the client freezing during whatever lag remains and handles the permanent-behind (discarded) case gracefully.
Test plan
packages/miden-multisig-client/src/multisig.test.ts(86/86 pass):syncState()now resolves, does not callnewAccount, needs no on-chain round-trip, and refreshes config from the local account.Multisig.accountadvances to the local nonce-1 account).tsc --noEmitadds no new errors from this change;multisig.tsappears in zero type errors. (The pre-existingguardian-client.code/GuardianErrorMetaerrors are unrelated — see #318 — and reproduce onmain.)Closes #343
Summary by CodeRabbit