Skip to content

Create multisig flow#505

Merged
dewabisma merged 17 commits into
multisig-implementationfrom
beast/create-multisig-flow
Jun 9, 2026
Merged

Create multisig flow#505
dewabisma merged 17 commits into
multisig-implementationfrom
beast/create-multisig-flow

Conversation

@dewabisma

@dewabisma dewabisma commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Create multisig account
  • Edit multisig account
  • View multisig details
  • List create multisig event

Notes

This PR use local chain and local subsquid indexer. So, the app constant use tunneling using ngrok. I will remove this once finish implementing the multisig. This whole multisig PR isn't supposed to be merged to main yet anyway.

Screenshots

  • Home with multisig event
Simulator Screenshot - iPhone 8 - 2026-06-03 at 17 24 42
  • Multisig event detail
Simulator Screenshot - iPhone 8 - 2026-06-03 at 17 24 45
  • Account list with multisig
Simulator Screenshot - iPhone 8 - 2026-06-03 at 17 25 24
  • Create multisig
Simulator Screenshot - iPhone 8 - 2026-06-03 at 17 25 51
  • Multisig account details
Simulator Screenshot - iPhone 8 - 2026-06-03 at 17 25 17

Note

High Risk
Submits signed chain extrinsics and mutates wallet/multisig account state; bugs could duplicate accounts, miss confirmations, or show wrong activity/balances.

Overview
Replaces add multisig from discover/paste with an on-chain create multisig flow: name, co-signers, threshold slider, predicted address preview, local auth, then background extrinsic submit with retries and on-chain polling until the account is saved.

Account & activity UX adds multisig rename via shared EditAccountScreen, a multisig account menu and Multisig details (signers/threshold), and wires pending/confirmed multisig creation into activity lists and detail sheets (fees, deposit, threshold, explorer). English and Indonesian strings cover the full multisig surface; approve/cancel sheets pick up l10n.

Infrastructure introduces pendingMultisigCreationsProvider, submission/polling/reconciliation services, home toasts for ready/timeout/fail, and clears pending state on logout. Multisig rows in the accounts sheet use the same badge pattern as regular accounts plus an edit menu.

Reviewed by Cursor Bugbot for commit 5bae0b8. Configure here.

@dewabisma dewabisma requested a review from n13 June 3, 2026 09:26
- reconcilate pending multisig creation
- track multisig creation

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Create multisig flow

Thorough pass over the create-multisig flow, services, providers, activity integration, and the Rust address-prediction. Overall this is a clean, well-structured feature with nice tests and good fail-loud logging. A few things should be addressed before this goes anywhere near main.

Blocking before merge

  • quantus_sdk/lib/src/constants/app_constants.dart — ngrok endpoints. Production RPC + GraphQL are replaced with personal ngrok tunnels, and the 3 redundant rpcEndpoints are collapsed to a single endpoint (loses failover):
    static const List<String> rpcEndpoints = ['https://a395-180-75-50-27.ngrok-free.app'];
    static const List<String> graphQlEndpoints = ['https://a46e-180-75-50-27.ngrok-free.app/v1/graphql'];
    Acknowledged as temporary in the description — flagging so it doesn't get merged by accident.

Functional concerns

  • Ineffective dedup in TransactionService.combineAndDeduplicateTransactions. Pending creations are processed first, so seenMultisigAddresses is still empty when the pending guard runs — seenMultisigAddresses.add(creation.multisigAddress) is always true. The confirmed MultisigCreatedEvent from otherTransfers then gets added too (different id), so during the confirm→reconcile window both a pending and a confirmed creation for the same address can render simultaneously. The polling service removing the pending row on confirmation mostly hides this, but the guard as written doesn't do what it looks like it does. Pre-scan otherTransfers for multisig addresses (or process confirmed first) so the pending row is genuinely suppressed when a confirmed one exists.

  • Nonce is hardcoded to 0 everywhere. defaultMultisigNonce = BigInt.zero, and the create flow never varies it. Since the on-chain address derivation is hash(pallet_id || sorted_signers || threshold || nonce), a user can never create two distinct multisigs with the same signer set + threshold — the preflight will throw MultisigAlreadyExistsException. The nonce is plumbed through every layer but never incremented/sourced. Is this intended, or should the nonce come from chain/creator state? Worth a comment confirming the intended behavior.

Medium

  • New post-frame callbacks (project rule: avoid post-frame callbacks).

    • mobile-app/lib/v2/screens/multisig/add_multisig_screen.dart:64_refreshPredictedAddress() is already async (it awaits the prediction), so it can be called directly from initState without addPostFrameCallback.
    • mobile-app/lib/v2/screens/multisig/propose/propose_amount_screen.dart:55 — same pattern for _fetchFee().
  • Duplicated + inconsistent "which filter shows a creation" logic (DRY). Pending visibility in filtered_all_transactions_provider.dart (filter != receive && accountIds.contains(tx.creatorId) → creator only for all/send) diverges from confirmed visibility in multisig_creation_reconciliation.dart._showsMultisigCreationForFilter (all signers for all, non-creator for receive). Net effect: a non-creator signer sees the confirmed creation under "All"/"Receive" but never the pending one. Centralize this rule and reuse it for both pending and confirmed paths.

  • In-flight tracking is lost on app restart. pendingMultisigCreationsProvider is in-memory only and the polling lives in a transient service, so if the app is killed during the 5-minute poll window, the pending state and polling are gone with no startup reconciliation. The on-confirm reconciliation is solid, but consider persisting pending creations so they can resume after a restart.

Low / nits

  • Stubbed methods still wired into providers. discoverForUser, lookupByAddress, getOpenProposals/getPastProposals, propose/approve/cancel, the fee estimates, and _dummyCurrentBlock all return hardcoded dummy data and feed real UI (multisig details, proposals). Fine for this feature branch, but these must be replaced before any user-facing release.
  • multisig_submission_service.dart: extrinsicHash is computed but only logged. Also note the retry layering — SubstrateService.submitExtrinsic already retries 3× internally and _submitAndTrackBackground wraps it in another 3× retry (~9 attempts). Benign given the deterministic address, just worth being aware of.
  • MultisigSigner.hasApproved/isYou appear unused by the create flow (scaffolding for approve, presumably).

Nice work

  • predict_multisig_address in Rust with order-independence, golden-vector, and invalid-SS58 tests.
  • Preflight existence check before submit, and the _predictSeq sequence guard against async prediction races.
  • Localization (en/id) wired throughout, and consistent fail-loud logging on the reconciliation/submission paths.

@n13

n13 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Ok nonce in create is as follows

Go to create screen, create msig, add signers, at the final step when we send the create tx off to the chain, check first if the msig already exists, and if so, just increase the nonce, check again if already exists, etc... until we find a free nonce

The nonce is for this exact case, so we can make > 1 msig with the same signers

- make nonce calculated on the fly
- only show history for creator
@dewabisma

Copy link
Copy Markdown
Collaborator Author

Add checksum to predicted multisig address

image

@n13

n13 commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Done — reviewed the current head (7778abb) against the two new commits since the last pass. Here's my re-review. (I did this in a throwaway worktree and didn't post anything to GitHub.)

Re-review: Create multisig flow (PR #505)

Nice progress — the two new commits (a8381a9 improve creation, 7778abb checksum) cleanly resolve most of the previous feedback. Status of the prior points plus a couple of new things below.

Prior points — now resolved ✅

  • Nonce on the fly — exactly the behavior n13 described. New MultisigService.resolveMultisigCreationParams walks nonces, skipping reservedAddresses (local + pending) and on-chain hits, returning the lowest free (nonce, address). Plumbed through the screen → startMultisigCreation → extrinsic. Good unit tests for lowest-free, reserved-skip, and exhaustion.
  • Post-frame callbacks removed — both add_multisig_screen.dart (initState) and propose_amount_screen.dart now call the async methods directly. Matches the project rule.
  • DRY divergence in creation visibility — pending (filtered_all_transactions_provider) and confirmed (_showsMultisigCreationForFilter) now agree: creator-only for all/send, hidden for receive. The old all/receive vs send mismatch is gone. The Rust creator_id GraphQL split was removed and the supplemental fetchMultisigCreationsForAccounts/mergeMultisigCreations path deleted (with no dangling refs), so creations come purely from account_event. Much simpler.
  • Checksum on predicted address — added, full address no longer truncated.

Prior point — still open ⚠️ (was "functional concern")

Dedup guard in TransactionService.combineAndDeduplicateTransactions is still non-functional.

    otherTransfers.sort((a, b) => b.timestamp.compareTo(a.timestamp));
    for (final transaction in otherTransfers) {
      if (transaction is MultisigCreatedEvent) {
        seenMultisigAddresses.add(transaction.multisigAddress);
      }
      if (seenIds.add(transaction.id)) {
        result.add(transaction);
      }
    }

seenMultisigAddresses is written here and in the pending loop, but .contains is never checked — it's dead. Pending id (pending_multisig_<addr>) and confirmed id (ae-multisig-<addr> / indexer id) differ, so seenIds won't suppress the duplicate. In practice the poller removes the pending row before injecting the confirmed one, so it's usually hidden — but a normal activity refresh can surface the confirmed account_event while the pending row is still present (up to the 5s poll interval), giving a brief double-render. Fix: make the guard real, e.g. if (transaction is MultisigCreatedEvent && !seenMultisigAddresses.add(transaction.multisigAddress)) continue; (or just delete the misleading set if you're relying on remove-before-insert).

Prior point — still present (acknowledged) 🔁

ngrok endpoints in quantus_sdk/lib/src/constants/app_constants.dart (just rotated to new tunnels, still single RPC). Per the PR description this branch isn't bound for main, so just flagging again so it doesn't slip through.

New issues introduced by the latest commits

  1. Checksum/predicted-address disappears on unrelated rebuilds (regression, 7778abb). In _PredictedAddressSectionState.didUpdateWidget, the else branch clears the checksum on every parent rebuild where the address didn't change:
  @override
  void didUpdateWidget(covariant _PredictedAddressSection oldWidget) {
    super.didUpdateWidget(oldWidget);

    final isAddressChanged = widget.address != oldWidget.address;
    final hasAddress = widget.address != null;

    if (isAddressChanged && hasAddress) {
      ref.read(humanReadableChecksumServiceProvider).getHumanReadableName(widget.address!).then((name) {
        if (mounted) setState(() => _checksum = name);
      });
    } else {
      setState(() => _checksum = null);
    }
  }

The parent does _accountName.addListener(() => setState(() {})), so every keystroke in the name field rebuilds this section with the same address → isAddressChanged == false_checksum is nulled and never recomputed. Since the display requires _checksum != null, the whole address block reverts to the placeholder as soon as the user edits the (very editable) name. Guard the clear with isAddressChanged:

if (isAddressChanged) {
  if (hasAddress) { /* fetch */ } else { setState(() => _checksum = null); }
}

(Also: the .then() has no seq guard, so rapid signer/threshold edits can apply a stale checksum to a newer address — minor.)

  1. Silent failures in _refreshPredictedAddress (against your fail-loud rule). Both the on MultisigNonceExhaustedException and the generic catch (_) branches reset to placeholder with no logging and no user-facing message — the create button just silently disables. Worth at least a quantusDebugPrint, and ideally a toast for nonce-exhaustion.

Things to confirm (not necessarily bugs)

  • Indexer account_event emission for creations. The send/all visibility and the now-unconditional isSend: true / amount: tx.creationFee rendering in tx_item.dart (and the removed creator checks in transaction_detail_sheet.dart) assume the creation account_event is emitted only for the creator. The query swapped the explicit creator_id: {_in: accounts} clause for {multisig_id: {_is_null: false}}. If the indexer ever emits that row for non-creator signers too, they'd see the creation under send/all and the fee would be misattributed to them. Just confirm the indexer is creator-only here.

Nits

  • _AddMultisigScreenState holds a directly-constructed HumanReadableChecksumService() (_checksumService) for the creator checksum while _PredictedAddressSection uses ref.read(humanReadableChecksumServiceProvider) — two ways to get the same service; pick one.
  • submitCreateMultisigExtrinsic retry (3×) still wraps SubstrateService.submitExtrinsic's internal 3× (~9 attempts), as noted last time — benign given the deterministic address.
  • Stub methods (discoverForUser, lookupByAddress, proposals, fees, _dummyCurrentBlock) still feed real UI — fine for this branch, must go before any release.

Verdict

The blocking nonce concern is well addressed and the visibility logic is now consistent and simpler — solid iteration. Before this merges anywhere user-facing I'd want: (1) the _checksum clear-on-rebuild fix, (2) the real dedup guard, plus the ngrok/stubs cleanup. The indexer-emission question is the one thing worth a quick confirmation since several display assumptions now rest on it.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

Fix All in Cursor

❌ 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 5bae0b8. Configure here.

result.add(creation);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate multisig activity rows

Medium Severity

combineAndDeduplicateTransactions dedupes only by id, but pending multisig rows use pending_multisig_{address} while confirmed rows use ae-multisig-{address} (or indexer ids). After refresh, activity can show both “Creating multisig” and “Multisig created” for the same address.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5bae0b8. Configure here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will not happen because we remove the pending before we add the success one.

maxRetries: maxRetries,
attempt: attempt + 1,
);
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submit skips nonce re-check

Medium Severity

Multisig creation checks whether the predicted address exists only once before enqueueing the extrinsic. The background submit always uses that fixed nonce and retries the same call, with no loop to pick the next free nonce at send time if the address was taken after preview.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5bae0b8. Configure here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's how it suppose to be. We don't have same wallet used by two different or more users at the same time.

counterpartyAddr: address,
customIcon: Icons.groups_outlined,
counterpartyDirectionLabel: l10n.activityTxMultisigLabel,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-creator sees creation cost

Low Severity

For MultisigCreatedEvent, activity rows always show totalCost, but the detail sheet hides the amount when creatorId is not the active account. Co-signers can see the creator’s fees in the list while the detail view shows “—”.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5bae0b8. Configure here.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: PR #505 — Create multisig flow

Scope of this review. Done as a static read in an isolated throwaway worktree (detached HEAD at head commit 5bae0b8b), diffed against the actual base branch origin/multisig-implementation (not main), since this is a stacked PR. flutter analyze / tests were not run — the worktree lacks .dart_tool and generated FRB bindings — so all findings below come from reading the code.

Diff size: 74 files, +5,869 / −567. The bulk is generated/localization (l10n/*, frb_generated*, Cargo.lock); the substantive logic is the new submission/polling/reconciliation services, the SDK MultisigService additions, the Rust address-prediction API, the new event models, and the rewritten add_multisig_screen.dart.


What's good

  • Local auth gates signing. add_multisig_screen._createMultisig calls LocalAuthService().authenticate(...) before startMultisigCreation (which signs/submits). Correct ordering.
  • Address derivation looks consistent. quantus_sdk/rust/src/api/multisig.rs derives hash(pallet_id || sorted_signers || threshold || nonce) with threshold: u32 / nonce: u64, matching the generated call encoding (U32Codec / U64Codec in pallet_multisig/pallet/call.dart) and SS58 prefix 189. Covered by order-independence, golden-vector, and invalid-SS58 tests.
  • Collision avoidance. resolveMultisigCreationParams picks the lowest free nonce (skipping reserved + on-chain addresses), and startMultisigCreation re-checks isMultisigOnChain and throws MultisigAlreadyExistsException before submitting.
  • Concurrency hygiene in the poller. _inFlight guards overlapping searches; timers are cleaned up on confirm/timeout/dispose.
  • Reasonable unit coverage for defaultThreshold, signer/threshold validation, nonce resolution, GraphQL parsing, and the Rust derivation.

Blocking

B1 — Production endpoints replaced with personal ngrok tunnels

quantus_sdk/lib/src/constants/app_constants.dart:15-16

static const List<String> rpcEndpoints = ['https://649d-60-51-184-252.ngrok-free.app'];
static const List<String> graphQlEndpoints = ['https://1798-60-51-184-252.ngrok-free.app/v1/graphql'];

The diff replaces the real production RPC/indexer hosts (a1/a2-planck.quantus.cat, matcha-latte.quantus.com, sub2.quantus.com) with a developer's ephemeral ngrok URLs. This affects the entire app, not just multisig, and would break every user / route all wallet traffic through a personal tunnel. This must never reach main. Revert to the production endpoints before merge.


Medium

M1 — On-chain extrinsic result is never verified; real failures surface as a generic "timeout"

mobile-app/lib/services/multisig_submission_service.dart:90-99, mobile-app/lib/services/multisig_creation_polling_service.dart:48-79

submitCreateMultisigExtrinsic returns once the tx is accepted into the pool (author_submitExtrinsic is fire-and-forget); the computed extrinsicHash is logged and then discarded. Confirmation is then inferred purely from indexer presence (isMultisigOnChain). A create that is included but reverts (e.g. insufficient deposit/fee, pallet error) is indistinguishable from a slow indexer — it just polls for 5 minutes and emits a generic timeout toast, after the fee has already been spent. This violates "fail early / no silent failure": a definite on-chain failure is reported to the user as an ambiguous timeout. Consider watching the extrinsic result (or querying the extrinsic's success/events by the hash you already compute) so a reverted create is reported distinctly.

M2 — No pre-submission balance check

mobile-app/lib/services/multisig_submission_service.dart:23-63, add_multisig_screen.dart:203-247

The flow computes networkFee and knows palletFee + deposit (PendingMultisigCreationEvent.totalCost), but never checks the creator can actually afford totalCost before authenticating + signing + submitting. An underfunded creator signs, submits, pays/loses fees, and then hits the same silent 5-minute timeout (M1). A balance vs. totalCost preflight (failing fast with a clear message) would close this.

M3 — In-flight creation state is not durable across app restart

mobile-app/lib/providers/pending_multisig_creations_provider.dart, mobile-app/lib/services/multisig_creation_polling_service.dart

pendingMultisigCreationsProvider (in-memory StateNotifier) and the poller's _timers are process-local. If the app is killed during the up-to-5-minute polling window, polling never resumes, the pending/toast UI is lost, and the local MultisigAccount (name + metadata) is only persisted after confirmation in _search. Recovery would depend on MultisigService.discoverForUser, which is still a dummy stub. Net effect: a multisig can be created on-chain while the local record is permanently lost. Persisting the pending draft and resuming polling on launch would fix this.

M4 — Silent default/zero substitutions mask bad data (fail-early violations)

  • quantus_sdk/lib/src/models/multisig_created_event.dart:91-92threshold = rawThreshold != null && rawThreshold >= 1 ? rawThreshold : 1. A missing/invalid indexer threshold is silently rendered as 1, so e.g. a 3-of-5 displays as 1-of-5. Prefer failing/logging over substituting a security-relevant default.
  • multisig_created_event.dart:111-119 _networkFeeFromGraphql returns BigInt.zero when no fee field is present — a silent zero for a money value.
  • multisig_created_event.dart:41-68 MultisigCreatedEvent.fromDraft defaults networkFee to 0. It is invoked by multisig_creation_reconciliation.dart:44-55 _loadCreatedEvent as the fallback when the indexer fetch fails, so the history row then understates totalCost (network fee shown as 0). The fetch error is logged, but the degraded value is shown silently.

M5 — DRY: PendingMultisigCreationEvent and MultisigCreatedEvent are near-duplicates

quantus_sdk/lib/src/models/pending_multisig_creation_event.dart vs quantus_sdk/lib/src/models/multisig_created_event.dart

Both declare the same eight fields (creatorId, multisigAddress, threshold, nonce, signers, palletFee, networkFee, deposit), the same totalCost getter, the same isCreator, and essentially identical fromDraft bodies (creator resolution, pallet constants, networkFee ?? zero, amount = sum). This duplicates non-trivial accounting logic. Extract a shared base class / mixin (or a single model with a pending|created status) so the cost computation lives in one place.


Nits

  • N1 — Nested retry amplification. _submitAndTrackBackground retries up to 3× and each call goes through SubstrateService.submitExtrinsic, which itself retries up to 3× (≈9 submit attempts). Duplicate accounts are prevented (deterministic address + pallet duplicate-rejection + preflight), but a broadcast-then-RPC-error case could still cost an extra failed-tx fee. Consider not stacking both retry layers for this signed extrinsic.
  • N2 — Error conflation in prediction. add_multisig_screen._refreshPredictedAddress (:182-200) collapses MultisigNonceExhaustedException and arbitrary network errors into the same "no predicted address" state (only quantusDebugPrint); the user can't distinguish "indexer unreachable" from "no nonce available."
  • N3 — Toast channel. multisigCreationToastProvider is a single global StateProvider<...?>; concurrent completions can overwrite one another, and an event emitted while no MultisigCreationToastListener is mounted is missed (ref.listen only fires on change).
  • N4 — Surrounding multisig ops remain stubs. discoverForUser, lookupByAddress, getOpen/PastProposals, propose/approve/cancel still return dummy data. Fine if intentional for this stacked PR, but note the create flow's only recovery path (M3) depends on the stubbed discoverForUser.

Verdict

Request changes. The ngrok endpoint swap (B1) is a hard blocker that must not merge to main, and there are several medium correctness/robustness issues (no extrinsic-result verification, no balance preflight, non-durable in-flight state, silent default/zero substitutions) plus a DRY duplication between the two event models. The core design — predicted-address preflight, auth-before-sign, and the Rust/runtime-consistent address derivation — is sound; addressing B1 and the mediums would get this to a good place.

(Static review only — analyze/tests not run; see scope note above.)

@n13

n13 commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

B1 should fix (just use normal endpoints)
M2 - M5 should fix
M1 is normal we do this with all tx, its ok

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: PR #505 — Create multisig flow

Scope. Static read in an isolated throwaway worktree at the new head 32a694e8, diffed against the actual base branch origin/multisig-implementation (not main). flutter analyze/tests were not run (no .dart_tool/FRB bindings), but I confirmed the new symbols referenced (queryBalance, the multisigCreateInsufficientBalance l10n key, MultisigAccount.toJson/fromJson) exist so the changes should compile.

This pass focuses on the 5 commits since my last review (5bae0b8b..32a694e8): fix: preflight balance check, fix: DRY violation, fix: silent or masking bad data, fix: persisting pending multisig creation, chore: formatting.


Status of previously-raised issues

✅ B1 — ngrok endpoints (blocker) — RESOLVED

quantus_sdk/lib/src/constants/app_constants.dart is back to the production hosts (a1/a2-planck.quantus.cat, matcha-latte.quantus.com, sub2.quantus.com/v1/graphql), and the 3-endpoint RPC failover list is restored. The blocker is gone.

✅ M2 — No pre-submission balance check — RESOLVED

MultisigSubmissionService._runCreationPreflight now queries the creator balance (substrateService.queryBalance) and throws MultisigInsufficientBalanceException when balance < totalCost (pallet fee + network fee + deposit) before auth/sign. The screen handles it with a clear toast (multisigCreateInsufficientBalance). Good.

✅ M3 — In-flight state not durable across restart — RESOLVED

Solid implementation: new PendingMultisigCreationRecord (with toJson/fromJson) persisted to SharedPreferences; the notifier loads on construction and re-saves on add/remove/update/clear; resumePendingCreations() (triggered via multisigCreationPollingServiceProvider from AppInitializer) resumes polling for non-expired records and runs _recoverExpired (on-chain check → confirm or drop) for expired ones. submittedAt is persisted so the timeout window survives a restart, and the preflight networkFee is persisted so the reconcile fallback no longer understates cost. Nicely covered by pending_multisig_creation_record_test.dart.

✅ M4 — Silent default/zero substitutions — RESOLVED

  • threshold now parses via positiveIntFromJson(..., 'threshold') which throws on missing/<1 instead of silently defaulting to 1.
  • _networkFeeFromGraphql now throws FormatException('Missing network fee …') instead of returning BigInt.zero.
  • fromDraft networkFee is now a required BigInt (no more ?? BigInt.zero), and the reconciliation fallback (_loadCreatedEvent) passes the real preflight networkFee, with the indexer-unavailable catch now logging the stack trace too. Matches the fail-loud rule.

✅ M5 — Duplicate event models (DRY) — RESOLVED

New abstract MultisigCreationEvent base holds the 8 shared fields + totalCost + isCreator; MultisigCreatedEvent and PendingMultisigCreationEvent both extend it, and MultisigCreationDraftFields.fromDraft centralizes draft→fields resolution. The duplicated accounting is gone.

🔁 M1 — Extrinsic result not verified (failures surface as timeout) — ACKNOWLEDGED

Per your note this is the established pattern for all txs in the app and is intentional. Not holding it against this PR.

🔁 ngrok / stubs (N4) — as expected

discoverForUser, lookupByAddress, proposals/fees remain stubs — fine for this stacked branch; must be replaced before any user-facing release.


New minor items (none blocking)

  • N-a (low) — Preflight runs twice per creation. add_multisig_screen._createMultisig calls preflightMultisigCreation(...) (pre-auth), and then startMultisigCreation(...) runs _runCreationPreflight(...) again. That doubles predictMultisigAddress + isMultisigOnChain + getFeeForCall + queryBalance (several round-trips) on every attempt. Correctness is fine (the second pass re-checks, and both exceptions are handled), but consider threading the first preflight's result through to startMultisigCreation to avoid the duplicate network work.
  • N-b (nit) — Dead helpers. After switching to notifier.add(...)/notifier.clear() directly, the free functions addPendingMultisigCreation and clearPendingMultisigCreations in pending_multisig_creations_provider.dart are now unused. Remove them (keeps it minimal; removePendingMultisigCreation is still used).
  • N-c (nit) — _recoverExpired swallows transient errors. If isMultisigOnChain throws, the catch only logs and the expired record stays persisted with no retry until the next app launch. Acceptable, but worth a comment/Telemetry given the fail-loud rule.
  • N-d (nit, previously raised) — transient double-render. Good change: the misleading dead seenMultisigAddresses set was removed from combineAndDeduplicateTransactions (now id-only). The residual race remains — a normal activity refresh during the ≤5s window between the indexer surfacing ae-multisig-<addr> and the poller removing pending_multisig_<addr> can briefly show both. You've assessed this as acceptable given remove-before-insert; flagging only for awareness.
  • N1 (previously raised) — nested retry amplification (_submitAndTrackBackground 3× over submitExtrinsic's 3×) is unchanged; benign given the deterministic address + duplicate rejection.

Verdict

Approve. The blocker (B1) and every medium (M2–M5) from the prior review are properly addressed, M1 is acknowledged as intended, and the persistence/recovery work is well done and tested. Remaining items are nits (chiefly the duplicated preflight and two dead helpers) that can be cleaned up in passing. Core design — auth-before-sign, predicted-address preflight, balance preflight, and the Rust/runtime-consistent address derivation — is sound.

(Static review only — analyze/tests not run; see scope note.)

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Great work!

* 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
@dewabisma dewabisma merged commit 9d26876 into multisig-implementation Jun 9, 2026
dewabisma added a commit that referenced this pull request Jun 12, 2026
* msig implementation 1

* linter fixes and package updates

* Create multisig flow (#505)

* 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

* Sign proposal flow (#510)

* 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

* Execute proposal flow (#511)

* 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

* Cancel proposal flow (#512)

* 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

* Multisig proposal polling (#513)

* 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

* fix: code review issues

---------

Co-authored-by: Nikolaus Heger <nheger@gmail.com>
@dewabisma dewabisma deleted the beast/create-multisig-flow branch June 22, 2026 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants