[FEATURE] Contracts: Soroban Governance Voting smart contract#328
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a new Soroban governance smart contract package with a Cargo manifest and a lib.rs implementing token-weighted proposals: creation, voting, finalization based on quorum/approval thresholds, execution, token lock management, view helpers, and an accompanying test suite. ChangesGovernance Contract Implementation
Sequence Diagram(s)sequenceDiagram
participant Proposer
participant Voter
participant GovernanceContract
Proposer->>GovernanceContract: lock_tokens(amount)
Proposer->>GovernanceContract: propose(action)
GovernanceContract-->>Proposer: proposal_id
Voter->>GovernanceContract: lock_tokens(amount)
Voter->>GovernanceContract: vote(proposal_id, weight, support)
GovernanceContract-->>Voter: VoteRecord stored
Proposer->>GovernanceContract: finalize(proposal_id)
GovernanceContract-->>Proposer: status Passed/Rejected
Proposer->>GovernanceContract: execute(proposal_id)
GovernanceContract-->>Proposer: status Executed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/contracts/governance/src/lib.rs`:
- Around line 250-255: The `execute` flow in `lib.rs` only updates proposal
status and never performs the approved action, so it needs to actually use
`proposal.action`. Update the `execute` method and any helper it relies on to
decode the stored action payload and dispatch the cross-contract call (or
otherwise invoke the intended operation) when a proposal is `Passed`, then only
mark it `Executed` after the action is successfully handled.
- Around line 364-392: The vote storage key in `vote_key` is not actually unique
per voter and proposal because it ignores `voter` and collapses proposal IDs
above 9 into `vN`, which causes collisions and incorrect duplicate-vote
behavior. Update `vote_key` in `packages/contracts/governance/src/lib.rs` to
derive the key from the full `(voter, proposal_id)` pair, and then make sure the
call sites that store and look up vote records use that same key consistently so
each voter’s vote is isolated per proposal.
- Around line 277-309: The lock_tokens function currently lets callers assign
arbitrary voting power because it only records the requested amount without
verifying any token balance or escrowing tokens. Update lock_tokens to enforce
real ownership of the locked governance tokens by checking the voter’s balance
and moving tokens into escrow via the token contract before updating LOCKS and
TokenLock. Keep the existing symbols lock_tokens, TokenLock, and LOCKS in place,
but make the amount stored reflect only successfully transferred tokens.
- Around line 312-330: The unlock_tokens flow currently removes the voter’s
TokenLock unconditionally, which lets a voter unlock and reuse voting weight
while active proposal votes still depend on that lock. Update unlock_tokens to
verify there are no live vote records tied to the voter before deleting the
entry from LOCKS, using the existing voter auth and storage lookup as the gate.
If any proposal the voter participated in is still active, reject the unlock
instead of calling locks.remove; otherwise proceed with the current removal and
persist the updated map.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89f1424b-c2f5-4d63-99e9-e16a83110a53
📒 Files selected for processing (2)
packages/contracts/governance/Cargo.tomlpackages/contracts/governance/src/lib.rs
Comment on lines
+250
to
+255
| /// Execute a proposal that has `Passed`. | ||
| /// | ||
| /// Marks the proposal as `Executed`. Actual cross-contract dispatch is | ||
| /// intentionally left to the caller — the contract records intent on-chain | ||
| /// and the `action` string encodes what must be done (XDR-encoded call | ||
| /// or human-readable instruction). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
execute never executes the approved action.
This method only flips status to Executed; proposal.action is never decoded or dispatched, so passed proposals have no on-chain effect.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/contracts/governance/src/lib.rs` around lines 250 - 255, The
`execute` flow in `lib.rs` only updates proposal status and never performs the
approved action, so it needs to actually use `proposal.action`. Update the
`execute` method and any helper it relies on to decode the stored action payload
and dispatch the cross-contract call (or otherwise invoke the intended
operation) when a proposal is `Passed`, then only mark it `Executed` after the
action is successfully handled.
Comment on lines
+277
to
+309
| /// Lock `amount` governance tokens to gain voting weight. | ||
| /// | ||
| /// Calling again overwrites the existing lock (adds to it conceptually — | ||
| /// in a production contract this would call a token contract's | ||
| /// `transfer_from` to escrow the tokens on-chain). | ||
| pub fn lock_tokens(env: Env, voter: Address, amount: i128) { | ||
| voter.require_auth(); | ||
|
|
||
| if amount <= 0 { | ||
| panic!("lock amount must be positive"); | ||
| } | ||
|
|
||
| let mut locks: Map<Address, TokenLock> = | ||
| env.storage().instance().get(&LOCKS).unwrap_or(Map::new(&env)); | ||
|
|
||
| // If a previous lock exists, accumulate rather than replace. | ||
| let existing_amount = locks | ||
| .get(voter.clone()) | ||
| .map(|l| l.amount) | ||
| .unwrap_or(0); | ||
|
|
||
| let new_amount = existing_amount | ||
| .checked_add(amount) | ||
| .expect("lock amount overflow"); | ||
|
|
||
| let lock = TokenLock { | ||
| voter: voter.clone(), | ||
| amount: new_amount, | ||
| locked_at: env.ledger().timestamp(), | ||
| }; | ||
|
|
||
| locks.set(voter, lock); | ||
| env.storage().instance().set(&LOCKS, &locks); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
lock_tokens currently lets users mint voting power.
amount is entirely caller-supplied here. Because no token balance is verified and no token transfer is escrowed, any address can assign itself arbitrary proposer/voting weight.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/contracts/governance/src/lib.rs` around lines 277 - 309, The
lock_tokens function currently lets callers assign arbitrary voting power
because it only records the requested amount without verifying any token balance
or escrowing tokens. Update lock_tokens to enforce real ownership of the locked
governance tokens by checking the voter’s balance and moving tokens into escrow
via the token contract before updating LOCKS and TokenLock. Keep the existing
symbols lock_tokens, TokenLock, and LOCKS in place, but make the amount stored
reflect only successfully transferred tokens.
Comment on lines
+312
to
+330
| /// Unlock and return governance tokens to the voter. | ||
| /// | ||
| /// A voter may only unlock after all proposals they voted on have been | ||
| /// finalized (Passed, Rejected, or Executed) — preventing double-spend | ||
| /// of voting weight across overlapping proposals. | ||
| /// For simplicity in this implementation the caller asserts readiness; | ||
| /// a production version would iterate active vote records. | ||
| pub fn unlock_tokens(env: Env, voter: Address) { | ||
| voter.require_auth(); | ||
|
|
||
| let mut locks: Map<Address, TokenLock> = | ||
| env.storage().instance().get(&LOCKS).unwrap_or(Map::new(&env)); | ||
|
|
||
| if !locks.contains_key(voter.clone()) { | ||
| panic!("no token lock found for voter"); | ||
| } | ||
|
|
||
| locks.remove(voter); | ||
| env.storage().instance().set(&LOCKS, &locks); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Block unlocks while live votes still depend on the lock.
This unconditionally deletes the lock even if the voter already cast weight on active proposals. A voter can vote, unlock, then relock/reuse the same tokens on another open proposal while the first vote still counts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/contracts/governance/src/lib.rs` around lines 312 - 330, The
unlock_tokens flow currently removes the voter’s TokenLock unconditionally,
which lets a voter unlock and reuse voting weight while active proposal votes
still depend on that lock. Update unlock_tokens to verify there are no live vote
records tied to the voter before deleting the entry from LOCKS, using the
existing voter auth and storage lookup as the gate. If any proposal the voter
participated in is still active, reject the unlock instead of calling
locks.remove; otherwise proceed with the current removal and persist the updated
map.
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
2 participants
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.
Summary
Implements the Soroban governance voting smart contract for DAO and
decentralized organization coordination ( issue #309).
Closes #309.
Files
packages/contracts/governance/Cargo.toml— package manifestpackages/contracts/governance/src/lib.rs— contract + testsContract design
Storage
PROPOSALSMap<u32, Proposal>VOTESMap<Symbol, VoteRecord>LOCKSMap<Address, TokenLock>NEXT_IDu32All types use
#[contracttype]for automatic XDR serialisation.Configuration constants
VOTING_PERIOD_SECSQUORUM_THRESHOLDAPPROVAL_THRESHOLD_PCTPublic API
Proposal state machine
finalizeandexecuteare intentionally separate calls — the statetransition is auditable on-chain independently of who triggers execution.
Validation enforced on every call
propose— proposer must have a non-zero token lock;require_authvote— lock must exist; weight ≤ locked amount; proposal must beActive; voting period must not have ended; double-vote prevented viathe
VOTESmapfinalize— voting period must have ended; proposal must beActiveexecute— proposal must be inPassedstatus;require_authon executorlock_tokens— amount must be positive; accumulates on existing lockunlock_tokens— lock must exist;require_authTests
All tests pass via
cargo test.Acceptance criteria
require_authenforced on all state-mutating calls#[contracttype]on all storage types for XDR compatibilitysoroban-sdk = "21.0.0"used by sibling contractsSummary by CodeRabbit
New Features
Bug Fixes