Repository navigation
Block adding one of this wallet's multisigs as a multisig signer - #715
exocognosis wants to merge 3 commits into
Conversation
A multisig account holds no key, so it can never sign an approval from the wallet. Adding one as a signer produced a multisig the app could not execute (Quantus-Network#708). When a signer is added, check it against local saved and pending multisigs, then against Multisig.Multisigs on chain. A hit opens a confirmation sheet that defaults to leaving the signer out; a failed lookup blocks the add with an error instead of letting it through.
|
Checked the lookup against a live chain (local dev node, quantus-node v1.0.2-Qm): Bell query.multisig.multisigs through KeepAliveHttpProvider, the same call isMultisigAccount makes, returns true for a created 2-of-2 and for a multisig that has a multisig signer, and false for two keyed accounts. The screen itself has only run in widget tests with the service faked; not run on a device or simulator. |
|
I think there's better fixes for this - this is an edge case of an edge case The users who do such a thing, can recover through CLI I was helping the original guy who did this, I just told him the prompt to give to his AI, and that resolved it - it involves constructing a complex msig To be reviewed, ideally find a minimal solution |
Drop the chain lookup, the confirm sheet and the helper. The add signer step now rejects an address that matches a multisig saved or pending in this wallet, with an inline explanation.
|
Fair. Trimmed to the minimal version: no chain query, no new service. The creation flow now checks the entered signer against multisig accounts this wallet itself knows, and blocks with an explanation if it matches. That misses multisigs created elsewhere, stated plainly in the description; those still have the CLI path from quantus-cli #189. |
|
Following up on the trimmed version from Wednesday: it is ready whenever you want another look. If you would rather leave this edge case to the CLI recovery path, I am fine closing this, your call. |
n13
left a comment
There was a problem hiding this comment.
Verdict: Approve
Reviewer: GPT-6.1 Sol
Reviewed head 107261d549edceb9c3d049a1fa41be28c7e5f4df against base 628583e24fb57629225e74814a183bdaeb7033dd. No blocking findings within the agreed wallet-local detection scope. The guard checks saved and pending multisigs before mutating the signer list; the Add Signer button and keyboard submission share this path.
Nonblocking wording correction: mobile-app/lib/l10n/app_en.arb:445 (and the Indonesian equivalent) says a multisig containing this signer can never execute from the wallet. It can still execute when the other keyed members meet the threshold, for example two keyed members of a 2-of-3 multisig. Consider saying this signer cannot approve in the wallet and CLI recovery may be needed if the remaining members cannot meet the threshold.
Validation: 73 focused tests passed: 19 mobile tests, including nine temporary reviewer widget tests, and 54 SDK multisig tests. The widget tests exercised saved/pending rejection through both input routes in English and Indonesian, whitespace trimming, error clearing, and ordinary/duplicate/invalid signer behavior, with validation and chain services faked. Formatting passed across 863 files; git diff --check passed; both generated localization getters match their ARB strings. No tracked files were changed.
Local workspace analysis was stopped at the required 10-second cap and remains incomplete. GitHub Analyze is still in progress; dependency-cooldown checks passed. No device or live-chain execution was performed.
Fixes the creation half of #708.
Minimal version. No chain query, no new service.
Change:
_reservedAddresses(), which the screen already builds from saved and pending multisigs.Files: the logic is one check in
add_multisig_screen.dart. The other five files are the new string:app_en.arb,app_id.arb, and the threeapp_localizations*.dartfiles that gen-l10n regenerates and the repo commits.Tradeoff: detection is local only. It misses multisigs created on another device or another wallet, since nothing in the wallet's store knows about them. That is accepted as the minimal scope. Anyone who still ends up there has the CLI path from Quantus-Network/quantus-cli#189.
Tests: none added. flutter analyze is clean on the changed files.