fix(staker): change UintTree key type from int64 to uint64#1230
fix(staker): change UintTree key type from int64 to uint64#1230junghoon-vans wants to merge 7 commits intomainfrom
Conversation
UintTree interface was exposing int64 as key type, but negative values were never supported (panicked at runtime). This changes all method signatures to uint64 to enforce the constraint at the type level. Adds explicit non-negative assertions at all int64→uint64 cast sites to prevent silent wrap-around from accidental negative timestamp values.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConsolidates duplicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Same fix as staker: UintTree interface was exposing int64 as key type, but negative values were never supported. Changes all method signatures to uint64 and adds explicit non-negative assertions at all cast sites.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
contract/r/gnoswap/staker/v1/reward_calculation_canonical_env_test.gno (1)
112-122:⚠️ Potential issue | 🔴 CriticalType mismatch in
Iteratecallback: expectsuint64key, receivesint64.The
emissionUpdates.Iteratecallback at line 112 declaresfunc(key int64, value any) bool, but after the PR changes,UintTree.Iteratenow invokes callbacks withkey uint64. This will cause a compilation error.🐛 Proposed fix
- result.emissionUpdates.Iterate(start, end, func(key int64, value any) bool { - heights = append(heights, key) + result.emissionUpdates.Iterate(uint64(start), uint64(end), func(key uint64, value any) bool { + heights = append(heights, int64(key))contract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gno (1)
1519-1519:⚠️ Potential issue | 🔴 CriticalMissing
uint64cast forHistoricalTicktimestamp key.Line 1519 uses
HistoricalTick().Set(baseTime-100, int32(0))without casting touint64, while all other occurrences in this file were updated. This inconsistency will cause a compilation error sinceUintTree.Setnow expectsuint64.🐛 Proposed fix
- poolResolver.HistoricalTick().Set(baseTime-100, int32(0)) + poolResolver.HistoricalTick().Set(uint64(baseTime-100), int32(0))contract/r/gnoswap/staker/pool.gno (1)
187-208:⚠️ Potential issue | 🟡 MinorGuard
currentTimebefore seeding theUintTrees.
NewPoolnow writes threeuint64(currentTime)keys, but this constructor still accepts anyint64. A negative value will wrap and initialize the pool with misordered timestamp state.Suggested fix
func NewPool(poolPath string, currentTime int64) *Pool { + if currentTime < 0 { + panic("currentTime must be non-negative") + } + pool := &Pool{ poolPath: poolPath, stakedLiquidity: NewUintTree(),contract/r/gnoswap/staker/v1/external_incentive_test.gno (2)
1485-1495:⚠️ Potential issue | 🟠 MajorUpdate this panic expectation to the new guard message.
addIncentiveIdByCreationTimenow panics with"creationTime must be non-negative", so this case will fail even when the negative-input path is working correctly.Suggested fix
- expectedPanicMessage: "negative value not supported", + expectedPanicMessage: "creationTime must be non-negative",
275-277:⚠️ Potential issue | 🔴 CriticalFix type mismatch: UintTree requires uint64 keys.
sr.NewUintTree()exposesGet/Set(key uint64, ...)methods, but the test helper at lines 275–321 declarestimestampasint64and passes it directly to these methods. Go does not allow implicit int64→uint64 conversion; these calls will not compile.Wrap
timestampwithuint64()at all call sites:Required conversions
- tree.Set(timestamp, poolIncentives) + tree.Set(uint64(timestamp), poolIncentives) - value, exists := tree.Get(timestamp) + value, exists := tree.Get(uint64(timestamp)) - tree.Set(timestamp, poolTree) + tree.Set(uint64(timestamp), poolTree) - value2, _ := tree.Get(timestamp) + value2, _ := tree.Get(uint64(timestamp)) - tree.Set(timestamp, poolTree2) + tree.Set(uint64(timestamp), poolTree2) - value3, _ := tree.Get(timestamp) + value3, _ := tree.Get(uint64(timestamp))Also update the panic expectation at line 1495: the actual implementation panics with
"creationTime must be non-negative", not"negative value not supported".contract/r/gnoswap/staker/v1/getter_test.gno (1)
688-690:⚠️ Potential issue | 🔴 CriticalCast
int64timestamps touint64forUintTree.SetcallsLines 688-690 and 756-758 pass
int64timestamps directly toRewardCache().Set()andGlobalRewardRatioAccumulation().Set(), but both methods requireuint64keys. The type mismatch will cause compilation to fail.Suggested fix
- state.pool.RewardCache().Set(state.fixedTimestamp, int64(1000)) - state.pool.RewardCache().Set(state.fixedTimestamp+100, int64(2000)) - state.pool.RewardCache().Set(state.fixedTimestamp+200, int64(3000)) + state.pool.RewardCache().Set(uint64(state.fixedTimestamp), int64(1000)) + state.pool.RewardCache().Set(uint64(state.fixedTimestamp+100), int64(2000)) + state.pool.RewardCache().Set(uint64(state.fixedTimestamp+200), int64(3000)) @@ - state.pool.GlobalRewardRatioAccumulation().Set(state.fixedTimestamp+100, acc1) - state.pool.GlobalRewardRatioAccumulation().Set(state.fixedTimestamp+200, acc2) - state.pool.GlobalRewardRatioAccumulation().Set(state.fixedTimestamp+300, acc3) + state.pool.GlobalRewardRatioAccumulation().Set(uint64(state.fixedTimestamp+100), acc1) + state.pool.GlobalRewardRatioAccumulation().Set(uint64(state.fixedTimestamp+200), acc2) + state.pool.GlobalRewardRatioAccumulation().Set(uint64(state.fixedTimestamp+300), acc3)
🧹 Nitpick comments (3)
contract/r/gnoswap/staker/v1/reward_calculation_types.gno (1)
21-50: Consider extractingEncodeUint/DecodeUintto a shared package.The
EncodeUintandDecodeUintfunctions are duplicated verbatim in bothtree.gnoand this file (as noted in context snippet 1). While Gno's package model may necessitate some duplication, consider extracting these utilities to a sharedp/package to reduce maintenance burden and ensure consistent behavior.contract/r/gnoswap/staker/v1/getter.gno (2)
466-470: Consider documenting theuint64→int64narrowing conversion.The callback receives
key uint64but converts toint64for the return type. While this is safe for realistic timestamp values (which fit comfortably inint64), this reverse conversion (from the newuint64key type back toint64) is the only such narrowing in this file. If the return type[]int64is intentional for API stability, a brief comment explaining why would help future maintainers.
572-574: Misleading parameter name:tickshould betimestamp.The parameter
tick uint64is actually a timestamp/key used to look up a historical tick value, not a tick itself. The function retrievesint32tick values stored at timestamp keys. This inconsistency with other similar functions (e.g.,GetPoolRewardCache(poolPath string, timestamp uint64)on line 476) could confuse callers.🔧 Suggested fix for parameter naming
// GetPoolHistoricalTick returns the historical tick at a specific timestamp for a pool. -func (s *stakerV1) GetPoolHistoricalTick(poolPath string, tick uint64) int32 { +func (s *stakerV1) GetPoolHistoricalTick(poolPath string, timestamp uint64) int32 { pool := s.getPoolByPoolPath(poolPath) - value, ok := pool.HistoricalTick().Get(tick) + value, ok := pool.HistoricalTick().Get(timestamp) if !ok { return 0 }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fada9db-700f-448d-9087-79de2f645c65
📒 Files selected for processing (17)
contract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/external_incentive_test.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_canonical_env_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_incentives.gnocontract/r/gnoswap/staker/v1/reward_calculation_incentives_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool.gnocontract/r/gnoswap/staker/v1/reward_calculation_tick.gnocontract/r/gnoswap/staker/v1/reward_calculation_tick_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_types.gnocontract/r/gnoswap/staker/v1/reward_calculation_types_test.gnocontract/r/gnoswap/staker/v1/staker.gno
Update remaining test files to use uint64 casts when passing int64 timestamps to UintTree methods.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contract/r/gnoswap/gov/staker/v1/getter.gno (1)
146-154:⚠️ Potential issue | 🟡 MinorValidate
snapshotTimebefore the empty-history fast paths.Right now the new panic only happens when history already exists.
GetTotalDelegationAmountAtSnapshot(-1)andGetUserDelegationAmountAtSnapshot(..., -1)both return(0, false)on empty/missing history, so invalid-input behavior depends on stored state. Move the check to the top of each function, and add one regression test for the empty-history case.💡 Suggested change
func (gs *govStakerV1) GetTotalDelegationAmountAtSnapshot(snapshotTime int64) (int64, bool) { + if snapshotTime < 0 { + panic("snapshotTime must be non-negative") + } history := gs.store.GetTotalDelegationHistory() if history.Size() == 0 { return 0, false } - - if snapshotTime < 0 { - panic("snapshotTime must be non-negative") - }Apply the same move in
GetUserDelegationAmountAtSnapshot.Also applies to: 192-208
contract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gno (1)
97-124:⚠️ Potential issue | 🟡 Minor
math.MaxInt64cutoffs can overwrite the real boundary snapshot.These helpers still build an exclusive upper bound with
cutoffTimestamp+1, except formath.MaxInt64. In that sentinel case,ReverseIterate(0, uint64(toTimestamp), ...)skips an entry stored exactly at the cutoff, and the followingSet(uint64(cutoffTimestamp), lastValue)replaces it with the previous value. For these timestamp-backed trees, usemath.MaxUint64as the upper bound whencutoffTimestamp == math.MaxInt64, and add a regression test for that boundary. The same upper-bound helper is worth applying in the snapshot getters too.💡 Possible fix
- toTimestamp := cutoffTimestamp - if cutoffTimestamp < math.MaxInt64 { - toTimestamp = safeAddInt64(toTimestamp, 1) - } + upperBound := math.MaxUint64 + if cutoffTimestamp < math.MaxInt64 { + upperBound = uint64(safeAddInt64(cutoffTimestamp, 1)) + } ... - history.ReverseIterate(0, uint64(toTimestamp), func(timestamp uint64, value any) bool { + history.ReverseIterate(0, upperBound, func(timestamp uint64, value any) bool {Apply the same change in
cleanUserDelegationHistory.Also applies to: 139-171
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04e6d5d7-80ed-4809-9aaa-424daae29afd
📒 Files selected for processing (8)
contract/r/gnoswap/gov/staker/tree.gnocontract/r/gnoswap/gov/staker/v1/getter.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot_cleanup_test.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot_test.gnocontract/r/gnoswap/gov/staker/v1/state.gnocontract/r/gnoswap/gov/staker/v1/state_delegation_history_test.gnocontract/r/gnoswap/gov/staker/v1/state_test.gno
✅ Files skipped from review due to trivial changes (1)
- contract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot_test.gno
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contract/r/gnoswap/gov/staker/v1/getter.gno (2)
155-166:⚠️ Potential issue | 🟠 MajorReconcile the
ReverseIterateByInt64end-bound contract before keepingsnapshotTime + 1.The new test in
contract/r/gnoswap/gov/staker/tree_test.gnotreatsReverseIterateByInt64(0, 10, ...)as including key10. If that is the intended contract, these snapshot getters now overshoot and can return the first record atT+1for a query atT.💡 If the upper bound is intended to be inclusive
- toTimestamp := snapshotTime - if toTimestamp < math.MaxInt64 { - toTimestamp = toTimestamp + 1 - } - - history.ReverseIterateByInt64(0, toTimestamp, func(key uint64, value any) bool { + history.ReverseIterateByInt64(0, snapshotTime, func(key uint64, value any) bool { @@ - toTimestamp := snapshotTime - if toTimestamp < math.MaxInt64 { - toTimestamp = toTimestamp + 1 - } - - userHistory.ReverseIterateByInt64(0, toTimestamp, func(key uint64, value any) bool { + userHistory.ReverseIterateByInt64(0, snapshotTime, func(key uint64, value any) bool {Also applies to: 209-220
147-154:⚠️ Potential issue | 🟡 MinorValidate negative
snapshotTimebefore the empty-history fast paths.Right now the same invalid input panics only when history exists; if the tree is empty or the user has no history, it returns
(0, false)instead. That makes the new non-negative contract state-dependent.Also applies to: 193-208
🧹 Nitpick comments (3)
contract/r/gnoswap/staker/v1/reward_calculation_pool.gno (1)
82-84: Extract duplicate panic message to a constant.The literal
"currentTime must be non-negative"and similar messages are duplicated 6+ times across this file. Per SonarCloud analysis, extract to a package-level constant for maintainability.♻️ Proposed refactor
Add at package level:
const errNonNegativeTime = "time value must be non-negative"Then replace all duplicate panic messages:
func (self *PoolResolver) CurrentGlobalRewardRatioAccumulation(currentTime int64) (time int64, acc *u256.Uint) { if currentTime < 0 { - panic("currentTime must be non-negative") + panic(errNonNegativeTime) }contract/r/gnoswap/staker/tree_test.gno (1)
32-42: Good panic test, but consider testing edge cases.The test correctly verifies that negative keys trigger the expected panic. Consider adding optional tests for boundary conditions:
- Key
0(minimum valid)- Key
math.MaxInt64(maximum valid int64 → fits in uint64)🧪 Optional edge-case test
func TestUintTreeSetByInt64EdgeCases(t *testing.T) { tree := NewUintTree() // Zero is valid tree.SetByInt64(0, "zero") v, ok := tree.GetByInt64(0) uassert.True(t, ok) uassert.Equal(t, "zero", v) // Max int64 is valid tree.SetByInt64(math.MaxInt64, "max") v, ok = tree.GetByInt64(math.MaxInt64) uassert.True(t, ok) uassert.Equal(t, "max", v) }contract/r/gnoswap/gov/staker/tree_test.gno (1)
18-42: Add a boundary regression for adjacent keys andmath.MaxInt64.These tests cover the basic adapter flow, but the fragile edge in this migration is the signed upper bound and nearby keys (
TvsT+1). A targeted case there would lock the intended end-bound semantics and catch snapshot off-by-one regressions much earlier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fafad499-6211-4389-a1b1-6362236f55cd
📒 Files selected for processing (17)
contract/r/gnoswap/gov/staker/tree.gnocontract/r/gnoswap/gov/staker/tree_test.gnocontract/r/gnoswap/gov/staker/util.gnocontract/r/gnoswap/gov/staker/v1/getter.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gnocontract/r/gnoswap/gov/staker/v1/state.gnocontract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/tree_test.gnocontract/r/gnoswap/staker/util.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gnocontract/r/gnoswap/staker/v1/external_incentive_test.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_canonical_env_test.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool.gnocontract/r/gnoswap/staker/v1/reward_calculation_tick.gnocontract/r/gnoswap/staker/v1/staker.gno
✅ Files skipped from review due to trivial changes (2)
- contract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gno
- contract/r/gnoswap/staker/v1/getter_test.gno
🚧 Files skipped from review as they are similar to previous changes (6)
- contract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gno
- contract/r/gnoswap/staker/v1/reward_calculation_tick.gno
- contract/r/gnoswap/staker/v1/staker.gno
- contract/r/gnoswap/staker/v1/external_incentive_test.gno
- contract/r/gnoswap/staker/v1/reward_calculation_canonical_env_test.gno
- contract/r/gnoswap/staker/pool.gno
Route timestamped tree access through package-level int64 conversion helpers and ByInt64 UintTree methods so staker and gov/staker stop repeating raw uint64 casts at call sites.
junghoon-vans
force-pushed
the
worktree-fix-uinttree-uint64
branch
2 times, most recently
from
22f5ec8 to
f8c1c82
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
contract/p/gnoswap/uinttree/uinttree.gno (1)
10-118: Add doc comments for the new sharedUintTreeAPI.This introduces a public package surface, but none of the exported helpers document their panic behavior or iterator/range semantics. A short doc pass here would make the signed→unsigned migration much safer for downstream callers.
As per coding guidelines, "Exported functions have documentation".
contract/p/gnoswap/uinttree/uinttree_test.gno (1)
7-50: Please lock down the boundary cases in this new test suite.The migration risk is at the edges, not the happy path. I'd add explicit cases for
math.MaxInt64and forIterateByInt64/ReverseIterateByInt64bound inclusivity so future changes can't silently reintroduce wrap-around or off-by-one behavior.contract/r/gnoswap/staker/v1/reward_calculation_pool.gno (1)
82-84: Consider one helper for the repeated time precondition.The same panic pattern is duplicated across the file now. A tiny helper like
mustNonNegativeTime(name, value)would keep the behavior and message aligned if another call site gets added later, and it should also clear the repeated-literal lint.Also applies to: 105-107, 120-122, 165-167, 187-189, 295-300, 460-462
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d70bf62-30b2-4d7a-8a35-2017467c184a
📒 Files selected for processing (21)
contract/p/gnoswap/uinttree/doc.gnocontract/p/gnoswap/uinttree/gnomod.tomlcontract/p/gnoswap/uinttree/uinttree.gnocontract/p/gnoswap/uinttree/uinttree_test.gnocontract/r/gnoswap/gov/staker/tree.gnocontract/r/gnoswap/gov/staker/tree_compat.gnocontract/r/gnoswap/gov/staker/tree_test.gnocontract/r/gnoswap/gov/staker/v1/getter.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gnocontract/r/gnoswap/gov/staker/v1/state.gnocontract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/tree_compat.gnocontract/r/gnoswap/staker/tree_test.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/reward_calculation_incentives.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool.gnocontract/r/gnoswap/staker/v1/reward_calculation_tick.gnocontract/r/gnoswap/staker/v1/reward_calculation_types.gnocontract/r/gnoswap/staker/v1/staker.gno
💤 Files with no reviewable changes (2)
- contract/r/gnoswap/staker/tree.gno
- contract/r/gnoswap/gov/staker/tree.gno
✅ Files skipped from review due to trivial changes (6)
- contract/p/gnoswap/uinttree/doc.gno
- contract/p/gnoswap/uinttree/gnomod.toml
- contract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gno
- contract/r/gnoswap/staker/pool.gno
- contract/r/gnoswap/gov/staker/tree_test.gno
- contract/r/gnoswap/staker/tree_test.gno
🚧 Files skipped from review as they are similar to previous changes (6)
- contract/r/gnoswap/staker/v1/staker.gno
- contract/r/gnoswap/staker/v1/calculate_pool_position_reward.gno
- contract/r/gnoswap/staker/v1/reward_calculation_tick.gno
- contract/r/gnoswap/gov/staker/v1/getter.gno
- contract/r/gnoswap/staker/v1/external_incentive.gno
- contract/r/gnoswap/staker/v1/reward_calculation_types.gno
junghoon-vans
force-pushed
the
worktree-fix-uinttree-uint64
branch
from
f8c1c82 to
e3c7b1b
Compare
Move the duplicated realm UintTree wrappers into p/gnoswap/uinttree so staker and gov/staker share one implementation while keeping realm-local compatibility helpers.
junghoon-vans
force-pushed
the
worktree-fix-uinttree-uint64
branch
from
e3c7b1b to
806e64e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contract/p/gnoswap/uinttree/tree.gno (1)
10-118: [MEDIUM] Rule: Document the exporteduinttreeAPI.This new shared package exposes constructors, iterators, conversion helpers, and
Clonewithout any contract docs. Consumers need the negative-int64panic behavior, the iterator callback stop convention, and the fact that Lines 90-91 only copy value references spelled out before this becomes the canonical tree implementation.As per coding guidelines,
Exported functions have documentation.contract/p/gnoswap/uinttree/tree_test.gno (1)
20-33: Strengthen the reverse-iterate coverage.Lines 26-28 stop on the first callback, so this only proves the first returned element is
"ten". It still passes if later in-range entries are skipped or the lower bound is mishandled.Proposed test shape
func TestUintTreeReverseIterateByInt64(t *testing.T) { tree := NewUintTree() + tree.SetByInt64(0, "zero") tree.SetByInt64(5, "five") tree.SetByInt64(10, "ten") + tree.SetByInt64(11, "eleven") - var latest string + var visited []string tree.ReverseIterateByInt64(0, 10, func(_ uint64, value any) bool { - latest = value.(string) - return true + visited = append(visited, value.(string)) + return false }) - if latest != "ten" { - t.Fatalf("expected ten, got %s", latest) + if len(visited) != 3 || visited[0] != "ten" || visited[1] != "five" || visited[2] != "zero" { + t.Fatalf("unexpected reverse order: %v", visited) } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d934924d-779f-4c5b-bd8b-98b09da9f11d
📒 Files selected for processing (8)
contract/p/gnoswap/uinttree/doc.gnocontract/p/gnoswap/uinttree/gnomod.tomlcontract/p/gnoswap/uinttree/tree.gnocontract/p/gnoswap/uinttree/tree_test.gnocontract/r/gnoswap/gov/staker/tree_compat.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/tree_compat.gnocontract/r/gnoswap/staker/v1/reward_calculation_types.gno
💤 Files with no reviewable changes (1)
- contract/r/gnoswap/staker/tree.gno
✅ Files skipped from review due to trivial changes (4)
- contract/p/gnoswap/uinttree/gnomod.toml
- contract/p/gnoswap/uinttree/doc.gno
- contract/r/gnoswap/gov/staker/tree_compat.gno
- contract/r/gnoswap/staker/tree_compat.gno
Switch the remaining timestamp-keyed tests to ByInt64 and drop non-negative checks that only protected old uint64 conversions, so tree-boundary validation stays centralized.
Remove the remaining alias, compatibility, and wrapper helpers so staker and gov/staker call the shared uinttree package directly for construction and key encoding.
|
jinoosss
left a comment
jinoosss
left a comment
There was a problem hiding this comment.
It would be good to discuss the direction of uinttree.
Since the range of keys actually used is int64, we need to determine whether it would be better to use the uint64 range.
3 participants
Summary
UintTreemethod signatures fromint64touint64in bothstakerandgov/stakerpackagesEncodeInt64/DecodeInt64helper functions (now usesEncodeUint/DecodeUintdirectly)int64 → uint64cast site to prevent silent wrap-aroundBackground
UintTreestores block timestamps and other non-negative values as keys. The interface previously acceptedint64and panicked at runtime on negative input. This change enforces the constraint at the type level by usinguint64, and adds defensive< 0checks at all call sites whereint64values are cast touint64.Changes
contract/r/gnoswap/staker/tree.gno:UintTreemethod signaturesint64→uint64v1/reward_calculation_types.gno: same (localUintTreecopy)v1/reward_calculation_pool.gno: assertions + casts inCurrentGlobalRewardRatioAccumulation,CurrentTick,CurrentStakedLiquidity,CurrentReward,cacheReward,calculateInternalReward,modifyDepositv1/reward_calculation_tick.gno: assertions + casts inCurrentOutsideAccumulation,updateCurrentOutsideAccumulationv1/reward_calculation_incentives.gno: assertions + casts instartUnclaimablePeriod,endUnclaimablePeriod,calculateUnclaimableRewardv1/staker.gno: assertion + cast forcurrentTimev1/calculate_pool_position_reward.gno: assertions + casts ingetExternalIncentiveIdsByv1/external_incentive.gno: assertions + casts inaddIncentiveIdByCreationTime,removeIncentiveIdByCreationTimev1/getter.gno: updatedIterateByOffsetcallbacks andGetcallscontract/r/gnoswap/gov/staker/tree.gno: sameUintTreefixv1/state.gno: assertions + casts inupdateTotalDelegationHistory,updateUserDelegationHistory,getLatestTotalDelegation,getLatestUserDelegationv1/staker_delegation_snapshot.gno: assertions + casts incleanTotalDelegationHistory,cleanUserDelegationHistoryv1/getter.gno: assertions + casts in delegation snapshot query functionsTest plan
gno test -v ./contract/r/gnoswap/staker/...—0 build errorsgno test -v ./contract/r/gnoswap/gov/staker/...—0 build errorsCloses #GSW-2546
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests