Multisig implementation#509
Conversation
…into multisig-implementation
* feat: localize copy writing * Format and regrenate runtime apis * feat: finish create multisig flow * fix: code reviews * fix: code reviews * feat: finish adding in-flight tracking for multisig creation - reconcilate pending multisig creation - track multisig creation * feat: improve multisig creation - make nonce calculated on the fly - only show history for creator * feat: add checksum and not truncate checksum * fix: create multisig screen * fix: add logging on error * feat: improve UX multisig creation pending and details sheet * fix: preflight balance check * fix: DRY violation * fix: silent or masking bad data * fix: persisting pending multisig creation * chore: formatting * Discover multisig flow (#506) * feat: finish adding discover multisig * fix: code review issues * fix: another code review issues * feat: add checksum on discover list item * fix: listview item non properly ordered without key * feat: refactor repeating multisig graphql handling * feat: make json reading throw early instead of silently droping it. Also have intFromJson for supporting hasura stringify number. * fix: use variable instead of string interpolation * feat: propagate error as to not confuse user * Create proposal flow (#507) * wip: create proposal - finish create flow - create history of proposal * feat: improve UX of propose amount and review screen * feat: finish improving the lifecycle of proposal creation and event listing presentation * feat: address code review issues * feat: another code reviews fixes * chore: formatting * fix: dart rule violation * fix: redudancy * fix: graphql query for multisig * fix: circle dependency * feat: explicitly show failed fee fetch - add retry button - show message - silently refetch network fee unless failed * fix: reduce risk of double proposal creation * fix: handle last effort before timeout * chore: formatting
* feat: finish approve proposal flow from multisig account * feat: show approve event in signer account * feat: resolve DRY and simplicity issues * feat: best effort timout and balance refresh * chore: formatting * chore: revert constants * chore: remove dead code
…into multisig-implementation
…into multisig-implementation
* feat: finish approve proposal flow from multisig account * feat: show approve event in signer account * feat: resolve DRY and simplicity issues * feat: best effort timout and balance refresh * chore: formatting * wip: add execute flow * feat: regenerate polkadart metadata, clean up deposit field and rename interceptor to guardian * fix: live state resolution and update executed proposal UI * feat: improve UX executed proposals * chore: formatting * chore: revert constants * feat: show activity in executor * chore: formatting * chore: revert back chain url * fix: code reviews * fix: execute event query and data model * chore: revert constants * chore: formatting
* feat: finish approve proposal flow from multisig account * feat: show approve event in signer account * feat: resolve DRY and simplicity issues * feat: best effort timout and balance refresh * chore: formatting * wip: add execute flow * feat: regenerate polkadart metadata, clean up deposit field and rename interceptor to guardian * fix: live state resolution and update executed proposal UI * feat: improve UX executed proposals * chore: formatting * chore: revert constants * feat: show activity in executor * chore: formatting * feat: add cancel proposal flow * feat: display cancel proposal event in proposer activity * chore: formatting * chore: revert constants * chore: revert back chain url * fix: code reviews * fix: execute event query and data model * chore: revert constants * chore: formatting * fix: code review issues
* feat: finish approve proposal flow from multisig account * feat: show approve event in signer account * feat: resolve DRY and simplicity issues * feat: best effort timout and balance refresh * chore: formatting * wip: add execute flow * feat: regenerate polkadart metadata, clean up deposit field and rename interceptor to guardian * fix: live state resolution and update executed proposal UI * feat: improve UX executed proposals * chore: formatting * chore: revert constants * feat: show activity in executor * chore: formatting * feat: add cancel proposal flow * feat: display cancel proposal event in proposer activity * chore: formatting * chore: revert constants * feat: finish implementing polling and refresh proposals * chore: formatting * chore: revert back chain url * fix: code reviews * fix: execute event query and data model * chore: revert constants * chore: formatting * fix: code review issues * fix: code review issues * fix: broken initialization of timezone * chore: add debug on error
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit bbd4053. Configure here.
| _ref.read(accountsProvider.notifier).reset(); | ||
| _ref.read(activeAccountProvider.notifier).reset(); | ||
| _ref.read(multisigAccountsProvider.notifier).reset(); | ||
| _ref.invalidate(discoveredMultisigsProvider); |
There was a problem hiding this comment.
Logout skips multisig pending providers
Medium Severity
logout() clears pendingTransactionsProvider and persisted pendingMultisigCreationsProvider, but leaves in-memory pendingMultisigApprovalsProvider, pendingMultisigProposalsProvider, pendingMultisigExecutionsProvider, and pendingMultisigCancellationsProvider untouched. After logout and re-login in the same process, stale approving/executing/cancelling/proposing UI and inflated pending balance deductions can persist, and extrinsic index pollers tied to the old session may keep running.
Triggered by learned rule: LogoutService must invalidate all pending-state providers
Reviewed by Cursor Bugbot for commit bbd4053. Configure here.
| final isPending = pendingExecution != null; | ||
| // A pending cancellation from this device blocks executing, otherwise the | ||
| // two extrinsics race on-chain and one fails with a wasted fee. | ||
| final canExecute = isActionable && !isPending && pendingCancellation == null && hasLocalSigner; |
There was a problem hiding this comment.
Execute ignores pending approval state
Medium Severity
On the proposal detail sheet, canExecute only gates on pending cancellation and execution, not a pending approval from the same device. If the UI switches to Execute while an approval extrinsic is still confirming, Execute can stay enabled and the user can submit a second conflicting extrinsic.
Additional Locations (1)
Triggered by learned rule: New multisig action flows must disable all conflicting action buttons while pending
Reviewed by Cursor Bugbot for commit bbd4053. Configure here.
| if (cancellation.proposerId == accountId) { | ||
| totalOutgoing += cancellation.memberCost; | ||
| } | ||
| } |
There was a problem hiding this comment.
Balance ignores pending multisig creation
Medium Severity
_calculatePendingOutgoing was extended for pending multisig proposals, executions, and cancellations, but not for pendingMultisigCreationsProvider. While creation is confirming, the creator’s effective balance still reflects the full on-chain balance, so propose/send flows can overestimate spendable funds even though creation fees are already committed.
Reviewed by Cursor Bugbot for commit bbd4053. Configure here.
| } catch (e, stackTrace) { | ||
| quantusDebugPrint('[AccountActivityReconcile] Error: $e'); | ||
| quantusDebugPrint('Stack trace: $stackTrace'); | ||
| } |
There was a problem hiding this comment.
Reconcile errors skip telemetry
Low Severity
The new appendConfirmedEventToHistory catch block logs with quantusDebugPrint only. In release builds those failures are invisible to TelemetryService, so multisig history reconcile errors after indexer confirmation won’t be tracked in production.
Triggered by learned rule: Catch blocks must route errors to TelemetryService, not just quantusDebugPrint
Reviewed by Cursor Bugbot for commit bbd4053. Configure here.
|
|
||
| TelemetryService().sendEvent('multisig_approve'); | ||
|
|
||
| unawaited(_submitApproveBackground(msig: msig, signer: signer, proposalId: proposal.id, pending: pending)); |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
The approve/execute/cancel submission flows sign actions using proposal.id sourced from indexer-backed proposal objects, without independently re-validating action-critical proposal details from a trusted chain source before submission. If indexer metadata is tampered (or stale/misbinding occurs), a signer can be shown benign transfer details but still approve/execute a different on-chain proposal ID.
Impact: this can enable signer deception that results in unintended multisig approvals/executions and potential fund loss.
Reviewed by Cursor Security Reviewer for commit bbd4053. Configure here.
n13
left a comment
There was a problem hiding this comment.
Reviewed at commit bbd4053, with both test suites run locally. Overall this is well put together: the generic ExtrinsicIndexerPollingService config keeps the four pending-extrinsic flows DRY, GraphQL consistently uses variables instead of interpolation, the Rust predict_multisig_address has order-independence and golden-vector tests, and the optimistic-pending → indexer-reconcile lifecycle is applied uniformly across create/propose/approve/execute/cancel. All 191 mobile-app tests pass locally.
That said, there is one blocking finding and a few things I'd fix before merge.
Blocking
1. Four quantus_sdk tests fail on this branch (verified locally):
ChainHistoryService.tryParseOtherTransferEvent parses live indexer proposal created shape with sparse multisig
ChainHistoryService.tryParseOtherTransferEvent parses multisig proposal created account events
ChainHistoryService.tryParseOtherTransferEvent parses multisig signer approved account events
ChainHistoryService.tryParseOtherTransferEvent parses signer approved with sparse multisig without wrong threshold
Root cause: MultisigProposal.fromIndexerJson calls dateTimeFromJson(record['updated_at']), which throws on missing/null. The nested proposal objects in the proposal-created and signer-approved fixtures carry created_at but no updated_at (the executed/cancelled fixtures have both, and those tests pass). The exception is swallowed by the catch in tryParseOtherTransferEvent, which returns null, so the tests see null instead of an event.
This matters beyond the tests: liveProposalCreatedFixture appears captured from the real indexer. If the indexer ever returns a null updated_at, the event silently disappears from activity history with only a developer.log. Suggest falling back to created_at when updated_at is absent (and aligning the fixtures with whatever the indexer actually guarantees).
2. CI never runs the SDK tests. ci.yaml scopes tests to --scope="resonance_network_wallet", so the ~900 lines of new tests under quantus_sdk/test/ never execute in CI — which is why finding 1 didn't fail the build. Worth widening the scope (the rust-dylib-dependent tests like generate_keys_test.dart would need the native lib built, or an exclusion).
Should fix
3. Creation retry can double-submit create_multisig. MultisigSubmissionService._submitAndTrackBackground retries up to 3 times, and each attempt re-signs via submitCreateMultisigExtrinsic. But SubstrateService.submitExtrinsic already retries the same signed bytes internally, and TransactionSubmissionService._submitProposalBackground documents exactly why outer retries are dangerous ("each attempt fetches a fresh nonce and can duplicate..."). If a first creation submit lands but the client sees an error (e.g. timeout after broadcast), the retry is re-signed with a fresh account nonce and burns the creator's network fee on a guaranteed on-chain failure. Recommend dropping the outer retry loop for consistency with the proposal path.
4. Logout leaves multisig pending state and pollers alive. Extending Bugbot's finding: besides the four in-memory pending providers not being reset, none of the ExtrinsicIndexerPollingService instances are stopped on logout — their 5-second timers keep polling the indexer for up to 5 minutes into the next session and can mutate the new session's state.
5. Effective balance ignores pending creations. (Bugbot also flagged.) _calculatePendingOutgoing covers pending proposals/executions/cancellations but not pendingMultisigCreationsProvider, so the creator's spendable balance is overstated while a creation confirms — and effectiveBalanceProviderFamily feeds the propose flow's member-balance check.
6. Execute can race a pending approval. (Bugbot also flagged.) In multisig_proposal_detail_sheet.dart, canExecute gates on pending execution and cancellation but not on a pending approval. When the indexer flips the proposal to approved while this device's approval extrinsic is still confirming, the primary button switches to an enabled Execute.
Minor / questions
-
MultisigService.isMultisigOnChainactually queries the indexer, not the chain, yet it backs creation preflight (address-collision check) and the creation poller's "confirmed" signal. An indexer lag window lets preflight pass for an address that already exists on-chain. Suggest renaming toisMultisigIndexed, or checking chain storage for the preflight. -
In
tryParseOtherTransferEvent,MultisigCreatedEvent.fromAccountEventis the only multisig parse not wrapped in try/catch — a malformed creation row fails the whole history page while the other four degrade to a logged skip. Intentional? -
MultisigAccountsNotifier.byAccountIdis unused, and its signature contradicts its body: declaredMultisigAccount?butfirstWhere(orElse: () => throw ...)never returns null. Drop it or fix the semantics. -
Several new failure paths log only via
quantusDebugPrint/developer.log(e.g. theappendConfirmedEventToHistorycatch, poller search errors, the parse warnings inChainHistoryService), which are invisible in release builds. Genuine failures should also go toTelemetryService(Bugbot flagged one instance of this). -
MultisigCreationPollingService._recoverExpired: after an app restart, an expired pending creation that isn't found in the indexer is removed silently — unlike the in-session timeout, which shows a toast. The user never learns the creation failed. -
+1 on the security reviewer's note: approve/execute/cancel sign an indexer-sourced
proposal.idwhile the user is shown indexer-sourced details. A chain-storage cross-check of (recipient, amount) for the proposal id before signing would close that deception window; reasonable as a follow-up given the indexer is first-party.
|
There's still a double retry loop? Hmm.. |
…into multisig-implementation
This is really weird, we already fix this. And I remember also do it in the multisig flow. Might be leftover |
n13
left a comment
There was a problem hiding this comment.
Re-reviewed at fa623a5 in a fresh worktree. The review delta since bbd4053 is the fix: code review issues commit plus a merge of main (version bump only). Verified each finding from the previous review against the code and by running both test suites and analyzers locally.
Previous blocking findings — both fixed
- Four failing
quantus_sdktests — fixed.MultisigProposal.fromIndexerJsonnow falls back tocreated_atwhen the indexer omitsupdated_at, so sparse proposal rows no longer vanish from history. All four tests pass locally. - CI never ran SDK tests — fixed.
ci.yamlnow runsflutter test --exclude-tags=nativeforquantus_sdk, with the dylib-dependentgenerate_keys_test.darttaggednativevia the newdart_test.yaml. Confirmed it's the only test needing the Rust lib, and the CI-style run passes (73 tests).
Previous should-fix findings — all fixed
- Creation double-submit — the outer retry loop in
MultisigSubmissionService._submitAndTrackBackgroundis gone; retries now live solely inSubstrateService.submitExtrinsic(same signed bytes), matching the proposal path. Failure now also clears pending state, toasts, and reports to telemetry. - Logout leaks —
logout()now stops all six pollers viastopAll()(before clearing state, so timers can't re-add it) and clears all four in-memory pending multisig providers. - Balance ignores pending creations —
_calculatePendingOutgoingnow reservestotalCost(pallet fee + network fee, same amount as the preflight check) for the creator while a creation confirms, in bothbalanceProviderandeffectiveBalanceProviderFamily. - Execute racing a pending approval —
canExecutenow gates onpendingApproval == null, and the cancel button likewise blocks on any other pending action.
Previous minor findings
All addressed: isMultisigOnChain renamed to isMultisigIndexed, the multisig-created parse is now wrapped (and the five parse sites DRY'd into _tryParseMultisigEvent), dead byAccountId removed, telemetry added at the terminal failure points (reconcile append, polling give-up, creation recovery, creation submit failure), and _recoverExpired now shows the timeout toast instead of silently dropping the expired creation. The chain-storage cross-check of proposal details before signing (security reviewer's note) remains open as an agreed follow-up.
Verification
quantus_sdk: 78/78 tests pass (including native-tagged with a locally built dylib); CI-style--exclude-tags=nativerun also passes.mobile-app: 191/191 tests pass.flutter analyzeclean on both packages.
Verdict: approve. All blocking and should-fix findings are resolved, with the indexer-detail cross-check tracked as a follow-up.


Note
High Risk
Large change to on-chain transfer and multisig flows with balance reservation and extrinsic/indexer reconciliation; mistakes could affect funds display or proposal state.
Overview
Adds end-to-end multisig support in the mobile wallet: create and discover shared accounts, propose transfers, and approve, execute, or cancel proposals—with biometric auth, fee/deposit breakdowns, and open/past proposal lists.
State and UX plumbing wires multisig into the rest of the app: persisted multisig accounts and pending creation drafts, pending extrinsic tracking for each action type, toast events for timeouts/failures, and activity/history rows plus detail copy in English and Indonesian. Effective balance now reserves funds for pending multisig proposal, execution, and cancellation costs on the member account.
Confirmation flow uses shared indexer polling (
ExtrinsicIndexerPollingService) and reconciliation helpers to clear pending state, refresh proposal providers, dedupe history (including multisig creation by address), and append confirmed events; creation polling resumes from storage on startup and is registered early inAppInitializerso listeners exist before account load. Global history refresh also invalidates active multisig proposal data; logout clears multisig and pending creation state.Smaller changes: app 1.5.2 (110), notification timezone init falls back to UTC on failure, and analyzer excludes
build/**.Reviewed by Cursor Bugbot for commit bbd4053. Configure here.