Skip to content

Execute proposal flow#511

Open
dewabisma wants to merge 20 commits into
multisig-implementationfrom
beast/execute-proposal-flow
Open

Execute proposal flow#511
dewabisma wants to merge 20 commits into
multisig-implementationfrom
beast/execute-proposal-flow

Conversation

@dewabisma

@dewabisma dewabisma commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Regenerate polkadart metadata
  • Implement execute proposal flow
  • Update proposal details sheet

Note

Medium Risk
Touches on-chain submission and balance-affecting execute flows with new polling paths; multisig creation preflight no longer reserves deposit, which can change whether creation is allowed vs the old app.

Overview
Adds an end-to-end multisig execute path after approvals hit threshold: confirm sheet (fee estimate + local auth), TransactionSubmissionService.executeProposal with optimistic pending state, indexer polling/reconciliation, and UI on proposal detail, activity rows, and error toasts—mirroring the existing approve flow.

Proposal detail & lists now branch on status: Execute when isReadyToExecute, Approve otherwise, with pending “Executing…” states; terminal proposals show an AT timestamp from updated_at instead of expiry. Open proposals sort by updated_at; live proposal resolution checks past proposals too.

SDK / chain metadata (spec 129) is regenerated: multisig creation deposit and dissolve APIs are dropped from models, preflight, and activity detail; creation cost is pallet + network fees only. MultisigService gains build/estimate/submit for multisig.execute. GraphQL and MultisigProposal use updated_at for ordering and activity timestamps. Generated pallets also pick up reversible-transfers guardian renames and other runtime doc/type updates bundled in the regen.

i18n adds execute-related strings (EN/ID) and tightens insufficient-balance copy for multisig creation.

Reviewed by Cursor Bugbot for commit 46577df. Configure here.

@dewabisma dewabisma requested a review from n13 June 9, 2026 12:01

@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 2 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 46577df. Configure here.


print('executing proposal id: ${widget.proposal.id}');
print('msig: ${widget.msig.accountId}');
print('signer: ${signer.accountId}');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Debug print left in execute sheet

Low Severity

The execute confirmation sheet calls print for fee loading and submission with proposal id, multisig address, and signer id. That mirrors temporary debugging, not the debugPrint pattern used on the approve sheet, and can leak account identifiers in production logs.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 46577df. Configure here.


removePendingMultisigExecution(ref, pending.id);
await reconcileIndexedExecution(ref, msig, pending);
return true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Execution poll ignores extrinsic outcome

Medium Severity

Execution polling clears pending state when the proposal’s indexer status is executed, without checking that this user’s submit extrinsic succeeded. If another signer executes first, the local pending row is dropped even when the user’s extrinsic later fails on-chain, so no submit-failure feedback and a wasted fee are possible.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 46577df. Configure here.

@dewabisma dewabisma changed the base branch from beast/sign-proposal-flow to multisig-implementation June 9, 2026 12:04
@n13

n13 commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Review complete. Here's my assessment of PR #511 — Execute proposal flow.

Overview

Adds the end-to-end multisig execute path: confirm sheet → executeProposal with optimistic pending state → indexer polling/reconciliation → UI updates across proposal detail, activity rows, and toasts. Also regenerates polkadart metadata (spec 129: deposit/dissolve removed, interceptorguardian), and switches proposal ordering/timestamps to updated_at. The architecture is good — it mirrors the existing approve flow and reuses the shared ExtrinsicIndexerPollingService, dedup logic, and reconciliation helpers, with decent test coverage. The removed pallet APIs (deposit, dissolve) are cleaned up consistently with no leftover references.

Should fix before merge

1. Leftover debug print callsmultisig_execute_confirm_sheet.dart has raw print statements logging the proposal id, multisig address, and signer id (_loadNetworkFee and _confirm). The approve sheet uses debugPrint; these look like forgotten debugging and leak account identifiers into production logs. (Bugbot flagged this too.)

2. quantus_sdk/pubspec.yaml polkadart endpoint changed to localhost:

 polkadart:
   output_dir: lib/generated
   chains:
-    planck: wss://a1-planck.quantus.cat
+    planck: ws://127.0.0.1:9944

This looks like local-dev config that leaked in — the chore: revert constants commit reverted other things but missed this. Anyone regenerating later will hit whatever local node they happen to run instead of the canonical chain. If the regen intentionally required a local spec-129 node, it should still be reverted before merge.

3. Exact duplicate test groupquantus_sdk/test/multisig_service_test.dart now contains two verbatim-identical MultisigService.buildApproveCall groups (lines 478 and 518 on the branch). The new buildExecuteCall group also copy-pastes the same msig fixture a third time. One group should be deleted and the fixture shared.

Worth discussing

4. Silent drop of a failed execution (Bugbot's medium finding is valid) — in _tryResolveExecutionTimeout, the pending row is removed as soon as the proposal status reads executed, regardless of whether this user's extrinsic succeeded. If another signer executes first and the user's extrinsic fails on-chain (e.g. already-executed error), the user gets success-looking UX and silently loses the fee. Note the contrast: _confirmIndexedExecution correctly verifies via searchExecutedByExtrinsicHash against the user's own hash. The timeout path could at least log distinctly or show an informational toast ("executed by another signer") when the hash search comes back empty, rather than resolving silently.

5. Fee-estimate failure swallowed_loadNetworkFee catches the error, hides the fee row, and lets the user execute anyway with no indication anything failed. The approve sheet has the same behavior, so it's consistent — but both violate fail-early; an inline error state would be better.

6. DRY: execute sheet is a ~190-line near-copy of the approve sheetmultisig_execute_confirm_sheet.dart differs from multisig_approve_confirm_sheet.dart only in strings and the service method called. The signer-lookup block (accountsProvider … firstWhere(myMemberAccountId)) is now duplicated four times across the two sheets. A shared confirm-sheet widget parameterized on labels + submit callback would eliminate ~150 duplicated lines. Same applies (more mildly) to estimate/submit Approve|Execute triplets in MultisigService and the parallel polling/toast/provider files — those mirror an established pattern, so acceptable, but the sheet copy is the one worth fixing.

7. Deploy-order dependency on the indexerMultisigProposal.fromIndexerJson now hard-requires updated_at (throws FormatException if absent), and the camelCase createdAt fallback was dropped. Every proposal query breaks against an indexer that hasn't deployed the new schema. Fine if the indexer ships first — just confirm that's coordinated.

Minor

  • _executeSection's note switch has two arms (_, false, _) and (_, _, false) returning the same string — collapse them.
  • _confirm does if (!authed || !mounted) { setState(...) } — calls setState on an unmounted widget if the sheet was dismissed during auth. Copied from the approve sheet, so pre-existing pattern, but it's a latent crash in both.
  • Transient duplicate row possible: if the indexed MultisigProposalExecutedEvent arrives via pagination before the pending row receives its extrinsicHash, the dedup keys won't match (pending: vs hash:). Self-heals on next refresh; probably not worth complicating.
  • multisig_activity_section.dart re-sorts open proposals by updatedAt client-side even though the GraphQL query already orders by updated_at desc — redundant, though harmless given pending merging.

What looks good

  • Pending-balance accounting is correct: only the executor's network fee is deducted from the executor, since the transfer itself leaves the multisig.
  • _resolveLiveProposal extension to also check past proposals fixes the stale-sheet issue after execution.
  • Terminal proposals showing an AT timestamp from updated_at instead of a meaningless expiry is a nice UX touch.
  • Test additions (GraphQL fixture parsing, dedup swap, updated_at mapping) cover the right seams.

Items 1–3 are quick fixes I'd block on; item 4 is a judgment call but at minimum deserves a log/toast given the "no silent failures" principle.

@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 (agent-assisted, diffed against multisig-implementation)

Verdict: GTG — no blockers found; two minor, non-blocking notes.

Minor

  1. Logout doesn't clear pending execution statemobile-app/lib/services/logout_service.dart clears pendingTransactionsProvider and pendingMultisigCreationsProvider, but not the new in-memory pendingMultisigExecutionsProvider (the same gap already exists for the pending proposals/approvals providers, so this extends a pre-existing pattern). After logout + re-login in the same process, stale "Executing…" rows could survive and execution pollers tied to the old session keep running.

  2. Possible false "executed by another signer" toast — in _tryResolveExecutionTimeout (multisig_execution_polling_service.dart), when the proposal status is executed but searchExecutedByExtrinsicHash returns null, the executedByOther toast is always shown. That branch is also reachable when this user's own execution succeeded but account-history indexing is still lagging past the poll timeout — the code comment acknowledges "most likely". Consider softening the copy or rechecking once more before asserting someone else executed it.

Checked and fine

  • MultisigApprovalToastListener exhaustively handles all MultisigExecutionToastKind values including executedByOther.
  • Execute submit path (executeProposal / _submitExecuteBackground) cleans up pending state and surfaces a toast on submit failure; optimistic state + indexer polling mirrors the existing approve flow.

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