Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces a comprehensive voting system for the Soroban ecosystem with delegation, checkpointing, and voting unit tracking. Adds governance votes module with historical vote queries, fungible and non-fungible token voting extensions integrating with voting infrastructure, and an example contract demonstrating usage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Account as Account Storage
participant Votes as Votes Module
participant Delegate as Delegate Tracking
participant Checkpoints as Checkpoint Storage
participant History as Historical Queries
User->>Votes: delegate(account, delegatee)
Votes->>Account: verify authorization
Votes->>Delegate: update delegatee reference
Votes->>Checkpoints: push checkpoint for old delegate
Votes->>Checkpoints: push checkpoint for new delegate
Votes->>User: emit DelegateChanged
User->>Votes: transfer_voting_units(from, to, amount)
Votes->>Delegate: get old_delegate (from)
Votes->>Delegate: get new_delegate (to)
Votes->>Checkpoints: update old_delegate checkpoint
Votes->>Checkpoints: update new_delegate checkpoint
Votes->>User: emit DelegateVotesChanged
User->>History: get_past_votes(account, timepoint)
History->>Checkpoints: binary search checkpoint at timepoint
History->>User: return historical voting power
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
+ Coverage 96.09% 96.24% +0.14%
==========================================
Files 54 57 +3
Lines 5225 5507 +282
==========================================
+ Hits 5021 5300 +279
- Misses 204 207 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/governance/src/votes/storage.rs`:
- Around line 258-312: The transfer_voting_units function currently calls
move_delegate_votes twice (once for sender and once for receiver), which emits
misleading zero-net events when both accounts delegate to the same address;
change the flow to first compute from_delegate = get_delegate(e, from_addr) and
to_delegate = get_delegate(e, to_addr) (for Option cases) and then invoke
move_delegate_votes once with those delegate Option<&Address> values so its
internal from==to short-circuit prevents redundant events; ensure you still
perform voting unit arithmetic (get_voting_units, checked_sub/checked_add,
set_voting_units) and still call push_total_supply_checkpoint for mint/burn
paths, but move the delegate lookup so a single call to move_delegate_votes
replaces the two separate calls.
🧹 Nitpick comments (3)
packages/governance/README.md (1)
118-121: Consider adding the fungible-votes example.The Examples section only references the timelock-controller example. Consider adding a reference to the new
examples/fungible-votes/example to showcase the Votes module usage.📝 Suggested addition
## Examples See the following examples in the repository: +- [`examples/fungible-votes/`](https://github.com/OpenZeppelin/stellar-contracts/tree/main/examples/fungible-votes) - Fungible token with voting power delegation - [`examples/timelock-controller/`](https://github.com/OpenZeppelin/stellar-contracts/tree/main/examples/timelock-controller) - Timelock controller with role-based access controlexamples/fungible-votes/src/contract.rs (1)
1-8: Unused import:MuxedAddress.The
MuxedAddresstype is imported but not used in this file.♻️ Suggested fix
-use soroban_sdk::{contract, contractimpl, Address, Env, MuxedAddress, String}; +use soroban_sdk::{contract, contractimpl, Address, Env, String};packages/governance/src/votes/storage.rs (1)
316-324: Addextend_ttlafter persistent storage writes to prevent premature expiration of voting data.The codebase consistently calls
extend_ttlafter.get()operations (visible inget_delegate,get_voting_units,get_checkpoint,get_total_supply_checkpoint), but does not call it after.set()operations inset_voting_units,delegate, or checkpoint creation/updates. Since.set()initializes entries with the network minimum TTL, there's a window where newly written data could expire before being read and extended. For critical historical voting data, extend TTL immediately after writes to ensure consistent persistence.♻️ Example tweak for this helper
fn set_voting_units(e: &Env, account: &Address, units: u128) { let key = VotesStorageKey::VotingUnits(account.clone()); if units == 0 { e.storage().persistent().remove(&key); } else { e.storage().persistent().set(&key, &units); + e.storage().persistent().extend_ttl(&key, VOTES_TTL_THRESHOLD, VOTES_EXTEND_AMOUNT); } }Apply the same pattern to
delegate()and checkpoint creation inpush_checkpointandpush_total_supply_checkpoint.
ozgunozerk
left a comment
ozgunozerk
left a comment
There was a problem hiding this comment.
Liked the overall structure. Few changes are requested. All of them are fairly minimal.
Only reviewed the half of it. Will continue to review tomorrow. Reminder to myself: push_checkpoint
ozgunozerk
left a comment
ozgunozerk
left a comment
There was a problem hiding this comment.
One structural/architectural change for fungible and non-fungible tokens is commented.
Thanks for the suggestions, now it looks much better 🙏 |
|
Before merging, let's try to hit the target for codecov, amazing work 💯 |
2 participants
Fixes #437
Fixes #439
PR Checklist
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.