Repository navigation
Conversation
|
Fixer loop does not support PRs from forks. Stopping. Use the bot-review label for a one-shot review. |
|
Closing - already done in #715 |
n13
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Reviewer: GPT-6.1 Sol
Reviewed head f82f3d5990332908ae40f72e22670886d8f88e0b against base 29277c8ed5bb62b4853c800e082ae168fa8e7dd6.
-
[P1] Add the localization key to the ARB sources before regenerating. mobile-app/lib/l10n/app_localizations.dart:681-685:
multisigCreateCannotUseMultisigwas added only to the three generated Dart files. Neitherapp_en.arbnorapp_id.arbcontains it. The app enablesflutter: generate: true, so localization generation removes this getter whileadd_multisig_screen.dart:119still references it, breaking compilation. Reproduced by runningflutter gen-l10non copies of this head's localization inputs, then compiling a Flutter test that accesses the new getter:The getter 'multisigCreateCannotUseMultisig' isn't defined. Add both ARB messages and the English metadata, then regenerate the Dart files. -
[P2] Prevent creation while an added signer is still being checked. mobile-app/lib/v2/screens/multisig/add_multisig_screen.dart:112-115:
_isAddingSignerdisables only Add Signer; the Create button's_isDisabledcondition ignores it. With an existing ready two-signer configuration, start adding a third signer and tap Create before the lookup completes._createMultisigcaptures the old signer list and threshold in its preflight; when the lookup completes, the screen adds the third signer and updates the displayed threshold while submission still uses the earlier draft. For the default thresholds, the screen can show 2-of-3 while creation submits 1-of-2. Include the pending signer check in the creation guard and serialize form changes with submission. Add a regression using a delayed signer lookup and delayed creation preflight.
Validation: inspected the full diff, both signer-entry callbacks, indexer lookup, and creation/submission paths. The isolated localization regeneration/compilation probe reproduced finding 1. Formatting checks fail on add_multisig_screen.dart locally and in GitHub CI; git diff --check also reports trailing whitespace at line 129. Local locked bootstrap is blocked by installed Flutter 3.44.4 versus the repository's pinned 3.47.7; the existing focused SDK test could not execute with dependencies unavailable, and workspace analysis was stopped at the 10-second limit. Finding 2 is based on tracing the asynchronous state and preflight arguments, not an executed widget regression.
The PR was open when review began and was closed before submission; this review covers the recorded head above.
What and why
Fixes a bug where a user could add another multisig account as a signer when creating a new multisig. Because a multisig has no private key, it can never sign a transaction. Such a multisig becomes permanently unexecutable — the funds get stuck with no way to move them out through the wallet.
This was reported on-chain; the user had to recover funds manually through the CLI.
Changes
add_multisig_screen.dart:_addSigneris nowFuture<void>and queries the chain viaMultisigService.isMultisigIndexed(address)after passing SS58 and duplicate checks.multisigCreateCannotUseMultisigand the signer is not added._isAddingSignerflag so the "Add Signer" button is disabled and shows a spinner while the check is in flight, preventing double-taps.multisigCreateInvalidSigner(fail closed — never silently add an unchecked signer).app_localizations.dart+_en+_id:multisigCreateCannotUseMultisig("Multisig accounts cannot be used as signers").Validation
flutter analyzeand the existing test suite. The change is small, purely additive, and follows the existing patterns in the file (same shape as the duplicate-signer check).isMultisigIndexedreturnsfalse→ added (unchanged behaviour).isMultisigIndexedreturnstrue→ field shows error, signer not added (new behaviour).invalidSigner, signer not added (fail closed)._isAddingSigner.Risks and mitigations
enandidonly, matching the existing locale set.Follow-ups (out of scope)
MultisigServicerather than the screen, so the same rules apply if signer entry is added elsewhere.fixes #708