Repository navigation
fix(proposal): strictly validate threshold and quorum before execution - #383
Merged
Cjay-Cyber-2 merged 4 commits intoSep 26, 2026
Merged
Conversation
The execute doc comment said an expired proposal reports Error::ProposalExpired, but since ASTROIDX556#256/ASTROIDX556#359 execute records the terminal Expired state, refunds the deposit, emits the expired event and returns Ok(()) - returning an error would roll that settlement back. Document the actual behaviour and how callers can tell it apart from a real execution. Refs ASTROIDX556#226 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pin every guard execute applies before a proposal may fire: - unknown id -> NotFound; non-proposer -> Unauthorized; proposer auth is the only authorization demanded - exact threshold equality (unanimous 3-of-3) executes, one below is refused with ProposalNotApproved - executing twice, or executing a closed, rejected, cancelled or failed proposal, is refused with ProposalNotApproved and moves nothing; the deposit is refunded exactly once - an expired proposal is settled once (Expired, deposit refunded, one expired event) and never becomes executed, however often execute runs - quorum/majority arithmetic cannot overflow and holds its tie/majority boundary at the MAX_APPROVERS allow-list Refs ASTROIDX556#226 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@Nife-tanny Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Cjay-Cyber-2
approved these changes
Sep 26, 2026
Cjay-Cyber-2
left a comment
Contributor
There was a problem hiding this comment.
Looks good — approving.
Contributor
CI Checks Failed — Action NeededThe continuous integration checks on this pull request are failing. I can't merge until all required checks pass. Failing checks:
What to do: review the failing checks in the "Checks" tab above, fix the issues in your code, and push the changes. |
ASTROIDX556#382 moved quorum_required / majority_required from ProposalContract to the new VoteBars type. The overflow test added for ASTROIDX556#226 still called them on ProposalContract, so after main was merged in the test build failed with E0599. Point it at VoteBars like the rest of the suite. Refs ASTROIDX556#226 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cjay-Cyber-2
approved these changes
Sep 26, 2026
Cjay-Cyber-2
left a comment
Contributor
There was a problem hiding this comment.
Looks good — approving.
Cjay-Cyber-2
approved these changes
Sep 26, 2026
Cjay-Cyber-2
left a comment
Contributor
There was a problem hiding this comment.
Looks good — approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #226
Why this PR is small
Most of #226 was already merged before I started. #344 (
1d139f2) added the quorum/majority/threshold re-check toexecute(ensure_vote_valid), plus tests for tied votes, exact quorum/majority equality and narrow misses. #256 / #359 added the expiry gate. #382 (for #333, a near-duplicate of this issue) has since merged too: it added more quorum/majority tests and moved the vote-bar helpers into aVoteBarstype.So this PR does not re-implement any of that. It only covers what #226 asks for that
maindid not have yet:executedoc comment said an expired proposal "reportsError::ProposalExpired", butexecuteactually recordsExpired, refunds the deposit, emitsexpiredand returnsOk(()). It has to returnOk, because an error would roll back that settlement (the design from fix(proposal): enforce expiry on every proposal interaction (#320) #359). I fixed the comment rather than the behaviour; see the questions below.MAX_APPROVERS(32) allow-list.No contract logic changes.
lib.rschanges only in a doc comment, and the rest is new tests.Rules
executeenforces (unchanged, now fully pinned by tests)Guard order in
execute:NotFound(1)now >= expires_at,0= never)Expired, refunds deposit once, emitsexpired, returnsOk(()). Repeat calls are no-ops. Never executes.require_auth)UnauthorizedApproved(so not Pending, Executed, Closed, Rejected, Cancelled or Failed)ProposalNotApproved(73)ceil(n × 50 / 100), approvals > n / 2 (strict majority, so a tie fails)ThresholdNotMet(61)TimelockNotExpired(91)PrerequisiteNotMetState is set to
Executedand the deposit refunded only after every guard passes.executemakes no other external call; the actual value movement happens in the wallet/treasury. A second call then fails at guard 4, so the refund happens exactly once.Error codes: none added. The shared
#[contracterror]enum is at Soroban's 50-variant limit (see the note inshared/src/errors.rs), so the existing codes are reused:NotFound= 1,ThresholdNotMet= 61,ProposalNotApproved= 73,Unauthorized,TimelockNotExpired= 91.Acceptance criteria
ensure_vote_valid(fix(proposal): harden timelock and expiry guards on execute and cancel #344), whose helpersquorum_required/majority_required/has_majoritylive onVoteBarssince test(proposal): cover quorum and majority vote-bar edge cases (issue … #382. Reviewed; integer-only, rounds up, tie ≠ majority. Newly pinned here:vote_bars_hold_at_the_approver_cap_and_do_not_overflow.rejected_proposal_cannot_be_executed,cancelled_proposal_cannot_be_executed,failed_proposal_cannot_be_executed,closed_proposal_cannot_be_executed_again,executing_twice_is_refused_and_refunds_the_deposit_once(allProposalNotApproved, nothing moves). Existing:execute_before_approved_fails.execute_unknown_proposal_reports_not_found(NotFound),only_the_proposer_may_execute(Unauthorized),execute_requires_the_proposer_authorization(checksenv.auths()).expired_execution_settles_once_and_never_executes. This returnsOk(see above), and the test checks that the proposal endsExpired, never becomes executed, emits exactly oneexpiredevent and refunds the deposit exactly once. Existing:expired_proposal_cannot_be_executed,expiry_gate_wins_over_the_timelock_gate.threshold_equal_to_the_whole_allow_list_executes_only_when_unanimous, and the 16-of-32 tie / 17-of-32 majority case invote_bars_hold_at_the_approver_cap_and_do_not_overflow.main:tied_vote_blocks_execution,exact_quorum_and_majority_boundary_executes,narrowly_missing_the_quorum_blocks_execution,narrowly_missing_the_threshold_never_approves_and_cannot_execute,quorum_calculation_rounds_up_with_integer_scaling,majority_check_never_accepts_a_tie.Avoiding conflicts
lib.rsis untouched apart from one doc comment, and there are no logic or error-enum changes, so nothing overlaps the other open proposal/error PRs.test.rs. Its imports sit in that section rather than the file's topuseblock, which several open PRs edit.mainafter this PR was opened, andmainwas then merged into this branch (82e94b9). There was no textual conflict, but test(proposal): cover quorum and majority vote-bar edge cases (issue … #382 movedquorum_required/majority_requiredfromProposalContracttoVoteBars, so the one new test that called them no longer compiled (E0599 in thetestjob).1c3b53dpoints that test atVoteBars, like the rest of the suite. The guard order and error codes above are unchanged by test(proposal): cover quorum and majority vote-bar edge cases (issue … #382.mainon their own, so this branch doesn't cause those conflicts.Verification
cargo fmt --checkcargo clippy --workspace -- -D warnings(CI flags)--all-targetsonastroid-proposal)cargo test --package astroid-proposalmainafter #382 + 11 new)cargo test -peach: shared, interfaces, budget, treasury, wallet, escrow, multisig, registry, policy)cargo build --target wasm32v1-none --release -p astroid-proposal)stellar contract build,cargo test --workspace)Local-only issue, unrelated to this PR: on my Windows machine
cargo test --workspacefails to compile theastroid-integration-testscrate (a rustc "only metadata stub found forcore" toolchain error), including on unmodifiedmain. CI on Linux is unaffected, so the workspace-wide and integration results come from CI.Questions for the maintainer
executereturningOk(()). The issue asks for a precise error code on expired proposals. ReturningError::ProposalExpiredwould roll back the expiry settlement (state, refund, event) that fix(proposal): enforce expiry on every proposal interaction (#320) #359 deliberately commits, so I kept the behaviour and fixed the docs. Would you preferexecuteto returnProposalExpiredwhen the proposal is alreadyExpired(nothing to roll back), and keepOkonly for the call that performs the settlement? Several existing tests callexecuteon stale proposals and expect success, so I didn't change this without your go-ahead.Executed/Closedproposal reportsProposalNotApproved.InvalidProposalState(72) might be more precise for "already executed". I left it unchanged to avoid a behaviour change; happy to switch if you prefer.cancelandfaildoc comments have the same mismatch (they sayProposalExpired, but both settle and returnOk). I can fix those here or in a follow-up.