Discover multisig flow

[dewabisma]

Summary

• Adding discover multisig option to add account menu
• Discover multisig screen

Screenshots

• Add account menu

• Discover multisig screen

---

Note

Medium Risk
Replaces stub discovery with live GraphQL and stricter indexer parsing, which can surface new errors or empty results; multisig account state changes on logout affect wallet session boundaries.

Overview
Adds a dedicated Discover Multisig path from Add Account (new menu row → DiscoverMultisigScreen) so users can find on-chain multisigs where their wallet accounts are signers and add them to the app. Create Multisig stays on its own screen; the paste SS58 address flow and related copy are removed in favor of discovery-only add.

SDK: MultisigService.discoverForUser is wired to a real Hasura GraphQL discover query (replacing stub/dummy data); lookupByAddress and multisigLookupProvider are dropped. Indexer parsing is stricter—signers/threshold must be valid (no silent defaults)—with shared helpers in json_dynamic_parse.dart and a shared _postGraphQl path.

State / lifecycle: Multisig list mutations always update provider state (not only when already loaded); multisigAccountsProvider gets reset() and logout invalidates discovery. Wallet import is documented/clarified as HD-only; multisigs are added via Discover Multisig.

^{Reviewed by Cursor Bugbot for commit 6ffead9. Configure here.}

[n13]

Looks good

I would suggest to show the human checkphrase on all multisigs, so we can easily distinguish them, talk about them etc

[dewabisma]

Added checksum

[n13]

I've reviewed the full diff plus the surrounding code on the PR branch. Here's my assessment.

Overall

Solid PR that cleanly swaps the multisig discovery stub for a real GraphQL-backed flow, removes the now-dead "add from address" path, wires logout cleanup, and adds good unit-test coverage for the mapping/query-building helpers. The localization, menu entry, and provider changes all hang together and there are no leftover references to the removed symbols (lookupByAddress, multisigLookupProvider, multisigAddPasteAddressSection, etc.) — I verified that. All l10n keys used by the new screen exist in app_en.arb. Nice work, especially the test coverage and the logout reset.

A few things worth addressing before merge:

High priority — checksum can render against the wrong row

In discover_multisig_screen.dart, each row fetches its human-readable checksum once in initState and stores it in State:

itemBuilder: (context, index) {
  final account = sorted[index];
  return _DiscoverMultisigRow(account: account, ...); // no key
}

_DiscoverMultisigRow has no Key, and _sortedDiscovered pushes added items to the bottom. So after you tap Add, savedIds changes → the list reorders → Flutter reuses the existing _DiscoverMultisigRowState objects by position. initState won't re-run, so _checksum (held in State) stays with the old position while the address (read from widget.account) updates — you end up showing one multisig's checkphrase next to another's address. That defeats the whole point of the checksum that n13 asked for.

Fix is a one-liner — give the row a stable identity:

return _DiscoverMultisigRow(
  key: ValueKey(account.accountId),
  account: account,
  ...
);

(Alternatively handle didUpdateWidget to refetch when accountId changes, but the key is cleaner.)

Medium — DRY: duplicated GraphQL request/error plumbing

discoverForUser (lines ~40–49) and the existing fetchMultisigFromIndexer (lines ~135–144) now contain byte-for-byte identical post → status-check → decode → errors-check blocks. Given the strict DRY guideline, these two in MultisigService should share a private helper, e.g.:

Future<Map<String, dynamic>?> _postGraphQl({required String query, Map<String, dynamic>? variables}) async {
  final body = {'query': query, if (variables != null) 'variables': variables};
  final response = await _graphQlEndpointService.post(body: jsonEncode(body));
  if (response.statusCode != 200) {
    throw Exception('GraphQL request failed with status: ${response.statusCode}. Body: ${response.body}');
  }
  final responseBody = jsonDecode(response.body) as Map<String, dynamic>;
  if (responseBody['errors'] != null) {
    throw Exception('GraphQL errors: ${responseBody['errors']}');
  }
  return responseBody['data'] as Map<String, dynamic>?;
}

Heads up: this exact pattern is also duplicated in account_discovery_service.dart, chain_history_service.dart (4×), and taskmaster_service.dart — so it's a pre-existing codebase-wide smell, not something this PR introduced. Collapsing the two copies within MultisigService is in-scope and easy; a shared GraphQlEndpointService helper for the rest could be a good follow-up.

Medium — silent fallbacks vs. "fail early"

In multisigAccountFromIndexerRecord:

final signers = signersRaw is List ? signersRaw.map((e) => e.toString()).toList() : <String>[];
final rawThreshold = record['threshold'] as int?;
final threshold = rawThreshold != null && rawThreshold >= 1 ? rawThreshold : 1;

Per the fail-early rule, malformed indexer records (missing/empty signers, missing/invalid threshold) should throw rather than silently degrade to an empty signer list or threshold = 1. A multisig with no signers or a fabricated threshold is bad data we'd rather surface immediately than persist into the user's account list.

Relatedly, threshold is read via a raw as int? cast while nonce goes through bigIntFromJson (which tolerates String/int). If the indexer ever serializes threshold as a string (Hasura does this for bigint/numeric scalars), the cast throws a CastError. Worth adding an intFromJson helper in json_dynamic_parse.dart (mirroring bigIntFromJson, or generalizing the int/num/String logic already in blockHeightFromJsonMap) and using it for consistency.

Low — GraphQL value interpolation vs. variables

buildDiscoverQuery inlines account IDs into the query string with a hand-rolled _escapeGraphqlString, whereas the existing byPkQuery uses a proper $id: String! variable. SS58 addresses are alphanumeric so real injection risk is ~nil, but using variables (e.g. pass the where/bool_exp or the id list as a variable) would be safer and consistent with the existing pattern, and lets you drop the custom escaping. Not a blocker. The _or of single-element _contains logic itself is correct for "any of my accounts is a signer".

Minor / nits

• Unhandled async in initState: the checksum .then(...) has no .catchError. If it throws, you get an unhandled async error and the row is stuck on commonLoading ("Loading…") forever — a silent failure per your rule. Worth a .catchError that logs and shows a fallback. (Same pattern exists elsewhere, e.g. add_multisig_screen.dart, so consistency-wise it's at least not new.)
• Dead name: discoverForUser builds name: 'Multisig $index', but the screen never displays it (it shows the checksum) and overwrites it on add via multisigCreateDefaultName(savedCount + 1). The index-based name is effectively unused — consider dropping it to avoid confusion.
• Over-fetch: discoverFields = _coreFields pulls timestamp and block { height hash }, which the discover mapping ignores. Harmless, just slightly more payload.
• Loading bridge: in discoveredMultisigsProvider, the AsyncLoading branch calls accountsServiceProvider.getAccounts() directly while also watching accountsProvider, so first load can fetch accounts twice. Minor, and the rethrow on AsyncError is a nice improvement.
• Logout re-population: MultisigAccountsNotifier.reset() sets state to data([]) but the provider isn't autoDispose, so confirm the list actually reloads from settings after switching wallets (vs. staying empty until restart). It mirrors the existing accountsProvider reset pattern, so likely fine — just worth a sanity check.
• Good: mounted checks are present and no post-frame callbacks were used. 👍

I have not posted anything to GitHub. Want me to drop this in as a PR review comment, or apply the high-priority ValueKey fix (and optionally the _postGraphQl DRY extraction) directly?

[cursor]

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 6ffead9. Configure here.}

[cursor]

Multisig state clobbered on add

High Severity

add, updateName, and remove now rebuild in-memory state from state.value ?? [] after persisting. While the notifier is still loading (or in error), value is null, so updates replace the list with a single change or an empty list. A finishing _load can also overwrite newer saves with a stale snapshot, hiding multisigs in the UI until restart.

Additional Locations (1)

• mobile-app/lib/providers/multisig_providers.dart#L40-L42

 

^{Reviewed by Cursor Bugbot for commit 6ffead9. Configure here.}

[cursor]

Discover ignores saved multisig load

Medium Severity

The screen treats multisigAccountsProvider as empty while it is still loading (value ?? []), so already-saved multisigs show an active Add button and default naming uses a zero count. Users can trigger duplicate add attempts and misleading labels until the provider finishes loading.

Additional Locations (1)

• mobile-app/lib/v2/screens/multisig/discover_multisig_screen.dart#L29-L31

 

^{Reviewed by Cursor Bugbot for commit 6ffead9. Configure here.}

[cursor]

One bad row fails discovery

Medium Severity

discoverForUser maps each indexer record without isolating parse failures. A single malformed signers, threshold, or id field throws from multisigAccountFromIndexerRecord and fails the entire discover query instead of skipping that row.

 

^{Reviewed by Cursor Bugbot for commit 6ffead9. Configure here.}

[cursor]

Strict multisig parse breaks history

Medium Severity

MultisigCreatedEvent.fromMultisigGraphql now throws on missing or invalid signers/threshold, but ChainHistoryService.tryParseOtherTransferEvent still calls it without catching. One bad multisig account event can abort the whole other-transfers fetch instead of being skipped.

 

^{Reviewed by Cursor Bugbot for commit 6ffead9. Configure here.}

[n13]

Review: PR #506 — Discover multisig flow

Review method: Static read-only review in an isolated throwaway git worktree, diffed against the PR's actual base branch beast/create-multisig-flow (not main), since this is a stacked PR.

git diff origin/beast/create-multisig-flow...HEAD --stat
16 files changed, +666 / -158

The worktree lacks .dart_tool / generated FRB bindings, so this is a static review only — no build, analyzer, or test execution was performed.

---

Summary

This PR replaces the stubbed multisig discovery with a real Hasura GraphQL discover query (filtered by the user as a signer), tightens indexer parsing to fail loudly, drops lookupByAddress / multisigLookupProvider and the paste-SS58 add flow, and wires multisig state into logout invalidation. The change is well-scoped, well-tested, and aligns strongly with the repo's "fail early" philosophy. I found no security issues, no dangling references, and no confirmed correctness bug in the normal operating path. There are two non-blocking medium design/DRY considerations and a few nits worth addressing before merge.

---

What's great

• Real discover query with good unit coverage. MultisigGraphql.discoverQuery / buildDiscoverVariables (quantus_sdk/lib/src/services/multisig_graphql.dart:60-95) are covered by tests for the query shape, single vs multi-account _or variables, and empty-input ArgumentError (quantus_sdk/test/multisig_service_test.dart:164-1197). Mapping helpers (parseMultisigDiscoverData, multisigAccountFromIndexerRecord, resolveMyMemberAccountId) are also unit-tested, including the throw-on-malformed paths.
• Stricter parsing genuinely fails loudly. intFromJson, nonEmptyStringListFromJson, and multisigThresholdFromJson (quantus_sdk/lib/src/models/json_dynamic_parse.dart:35-59) throw FormatException on missing/invalid/empty/out-of-range values rather than silently defaulting. MultisigCreatedEvent.fromMultisigGraphql (quantus_sdk/lib/src/models/multisig_created_event.dart:80-106) now uses them, replacing the old threshold ?? 1 / signers ?? [] silent defaults — and the tests were updated to assert the throws (quantus_sdk/test/chain_history_service_test.dart:49-92). This is exactly the "fail early, no silent defaults" behavior the repo rules call for.
• Intra-service DRY improvement. _postGraphQl (quantus_sdk/lib/src/services/multisig_service.dart:188-201) consolidates the POST + status-code + errors + data handling, removing the inline duplicate previously in fetchMultisigFromIndexer and sharing it with the new discover path.
• Logout now clears multisig state. logout_service.dart:34-35 adds multisigAccountsProvider.notifier).reset() and invalidate(discoveredMultisigsProvider). This fixes a real cross-session leak (previously, saved multisigs from a prior wallet were never cleared on logout).
• Mutations no longer silently dropped. add / updateName / remove now operate on state.value ?? [] instead of state.whenData(...) (mobile-app/lib/providers/multisig_providers.dart:27-50). Previously, mutating while the notifier was in loading/error was a no-op; now it always applies.
• Clean removal. No dangling references remain to lookupByAddress, multisigLookupProvider, _dummyMultisigs, or the removed l10n keys (multisigAddPasteAddressSection, multisigAddAddressHint, multisigAddFromAddressButton) — verified by repo-wide search.
• Discover provider edge handling. discoveredMultisigsProvider (mobile-app/lib/providers/multisig_providers.dart:71-87) rethrows on AsyncError (fail-early) and, while accounts are still loading, fetches them directly instead of returning a misleading empty result. ListView rows use ValueKey(account.accountId) so reordering is correct.

---

Medium (non-blocking — design / DRY)

M1. Stricter parsing widens the failure blast radius in activity history

MultisigCreatedEvent.fromAccountEvent is invoked by ChainHistoryService.tryParseOtherTransferEvent (quantus_sdk/lib/src/services/chain_history_service.dart:431-433), which runs inside _pageFromEvents without a per-event try/catch (chain_history_service.dart:385-398), and fetchOtherTransfers rethrows on any exception (chain_history_service.dart:571-577).

Consequence: now that parsing throws on an invalid threshold/empty signers, a single malformed multisig row will abort the entire "other transfers" history page, rather than skipping that one row. Pre-PR it would have rendered the row with threshold=1/empty signers (wrong, but non-fatal).

In practice this only triggers on genuine indexer corruption / schema regression (on-chain creation guarantees valid threshold/signers), and failing loudly on that is consistent with the repo's philosophy — so this is a deliberate tradeoff, not a bug. But please make it a conscious one: consider isolating per-event parsing in _pageFromEvents (skip + log the bad row loudly) so one corrupt record can't take down the whole feed, while still surfacing the error.

M2. _postGraphQl duplicates a GraphQL-POST pattern that exists across many services (DRY)

_postGraphQl is a good consolidation within MultisigService, but the identical "POST → check statusCode != 200 → check responseBody['errors'] → return data" boilerplate is repeated across the SDK: chain_history_service.dart (×4, e.g. :559-566), account_discovery_service.dart:59-65, taskmaster_service.dart:558-564, wormhole_utxo_service.dart. The natural shared home already exists: GraphQlEndpointService (quantus_sdk/lib/src/services/network/redundant_endpoint.dart:18, post() at :108).

This duplication is mostly pre-existing and not introduced by this PR (the PR is net-positive on DRY locally), so I'm not blocking on it. But per the repo's strict DRY rule, the cleanest move is to lift _postGraphQl into GraphQlEndpointService (e.g. a query(...) helper) and have all callers reuse it — ideally as a small follow-up rather than re-adding yet another private copy.

---

Nits / low severity

• reset() diverges from the established convention. MultisigAccountsNotifier.reset() sets AsyncValue.data([]) (mobile-app/lib/providers/multisig_providers.dart:59-61), whereas every sibling (AccountsNotifier, ActiveAccountNotifier, AccountAssociationsNotifier) resets to AsyncValue.loading() (account_providers.dart:51-53,88-90). Since these providers are re-created on the next login (they watch settingsServiceProvider / accountsServiceProvider), this is effectively cosmetic — but matching the loading() convention avoids a subtle stale-empty window and keeps the codebase consistent.
• buildDiscoverVariables single-vs-multiple branch is unnecessary (multisig_graphql.dart:79-92). The _or form handles a single account id correctly, so the special case adds branching for no functional gain. Harmless and tested, but could be one code path.
• Discover-time name is dead. discoverForUser passes name: 'Multisig $index' (multisig_service.dart:56), but the name is always overwritten on add via multisigCreateDefaultName(savedCount + 1) (discover_multisig_screen.dart:30-32), and the row UI shows the checksum, not account.name. The index counter / name argument are effectively unused output.
• Hardcoded English fallback string. _DiscoverMultisigRow sets _checksum = 'Error getting checksum' on checksum-lookup failure (discover_multisig_screen.dart), which is not localized like the rest of the screen. It does log via quantusDebugPrint first (so fail-early is satisfied), but the user-visible string should come from l10n.
• Effectively-unreachable defensive code. In discoverForUser, the seen dedup and the if (myMember == null) continue are unreachable in practice (Hasura returns distinct rows, and the where clause already guarantees one of myAccountIds is a signer, so resolveMyMemberAccountId will always match). Harmless, but the silent continue mildly conflicts with the "no silent drops" rule if it ever did fire — a debug log there would be more in keeping with the repo style.

---

Things to verify outside static review

• SS58 format parity. The discover _contains filter and resolveMyMemberAccountId rely on the local account.accountId strings being byte-identical to the indexer's signers entries (same SS58 prefix/encoding). If the indexer stores a different format, discovery silently returns empty. Worth confirming against the indexer schema. Also confirm signers is a Postgres array column so _contains maps to @> membership as intended.
• Tests/analyzer. multisig_service_test.dart and chain_history_service_test.dart additions look correct by inspection; please confirm flutter test + dart analyze are green in CI (not runnable in this worktree).

---

Verdict

Comment (approve with suggestions). No blocking correctness or security issues and no dangling references; the discover query, strict parsing, and logout wiring are solid and well-tested. The two medium items (activity-history failure blast radius from the stricter parse, and lifting the duplicated GraphQL-POST helper into GraphQlEndpointService) are design/DRY improvements rather than bugs and can be addressed here or as a fast follow-up; the reset() convention mismatch and other nits are minor.

[n13]

Looking great, GTG!