Conversation
WalkthroughThis PR harmonizes the governance module's clock mechanism by converting from timestamp-based to ledger sequence-based timing. The timelock module's public API is renamed from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Pull request overview
This PR migrates the governance module from using timestamps (u64) to ledger sequence numbers (u32) as the clock mode, harmonizing the time counting mechanism across the votes and timelock modules. This addresses issue #567 by making the clock mode more natural and consistent with other parts of the library.
Changes:
- Changed checkpoint tracking from timestamps to ledger sequence numbers in the votes module
- Updated timelock operations to use ledger sequence numbers instead of timestamps
- Renamed functions and storage keys to reflect the new clock mode (e.g.,
get_timestamp→get_operation_ledger)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/governance/src/votes/test.rs | Updated all test calls from set_timestamp to set_sequence_number and updated comments |
| packages/governance/src/votes/storage.rs | Changed Checkpoint struct from timestamp: u64 to ledger: u32, updated all related functions and documentation |
| packages/governance/src/votes/mod.rs | Updated API documentation and function signatures to use ledger terminology instead of timestamp |
| packages/governance/src/timelock/test.rs | Updated test calls and function references to use ledger sequence numbers |
| packages/governance/src/timelock/storage.rs | Renamed Timestamp storage key to OperationLedger, updated get_timestamp to get_operation_ledger, changed all u64 types to u32 |
| packages/governance/src/timelock/mod.rs | Updated exports and renamed constants from UNSET_TIMESTAMP/DONE_TIMESTAMP to UNSET_LEDGER/DONE_LEDGER |
| packages/governance/README.md | Updated documentation to reflect ledger sequence number usage |
| examples/timelock-controller/src/test.rs | Updated test code to use sequence_number instead of timestamp |
| examples/timelock-controller/src/contract.rs | Updated function names and documentation to use ledger terminology |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let current_timestamp = e.ledger().timestamp(); | ||
| let ready_timestamp = current_timestamp + (delay as u64); | ||
| let current_ledger = e.ledger().sequence(); | ||
| let ready_ledger = current_ledger + delay; |
There was a problem hiding this comment.
Potential arithmetic overflow: adding current_ledger + delay without overflow checking. If current_ledger and delay sum to a value greater than u32::MAX, this will panic or wrap. Consider using checked_add and returning an appropriate error.
| let ready_ledger = current_ledger + delay; | |
| let ready_ledger = current_ledger.saturating_add(delay); |
| @@ -384,7 +382,7 @@ fn lookup_checkpoint_at( | |||
| while low < high { | |||
| let mid = (low + high).div_ceil(2); | |||
There was a problem hiding this comment.
Potential arithmetic overflow: computing (low + high).div_ceil(2) without overflow checking. If both low and high are large u32 values (e.g., near u32::MAX), their sum could overflow. Consider using low + (high - low) / 2 instead, which avoids overflow.
| let mid = (low + high).div_ceil(2); | |
| let mid = low + (high - low + 1) / 2; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #569 +/- ##
==========================================
- Coverage 96.24% 96.23% -0.01%
==========================================
Files 57 57
Lines 5507 5502 -5
==========================================
- Hits 5300 5295 -5
Misses 207 207 ☔ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/governance/src/timelock/mod.rs`:
- Around line 95-100: UNSET_LEDGER (currently 0) and DONE_LEDGER (currently 1)
may collide with valid ledger sequence numbers; change these sentinel values to
numbers outside the valid ledger range (e.g., u32::MAX or a clearly reserved
constant) or enforce a minimum delay >= 2 so real ledgers cannot equal the
sentinels; update all references/tests/docs that assert 0/1 and adjust any logic
in functions that compare against UNSET_LEDGER or DONE_LEDGER (e.g.,
scheduling/checking code) to use the new sentinel constants and ensure behavior
remains correct when min_delay is validated.
| /// Sentinel value for an operation that has not been scheduled. | ||
| pub const UNSET_TIMESTAMP: u64 = 0; | ||
| pub const UNSET_LEDGER: u32 = 0; | ||
|
|
||
| /// Sentinel value used to mark an operation as done. | ||
| /// Using 1 instead of 0 to distinguish from unset operations. | ||
| pub const DONE_TIMESTAMP: u64 = 1; | ||
| pub const DONE_LEDGER: u32 = 1; |
There was a problem hiding this comment.
Avoid sentinel collisions with real ledger sequence numbers.
UNSET_LEDGER = 0 and DONE_LEDGER = 1 are valid ledger sequences. If min_delay can be 0 and the current ledger is 1, a newly scheduled operation could be classified as Done and become non-executable. Consider sentinel values outside the valid range or enforce a minimum delay ≥ 1. If you change the sentinel values, update docs/tests that assert 0/1.
🔧 Possible sentinel adjustment
-/// Sentinel value for an operation that has not been scheduled.
-pub const UNSET_LEDGER: u32 = 0;
-
-/// Sentinel value used to mark an operation as done.
-/// Using 1 instead of 0 to distinguish from unset operations.
-pub const DONE_LEDGER: u32 = 1;
+/// Sentinel value for an operation that has not been scheduled.
+/// Use high values to avoid collisions with real ledger sequences.
+pub const UNSET_LEDGER: u32 = u32::MAX;
+
+/// Sentinel value used to mark an operation as done.
+pub const DONE_LEDGER: u32 = u32::MAX - 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Sentinel value for an operation that has not been scheduled. | |
| pub const UNSET_TIMESTAMP: u64 = 0; | |
| pub const UNSET_LEDGER: u32 = 0; | |
| /// Sentinel value used to mark an operation as done. | |
| /// Using 1 instead of 0 to distinguish from unset operations. | |
| pub const DONE_TIMESTAMP: u64 = 1; | |
| pub const DONE_LEDGER: u32 = 1; | |
| /// Sentinel value for an operation that has not been scheduled. | |
| /// Use high values to avoid collisions with real ledger sequences. | |
| pub const UNSET_LEDGER: u32 = u32::MAX; | |
| /// Sentinel value used to mark an operation as done. | |
| pub const DONE_LEDGER: u32 = u32::MAX - 1; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/governance/src/timelock/mod.rs` around lines 95 - 100, UNSET_LEDGER
(currently 0) and DONE_LEDGER (currently 1) may collide with valid ledger
sequence numbers; change these sentinel values to numbers outside the valid
ledger range (e.g., u32::MAX or a clearly reserved constant) or enforce a
minimum delay >= 2 so real ledgers cannot equal the sentinels; update all
references/tests/docs that assert 0/1 and adjust any logic in functions that
compare against UNSET_LEDGER or DONE_LEDGER (e.g., scheduling/checking code) to
use the new sentinel constants and ensure behavior remains correct when
min_delay is validated.
1 participant
Fixes #567
PR Checklist
Summary by CodeRabbit
Refactor
Documentation
Tests