Discover multisig flow

[dewabisma]

Summary

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

Screenshots

• Add account menu

• Discover multisig screen

[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?