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:
WalkthroughThe pull request introduces a comprehensive Governor base contract module for Soroban, establishing core traits (Votes, Counting, Governor), governance abstractions (ProposalState, ProposalCore), error handling, event infrastructure, and persistent storage management with key operations for proposal lifecycle (propose, execute, cancel, prepare_vote). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 #563 +/- ##
==========================================
- Coverage 96.24% 92.80% -3.44%
==========================================
Files 57 58 +1
Lines 5507 5711 +204
==========================================
Hits 5300 5300
- Misses 207 411 +204 ☔ 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: 2
🤖 Fix all issues with AI agents
In `@packages/governance/src/governor/mod.rs`:
- Around line 523-527: The doc comments for GOVERNOR_EXTEND_AMOUNT and
GOVERNOR_TTL_THRESHOLD are swapped; update the comment above
GOVERNOR_EXTEND_AMOUNT to read that it is the "TTL extension amount for storage
entries (in ledgers)" and update the comment above GOVERNOR_TTL_THRESHOLD to
read that it is the "TTL threshold for extending storage entries (in ledgers)".
Locate the two constants GOVERNOR_EXTEND_AMOUNT and GOVERNOR_TTL_THRESHOLD in
mod.rs and swap or correct their preceding doc comments so each accurately
describes the associated constant.
- Around line 519-527: Proposal TTL constants GOVERNOR_EXTEND_AMOUNT and
GOVERNOR_TTL_THRESHOLD are declared but never used, so add persistent TTL
extension calls whenever a proposal is created or its state is updated: after
the persistent set of a proposal in propose() (use
e.storage().persistent().set(&GovernorStorageKey::Proposal(proposal_id.clone()),
&proposal)), and likewise after updating the stored proposal in execute() and
cancel(), call
e.storage().persistent().extend_ttl(&GovernorStorageKey::Proposal(proposal_id.clone()),
GOVERNOR_TTL_THRESHOLD, GOVERNOR_EXTEND_AMOUNT); ensure you reference the exact
symbols GOVERNOR_EXTEND_AMOUNT, GOVERNOR_TTL_THRESHOLD,
GovernorStorageKey::Proposal, extend_ttl, and the existing
storage().persistent().set/update locations so the TTL is extended when
proposals are stored or modified.
🧹 Nitpick comments (3)
packages/governance/src/governor/mod.rs (2)
107-121: Placeholder traits acknowledged — ensure cleanup is tracked.Both
VotesandCountingare temporary stubs with TODO comments. Since these define the public API surface that implementers will code against, consider linking the specific tracking issues/PRs in the TODO comments so they don't become stale.
323-334: Snapshot used for proposer threshold check differs from the voting snapshot.
proposechecks the proposer's voting power atcurrent_ledger(Line 332), but the snapshot recorded for voting isvote_start = current_ledger + voting_delay. This is a deliberate design choice (matching OpenZeppelin's Solidity Governor), but worth being explicit about in the doc comment — a proposer could have sufficient votes at proposal time but not at the voting snapshot (or vice versa).packages/governance/src/governor/storage.rs (1)
547-577: Double storage read for proposal core data incancel.
get_proposal_coreis called on Line 557 to load the proposal, and thenget_proposal_stateon Line 563 internally callsget_proposal_coreagain. You could use the already-loadedproposalto derive the state inline, avoiding the extra persistent storage read.♻️ Proposed refactor: reuse loaded proposal
let proposal_id = hash_proposal(e, &targets, &functions, &args, description_hash); // Get proposal and verify it exists let mut proposal = get_proposal_core(e, &proposal_id); // Only proposer can cancel proposal.proposer.require_auth(); // Can only cancel proposals that haven't reached a terminal state - let state = get_proposal_state(e, &proposal_id); + let state = derive_state_from_core(e, &proposal); match state { ProposalState::Pending | ProposalState::Active | ProposalState::Succeeded => {} _ => panic_with_error!(e, GovernorError::ProposalNotCancellable), }This would require extracting the state derivation logic from
get_proposal_stateinto a helper that accepts a&ProposalCoredirectly.
| if state == ProposalState::Executed { | ||
| panic_with_error!(e, GovernorError::ProposalAlreadyExecuted); | ||
| } | ||
| if state != ProposalState::Succeeded { | ||
| panic_with_error!(e, GovernorError::ProposalNotSuccessful); | ||
| } |
There was a problem hiding this comment.
missing check for ProposalState::Queued when proposal_needs_queuing returns true
There was a problem hiding this comment.
It is not missing, it is deliberate. Queue related logic should be implemented/overridden by the Queue extension/contract.
We define errors and types in the base module, that's why only the Queue state is defined, not the logic for it.
brozorec
left a comment
brozorec
left a comment
There was a problem hiding this comment.
Great progress 🙌 couple more points
- I noticed some discrepancies in measuring time/delays in the gov module, e.g. Governor uses ledgers, while Votes and Timelock use timestamps. I don't have a strong opinion, but we should def harmonize them.
- For coherency with the other modules, I suggest re-exporting all public functions and types and use them without absolute or relative path referencing.
| //! - *GovernorSettings* provides configurable parameters like voting delay, | ||
| //! voting period, and proposal threshold. |
There was a problem hiding this comment.
Actually, why do we need to move settings in an extension?
There was a problem hiding this comment.
Because tweaking/changing the voting delay, voting period, and proposal threshold is probably won't happen after once they are set in the constructor for the majority of the use cases. So, exposing these methods in the base trait, I think is an overkill. The base trait is already convoluted, so I tried to minimize it as much as I could.
If one needs to tweak these, they can always use the extension.
Co-authored-by: Boyan Barakov <9572072+brozorec@users.noreply.github.com>
Co-authored-by: Boyan Barakov <9572072+brozorec@users.noreply.github.com>
Co-authored-by: Boyan Barakov <9572072+brozorec@users.noreply.github.com>
That was a deliberate choice as well. I think, having the following is confusing: pub Trait myTrait {
fn my_method() {
my_method();
}
}Seems like a recursive call. So, instead: pub Trait myTrait {
fn my_method() {
storage::my_method();
}
}seems much better I think. I wanted to showcase that in this PR. If you liked it, we can refactor the rest of the library w.r.t this approach |
Definitely agreed. I think |
Yes, I'm also leaning more towards using ledgers. Created an issue #567
Ok, agreed 👍 but that should be only for the default implementations and we should still re-export them, right? I think |
2 participants
Fixes #556
PR Checklist
Summary by CodeRabbit
New Features
Tests