feat(staker): add missing getters and remove API functions #1146
Conversation
|
Warning Rate limit exceeded@junghoon-vans has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughRemoves the v1 JSON API/serialization layer and its tests; replaces ApiGet* JSON-returning endpoints with strongly-typed Get* getters across staker (core, v1, mocks, test wrappers); adds deep-clone helpers and many read-only accessors; standardizes numeric formatting to formatAnyInt in v1 code and events. 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. 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: 1
🧹 Nitpick comments (2)
contract/r/gnoswap/staker/v1/getter.gno (2)
249-254: Note:GetDepositStakeTimestampduplicatesGetDepositStakeTimefunctionality.Both methods (lines 249-254 and 200-205) return
deposit.StakeTime(). Consider consolidating into a single method or documenting the distinction if there's a semantic difference.
369-378: Consider performance implications ofGetTotalStakedPositionCount.This method iterates over all stakers and their deposit trees to count positions, resulting in O(n×m) complexity where n is the number of stakers and m is the average deposits per staker. For large deployments, this could be expensive.
Consider maintaining a running counter that's updated on stake/unstake operations if this getter is called frequently.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
contract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/getter.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/api.gnocontract/r/gnoswap/staker/v1/api_test.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gnocontract/r/gnoswap/staker/v1/external_deposit_fee.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/v1/json.gnocontract/r/gnoswap/staker/v1/json_test.gnocontract/r/gnoswap/staker/v1/manage_pool_tier_and_warmup.gnocontract/r/gnoswap/staker/v1/staker.gno
💤 Files with no reviewable changes (5)
- contract/r/gnoswap/staker/v1/api_test.gno
- contract/r/gnoswap/staker/v1/json_test.gno
- contract/r/gnoswap/staker/v1/getter_test.gno
- contract/r/gnoswap/staker/v1/api.gno
- contract/r/gnoswap/staker/v1/json.gno
🧰 Additional context used
📓 Path-based instructions (1)
**/*.gno
⚙️ CodeRabbit configuration file
**/*.gno: # Gno Smart Contract Code Review RulesAbout Gno
Gno is a blockchain smart contract language based on Go. Key differences from Go:
- Realms (r/): Stateful contracts with persistent storage
- Packages (p/): Pure library code without state
- Realm crossing: Explicit syntax for inter-realm calls using
crosskeyword- Access control: Uses
runtime.PreviousRealm()andruntime.OriginCaller()for caller verification- State persistence: Global variables in realms persist across transactions
- Determinism: No goroutines, limited stdlib, panic-based error handling in realms
Critical Security Violations
Flag immediately as [CRITICAL]:
- Exported global variables - Allows anyone to modify state
var Admin address // CRITICAL: Anyone can reassign
- Missing access control - No caller verification on state mutations
func DeleteData(_ realm) { data.Clear() // CRITICAL: Anyone can delete }
- Error return for security - Transaction succeeds even when unauthorized
func AdminOnly(_ realm) error { if !isAdmin() { return errors.New("denied") // CRITICAL: Should panic } }
- Direct external realm modification - Readonly taint violation
otherrealm.GlobalVar = "hacked" // CRITICAL: Cannot modify external realm stateRequired Patterns
Access Control (MUST on all state mutations)
Every function that modifies state MUST verify the caller:
import "chain/runtime" func MutatingFunction(cur realm, params) { // For immediate caller (could be user or realm) caller := runtime.PreviousRealm().Address() // For original transaction signer (always a user) signer := runtime.OriginCaller() if caller != authorized { panic("unauthorized") // MUST panic, not return error } // ... mutation logic ... }Why panic? Panics abort and rollback the transaction. Returning errors allows unauthorized transactions t...
Files:
contract/r/gnoswap/staker/v1/manage_pool_tier_and_warmup.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gnocontract/r/gnoswap/staker/v1/external_deposit_fee.gnocontract/r/gnoswap/staker/v1/staker.gnocontract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/getter.gno
🧠 Learnings (1)
📚 Learning: 2026-01-05T03:20:38.341Z
Learnt from: notJoon
Repo: gnoswap-labs/gnoswap PR: 1118
File: contract/r/gnoswap/access/access.gno:37-43
Timestamp: 2026-01-05T03:20:38.341Z
Learning: In the gnoswap codebase, enforce consistency where event attribute names use prevAddr to refer to the caller/previous realm address in emitted events, as this is the established convention. When reviewing or adding new contracts, prefer using prevAddr over caller for event fields; if a semantic renaming is ever considered, document the rationale and ensure all related events across contracts are updated to maintain consistency.
Applied to files:
contract/r/gnoswap/staker/v1/manage_pool_tier_and_warmup.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gnocontract/r/gnoswap/staker/v1/external_deposit_fee.gnocontract/r/gnoswap/staker/v1/staker.gnocontract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/getter.gno
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/staker, contract/r/gnoswap/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/store, contract/p/gnoswap/store)
- GitHub Check: run-integration-test
🔇 Additional comments (17)
contract/r/gnoswap/staker/v1/calculate_pool_position_reward_test.gno (1)
1872-1873: LGTM! Test expectations aligned with production formatting.The update from
formatInttoformatAnyIntcorrectly aligns test expectations with the formatting changes in production code. This ensures tests validate the actual emitted event format.contract/r/gnoswap/staker/v1/manage_pool_tier_and_warmup.gno (1)
38-39: LGTM! Consistent formatting updates across administrative functions.The formatting changes from
formatInttoformatAnyIntare consistently applied across all pool tier and warmup management functions. This aligns with the broader refactoring effort across the staker module.Also applies to: 65-66, 89-90, 109-110
contract/r/gnoswap/staker/v1/external_incentive.gno (1)
118-123: Verify downstream compatibility for incentive event format changes.The formatting changes affect critical fields in
CreateExternalIncentiveandEndExternalIncentiveevents (reward amounts, timestamps, deposit amounts). Ensure that downstream consumers (indexers, front-end applications, monitoring systems) can handle the new format produced byformatAnyInt.If you have integration tests or can verify with off-chain indexers, confirm they parse these events correctly. You may also want to check documentation to ensure the event schema is updated if necessary.
Also applies to: 228-229
contract/r/gnoswap/staker/v1/staker.gno (2)
383-385: Formatting change for tick values in StakeToken event.The change from
formatInttoformatAnyIntforpositionUpperTick,positionLowerTick, andcurrentTickshould maintain the same numeric representation. Tick values are int32 and are critical for position range tracking.
495-497: Verify widespread formatting changes don't break event consumers.The formatting changes in
CollectRewardaffect multiple high-frequency events with critical financial data (reward amounts, timestamps, penalties). Given the scope of these changes across the reward collection flow, ensure:
- Off-chain indexers and UIs correctly parse the new format
- Historical event data remains interpretable
- Any event-based alerting or monitoring systems are updated
#!/bin/bash # Verify formatAnyInt is consistently used across all staker v1 events rg -n 'chain\.Emit' contract/r/gnoswap/staker/v1/ --type=gno | rg -v 'formatAnyInt' | rg 'format(Int|Uint)'This script will find any remaining uses of the old formatters in event emissions that might need updating for consistency.
Also applies to: 515-519, 580-583, 594-601, 617-617, 667-668, 678-683
contract/r/gnoswap/staker/v1/external_deposit_fee.gno (1)
70-71: No action needed —formatAnyIntcorrectly handles int64 values.The
formatAnyIntfunction incontract/r/gnoswap/staker/v1/utils.gnohas a dedicated case forint64that directly callsstrconv.FormatInt(v, 10), which is the standard Go approach for converting int64 to base-10 string representation. When called with int64 values (bothprevDepositGnsAmountandamountare int64), the function produces the correct output format suitable for event emission and downstream consumers.contract/r/gnoswap/staker/_mock_test.gno (2)
139-193: LGTM!The new incentive-related mock getters follow a consistent pattern and correctly handle the case when data is not found by returning sensible defaults.
483-521: LGTM!The aggregate getter mocks (
GetTotalEmissionSent,GetAllowedTokens,GetWarmupTemplate,GetTotalStakedPositionCount,GetStakedPositionsByOwner) are implemented correctly with appropriate default values.contract/r/gnoswap/staker/types.gno (1)
53-103: LGTM! Interface expansion is well-structured.The
IStakerGetterinterface additions are comprehensive and follow consistent naming conventions. The new methods appropriately expose incentive state, deposit details, and aggregate staking data.contract/r/gnoswap/staker/getter.gno (4)
12-45: LGTM!The new incentive-related getter wrappers are well-documented and correctly delegate to the implementation layer.
62-75: LGTM!The deposit-related getter wrappers maintain the established pattern with clear documentation.
177-185: LGTM!The pool getter wrappers (
GetPoolStakedLiquidity,GetPoolsByTier) are correctly implemented.
227-250: LGTM!The aggregate getter wrappers expose system-wide state appropriately with clear documentation.
contract/r/gnoswap/staker/v1/getter.gno (4)
353-366: Good defensive copying practice.The slice copying in
GetAllowedTokensandGetWarmupTemplateprevents external callers from modifying internal state. This is a good security practice.
380-395: LGTM!The
GetStakedPositionsByOwnerimplementation correctly handles the case when the owner has no staked positions by returning an empty slice. The pre-allocation withdepositTree.Size()is a nice optimization.
403-417: LGTM!The initialization change to
make([]sr.ExternalIncentive, 0)is a good practice, ensuring a non-nil empty slice is returned when no incentives match.
136-143: No issues withtime.Now()usage — this is the correct pattern in Gno.The
IsIncentiveActivemethod correctly usestime.Now().Unix()to check incentive timing. In Gno,time.Now()returns the deterministic block/transaction inclusion timestamp (not the host OS clock), ensuring all validators see the same value. This is the intended way to gate on-chain logic based on time.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gnotests/integration/testdata/staker/end_non_existent_external_incentive_fail.txtartests/integration/testdata/staker/staker_create_external_incentive.txtar
🧰 Additional context used
📓 Path-based instructions (1)
**/*.gno
⚙️ CodeRabbit configuration file
**/*.gno: # Gno Smart Contract Code Review RulesAbout Gno
Gno is a blockchain smart contract language based on Go. Key differences from Go:
- Realms (r/): Stateful contracts with persistent storage
- Packages (p/): Pure library code without state
- Realm crossing: Explicit syntax for inter-realm calls using
crosskeyword- Access control: Uses
runtime.PreviousRealm()andruntime.OriginCaller()for caller verification- State persistence: Global variables in realms persist across transactions
- Determinism: No goroutines, limited stdlib, panic-based error handling in realms
Critical Security Violations
Flag immediately as [CRITICAL]:
- Exported global variables - Allows anyone to modify state
var Admin address // CRITICAL: Anyone can reassign
- Missing access control - No caller verification on state mutations
func DeleteData(_ realm) { data.Clear() // CRITICAL: Anyone can delete }
- Error return for security - Transaction succeeds even when unauthorized
func AdminOnly(_ realm) error { if !isAdmin() { return errors.New("denied") // CRITICAL: Should panic } }
- Direct external realm modification - Readonly taint violation
otherrealm.GlobalVar = "hacked" // CRITICAL: Cannot modify external realm stateRequired Patterns
Access Control (MUST on all state mutations)
Every function that modifies state MUST verify the caller:
import "chain/runtime" func MutatingFunction(cur realm, params) { // For immediate caller (could be user or realm) caller := runtime.PreviousRealm().Address() // For original transaction signer (always a user) signer := runtime.OriginCaller() if caller != authorized { panic("unauthorized") // MUST panic, not return error } // ... mutation logic ... }Why panic? Panics abort and rollback the transaction. Returning errors allows unauthorized transactions t...
Files:
contract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gno
🧠 Learnings (1)
📚 Learning: 2026-01-05T03:20:38.341Z
Learnt from: notJoon
Repo: gnoswap-labs/gnoswap PR: 1118
File: contract/r/gnoswap/access/access.gno:37-43
Timestamp: 2026-01-05T03:20:38.341Z
Learning: In the gnoswap codebase, enforce consistency where event attribute names use prevAddr to refer to the caller/previous realm address in emitted events, as this is the established convention. When reviewing or adding new contracts, prefer using prevAddr over caller for event fields; if a semantic renaming is ever considered, document the rationale and ensure all related events across contracts are updated to maintain consistency.
Applied to files:
contract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gno
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (6)
contract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gno (1)
166-213: LGTM! New getter implementations follow the established delegation pattern correctly.All 18 new getter methods consistently implement the activation-guarded delegation pattern used throughout this test wrapper:
- Proper activation checks using method name strings
- Clear panic messages when methods are not activated
- Correct delegation to the underlying instance
- Appropriate return types for each accessor
The additions properly expand the test wrapper's API surface to match the updated staker interface without introducing inconsistencies.
Also applies to: 236-255, 313-318, 397-409, 467-499
contract/r/scenario/upgrade/implements/mock/staker/test_impl.gno (1)
189-596: LGTM! New getter methods properly implemented.All 18 new getter methods (GetIncentiveCreatedTimestamp, GetIncentiveTotalRewardAmount, GetIncentiveDistributedRewardAmount, GetIncentiveRemainingRewardAmount, GetIncentiveDepositGnsAmount, GetIncentiveRefunded, IsIncentiveActive, GetDepositStakeTimestamp, GetDepositCollectedInternalReward, GetDepositCollectedExternalReward, GetDepositExternalIncentiveIdList, GetPoolStakedLiquidity, GetPoolsByTier, GetTotalEmissionSent, GetAllowedTokens, GetWarmupTemplate, GetTotalStakedPositionCount, GetStakedPositionsByOwner) correctly follow the ExecuteFn delegation pattern. Type assertions match return types, and the implementation maintains mockability for testing scenarios.
contract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gno (1)
97-287: LGTM! New getter methods properly delegated.All 18 new getter methods correctly delegate to the underlying instance. The straightforward pass-through pattern is appropriate for the v3_valid test scenario and maintains consistency with the existing delegation approach in this wrapper.
contract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gno (1)
161-495: LGTM! New getter methods with proper activation guards.All 18 new getter methods correctly implement the activation-guard pattern, checking
isActive()before delegating to the underlying instance. Panic messages are consistent and descriptive. This implementation is appropriate for the v2_invalid test scenario to verify upgrade activation behavior.tests/integration/testdata/staker/staker_create_external_incentive.txtar (2)
55-55: LGTM: Whitespace cleanup.Trailing space removal improves formatting consistency.
110-110: The functionGetExternalIncentiveByPoolPathis properly defined and exported in the staker package (contract/r/gnoswap/staker/getter.gno), with matching implementations across v1 and test code. Despite the singular name, it returns[]ExternalIncentive(an array), so the test expectations remain compatible. No references to the oldApiGetExternalIncentivesByPoolPathAPI remain in the codebase. The naming change is intentional and aligns with the PR objective of replacing JSON API getters with strongly-typed getters.
tests/integration/testdata/staker/end_non_existent_external_incentive_fail.txtar
Show resolved
Hide resolved
tests/integration/testdata/staker/end_non_existent_external_incentive_fail.txtar
Show resolved
Hide resolved
| SetUnStakingFee(fee int64) | ||
| } | ||
|
|
||
| type IStakerGetter interface { |
There was a problem hiding this comment.
We need functions to get Pool, Incentive, and Deposit.
And We also need a getter to query the available rewards.
- CollectableEmissionReward(positionId uint64) int64
- CollectableExternalIncentiveReward(positionId uint64, incentiveId string) int64
getter functions
`GetStakedPositionsByUser`
`GetTotalStakedUserCount`, `GetTotalStakedUserPositionCount(user address)` methods
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
contract/r/gnoswap/staker/tree.gno (1)
80-124: Consider documenting or restricting the default case in clone helpers.The
cloneUintTreeValueandcloneAvlTreeValuefunctions have default cases (lines 92-93, 121-122) that return values as-is without cloning. If unknown pointer types are stored in the tree, they won't be deep copied, potentially allowing external callers to mutate internal state.Consider either:
- Adding a panic in the default case to fail-fast if unexpected types are encountered, or
- Documenting that only the listed types should be stored in these trees.
🛡️ Option 1: Fail-fast on unexpected types
func cloneUintTreeValue(value any) any { switch v := value.(type) { case *u256.Uint: return v.Clone() case *avl.Tree: return cloneAvlTree(v) case []string: copied := make([]string, len(v)) copy(copied, v) return copied case int64, int32, uint64, bool, string: return v default: - return value + panic(ufmt.Sprintf("cloneUintTreeValue: unsupported type %T", value)) } } func cloneAvlTreeValue(value any) any { switch v := value.(type) { case []string: copied := make([]string, len(v)) copy(copied, v) return copied case *u256.Uint: return v.Clone() case int64, int32, uint64, bool, string: return v default: - return value + panic(ufmt.Sprintf("cloneAvlTreeValue: unsupported type %T", value)) } }contract/r/gnoswap/staker/types.gno (1)
108-108: Consider renaming parameter for consistency.The function is named
GetStakedPositionsByUserbut the parameter is namedowner. For consistency with the function name and the apparent intent to use "user" terminology (as mentioned in the PR summary whereGetStakedPositionsByOwnerwas renamed toGetStakedPositionsByUser), consider renaming the parameter:- GetStakedPositionsByUser(owner address, offset, count int) []uint64 + GetStakedPositionsByUser(user address, offset, count int) []uint64contract/r/gnoswap/staker/v1/getter_test.gno (1)
630-694: Consider asserting expected behavior for incentive active status.The test currently only verifies that
IsIncentiveActivedoesn't panic for various incentive states, but doesn't assert the expected boolean return value. This could miss logic errors.♻️ Suggested improvement
tests := []struct { name string incentiveId string startTimestamp int64 endTimestamp int64 refunded bool + expectActive bool }{ { name: "future incentive (not yet started)", incentiveId: "future-incentive", startTimestamp: fixedTime + 1000, endTimestamp: fixedTime + 2000, refunded: false, + expectActive: false, // not started yet }, // ... add expectActive to other cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // ... setup ... - // IsIncentiveActive uses time.Now(), so we just verify it doesn't panic - _ = instance.IsIncentiveActive(poolPath, tt.incentiveId) + result := instance.IsIncentiveActive(poolPath, tt.incentiveId) + // Note: time.Now() makes exact assertions tricky, but we can verify refunded case + if tt.refunded { + uassert.Equal(t, false, result) + } }) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
contract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/deposit.gnocontract/r/gnoswap/staker/getter.gnocontract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/upgrade_test.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gno
🚧 Files skipped from review as they are similar to previous changes (1)
- contract/r/gnoswap/staker/getter.gno
🧰 Additional context used
📓 Path-based instructions (1)
**/*.gno
⚙️ CodeRabbit configuration file
**/*.gno: # Gno Smart Contract Code Review RulesAbout Gno
Gno is a blockchain smart contract language based on Go. Key differences from Go:
- Realms (r/): Stateful contracts with persistent storage
- Packages (p/): Pure library code without state
- Realm crossing: Explicit syntax for inter-realm calls using
crosskeyword- Access control: Uses
runtime.PreviousRealm()andruntime.OriginCaller()for caller verification- State persistence: Global variables in realms persist across transactions
- Determinism: No goroutines, limited stdlib, panic-based error handling in realms
Critical Security Violations
Flag immediately as [CRITICAL]:
- Exported global variables - Allows anyone to modify state
var Admin address // CRITICAL: Anyone can reassign
- Missing access control - No caller verification on state mutations
func DeleteData(_ realm) { data.Clear() // CRITICAL: Anyone can delete }
- Error return for security - Transaction succeeds even when unauthorized
func AdminOnly(_ realm) error { if !isAdmin() { return errors.New("denied") // CRITICAL: Should panic } }
- Direct external realm modification - Readonly taint violation
otherrealm.GlobalVar = "hacked" // CRITICAL: Cannot modify external realm stateRequired Patterns
Access Control (MUST on all state mutations)
Every function that modifies state MUST verify the caller:
import "chain/runtime" func MutatingFunction(cur realm, params) { // For immediate caller (could be user or realm) caller := runtime.PreviousRealm().Address() // For original transaction signer (always a user) signer := runtime.OriginCaller() if caller != authorized { panic("unauthorized") // MUST panic, not return error } // ... mutation logic ... }Why panic? Panics abort and rollback the transaction. Returning errors allows unauthorized transactions t...
Files:
contract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/upgrade_test.gnocontract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/deposit.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/gnoswap/staker/types.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gno
🧠 Learnings (1)
📚 Learning: 2026-01-05T03:20:38.341Z
Learnt from: notJoon
Repo: gnoswap-labs/gnoswap PR: 1118
File: contract/r/gnoswap/access/access.gno:37-43
Timestamp: 2026-01-05T03:20:38.341Z
Learning: In the gnoswap codebase, enforce consistency where event attribute names use prevAddr to refer to the caller/previous realm address in emitted events, as this is the established convention. When reviewing or adding new contracts, prefer using prevAddr over caller for event fields; if a semantic renaming is ever considered, document the rationale and ensure all related events across contracts are updated to maintain consistency.
Applied to files:
contract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/upgrade_test.gnocontract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/deposit.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/gnoswap/staker/types.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gno
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: run-integration-test
🔇 Additional comments (19)
contract/r/gnoswap/staker/upgrade_test.gno (1)
563-645: LGTM! Well-structured tests for new getter functions.The tests for
GetTotalEmissionSentandGetAllowedTokensfollow the established testing patterns in this file:
- Proper realm setup using
testing.SetRealm- Correct use of the upgrade system
- Verification of delegation to the implementation
- Appropriate assertions
contract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gno (1)
159-542: LGTM! Test mock implementation follows established patterns.The new getter methods in the
TestStakerwrapper correctly:
- Check activation status before delegating
- Panic with descriptive messages when methods are not activated
- Delegate to the underlying instance when active
This maintains consistency with the existing test infrastructure in this file and supports the upgrade testing scenarios.
contract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gno (1)
93-311: LGTM! Test implementation correctly delegates to underlying instance.The new getter methods in the
TestStakerwrapper appropriately delegate all calls to the underlying instance. This implementation is consistent with the existing pattern in this file and supports the v3_valid upgrade testing scenario.contract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gno (1)
154-537: LGTM - Consistent activation-guarded delegation pattern.The new getter methods follow a consistent pattern: check activation status, panic if not activated, then delegate to the underlying v1 instance. This is appropriate for test scaffolding that simulates contract upgrades with selective method support.
contract/r/gnoswap/staker/deposit.gno (3)
233-254: Well-implemented deep copy for Deposit.The Clone method correctly handles the nil case and performs deep copies of all mutable fields. The use of helper functions keeps the code organized and reusable.
256-264: Warmup clone is correct for value types.Since
Warmupis a struct containing only primitive types (int, int64, uint64), usingcopy()is sufficient for a deep copy.
273-305: AVL tree clone helpers are correctly implemented.Both
cloneAvlTreeInt64andcloneAvlTreeBoolproperly handle nil trees, create new trees, and copy all entries with correct type assertions.contract/r/gnoswap/staker/pool.gno (4)
167-184: LGTM - Pool.Clone() properly deep copies all nested structures.The implementation correctly clones all mutable fields including
UintTreeinstances and nestedIncentives. The nil check at the start prevents panics on nil receivers.
624-636: Ticks.Clone() correctly uses value receiver and returns value type.Since
Ticksis a value type wrapping a tree pointer, the clone correctly creates a new tree and copies all tick entries via deep cloning.
292-313: Incentives.Clone() properly clones nested ExternalIncentive objects.The implementation iterates through all incentives, clones each one, and creates a new tree. The unclaimablePeriods UintTree is also cloned.
707-719: Tick.Clone() properly handles nil and clones all fields.The implementation correctly nil-checks and calls Clone() on all pointer fields:
stakedLiquidityGross(*u256.Uint)stakedLiquidityDelta(*i256.Int)outsideAccumulation(*UintTree)All three types have Clone() methods and are properly invoked.
contract/r/gnoswap/staker/v1/getter_test.gno (1)
14-116: Well-structured test state setup.The
testStatestruct andsetupBasicTestStatehelper provide a clean, reusable foundation for testing getters. Good use of direct store access for complex state setup while using manager methods where appropriate.contract/r/scenario/upgrade/implements/mock/staker/test_impl.gno (1)
179-646: Consistent mock implementation for new getter methods.All new getter methods follow the established
ExecuteFnpattern, enabling mock function injection while defaulting to the actual v1 implementation. The type assertions are correct for the expected return types.contract/r/gnoswap/staker/_mock_test.gno (1)
131-569: LGTM - MockStaker correctly implements the expanded getter interface.The mock implementation properly retrieves values from
MockResponsewith appropriate default values when keys are not set. Each getter uses the correct type assertion for its return type.contract/r/gnoswap/staker/v1/getter.gno (5)
35-38: Clean public wrapper for internal pool getter.The
GetPoolmethod properly delegates togetPoolByPoolPath, maintaining the internal validation and error handling.
209-227: CollectableReward methods correctly calculate pending rewards.Both methods properly use
calcPositionRewardwith current time/height and return the appropriate reward amounts. The external reward method correctly handles missing incentive IDs by returning 0.
389-403: Good defensive copying for slice returns.
GetAllowedTokensandGetWarmupTemplatecorrectly return copies of the internal slices, preventing callers from modifying internal state.
421-443: Pagination implementation handles edge cases correctly.
GetStakedPositionsByUserproperly handles:
count <= 0returns empty slice- Negative offset is normalized to 0
- Missing user returns empty slice
- Uses
IterateByOffsetfor efficient pagination
450-466: Minor: Changed initialization style for incentives slice.The change from
[]sr.ExternalIncentive{}tomake([]sr.ExternalIncentive, 0)is functionally equivalent but more explicit about intent. Both produce a non-nil empty slice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
contract/r/gnoswap/staker/v1/getter_test.gno (2)
196-200: Clarify the reward amount semantics in the comment.The comment explains that
rewardAmountrepresents the remaining reward initially set tototalRewardAmount. Consider clarifying thatGetIncentiveRemainingRewardAmountreturns the currentrewardAmountfield, which is distinct from distributed rewards and is not automatically decremented whendistributedRewardAmountincreases (unless explicitly updated via other mechanisms).📝 Suggested comment improvement
- // Note: RewardAmount is the remaining reward (set at creation, then updated) - // The NewExternalIncentive sets rewardAmount = totalRewardAmount initially - // So remaining should be 10_000_000 (initial) since we haven't modified it + // Note: RewardAmount field represents the remaining/available reward amount. + // NewExternalIncentive initially sets rewardAmount = totalRewardAmount (10_000_000). + // The remaining amount is independent of distributedRewardAmount unless explicitly updated + // by reward distribution logic, so it remains at the initial 10_000_000 in this test. uassert.Equal(t, int64(10_000_000), remainingReward)
685-689: Non-deterministic test behavior due to time.Now().The test only verifies that
IsIncentiveActivedoesn't panic but doesn't assert the expected active/inactive status. Since the method usestime.Now(), the test results depend on when it runs, making it non-deterministic.Consider one of these approaches:
- Add time mocking: Inject a time provider into the implementation to allow controlled testing
- Assert expected behavior: If the incentive times are known relative to the test execution time, assert the expected boolean result
- Document the limitation: If non-determinism is acceptable here, add a comment explaining why correctness isn't verified
💡 Example: Assert expected behavior for future incentives
// IsIncentiveActive uses time.Now(), so we just verify it doesn't panic - _ = instance.IsIncentiveActive(poolPath, tt.incentiveId) + isActive := instance.IsIncentiveActive(poolPath, tt.incentiveId) + // Future incentives (not yet started) should be inactive + if tt.name == "future incentive (not yet started)" { + uassert.False(t, isActive) + } + // Refunded incentives should always be inactive + if tt.refunded { + uassert.False(t, isActive) + } })contract/r/gnoswap/staker/v1/getter.gno (3)
309-314: Consider defensive copy for slice return.Returning a slice directly allows external callers to modify the slice contents. Consider creating a defensive copy similar to
GetAllowedTokensandGetWarmupTemplate.♻️ Proposed defensive copy
// GetDepositExternalIncentiveIdList returns external incentive IDs for a deposit. func (s *stakerV1) GetDepositExternalIncentiveIdList(lpTokenId uint64) []string { deposit := s.getDeposit(lpTokenId) - - return deposit.GetExternalIncentiveIdList() + + ids := deposit.GetExternalIncentiveIdList() + result := make([]string, len(ids)) + copy(result, ids) + return result }
443-459: Optional style change noted.Changing from
[]sr.ExternalIncentive{}tomake([]sr.ExternalIncentive, 0)is a minor style preference with no functional difference. Both create an empty slice with zero length.The method appends struct values (not pointers), which creates copies and is safe from external mutation.
72-75: ExternalIncentive fields are properly unexported; however,GetIncentive()should return a defensive copy for consistency.All fields in
ExternalIncentive(contract/r/gnoswap/staker/pool.gno) are unexported with protected access through setter methods, so external state mutation is not possible. However, the base staker'sGetIncentive()function (contract/r/gnoswap/staker/getter.gno) returns a defensive copy viaClone(), while the v1 implementation returns the direct pointer. For consistency and to follow the established pattern in the codebase, returns.getIncentive(poolPath, incentiveId).Clone()instead.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/getter.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gno
🧰 Additional context used
📓 Path-based instructions (1)
**/*.gno
⚙️ CodeRabbit configuration file
**/*.gno: # Gno Smart Contract Code Review RulesAbout Gno
Gno is a blockchain smart contract language based on Go. Key differences from Go:
- Realms (r/): Stateful contracts with persistent storage
- Packages (p/): Pure library code without state
- Realm crossing: Explicit syntax for inter-realm calls using
crosskeyword- Access control: Uses
runtime.PreviousRealm()andruntime.OriginCaller()for caller verification- State persistence: Global variables in realms persist across transactions
- Determinism: No goroutines, limited stdlib, panic-based error handling in realms
Critical Security Violations
Flag immediately as [CRITICAL]:
- Exported global variables - Allows anyone to modify state
var Admin address // CRITICAL: Anyone can reassign
- Missing access control - No caller verification on state mutations
func DeleteData(_ realm) { data.Clear() // CRITICAL: Anyone can delete }
- Error return for security - Transaction succeeds even when unauthorized
func AdminOnly(_ realm) error { if !isAdmin() { return errors.New("denied") // CRITICAL: Should panic } }
- Direct external realm modification - Readonly taint violation
otherrealm.GlobalVar = "hacked" // CRITICAL: Cannot modify external realm stateRequired Patterns
Access Control (MUST on all state mutations)
Every function that modifies state MUST verify the caller:
import "chain/runtime" func MutatingFunction(cur realm, params) { // For immediate caller (could be user or realm) caller := runtime.PreviousRealm().Address() // For original transaction signer (always a user) signer := runtime.OriginCaller() if caller != authorized { panic("unauthorized") // MUST panic, not return error } // ... mutation logic ... }Why panic? Panics abort and rollback the transaction. Returning errors allows unauthorized transactions t...
Files:
contract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/gnoswap/staker/getter.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gnocontract/r/gnoswap/staker/_mock_test.gno
🧠 Learnings (1)
📚 Learning: 2026-01-05T03:20:38.341Z
Learnt from: notJoon
Repo: gnoswap-labs/gnoswap PR: 1118
File: contract/r/gnoswap/access/access.gno:37-43
Timestamp: 2026-01-05T03:20:38.341Z
Learning: In the gnoswap codebase, enforce consistency where event attribute names use prevAddr to refer to the caller/previous realm address in emitted events, as this is the established convention. When reviewing or adding new contracts, prefer using prevAddr over caller for event fields; if a semantic renaming is ever considered, document the rationale and ensure all related events across contracts are updated to maintain consistency.
Applied to files:
contract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/scenario/upgrade/implements/mock/staker/test_impl.gnocontract/r/gnoswap/staker/getter.gnocontract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gnocontract/r/gnoswap/staker/_mock_test.gno
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/halt, contract/r/scenario/halt)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/launchpad, contract/r/scenario/launchpad)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: run-integration-test
🔇 Additional comments (18)
contract/r/gnoswap/staker/v1/getter_test.gno (1)
325-338: Excellent clone/copy verification tests.The tests properly verify that returned values are clones/copies that don't affect internal state when modified. This is critical for maintaining encapsulation and preventing accidental mutation of contract state through getter methods.
Also applies to: 473-501
contract/r/gnoswap/staker/types.gno (1)
53-108: Well-designed getter interface expansion.The IStakerGetter interface has been properly expanded with a comprehensive set of accessor methods. The naming is consistent (Get* prefix), return types are appropriate, and the interface provides good read-only access to staker state without exposing internal implementation details.
contract/r/scenario/upgrade/implements/mock/staker/test_impl.gno (1)
179-640: Consistent mock wrapper implementation.The mock test implementation follows a consistent ExecuteFn pattern that allows selective method mocking while delegating to the underlying v1 implementation by default. This provides good flexibility for upgrade testing scenarios.
contract/r/gnoswap/staker/getter.gno (2)
7-32: Good use of Clone() to prevent state mutation.The GetPool, GetIncentive, and GetDeposit methods properly return cloned copies rather than references to internal state. This prevents external callers from accidentally or maliciously mutating contract state through returned object references.
154-157: No issue found.GetExternalIncentiveByPoolPathcorrectly returns independent copies ofExternalIncentivestructs via value append semantics. Since the struct contains only primitive types, value copying provides complete isolation from internal state.contract/r/scenario/upgrade/implements/security/insecure_staker_callback/test_impl.gno (1)
159-535: Well-structured activation-based test implementation.The security test implementation properly guards each method with activation checks, panicking with descriptive messages when methods are called before being activated. This pattern is effective for testing upgrade scenarios and ensuring that only explicitly enabled methods are accessible during testing.
contract/r/gnoswap/staker/v1/getter.gno (9)
105-154: LGTM! Safe primitive return types.These getter methods return primitive types (int64, bool) which are copied by value, preventing any external mutation of internal state.
209-227: LGTM! Safe calculation methods.These methods perform read-only calculations and return primitive int64 values, which are safe from external mutation.
345-354: LGTM! Safe string representation.Returning a string is safe as strings are immutable in Gno. The nil check and zero handling are correct.
356-375: LGTM! Safe slice construction.This method builds and returns a new slice, preventing external mutation of internal state.
382-388: LGTM! Excellent defensive copying.Using
copy()to return a defensive copy of the slice is a best practice that prevents external mutation of internal state.
390-396: LGTM! Consistent defensive copying.Properly uses defensive copy to protect internal state.
414-436: LGTM! Safe pagination implementation.This method builds a new slice with proper offset/count handling, and returns primitive uint64 values which are safe from mutation.
35-38: All fields insr.Poolare unexported—no security issue.The
Poolstruct (contract/r/gnoswap/staker/pool.gno) has only unexported fields (lowercase identifiers), preventing external callers from bypassing access control. Returning a pointer toPoolinGetPool()is safe and follows the correct Gno pattern for encapsulation.Likely an incorrect or invalid review comment.
204-207: No issue found. TheDepositstruct ingno.land/r/gnoswap/stakerhas all fields unexported (lowercase), preventing external modification. Returning a pointer toDepositis architecturally sound.Likely an incorrect or invalid review comment.
contract/r/scenario/upgrade/implements/v3_valid/staker/test_impl.gno (1)
93-307: LGTM! Proper test delegation.The test implementation correctly delegates all new getter methods to the underlying v1 instance. This is appropriate for upgrade testing scenarios.
contract/r/scenario/upgrade/implements/v2_invalid/staker/test_impl.gno (1)
154-530: LGTM! Consistent activation pattern.The test implementation correctly applies activation guards to all new getter methods, consistent with the existing pattern. This is appropriate for testing incomplete or invalid upgrade scenarios.
contract/r/gnoswap/staker/_mock_test.gno (1)
131-561: LGTM! Consistent mock implementation.The mock implementation correctly follows the established
MockResponsepattern for all new getter methods. Type assertions and zero-value returns are handled appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contract/r/gnoswap/staker/tree.gno (1)
74-86: Shallow copy in Clone() may not provide true isolation.The
Clone()method performs a shallow copy of values (line 81). If the tree stores pointers to mutable objects (e.g.,*Pool,*ExternalIncentive), both the original and cloned tree will reference the same underlying objects. Mutations to these objects will affect both trees, which may not provide the isolation expected from a "clone" operation.Problem: The current implementation only copies the tree structure, not the values themselves. This can lead to unexpected aliasing issues where modifications through one tree affect the other.
Recommendation:
If deep isolation is required, consider one of these approaches:
- Document that this is a shallow clone and callers must not mutate values
- Require values to implement a
Clonerinterface and deep-copy them- Rename to
ShallowClone()to clarify semanticsBased on the usage in
getter.gnowhere Pool/Incentive/Deposit objects are cloned separately after retrieval, the current approach may be acceptable if properly documented.📝 Add documentation to clarify shallow copy semantics
+// Clone creates a shallow copy of the tree structure. +// Note: Values are not cloned - both trees will reference the same value objects. +// Callers must ensure values are not mutated or perform additional cloning as needed. func (self *UintTree) Clone() *UintTree { if self == nil { return nil } cloned := NewUintTree() self.tree.Iterate("", "", func(key string, value any) bool { cloned.tree.Set(key, value) return false }) return cloned }contract/r/gnoswap/staker/_mock_test.gno (1)
339-345: Consider standardizing default return values for slice types.The mock getter methods return slices inconsistently when data is not found:
- Some return
nil: lines 342, 526, 534, 558, 574, 598, 614, 640- Others return empty slices: lines 430, 446
While both behave similarly in most Go/Gno contexts (
len()andrange), they differ in nil checks and JSON serialization. For test mocks, consistency improves maintainability and reduces surprises.♻️ Standardize to return nil consistently
For example, update line 430 and 446:
func (m *MockStaker) GetPoolIncentiveIdList(poolPath string) []string { res, ok := m.Response.Get("GetPoolIncentiveIdList") if !ok { - return []string{} + return nil } return res[0].([]string) } func (m *MockStaker) GetPoolsByTier(tier uint64) []string { res, ok := m.Response.Get("GetPoolsByTier") if !ok { - return []string{} + return nil } return res[0].([]string) }Also applies to: 427-449, 523-561, 571-641
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/getter.gnocontract/r/gnoswap/staker/pool.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gno
🚧 Files skipped from review as they are similar to previous changes (1)
- contract/r/gnoswap/staker/pool.gno
🧰 Additional context used
📓 Path-based instructions (1)
**/*.gno
⚙️ CodeRabbit configuration file
**/*.gno: # Gno Smart Contract Code Review RulesAbout Gno
Gno is a blockchain smart contract language based on Go. Key differences from Go:
- Realms (r/): Stateful contracts with persistent storage
- Packages (p/): Pure library code without state
- Realm crossing: Explicit syntax for inter-realm calls using
crosskeyword- Access control: Uses
runtime.PreviousRealm()andruntime.OriginCaller()for caller verification- State persistence: Global variables in realms persist across transactions
- Determinism: No goroutines, limited stdlib, panic-based error handling in realms
Critical Security Violations
Flag immediately as [CRITICAL]:
- Exported global variables - Allows anyone to modify state
var Admin address // CRITICAL: Anyone can reassign
- Missing access control - No caller verification on state mutations
func DeleteData(_ realm) { data.Clear() // CRITICAL: Anyone can delete }
- Error return for security - Transaction succeeds even when unauthorized
func AdminOnly(_ realm) error { if !isAdmin() { return errors.New("denied") // CRITICAL: Should panic } }
- Direct external realm modification - Readonly taint violation
otherrealm.GlobalVar = "hacked" // CRITICAL: Cannot modify external realm stateRequired Patterns
Access Control (MUST on all state mutations)
Every function that modifies state MUST verify the caller:
import "chain/runtime" func MutatingFunction(cur realm, params) { // For immediate caller (could be user or realm) caller := runtime.PreviousRealm().Address() // For original transaction signer (always a user) signer := runtime.OriginCaller() if caller != authorized { panic("unauthorized") // MUST panic, not return error } // ... mutation logic ... }Why panic? Panics abort and rollback the transaction. Returning errors allows unauthorized transactions t...
Files:
contract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/getter.gno
🧠 Learnings (1)
📚 Learning: 2026-01-05T03:20:38.341Z
Learnt from: notJoon
Repo: gnoswap-labs/gnoswap PR: 1118
File: contract/r/gnoswap/access/access.gno:37-43
Timestamp: 2026-01-05T03:20:38.341Z
Learning: In the gnoswap codebase, enforce consistency where event attribute names use prevAddr to refer to the caller/previous realm address in emitted events, as this is the established convention. When reviewing or adding new contracts, prefer using prevAddr over caller for event fields; if a semantic renaming is ever considered, document the rationale and ensure all related events across contracts are updated to maintain consistency.
Applied to files:
contract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/types.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/_mock_test.gnocontract/r/gnoswap/staker/getter.gno
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/version_manager, contract/p/gnoswap/version_manager)
- GitHub Check: run-integration-test
…centives in test implementations
…s for warmups and AVL trees
|
Description
data)
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.