feat(governance-token): add voting checkpoint history and latest chec…#508
Open
MarcusDavidG wants to merge 4 commits intotheblockcade:mainfrom
Open
feat(governance-token): add voting checkpoint history and latest chec…#508MarcusDavidG wants to merge 4 commits intotheblockcade:mainfrom
MarcusDavidG wants to merge 4 commits intotheblockcade:mainfrom
Conversation
…kpoint accessor Closes theblockcade#466 - Add Checkpoint { ledger, balance } contracttype - Write checkpoint on every mint, burn, and transfer - latest_checkpoint(holder) -> Option<Checkpoint>: returns most recent snapshot or None for unknown holders - checkpoint_history(holder, limit) -> Vec<Checkpoint>: returns up to limit (capped at 50) most-recent checkpoints oldest-first; empty vec for unknown holders - Same-ledger writes overwrite rather than duplicate; oldest entry evicted when cap (50) is reached - Remove unused stellarcade-shared dependency from Cargo.toml - Add tests: latest checkpoint after mint, missing holder returns None/empty, bounded history, checkpoint on transfer - Update contracts/governance-token/README.md and docs/contracts/governance-token.md
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds per-holder balance checkpointing to the governance-token contract (new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Contract as GovernanceToken
participant Storage
Client->>Contract: call mint/burn/transfer(...)
Contract->>Storage: update `Balance` for holder(s)
Contract->>Contract: write_checkpoint(holder, new_balance)
Contract->>Storage: read/write `Checkpoints\<Address>` (overwrite if same ledger, evict oldest if >50)
Client->>Contract: call latest_checkpoint / checkpoint_history / checkpoint_at_ledger
Contract->>Storage: read `Checkpoints\<Address>`
Contract-->>Client: return Option\<Checkpoint\> / Vec\<Checkpoint\>
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)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/governance-token/src/lib.rs (1)
368-425: The new tests still miss the eviction and multi-ledger ordering path.All of these assertions run at the default ledger sequence, so they validate missing-holder behavior and same-ledger overwrite, but not the oldest-first ordering or head-eviction branch once history grows past 50 distinct ledgers. Please add one case that advances the ledger between writes and pushes past
MAX_CHECKPOINTS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance-token/src/lib.rs` around lines 368 - 425, Add a test that advances the ledger sequence between checkpointing operations and writes more than MAX_CHECKPOINTS distinct-ledger checkpoints to exercise oldest-first ordering and head eviction: use setup() to get (env, _, _, client), create a holder Address, loop to mint (or transfer/burn) small amounts while advancing the ledger sequence between each operation (so each write is on a new ledger), repeat for MAX_CHECKPOINTS + 1 iterations, then call client.checkpoint_history(&holder, &MAX_CHECKPOINTS) and assert the returned history length equals MAX_CHECKPOINTS and that the earliest returned checkpoint corresponds to the most recent MAX_CHECKPOINTS ledgers (i.e., the oldest checkpoint was evicted); also verify client.latest_checkpoint(&holder) reflects the final balance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/governance-token/README.md`:
- Around line 25-45: The README's method list is out of sync with the contract
ABI: replace the stale signatures `init(admin: Address, token_config:
TokenConfig)` and `balance_of(owner: Address)` with the actual exported
signatures `init(admin, name, symbol, decimals)` and `balance(owner)` and ensure
naming matches throughout (e.g., use `latest_checkpoint(holder: Address)`,
`checkpoint_history(holder: Address, limit: u32)`, and the storage item
`Checkpoints`); update any occurrence of `balance_of`/`token_config` to
`balance`/the explicit init parameters and confirm the method descriptions,
parameter names, and return types in the README now match the contract symbols
(`init`, `balance`, `latest_checkpoint`, `checkpoint_history`, `Checkpoints`).
In `@docs/contracts/governance-token.md`:
- Around line 197-211: Update the "Checkpoint Ordering & Retention" section to
explicitly state when checkpoints are written: on mint, burn, and transfer
operations affecting a holder's balance. Mention that each such operation
records (or updates) a Checkpoint (the Checkpoint struct with fields ledger and
balance) for the involved holder(s), follows the per-holder ascending ledger
ordering, overwrites multiple changes in the same ledger, and participates in
the 50-checkpoint retention/eviction policy; also clarify the behavior for
unknown holders remains None/empty list.
---
Nitpick comments:
In `@contracts/governance-token/src/lib.rs`:
- Around line 368-425: Add a test that advances the ledger sequence between
checkpointing operations and writes more than MAX_CHECKPOINTS distinct-ledger
checkpoints to exercise oldest-first ordering and head eviction: use setup() to
get (env, _, _, client), create a holder Address, loop to mint (or
transfer/burn) small amounts while advancing the ledger sequence between each
operation (so each write is on a new ledger), repeat for MAX_CHECKPOINTS + 1
iterations, then call client.checkpoint_history(&holder, &MAX_CHECKPOINTS) and
assert the returned history length equals MAX_CHECKPOINTS and that the earliest
returned checkpoint corresponds to the most recent MAX_CHECKPOINTS ledgers
(i.e., the oldest checkpoint was evicted); also verify
client.latest_checkpoint(&holder) reflects the final balance.
🪄 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: 6bfbf94d-414e-49a1-b67b-ba29a1624813
📒 Files selected for processing (4)
contracts/governance-token/Cargo.tomlcontracts/governance-token/README.mdcontracts/governance-token/src/lib.rsdocs/contracts/governance-token.md
💤 Files with no reviewable changes (1)
- contracts/governance-token/Cargo.toml
- README: fix stale init signature (token_config -> name/symbol/decimals) and balance_of -> balance - docs: clarify checkpoint write triggers (mint/burn/transfer) in Ordering & Retention section - tests: add test_checkpoint_eviction_and_ordering to cover oldest-first ordering and head eviction past MAX_CHECKPOINTS across distinct ledgers
There was a problem hiding this comment.
🧹 Nitpick comments (3)
contracts/governance-token/README.md (1)
44-44: Minor: Storage key name mismatch.The storage documentation says
Checkpointsbut the contract usesDataKey::Checkpoints(Address)- consider clarifying that this is a per-holder key:📝 Suggested clarification
-- `Checkpoints`: Per-holder ordered list of `Checkpoint` entries (bounded to 50). +- `Checkpoints(Address)`: Per-holder ordered list of `Checkpoint` entries (bounded to 50).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance-token/README.md` at line 44, Update the README entry for "Checkpoints" to reflect that checkpoints are stored per-holder under the storage key DataKey::Checkpoints(Address): describe it as a per-holder ordered list of Checkpoint entries (bounded to 50) and mention the exact storage key symbol DataKey::Checkpoints(Address) so readers understand it's scoped by holder address.contracts/governance-token/src/lib.rs (2)
248-279: Governance voting should use balance snapshots, not current balance.The governance contract votes using
TokenClient::balance(&voter)at voting time, allowing voters to manipulate their weight by acquiring tokens immediately before voting and disposing afterward. While Soroban's transaction atomicity prevents single-transaction flash-loan-style attacks, the window remains exploitable across separate transactions.The governance-token contract already captures ledger-aware checkpoints (with
ledger: u32andbalance: i128fields) and the Proposal struct storesstart_ledger. However, voting never queries the checkpoint at the proposal'sstart_ledger— instead it uses current balance.Consider implementing
checkpoint_at_ledger(holder: Address, ledger: u32) -> Option<Checkpoint>to enable snapshot voting. Voters could then be weighted by their balance at the proposal's start ledger, not at voting time. Alternatively, document why current-balance voting is preferred (e.g., if it's intentional policy).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance-token/src/lib.rs` around lines 248 - 279, The contract currently uses current balances (e.g., TokenClient::balance) when tallying votes instead of ledgered snapshots; add a function checkpoint_at_ledger(env: Env, holder: Address, ledger: u32) -> Option<Checkpoint> that searches the stored Vec<Checkpoint> (same storage key used by latest_checkpoint/checkpoint_history and type Checkpoint) and returns the checkpoint whose ledger equals or is the most-recent <= requested ledger, then update the vote-weight calculation in the proposal-voting logic (where Proposal.start_ledger is available) to call checkpoint_at_ledger(holder, proposal.start_ledger) and use its balance if present (fallback to zero if None) rather than querying current TokenClient::balance; ensure this new helper reuses existing storage access patterns used by latest_checkpoint/checkpoint_history.
102-109: UseVec::slice()for more efficient eviction.The current eviction creates a new
Vecand copies elements one-by-one. While bounded to 50 iterations,soroban-sdkVec supportsslice()which is more efficient. The integer-overflow vulnerability (CVE-2026-24889) that previously affectedslice()is patched in soroban-sdk 25.0.2 (the version in use), making this refactor safe.♻️ Suggested refactor
// Evict oldest entry when the cap is reached. if history.len() >= MAX_CHECKPOINTS { - let mut trimmed: Vec<Checkpoint> = Vec::new(env); - for i in 1..history.len() { - trimmed.push_back(history.get(i).unwrap()); - } - history = trimmed; + history = history.slice(1..); }As a best practice, also consider setting
overflow-checks = truein[profile.release]inCargo.tomlto ensure arithmetic operations fail safely in all build configurations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance-token/src/lib.rs` around lines 102 - 109, The eviction code for history currently allocates a new Vec and pushes elements one-by-one (using trimmed and push_back) which is inefficient; replace that logic by using Vec::slice on history to create a view of the elements after the first entry (e.g., slice(1, history.len())) and assign that slice back to history (or copy the slice in one operation) so you avoid the per-element loop; update uses of MAX_CHECKPOINTS, history, and Checkpoint to operate with the slice-based eviction and ensure bounds are checked (use history.len() for the slice end). Also consider enabling overflow-checks = true in Cargo.toml [profile.release] as a follow-up to guard arithmetic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/governance-token/README.md`:
- Line 44: Update the README entry for "Checkpoints" to reflect that checkpoints
are stored per-holder under the storage key DataKey::Checkpoints(Address):
describe it as a per-holder ordered list of Checkpoint entries (bounded to 50)
and mention the exact storage key symbol DataKey::Checkpoints(Address) so
readers understand it's scoped by holder address.
In `@contracts/governance-token/src/lib.rs`:
- Around line 248-279: The contract currently uses current balances (e.g.,
TokenClient::balance) when tallying votes instead of ledgered snapshots; add a
function checkpoint_at_ledger(env: Env, holder: Address, ledger: u32) ->
Option<Checkpoint> that searches the stored Vec<Checkpoint> (same storage key
used by latest_checkpoint/checkpoint_history and type Checkpoint) and returns
the checkpoint whose ledger equals or is the most-recent <= requested ledger,
then update the vote-weight calculation in the proposal-voting logic (where
Proposal.start_ledger is available) to call checkpoint_at_ledger(holder,
proposal.start_ledger) and use its balance if present (fallback to zero if None)
rather than querying current TokenClient::balance; ensure this new helper reuses
existing storage access patterns used by latest_checkpoint/checkpoint_history.
- Around line 102-109: The eviction code for history currently allocates a new
Vec and pushes elements one-by-one (using trimmed and push_back) which is
inefficient; replace that logic by using Vec::slice on history to create a view
of the elements after the first entry (e.g., slice(1, history.len())) and assign
that slice back to history (or copy the slice in one operation) so you avoid the
per-element loop; update uses of MAX_CHECKPOINTS, history, and Checkpoint to
operate with the slice-based eviction and ensure bounds are checked (use
history.len() for the slice end). Also consider enabling overflow-checks = true
in Cargo.toml [profile.release] as a follow-up to guard arithmetic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3404d9e2-f307-46e7-9a66-bca1f5ed420a
📒 Files selected for processing (3)
contracts/governance-token/README.mdcontracts/governance-token/src/lib.rsdocs/contracts/governance-token.md
✅ Files skipped from review due to trivial changes (1)
- docs/contracts/governance-token.md
…mments - README: clarify storage key as Checkpoints(Address) (per-holder scoped) - lib.rs: replace manual eviction loop with history.slice(1..history.len()) - lib.rs: add checkpoint_at_ledger(holder, ledger) -> Option<Checkpoint> for snapshot-based vote weighting - docs: document checkpoint_at_ledger method signature and semantics
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/governance-token/src/lib.rs (2)
447-473: Missing test coverage forcheckpoint_at_ledger.The test suite covers
latest_checkpointandcheckpoint_historythoroughly, including edge cases like missing holders and eviction. However,checkpoint_at_ledger— a key method for snapshot-based vote weighting — has no test coverage. Consider adding tests for:
- Finding a checkpoint at an exact ledger match
- Finding the most recent checkpoint before a given ledger
- Returning
Nonewhen the requested ledger precedes all checkpoints- Returning
Nonefor unknown holdersWould you like me to generate test cases for
checkpoint_at_ledger?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance-token/src/lib.rs` around lines 447 - 473, Add unit tests covering the checkpoint_at_ledger function: write tests (using setup(), Address::generate, client, and env.ledger().with_mut to advance ledger and client.mint) that assert (1) exact ledger match returns that checkpoint, (2) a ledger between checkpoints returns the most recent prior checkpoint, (3) a ledger before all checkpoints returns None, and (4) unknown holder returns None; use MAX_CHECKPOINTS to create checkpoints and re-use patterns from test_checkpoint_eviction_and_ordering for ledger sequencing and assertions against client.checkpoint_at_ledger and client.latest_checkpoint.
281-299: Linear search is acceptable for bounded history, but consider binary search for future optimization.The
checkpoint_at_ledgerimplementation correctly finds the latest checkpoint withledger <= requestedby iterating forward through the ascending-ordered history and updating the result until it overshoots. WithMAX_CHECKPOINTS = 50, linear search is acceptable, but a binary search could be a future optimization if this becomes a hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/governance-token/src/lib.rs` around lines 281 - 299, The current checkpoint_at_ledger function scans history linearly from start and updates result until an overshoot; to optimize, replace that forward linear scan with a binary search over the ascending-ordered Vec<Checkpoint> (retrieved from DataKey::Checkpoints) to find the rightmost Checkpoint with cp.ledger <= ledger and return it as Option<Checkpoint>. Locate the function checkpoint_at_ledger and change the loop logic to use a standard binary-search pattern (low/high/mid) comparing history.get(mid).unwrap().ledger to ledger, moving low or high accordingly and returning the found checkpoint or None; keep all other storage retrieval and types (Checkpoint, Env, Address) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/governance-token/src/lib.rs`:
- Around line 447-473: Add unit tests covering the checkpoint_at_ledger
function: write tests (using setup(), Address::generate, client, and
env.ledger().with_mut to advance ledger and client.mint) that assert (1) exact
ledger match returns that checkpoint, (2) a ledger between checkpoints returns
the most recent prior checkpoint, (3) a ledger before all checkpoints returns
None, and (4) unknown holder returns None; use MAX_CHECKPOINTS to create
checkpoints and re-use patterns from test_checkpoint_eviction_and_ordering for
ledger sequencing and assertions against client.checkpoint_at_ledger and
client.latest_checkpoint.
- Around line 281-299: The current checkpoint_at_ledger function scans history
linearly from start and updates result until an overshoot; to optimize, replace
that forward linear scan with a binary search over the ascending-ordered
Vec<Checkpoint> (retrieved from DataKey::Checkpoints) to find the rightmost
Checkpoint with cp.ledger <= ledger and return it as Option<Checkpoint>. Locate
the function checkpoint_at_ledger and change the loop logic to use a standard
binary-search pattern (low/high/mid) comparing history.get(mid).unwrap().ledger
to ledger, moving low or high accordingly and returning the found checkpoint or
None; keep all other storage retrieval and types (Checkpoint, Env, Address)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11a6576c-f7f5-41c9-9c84-4ad95194228e
📒 Files selected for processing (3)
contracts/governance-token/README.mdcontracts/governance-token/src/lib.rsdocs/contracts/governance-token.md
- Replace linear scan with binary search (O(log n)) in checkpoint_at_ledger - Add test_checkpoint_at_ledger_exact_match - Add test_checkpoint_at_ledger_between_checkpoints - Add test_checkpoint_at_ledger_before_all_checkpoints_returns_none - Add test_checkpoint_at_ledger_unknown_holder_returns_none
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
1 participant
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.
Description
Adds voting checkpoint visibility to the governance token contract so callers can inspect a holder's balance history without reconstructing it off-
chain. Each mint, burn, or transfer now writes a Checkpoint { ledger, balance } snapshot for the affected holder(s). Two new read-only accessors
expose this data.
Fixes #466
Type of change
How Has This Been Tested?
Five new unit tests were added in contracts/governance-token/src/lib.rs and run via cargo +nightly test -p stellarcade-governance-token:
ambiguous zero)
duplicate
All 7 tests (5 new + 2 existing) pass.
Checklist:
Summary by CodeRabbit
New Features
Chores
Documentation