chore(pool): codebase maintenance and cleanup #1133
Conversation
Add extensive edge case test coverage for pool/v1 package: - TestSafeAddInt64 (16 cases): int64 addition overflow/underflow - TestSafeSubInt64 (18 cases): int64 subtraction overflow/underflow - TestPosition_TickBoundaryEdgeCases (5 cases): MIN_TICK/MAX_TICK boundaries - TestPosition_LiquidityBoundaryEdgeCases (4 cases): maxLiquidityPerTick limits - TestPosition_AmountOverflowEdgeCases (1 case): TokensOwed overflow detection
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughRefactors oracle/twap to return harmonic mean liquidity, tightens sqrtPriceX96 upper-bound validation to be exclusive, simplifies position key API, renames observation cardinality to "Next", and updates many tests, mocks, proxies, and scenario files to match the new APIs and behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pool
participant Oracle
participant TWAPCalc
Client->>Pool: GetTWAP(poolPath, secondsAgo)
Pool->>Oracle: getTWAP(pool, secondsAgo)
Oracle->>Oracle: validate secondsAgo (not too old)
Oracle->>Oracle: locate observations (binarySearch / circular buffer)
Oracle->>TWAPCalc: interpolate tick & compute cumulatives
TWAPCalc->>TWAPCalc: calculate arithmetic mean tick
TWAPCalc->>TWAPCalc: compute harmonic mean liquidity (secondsPerLiquidity)
TWAPCalc->>Oracle: return (meanTick, harmonicMeanLiquidity)
Oracle->>Pool: return (twapTick, harmonicMeanLiquidity, err)
Pool->>Client: return (twapTick, harmonicMeanLiquidity, err)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Add comprehensive edge case tests to existing test files: pool_test.gno (+116 lines): - TestCollectToken_EdgeCases (12 cases): collectToken function with various combinations of request/owed/balance amounts manager_test.gno (+136 lines): - TestSetFeeProtocol_BoundaryValues (13 cases): fee protocol validation including boundary values (0, 4-10, invalid ranges) utils_test.gno (+82 lines): - TestU256Min_EdgeCases (9 cases): u256Min utility function with zero, equal, and max values Total: 34 new test cases covering previously untested edge cases
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
from
15d991a to
5ca5130
Compare
- factory_param.gno used `Gt(maxSqrtRatio)` (inclusive) - tick_math.gno uses `Gte(maxSqrtRatio)` (exclusive) - Pools at MAX_SQRT_RATIO passed validation but panicked in TickMathGetTickAtSqrtRatio - Changed to `Gte(maxSqrtRatio)` to match half-open interval [MIN, MAX) - Added CreatePool boundary value tests (MIN, MAX, MAX±1) - Updated test expectations to reflect correct behavior - Prevents invalid pools from being created at MAX_SQRT_RATIO - Ensures validation matches actual usage in tick conversion
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
from
1cf2965 to
e551901
Compare
![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 (1)
contract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno (1)
70-98: Consider adding positive assertions for Tests 1 and 2.The current tests verify that MIN_SQRT_RATIO and MAX_SQRT_RATIO - 1 don't panic, which is appropriate for boundary regression testing. However, you could strengthen the validation by adding assertions that:
- The pools were successfully created (e.g., verify pool exists)
- The returned tick values are within expected ranges
This would provide more confidence that the boundary values are not just "not rejected" but actually produce valid results.
Example enhancement
// After line 82, add: poolKey := pool.GetPoolKey(barPath, fooPath, 500) if !pool.DoesPoolExist(poolKey) { println("[✗] Pool creation failed for MIN_SQRT_RATIO") } // After line 77, add: if tickAtMin < common.MinTick() || tickAtMin > common.MaxTick() { println("[✗] Tick out of valid range for MIN_SQRT_RATIO") }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contract/r/scenario/pool/tickmath_boundary_alignment_filetest.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/scenario/pool/tickmath_boundary_alignment_filetest.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/pool/tickmath_boundary_alignment_filetest.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). (42)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/scenario/halt, contract/r/scenario/halt)
- 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/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/gns, contract/r/scenario/gns)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/pool, contract/r/gnoswap/pool)
- GitHub Check: test-gnoswap (gnoswap/int256, contract/p/gnoswap/int256)
- GitHub Check: test-gnoswap (gnoswap/gov/governance/v1, contract/r/gnoswap/gov/governance/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/xgns, contract/r/gnoswap/gov/xgns)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/launchpad, contract/r/scenario/launchpad)
- GitHub Check: test-gnoswap (gnoswap/gns, contract/r/gnoswap/gns)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/emission, contract/r/gnoswap/emission)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/r/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/referral, contract/r/gnoswap/referral)
- GitHub Check: test-gnoswap (gnoswap/scenario/patterns/callback, contract/r/scenario/patterns/callback)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/position, contract/r/gnoswap/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/protocol_fee, contract/r/scenario/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/gnft, contract/r/gnoswap/gnft)
- GitHub Check: test-gnoswap (gnoswap/common, contract/r/gnoswap/common)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee, contract/r/gnoswap/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/scenario/emission, contract/r/scenario/emission)
- GitHub Check: test-gnoswap (gnoswap/uint256, contract/p/gnoswap/uint256)
- GitHub Check: test-gnoswap (gnoswap/version_manager, contract/p/gnoswap/version_manager)
- GitHub Check: test-gnoswap (gnoswap/fuzz, contract/p/gnoswap/fuzz)
- GitHub Check: test-gnoswap (gnoswap/access, contract/r/gnoswap/access)
- GitHub Check: test-gnoswap (gnoswap/gov/governance, contract/r/gnoswap/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/community_pool, contract/r/gnoswap/community_pool)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/p/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/gnsmath, contract/p/gnoswap/gnsmath)
- GitHub Check: run-integration-test
🔇 Additional comments (1)
contract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno (1)
49-184: Well-structured regression test.The test properly:
- Uses
testing.SetRealm()to establish admin context for pool creation- Uses
crosskeyword correctly for realm-crossing calls togns.Approve()andpool.CreatePool()- Employs panic recovery blocks to verify boundary rejection behavior
- Documents expected behavior and results with clear diagnostic output
- Aligns with the PR objective of ensuring tick_math and pool validation share consistent boundary semantics
The half-open interval [MIN, MAX) validation is correctly verified across all four test cases.
contract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno
Outdated
Show resolved
Hide resolved
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
from
5dec3e8 to
aa1e362
Compare
Add scenario test to verify that pool.validateSqrtPriceX96 and tick_math.TickMathGetTickAtSqrtRatio use consistent boundary validation. Test Coverage: - MIN_SQRT_RATIO (inclusive, should accept) - MAX_SQRT_RATIO - 1 (valid, should accept) - MAX_SQRT_RATIO (exclusive, should reject) - MAX_SQRT_RATIO + 1 (invalid, should reject) This regression test prevents future drift between the two validation functions, ensuring they both use half-open interval [MIN, MAX). Related: e551901
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
2 times, most recently
from
94b627e to
37b63da
Compare
Verify that tick_math.TickMathGetTickAtSqrtRatio and pool validation use consistent boundary checks for sqrtPriceX96 range. Test cases: - MIN_SQRT_RATIO (inclusive): Both should accept - MAX_SQRT_RATIO - 1 (valid): Both should accept - MAX_SQRT_RATIO (exclusive): Both should reject - MAX_SQRT_RATIO + 1 (out of range): Both should reject
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
contract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno (1)
27-27: Remove unused constant.The
maxApproveconstant is defined but never used in the test.
🧹 Nitpick comments (4)
contract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno (2)
78-97: Improve panic verification to catch false negatives.The recover blocks only print messages when a panic occurs. If
common.TickMathGetTickAtSqrtRatioorpool.CreatePoolunexpectedly succeed (no panic), the test silently continues without verification. This could mask regressions where MAX_SQRT_RATIO boundary validation is inadvertently relaxed.🔎 Proposed fix to verify panics occurred
println("[INFO] Test 3: MAX_SQRT_RATIO (exclusive)") +tickMathPanicked := false func() { defer func() { if r := recover(); r != nil { println("[EXPECTED] tick_math rejected") + tickMathPanicked = true } }() common.TickMathGetTickAtSqrtRatio(maxSqrtRatio) }() +if !tickMathPanicked { + panic("tick_math should have rejected MAX_SQRT_RATIO") +} +poolPanicked := false func() { defer func() { if r := recover(); r != nil { println("[EXPECTED] pool rejected") + poolPanicked = true } }() gns.Approve(cross, poolAddr, poolCreationFee) pool.CreatePool(cross, barPath, bazPath, 10000, MAX_SQRT_RATIO) }() +if !poolPanicked { + panic("pool should have rejected MAX_SQRT_RATIO") +} println()
99-118: Improve panic verification to catch false negatives.Similar to Test 3, the recover blocks for MAX_SQRT_RATIO + 1 don't verify that panics actually occurred. If the validation unexpectedly passes, the test continues silently.
🔎 Proposed fix to verify panics occurred
println("[INFO] Test 4: MAX_SQRT_RATIO + 1 (out of range)") +tickMathPanicked := false func() { defer func() { if r := recover(); r != nil { println("[EXPECTED] tick_math rejected") + tickMathPanicked = true } }() common.TickMathGetTickAtSqrtRatio(maxPlusOne) }() +if !tickMathPanicked { + panic("tick_math should have rejected MAX_SQRT_RATIO + 1") +} +poolPanicked := false func() { defer func() { if r := recover(); r != nil { println("[EXPECTED] pool rejected") + poolPanicked = true } }() gns.Approve(cross, poolAddr, poolCreationFee) pool.CreatePool(cross, fooPath, quxPath, 100, maxPlusOne.ToString()) }() +if !poolPanicked { + panic("pool should have rejected MAX_SQRT_RATIO + 1") +} println()contract/r/scenario/pool/swap_balance_mixed_positions_filetest.gno (1)
301-302: Clarify the purpose of this error comment.The trailing error comment lacks context. Is this an expected test failure, a known environment limitation, or a TODO to fix? Consider either:
- Adding a more descriptive comment explaining the expected behavior
- Removing it if it was debugging output
- Creating an issue to track if it's a bug
Would you like me to open an issue to track this permission error, or can you clarify the intended behavior?
contract/r/scenario/pool/swap_balance_extreme_filetest.gno (1)
179-193: Consider adding assertion checks for critical invariants.While filetests primarily validate through output inspection, adding explicit panic-based assertions for critical invariants (like conservation laws) would make failures more obvious and prevent silent issues.
🔎 Example: Add assertion for conservation
conservationBar := adminBarChange + poolBarChange conservationFoo := adminFooChange + poolFooChange println("[VERIFY] Conservation (CRITICAL):") println("[VERIFY] - Admin bar + Pool bar:", conservationBar, "(MUST be 0)") println("[VERIFY] - Admin foo + Pool foo:", conservationFoo, "(MUST be 0)") + +if conservationBar != 0 || conservationFoo != 0 { + panic("Conservation violated: tokens were created or destroyed") +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contract/r/scenario/pool/swap_balance_extreme_filetest.gnocontract/r/scenario/pool/swap_balance_mixed_positions_filetest.gnocontract/r/scenario/pool/swap_balance_single_in_range_filetest.gnocontract/r/scenario/pool/tickmath_boundary_alignment_filetest.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/scenario/pool/swap_balance_mixed_positions_filetest.gnocontract/r/scenario/pool/swap_balance_single_in_range_filetest.gnocontract/r/scenario/pool/swap_balance_extreme_filetest.gnocontract/r/scenario/pool/tickmath_boundary_alignment_filetest.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/pool/swap_balance_mixed_positions_filetest.gnocontract/r/scenario/pool/swap_balance_single_in_range_filetest.gnocontract/r/scenario/pool/swap_balance_extreme_filetest.gnocontract/r/scenario/pool/tickmath_boundary_alignment_filetest.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). (11)
- GitHub Check: test-gnoswap (gnoswap/uint256, contract/p/gnoswap/uint256)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/halt, contract/r/scenario/halt)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- 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/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/int256, contract/p/gnoswap/int256)
- GitHub Check: run-integration-test
🔇 Additional comments (15)
contract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno (2)
64-65: Correct realm crossing syntax.All calls to
gns.Approveandpool.CreatePoolproperly use thecrosskeyword for realm boundary crossing. This follows Gno best practices for crossing functions.Also applies to: 73-74, 94-95, 115-116
31-35: Proper test realm setup.The test correctly initializes the admin realm using
testing.NewUserRealmand sets the caller context withtesting.SetRealm, following Gno testing best practices. The boundary value calculations using uint256 arithmetic are appropriate.Also applies to: 53-58
contract/r/scenario/pool/swap_balance_mixed_positions_filetest.gno (3)
1-46: LGTM!The imports, constants, and global variables are appropriately structured for a filetest scenario. The use of blank imports for realm registration side effects and the defined price bounds are correct.
204-286: Test logic is well-structured.The exact output swap test appropriately mirrors the exact input test structure. The verification of balance conservation and liquidity usage is comprehensive.
288-299: LGTM!The mock swap callback correctly handles token transfers for positive deltas (tokens owed to the pool) and properly sets the admin realm context for authorization.
contract/r/scenario/pool/swap_balance_single_in_range_filetest.gno (5)
1-46: LGTM!The imports and constants are well-structured. The blank imports for side-effect initialization (
rbac,pool/v1,protocol_fee/v1) are appropriate for filetest setup.
48-65: LGTM!The main function provides clear orchestration of the test scenario with descriptive output for each phase.
67-96: LGTM!The setup function properly manages realm context - using
adminRealmfor pool creation and token approvals, then switching toposRealmfor minting. This follows the expected access control patterns where different roles perform their authorized operations.
98-179: LGTM!The test function follows a clear pattern: capture before state, execute swap, capture after state, verify balance conservation. The verification logic correctly checks that
adminChange + poolChange = 0for both tokens, ensuring no tokens are created or destroyed.
181-258: LGTM!The exact output swap test follows the same verification pattern as exact input, correctly using a negative amount (
"-50000") to specify exact output semantics and validating the inverse price movement direction.contract/r/scenario/pool/swap_balance_extreme_filetest.gno (5)
1-46: Test setup follows Gno testing patterns correctly.The realm context setup using
testing.NewUserRealm()andtesting.NewCodeRealm()is appropriate for scenario testing. The constants and imports are well-organized for testing extreme swap scenarios.
71-104: Proper realm crossing and test context management.The setup correctly uses
testing.SetRealm()to switch execution context and applies thecrosskeyword for all realm-crossing function calls (CreatePool, Approve, Mint). This follows Gno testing best practices.
140-142: Callback realm parameter usage is correct.The anonymous callback function properly declares the
cur realmparameter, which is required for functions that cross realm boundaries. The callback then delegates tomockSwapCallback()which doesn't need the realm parameter since it's a helper function that usestesting.SetRealm()internally.
364-375: Mock callback implementation is appropriate for testing.The callback correctly uses
testing.SetRealm()to establish context and applies thecrosskeyword when callingSafeGRC20Transfer(). Returningnilunconditionally is acceptable for this test scenario.
377-378: This is a standard filetest format pattern. The// Error:comment documents the expected test output for the filetest execution. Multiple other filetest files in the repository (e.g.,swap_balance_single_in_range_filetest.gno,swap_balance_mixed_positions_filetest.gno,tickmath_boundary_alignment_filetest.gno) use the same pattern to document expected errors or results. No clarification or removal is needed.Likely an incorrect or invalid review comment.
contract/r/scenario/pool/swap_balance_mixed_positions_filetest.gno
Outdated
Show resolved
Hide resolved
contract/r/scenario/pool/swap_balance_mixed_positions_filetest.gno
Outdated
Show resolved
Hide resolved
contract/r/scenario/pool/swap_balance_single_in_range_filetest.gno
Outdated
Show resolved
Hide resolved
Add scenario tests to verify token balance conservation during swaps: - Single in-range position: Tests ExactInput/ExactOutput swaps with balance verification - Mixed positions: Tests swaps with in-range and out-of-range positions - Extreme price movements: Tests balance conservation under large swaps Each test verifies: - Balance conservation: admin + pool = 0 for each token - Price movement: tick changes as expected - Correct swap amounts
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
from
37b63da to
9f5f9de
Compare
Update protocol fee documentation to accurately reflect code validation: - Change "0-10%" to "0% or 4-10%" - Values 1, 2, 3 are not valid (enforced by isValidFeeProtocolValue) Updated in both Configuration and Fee Mechanics sections.
Remove dead code from tickmath boundary alignment test. The maxApprove constant was defined but never used.
Refactor pool scenario tests to follow router test conventions with improved balance tracking and exact conservation validation. Changes: - Add BalanceSnapshot pattern with protocol fee tracking - Implement exact balance conservation validation (user + pool + protocol fee = 0) - Enable protocol fee with SetFeeProtocol in init() - Use independent PKGPATHs for isolated test execution - Simplify tickmath boundary test to avoid cross-realm panic issues All tests now pass independently and as part of the full test suite.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contract/r/scenario/pool/swap_balance_single_in_range_filetest.gno (1)
249-260: [CRITICAL] Realm Context: Removetesting.SetRealmfrom callback—causes permission denialLine 250 calls
testing.SetRealm(adminRealm)inside the callback whilepool.Swapis executing underrouterRealm(set at lines 153, 191). This realm context switch mid-execution breaks caller verification inGetTokenTeller(), causing "read permission denied" errors whenSafeGRC20Transferattempts token operations.Problem: The callback inherits the
cur realmparameter but immediately overwrites the realm context. Token transfer operations then fail because the realm identity no longer matches the approved caller context.Fix:
func mockSwapCallback(token0Path string, token1Path string, amount0Delta int64, amount1Delta int64) error { - testing.SetRealm(adminRealm) - if amount0Delta > 0 { common.SafeGRC20Transfer(cross, token0Path, poolAddr, amount0Delta) } if amount1Delta > 0 { common.SafeGRC20Transfer(cross, token1Path, poolAddr, amount1Delta) } return nil }Rationale: The realm context is already correctly established before
pool.Swapis called (lines 153, 191). The callback must preserve this context for token operations to succeed. Other working test scenarios (referenced in past reviews) omittesting.SetRealm()inside callbacks for this reason.Based on past review comments, this issue has been flagged before and must be resolved.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contract/r/gnoswap/pool/README.mdcontract/r/gnoswap/pool/v1/README.mdcontract/r/scenario/pool/swap_balance_extreme_filetest.gnocontract/r/scenario/pool/swap_balance_mixed_positions_filetest.gnocontract/r/scenario/pool/swap_balance_single_in_range_filetest.gnocontract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno
🚧 Files skipped from review as they are similar to previous changes (3)
- contract/r/scenario/pool/tickmath_boundary_alignment_filetest.gno
- contract/r/scenario/pool/swap_balance_mixed_positions_filetest.gno
- contract/r/scenario/pool/swap_balance_extreme_filetest.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/scenario/pool/swap_balance_single_in_range_filetest.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/pool/swap_balance_single_in_range_filetest.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/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/test/fuzz, contract/r/gnoswap/test/fuzz)
- 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: run-integration-test
🔇 Additional comments (3)
contract/r/gnoswap/pool/README.md (1)
12-12: Excellent clarification of Protocol Fee boundaries.The updates from "0-10%" to "0% or 4-10%" correctly reflect the tightened validation rules and improve documentation clarity by explicitly excluding the intermediate range (1-3%). Both the Configuration section and Fee Mechanics section are consistently updated, ensuring readers understand that protocol fees can only be disabled (0%) or configured within 4-10%.
Also applies to: 190-190
contract/r/gnoswap/pool/v1/README.md (2)
12-12: Protocol fee constraint clarification looks good.The update from "0-10%" to "0% or 4-10%" accurately reflects the tightened fee protocol options mentioned in the PR summary, providing clearer guidance on valid protocol fee configurations.
77-135: New Swap Callback documentation section is a valuable addition.The comprehensive callback documentation covering signature, delta convention, direction examples, and security notes significantly improves developer guidance. The delta convention table (lines 88-92) and swap direction examples (lines 94-104) clearly explain the positive/negative semantics.
The security recommendation (line 131) to verify the caller is particularly important for preventing unauthorized callback invocations.
Update callback example to follow actual codebase patterns: - Use common.SafeGRC20Transfer with cross keyword for realm-crossing calls - Return error instead of panic for unauthorized caller - Align with router/v1 implementation pattern
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
contract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno (1)
32-33: Unused variablerouterAddr.The
routerAddrvariable is declared but never used in this test file. Consider removing it to keep the code clean.🔎 Proposed fix
var ( adminAddr, _ = access.GetAddress(prbac.ROLE_ADMIN.String()) adminRealm = testing.NewUserRealm(adminAddr) positionAddr, _ = access.GetAddress(prbac.ROLE_POSITION.String()) posRealm = testing.NewUserRealm(positionAddr) - routerAddr, _ = access.GetAddress(prbac.ROLE_ROUTER.String()) poolAddr, _ = access.GetAddress(prbac.ROLE_POOL.String()) poolCreationFee int64 = 100_000_000 )contract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gno (1)
38-39: Unused variablerouterAddr.Similar to the other scenario file,
routerAddris declared but never used. Consider removing it.🔎 Proposed fix
var ( adminAddr, _ = access.GetAddress(prbac.ROLE_ADMIN.String()) adminRealm = testing.NewUserRealm(adminAddr) positionAddr, _ = access.GetAddress(prbac.ROLE_POSITION.String()) posRealm = testing.NewUserRealm(positionAddr) - routerAddr, _ = access.GetAddress(prbac.ROLE_ROUTER.String()) poolAddr, _ = access.GetAddress(prbac.ROLE_POOL.String()) poolCreationFee int64 = 100_000_000 )contract/r/gnoswap/pool/v1/README.md (2)
87-92: Markdown formatting: add blank lines around table.The static analysis tool flagged that tables should be surrounded by blank lines for proper rendering.
🔎 Proposed fix
**Delta Convention**: + | Delta | Meaning | |-------|---------| | Positive (`> 0`) | Amount the pool must RECEIVE (input token) | | Negative (`< 0`) | Amount the pool has SENT (output token) | + **Swap Direction Examples**:
159-164: Markdown formatting: fenced code blocks should specify a language.Consider adding a language identifier (e.g.,
textor leave empty with triple backticks) for formula/pseudo-code blocks. This is a minor linting suggestion.🔎 Proposed fix
**Range Liquidity Formula**: -``` +```text L = amount / (sqrt(upper) - sqrt(lower)) // current < lower L = amount * sqrt(current) / (upper - current) // lower < current < upper L = amount / (sqrt(current) - sqrt(lower)) // current > upper</details> </blockquote></details> <details> <summary>contract/r/scenario/pool/oracle_twap_full_scenario_filetest.gno (1)</summary><blockquote> `40-41`: **Unused variable `routerAddr`.** Consistent with other scenario files, `routerAddr` is declared but not used. <details> <summary>🔎 Proposed fix</summary> ```diff var ( adminAddr, _ = access.GetAddress(prbac.ROLE_ADMIN.String()) adminRealm = testing.NewUserRealm(adminAddr) positionAddr, _ = access.GetAddress(prbac.ROLE_POSITION.String()) posRealm = testing.NewUserRealm(positionAddr) - routerAddr, _ = access.GetAddress(prbac.ROLE_ROUTER.String()) poolAddr, _ = access.GetAddress(prbac.ROLE_POOL.String()) poolCreationFee int64 = 100_000_000 )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contract/r/gnoswap/pool/README.mdcontract/r/gnoswap/pool/v1/README.mdcontract/r/scenario/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno
🚧 Files skipped from review as they are similar to previous changes (1)
- contract/r/gnoswap/pool/README.md
🧰 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/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gnocontract/r/scenario/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.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/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gnocontract/r/scenario/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.gno
🪛 markdownlint-cli2 (0.18.1)
contract/r/gnoswap/pool/v1/README.md
88-88: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
160-160: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (9)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- 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/position/v1, contract/r/gnoswap/position/v1)
- 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/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (7)
contract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno (2)
58-158: LGTM!The test scenario is well-structured with clear logging, proper realm switching for different operations, and a logical flow: pool creation → liquidity provision → time skip → swap → time skip → TWAP queries. The test covers the intended use case of verifying TWAP calculations across price changes.
160-171: LGTM!The mock callback correctly handles the delta convention (positive = amount pool must receive) and uses
SafeGRC20Transferfor token transfers. Appropriate for a test scenario.contract/r/scenario/pool/oracle_twap_basic_filetest.gno (1)
55-132: LGTM!This basic TWAP scenario provides good coverage for the steady-state case where no price changes occur. The test correctly verifies that TWAP remains at tick 0 after 4 hours of elapsed time without any swaps.
contract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gno (1)
55-194: LGTM!This test effectively validates TWAP behavior with increased observation cardinality. The bidirectional swaps (token0→token1, then token1→token0) and shorter time intervals provide good coverage for the cardinality feature.
contract/r/gnoswap/pool/v1/README.md (1)
107-127: LGTM!The callback example has been improved:
- Uses
SafeGRC20Transferwith thecrosskeyword for realm-crossing calls- Returns an error for unauthorized callers (consistent with the function's error return type)
- Clear documentation of the security check and delta handling
contract/r/scenario/pool/oracle_twap_full_scenario_filetest.gno (2)
57-245: LGTM!This is the most comprehensive TWAP scenario test, covering:
- Cardinality increase (200 slots)
- Multiple swaps in both directions
- Additional position minting mid-test
- TWAP queries across various intervals
The test effectively validates that the TWAP oracle correctly weights price changes and handles liquidity modifications.
247-258: LGTM!The mock callback follows the same pattern as other scenario files, maintaining consistency across the test suite.
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
from
6de17b8 to
bc75d51
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contract/r/gnoswap/pool/v1/README.md (1)
208-241: Pool Creation Griefing recovery mechanism is well-documented and actionable.The expanded section clearly explains the problem, impact, recovery strategy, and prevention measures. The atomic recovery example (lines 227–235) is practical and helps readers understand the arbitrage-as-recovery pattern. The mention that gas and protocol fees are the net cost is transparent.
One minor enhancement: consider noting that the "wide-range liquidity" mentioned in step 1 should be defined more specifically (e.g., "full range from min to max tick" or "as wide as economically feasible given the distorted price").
contract/r/scenario/pool/oracle_twap_basic_filetest.gno (1)
43-49: Unused constants:halfBlocksandhalfSeconds.These constants are defined but not used in this file. They appear to be shared across TWAP test scenarios for consistency.
Consider removing unused constants or extracting shared constants to a common test utilities package if multiple filetest scenarios need them.
contract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno (1)
32-33: Unused variable:routerAddr.
routerAddris retrieved but never used. The swap operation usestesting.NewCodeRealm("gno.land/r/gnoswap/router")directly instead.🔎 Proposed fix
var ( adminAddr, _ = access.GetAddress(prbac.ROLE_ADMIN.String()) adminRealm = testing.NewUserRealm(adminAddr) positionAddr, _ = access.GetAddress(prbac.ROLE_POSITION.String()) posRealm = testing.NewUserRealm(positionAddr) - routerAddr, _ = access.GetAddress(prbac.ROLE_ROUTER.String()) poolAddr, _ = access.GetAddress(prbac.ROLE_POOL.String())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contract/r/gnoswap/pool/README.mdcontract/r/gnoswap/pool/v1/README.mdcontract/r/gnoswap/pool/v1/manager.gnocontract/r/scenario/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno
✅ Files skipped from review due to trivial changes (1)
- contract/r/gnoswap/pool/v1/manager.gno
🚧 Files skipped from review as they are similar to previous changes (3)
- contract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gno
- contract/r/scenario/pool/oracle_twap_full_scenario_filetest.gno
- contract/r/gnoswap/pool/README.md
🧰 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/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.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/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.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). (47)
- GitHub Check: test-gnoswap (gnoswap/scenario/gns, contract/r/scenario/gns)
- GitHub Check: test-gnoswap (gnoswap/scenario/rbac, contract/r/scenario/rbac)
- GitHub Check: test-gnoswap (gnoswap/scenario/launchpad, contract/r/scenario/launchpad)
- GitHub Check: test-gnoswap (gnoswap/scenario/protocol_fee, contract/r/scenario/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/halt, contract/r/scenario/halt)
- GitHub Check: test-gnoswap (gnoswap/scenario/patterns/callback, contract/r/scenario/patterns/callback)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/referral, contract/r/gnoswap/referral)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/router, contract/r/gnoswap/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/emission, contract/r/scenario/emission)
- GitHub Check: test-gnoswap (gnoswap/gov/governance/v1, contract/r/gnoswap/gov/governance/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/pool, contract/r/gnoswap/pool)
- GitHub Check: test-gnoswap (gnoswap/gov/staker, contract/r/gnoswap/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/r/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee, contract/r/gnoswap/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee/v1, contract/r/gnoswap/protocol_fee/v1)
- GitHub Check: test-gnoswap (gnoswap/gns, contract/r/gnoswap/gns)
- GitHub Check: test-gnoswap (gnoswap/halt, contract/r/gnoswap/halt)
- GitHub Check: test-gnoswap (gnoswap/position, contract/r/gnoswap/position)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/launchpad, contract/r/gnoswap/launchpad)
- GitHub Check: test-gnoswap (gnoswap/gov/governance, contract/r/gnoswap/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/gov/xgns, contract/r/gnoswap/gov/xgns)
- GitHub Check: test-gnoswap (gnoswap/community_pool, contract/r/gnoswap/community_pool)
- GitHub Check: test-gnoswap (gnoswap/common, contract/r/gnoswap/common)
- GitHub Check: test-gnoswap (gnoswap/gnsmath, contract/p/gnoswap/gnsmath)
- GitHub Check: test-gnoswap (gnoswap/gnft, contract/r/gnoswap/gnft)
- GitHub Check: test-gnoswap (gnoswap/int256, contract/p/gnoswap/int256)
- GitHub Check: test-gnoswap (gnoswap/uint256, contract/p/gnoswap/uint256)
- GitHub Check: test-gnoswap (gnoswap/emission, contract/r/gnoswap/emission)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/p/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/access, contract/r/gnoswap/access)
- GitHub Check: test-gnoswap (gnoswap/store, contract/p/gnoswap/store)
- GitHub Check: test-gnoswap (gnoswap/fuzz, contract/p/gnoswap/fuzz)
- GitHub Check: run-integration-test
🔇 Additional comments (10)
contract/r/gnoswap/pool/v1/README.md (3)
77-135: Excellent callback documentation improvements addressing previous feedback.The Swap Callback section is comprehensive and now correctly implements the guidance from the prior review:
- Uses error returns instead of panics (line 114) ✓
- Includes the
crosskeyword for realm-crossing transfers (lines 119, 123) ✓- Provides clear Delta Convention table and practical examples ✓
- Emphasizes caller verification for security ✓
One minor discrepancy to clarify: the callback signature shown on line 84 (
_ *pool.CallbackMarkerparameter) doesn't match the example implementation on line 108 (which omits it). If the CallbackMarker is genuinely optional or filtered during marshaling, consider adding an explanatory comment. If it's required, the example should include the parameter (even if unused via_).Please confirm whether the
CallbackMarkerparameter is actually part of the callback signature or if it's documentation of the internal marshaling behavior. If it's part of the signature, update line 108 to match line 84.
12-12: Protocol fee description is now consistent.Lines 12 and 188–192 now both state "0% or 4–10% of swap fees" (with denominator 4–10), which aligns well with the tightened validation mentioned in the PR objectives. This clarity helps operators understand fee configurability.
Also applies to: 188-192
29-29: Security-conscious documentation: price range and validation clarity.Line 29 adds "Liquidity activated only when price in range" to the Tick System, reinforcing the core mechanism. Line 41 transparently states "No price validation performed (see Security Considerations)", which is important given the Pool Creation Griefing section discusses this risk. This honest framing helps developers make informed decisions about when to use price validation externally.
Also applies to: 41-41
contract/r/scenario/pool/oracle_twap_basic_filetest.gno (3)
1-21: LGTM - Clean imports and package setup for filetest scenario.The imports are well-organized, with side-effect imports for realm initialization (
_ "gno.land/r/gnoswap/pool/v1"and_ "gno.land/r/gnoswap/protocol_fee/v1") appropriately used for ensuring the realms are loaded.
55-132: Well-structured TWAP test scenario with clear documentation.The test follows a logical flow:
- Pool creation with proper realm context
- Liquidity minting with position realm
- Initial TWAP query (0 seconds ago)
- Time skip simulation
- Multiple TWAP queries at different intervals
The use of
testing.SkipHeights()to simulate time passage is appropriate for testing time-dependent oracle behavior.
134-169: Expected output documentation is comprehensive.The output block clearly documents the expected TWAP behavior, confirming that the tick remains at 0 throughout since no price-changing swaps occurred. This serves as both documentation and verification.
contract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno (4)
1-21: LGTM - Imports match the basic TWAP test with same dependencies.The import structure is consistent with the basic TWAP test file, which is good for maintainability across the test suite.
104-126: Swap execution with proper realm context and callback handling.The swap is correctly executed with the router realm context, and the callback function handles the token transfers. The use of
minPriceas the price limit for azeroForOne=trueswap is correct (pushing price down to minimum).
160-171: Callback implementation is correct for test purposes.The
mockSwapCallbackproperly handles token transfers based on the delta signs. Setting the realm toadminRealmbefore transfers ensures the caller has the necessary token approvals to complete the swap.Note: In production callback implementations, this realm manipulation wouldn't be possible, but it's appropriate for test scenarios.
198-208: Expected TWAP values correctly demonstrate weighted averaging.The expected outputs show proper TWAP behavior:
- 1h TWAP (-1906): Reflects only the post-swap price since the swap happened 2 hours ago
- 2h TWAP (-1906): Matches the post-swap tick since the price change occurred exactly 2 hours ago
- 4h TWAP (-953): Correctly weighted average of tick 0 (2h) and tick -1906 (2h), yielding approximately -953
This validates the oracle's time-weighted price calculation.
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
2 times, most recently
from
cae36db to
7d3f5f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
contract/r/gnoswap/pool/v1/README.md (3)
87-92: Add blank lines around table (markdown lint MD058).Tables should be surrounded by blank lines for proper markdown formatting. Add a blank line before line 88 (before the
|header) to comply with markdown style guidelines.🔎 Proposed formatting fix
**Delta Convention**: + | Delta | Meaning | |-------|---------| | Positive (`> 0`) | Amount the pool must RECEIVE (input token) | | Negative (`< 0`) | Amount the pool has SENT (output token) | +
160-160: Add language identifier to fenced code block (markdown lint MD040).The liquidity math formula code block is missing a language specifier. Use
```textor```godepending on whether this is pseudocode or Go code.🔎 Proposed fix
-L = amount / (sqrt(upper) - sqrt(lower)) // current < lower +L = amount / (sqrt(upper) - sqrt(lower)) // current < lower L = amount * sqrt(current) / (upper - current) // lower < current < upper L = amount / (sqrt(current) - sqrt(lower)) // current > upper +```Note: Replace opening fence on line 160 from
```to```text(or```goif representing executable code).
183-183: Add language identifier to fenced code block (markdown lint MD040).The fee calculation code block is missing a language specifier. Use
```textfor pseudocode representation.🔎 Proposed fix
-``` +```text fees = feeGrowthInside * liquidity feeGrowthInside = feeGrowthGlobal - feeGrowthOutside -``` +```contract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gno (1)
36-41: Consider checking errors from access.GetAddress in test setup.The error returns from
access.GetAddressare discarded. While this test assumes successful setup, explicitly checking these errors would make test failures more debuggable if the access configuration is incorrect.Optional improvement for test robustness
var ( - adminAddr, _ = access.GetAddress(prbac.ROLE_ADMIN.String()) + adminAddr = mustGetAddress(prbac.ROLE_ADMIN.String()) adminRealm = testing.NewUserRealm(adminAddr) - positionAddr, _ = access.GetAddress(prbac.ROLE_POSITION.String()) + positionAddr = mustGetAddress(prbac.ROLE_POSITION.String()) posRealm = testing.NewUserRealm(positionAddr) - routerAddr, _ = access.GetAddress(prbac.ROLE_ROUTER.String()) - poolAddr, _ = access.GetAddress(prbac.ROLE_POOL.String()) + routerAddr = mustGetAddress(prbac.ROLE_ROUTER.String()) + poolAddr = mustGetAddress(prbac.ROLE_POOL.String()) poolCreationFee int64 = 100_000_000 ) + +func mustGetAddress(role string) address { + addr, err := access.GetAddress(role) + if err != nil { + panic("setup failed: " + err.Error()) + } + return addr +}contract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno (1)
28-36: Consider checking errors from access.GetAddress in test setup.Similar to the other TWAP tests, the error returns are discarded. For improved test debuggability, consider validating these during setup.
contract/r/scenario/pool/oracle_twap_basic_filetest.gno (1)
28-35: Consider checking errors from access.GetAddress in test setup.For consistency with the recommendation on the other TWAP tests, consider validating setup errors.
contract/r/scenario/pool/oracle_twap_full_scenario_filetest.gno (1)
36-44: Consider checking errors from access.GetAddress in test setup.Consistent with the other TWAP scenario tests, validating these errors would improve debuggability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contract/r/gnoswap/pool/README.mdcontract/r/gnoswap/pool/v1/README.mdcontract/r/gnoswap/pool/v1/manager.gnocontract/r/scenario/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno
🚧 Files skipped from review as they are similar to previous changes (1)
- contract/r/gnoswap/pool/v1/manager.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/scenario/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_basic_filetest.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/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_basic_filetest.gno
🪛 markdownlint-cli2 (0.18.1)
contract/r/gnoswap/pool/v1/README.md
88-88: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
160-160: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (12)
- GitHub Check: test-gnoswap (gnoswap/scenario/gns, contract/r/scenario/gns)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (13)
contract/r/gnoswap/pool/README.md (4)
41-41: Update or remove outdated price validation note.The note states "No price validation performed," but the PR objective is to tighten sqrtPrice upper-bound validation (making it exclusive). If validation is now enforced, this note should be updated to reflect the new validation behavior and boundaries.
Can you confirm whether price validation is now performed during pool creation, and update the note accordingly if validation is active?
12-12: Configuration and concept updates are clear.The Protocol Fee range, Max Liquidity Per Tick, and Tick System clarifications are well-defined and align with the validation changes mentioned in the PR objectives.
Also applies to: 16-16, 29-29
77-134: Comprehensive Swap Callback documentation is well-structured.The new Swap Callback section provides clear guidance on:
- Callback signature and delta convention (positive/negative meanings)
- Concrete examples for both swap directions (zeroForOne cases)
- Implementation example using SafeGRC20Transfer for security
- Important notes on caller verification, minimum delta transfers, and balance validation
This aligns well with the behavioral changes (error handling vs. panic) and security improvements (SafeGRC20Transfer usage) mentioned in the enriched summary.
188-193: Protocol fees documentation is now clearer.The update clarifying "Optional 0% or 4-10% of swap fees" with the denominator range (4-10) provides more precision than before, making it easier for integrators to understand the fee configuration options.
contract/r/gnoswap/pool/v1/README.md (2)
108-127: Previous callback issues have been successfully fixed.The callback implementation example has been corrected from the past review:
- Line 114: Error handling now returns
errors.New()instead of usingpanic()- Lines 119, 123: Calls to
common.SafeGRC20Transfernow include thecrosskeyword for proper realm-crossing callsThese corrections align the example with Gno language requirements and proper error handling patterns.
208-241: Pool Creation Griefing section is well-documented with clear recovery mechanism.The expanded security section now provides:
- Clear problem statement and impact analysis
- Concrete recovery mechanism with step-by-step example
- Prevention strategies and deterrents
This documentation helps developers understand both the risk and mitigation strategies.
contract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gno (2)
198-209: LGTM: Proper realm context handling in swap callback.The
mockSwapCallbackcorrectly sets the realm toadminRealmbefore performing token transfers, which is necessary for the SafeGRC20Transfer calls to succeed with proper authorization context.
57-196: LGTM: Test follows proper Gno patterns.The test demonstrates correct usage of:
testing.SetRealm()to establish caller contextcrosskeyword for realm-crossing function callstesting.SkipHeights()for time simulation- Structured scenario logging for clear test output
The test flow logically validates TWAP calculations with increased observation cardinality over multiple time intervals and price changes.
contract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno (2)
160-171: LGTM: Proper realm context handling in swap callback.The callback correctly establishes realm context before token transfers.
58-158: LGTM: Test validates TWAP behavior with price changes.This scenario correctly tests TWAP weighted averaging by:
- Maintaining stable price (tick 0) for 2 hours
- Executing a price-changing swap at the midpoint
- Maintaining new price for another 2 hours
- Querying TWAP over 1h, 2h, and 4h intervals
The expected output shows TWAP correctly reflects the weighted average (-953 over 4h = average of 0 for 2h and -1906 for 2h).
contract/r/scenario/pool/oracle_twap_basic_filetest.gno (1)
55-132: LGTM: Clean baseline TWAP test.This test establishes the baseline TWAP behavior when price remains stable:
- Creates pool at tick 0
- Adds liquidity
- Advances time without any swaps
- Validates TWAP remains at tick 0 across all query intervals
This provides a good foundation for understanding TWAP behavior before testing more complex scenarios with price changes.
contract/r/scenario/pool/oracle_twap_full_scenario_filetest.gno (2)
247-258: LGTM: Proper realm context handling in swap callback.The callback implementation correctly sets realm context before performing token transfers.
57-245: LGTM: Comprehensive TWAP scenario test.This test provides excellent coverage of TWAP behavior under realistic conditions:
- Increased observation cardinality (200 slots)
- Multiple price-changing swaps over 4 hours
- Mid-scenario liquidity addition
- TWAP queries at 1h, 2h, and 4h intervals
The scenario validates that TWAP correctly computes weighted averages across:
- Multiple price levels (tick 0 → -976 → -560 → -818)
- Changes in liquidity depth
- Extended time periods with sufficient observation slots
The expected outputs demonstrate proper TWAP calculation: -818 (1h), -689 (2h), -589 (4h), showing the weighted contribution of earlier price levels diminishing over longer windows.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/r/gnoswap/pool/v1/position_test.gno (1)
104-138: Possible compile error: undeclarederrvariable.Line 119 uses
err =buterrwas never declared in this scope. The previous version likely hadpositionKey, err := getPositionKey(...)which declarederr, but with the signature change,erris no longer declared.🐛 Proposed fix
// Create position through modifyPosition liquidityDelta := i256.MustFromDecimal(scenario.liquidity) params := ModifyPositionParams{ owner: scenario.owner, tickLower: scenario.tickLower, tickUpper: scenario.tickUpper, liquidityDelta: liquidityDelta, } - _, amount0, amount1, err = modifyPosition(pool, params) + _, amount0, amount1, err := modifyPosition(pool, params) if err != nil { t.Fatalf("Failed to create position: %v", err) }
🧹 Nitpick comments (1)
contract/r/gnoswap/pool/v1/position.gno (1)
34-42: Documentation inconsistency with new function signature.The doc comment on line 34 still states "Returns base64-encoded position key or error" but the function now returns only a
string. The signature simplification is appropriate sincestrconv.Itoaandbase64.StdEncoding.EncodeToStringhave no failure modes for these inputs.📝 Suggested doc update
-// Returns base64-encoded position key or error. +// Returns base64-encoded position key. // Combines position package path, tickLower, and tickUpper. func getPositionKey( tickLower int32, tickUpper int32, -) string { +) string {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contract/r/gnoswap/pool/v1/pool.gnocontract/r/gnoswap/pool/v1/pool_test.gnocontract/r/gnoswap/pool/v1/position.gnocontract/r/gnoswap/pool/v1/position_test.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gno
🚧 Files skipped from review as they are similar to previous changes (2)
- contract/r/gnoswap/pool/v1/pool.gno
- contract/r/gnoswap/pool/v1/pool_test.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/pool/v1/position.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gnocontract/r/gnoswap/pool/v1/position_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/pool/v1/position.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gnocontract/r/gnoswap/pool/v1/position_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). (6)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- 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/emission, contract/r/scenario/emission)
- GitHub Check: run-integration-test
🔇 Additional comments (8)
contract/r/gnoswap/pool/v1/position.gno (1)
378-378: LGTM!The call site correctly updated to use the simplified single-return signature. The position key generation is now cleaner without unnecessary error handling.
contract/r/scenario/position/position_reposition_tick_role_change_filetest.gno (3)
32-37: LGTM!Constants for price bounds and max int64 are correctly defined for use in the scenario tests.
366-376: LGTM!The mock swap callback correctly handles token transfers based on swap direction. Setting the admin realm inside the callback enables cross-realm token transfers as expected in the test context.
181-193: Test expectations contradict tick deletion logic.The test expects to query tick 9000 after its liquidity is fully removed, but the implementation deletes ticks when they flip from non-zero to zero liquidity. In
position.gno(lines 393-397), whenliquidityDelta.IsNeg()andflippedLower=true,deleteTick(p, positionParams.tickLower)is called. Subsequently,GetTickInitialized()callsmustGetTick(), which panics if the tick doesn't exist.This test will panic at line 189 when querying tick 9000. Either:
- Remove this check (deleted ticks cannot be queried)
- Or modify the implementation to persist ticks with zero liquidity for fee tracking (standard in Uniswap v3)
If persistence is intended, document why ticks remain initialized despite zero liquidity.
contract/r/gnoswap/pool/v1/position_test.gno (4)
248-261: LGTM!The test correctly adapted to use the single-return
getPositionKeysignature. The added uniqueness verification (altKey comparison) is a good test enhancement ensuring different tick ranges produce different position keys.
1411-1446: LGTM!Good addition of boundary edge case tests for tick extremes (MIN_TICK, MAX_TICK, full range). The tests verify that
getPositionKeygenerates valid keys at tick boundaries.
1448-1517: LGTM!Comprehensive liquidity boundary testing covering minimum values, large values near
maxLiquidityPerTick, and overflow scenarios. The tests properly verify both success and panic conditions.
1519-1556: LGTM!Good addition of overflow edge case testing for
TokensOwedvalues. The test properly verifies that amounts are calculated and tokens owed accumulate correctly during the position lifecycle.
contract/r/scenario/position/position_reposition_tick_role_change_filetest.gno
Outdated
Show resolved
Hide resolved
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
from
3c0c469 to
2d2f135
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contract/r/gnoswap/pool/v1/position.gno (1)
19-43: Update documentation to match the simplified function signature.The documentation on line 34 states "Returns base64-encoded position key or error" but the function now only returns a string (line 39). Update the documentation to reflect that this function always succeeds and returns only the position key.
📝 Suggested documentation fix
// getPositionKey generates a unique base64-encoded key for a liquidity position. // // Creates deterministic identifier for position tracking. // Ensures unique positions per (position package path, price range) combination. // // Used internally for position state management. // // Parameters: // - tickLower: Lower boundary tick of position range // - tickUpper: Upper boundary tick of position range // // Key Format: // // base64(position_package_path + "__" + tickLower + "__" + tickUpper) // -// Returns base64-encoded position key or error. -// Combines position package path, tickLower, and tickUpper. +// Returns: +// - string: base64-encoded position key +// +// Note: Tick validation must be performed by the caller before invoking this function. func getPositionKey( tickLower int32, tickUpper int32, ) string {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contract/r/gnoswap/pool/v1/pool.gnocontract/r/gnoswap/pool/v1/pool_test.gnocontract/r/gnoswap/pool/v1/position.gnocontract/r/gnoswap/pool/v1/position_test.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.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/pool/v1/position.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gnocontract/r/gnoswap/pool/v1/pool_test.gnocontract/r/gnoswap/pool/v1/position_test.gnocontract/r/gnoswap/pool/v1/pool.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/pool/v1/position.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gnocontract/r/gnoswap/pool/v1/pool_test.gnocontract/r/gnoswap/pool/v1/position_test.gnocontract/r/gnoswap/pool/v1/pool.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). (9)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (14)
contract/r/gnoswap/pool/v1/position.gno (1)
378-378: LGTM! Simplified position key generation.The removal of error handling from
getPositionKeycalls is safe since tick validation occurs earlier inmodifyPosition(line 230 viavalidateTicks). This simplification improves code readability.contract/r/gnoswap/pool/v1/pool.gno (3)
157-157: LGTM! Consistent with simplified position key API.Direct assignment reflects the updated
getPositionKeysignature that no longer returns an error.
211-211: LGTM! Consistent API usage.Position key generation simplified across all call sites.
258-260: LGTM! More idiomatic error handling.The short variable declaration pattern (
if err := ...; err != nil) is more idiomatic and reduces variable scope.contract/r/gnoswap/pool/v1/pool_test.gno (3)
638-641: LGTM! Improved boundary test clarity.The explicit MAX_SQRT_RATIO value and exclusivity clarification improve test documentation. The boundary condition is now clearly documented as exclusive.
750-750: LGTM! Consistent with updated API.Position key generation updated to match the new signature.
1023-1137: LGTM! Comprehensive edge case coverage for collectToken.Excellent test coverage including:
- Equal amounts scenario
- Minimum value scenarios for each parameter
- Zero value scenarios
- MAX_UINT128 boundary cases
- Small differences detection
This significantly improves the reliability of the token collection logic.
contract/r/scenario/position/position_reposition_tick_role_change_filetest.gno (2)
1-97: LGTM! Well-structured scenario test with clear documentation.This scenario test effectively validates the complex tick role transition behavior where a tick changes from lower to upper boundary. The step-by-step approach with clear logging makes it easy to understand and debug.
367-377: LGTM! Proper token transfer in swap callback.The
mockSwapCallbackcorrectly usesSafeGRC20Transferwith thecrosskeyword for realm crossing and handles bothzeroForOnedirections appropriately.contract/r/gnoswap/pool/v1/position_test.gno (5)
104-139: LGTM! Simplified position creation helper.The removal of error handling from
getPositionKey(line 108) is consistent with the API change and simplifies the test helper function.
250-263: LGTM! Improved test with uniqueness validation.The addition of the uniqueness check (lines 255-258) is valuable - it verifies that different tick ranges produce different position keys, which is a critical invariant for the position tracking system.
1413-1447: LGTM! Comprehensive tick boundary edge case coverage.These tests validate that MIN_TICK and MAX_TICK values are properly handled as position boundaries. The full range test (MIN_TICK to MAX_TICK) is particularly important for ensuring the system can handle the entire valid tick spectrum.
1449-1518: LGTM! Thorough liquidity boundary validation.Excellent coverage of liquidity edge cases:
- Minimum viable liquidity (1)
- Liquidity near maxLiquidityPerTick boundary
- Overflow detection when exceeding maxLiquidityPerTick
This prevents potential overflow attacks and ensures proper liquidity constraints.
1520-1557: LGTM! Amount overflow protection testing.The test validates that tokensOwed calculations handle accumulation correctly without overflow, which is critical for preventing loss of funds or state corruption.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contract/r/gnoswap/pool/v1/oracle.gnocontract/r/gnoswap/pool/v1/oracle_integration_test.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/pool/v1/oracle_integration_test.gnocontract/r/gnoswap/pool/v1/oracle.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/pool/v1/oracle_integration_test.gnocontract/r/gnoswap/pool/v1/oracle.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/test/fuzz, contract/r/gnoswap/test/fuzz)
- 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/scenario/gov/governance, contract/r/scenario/gov/governance)
- 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/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/gnsmath, contract/p/gnoswap/gnsmath)
- GitHub Check: run-integration-test
🔇 Additional comments (7)
contract/r/gnoswap/pool/v1/oracle.gno (3)
425-427: Improved error message for zero cardinality.The new error message is clearer than the previous implementation. Good improvement for developer experience.
163-164: Optimization approved: grow now skips pre-initialized slots.Starting the loop from
currentCardinalityinstead of 0 is a good optimization that avoids unnecessary checks for already-initialized slots.
123-133: Zero liquidity handling is correct and well-tested.The code at lines 123-133 uses
max(1, liquidity)to prevent division by zero in thesecondsPerLiquiditycalculation. This approach is:
- Tested: oracle_test.gno includes an explicit "zero liquidity" test case (line 271)
- Validated: oracle_integration_test.gno confirms the expected calculation when liquidity is zero (line 466)
- Documented: The behavior is explicitly commented in the code
- Consistent: The pattern matches how zero liquidity is handled throughout the codebase (sqrt_price_math.gno, position.gno, tick.gno, etc.)
No concerns remain.
contract/r/gnoswap/pool/v1/oracle_integration_test.gno (4)
103-111: Good refactor: dynamic cardinality growth improves test flexibility.The change from static
increaseObservationCardinalityLimitByPool(pool, 10)to dynamic calculation based on scenario count makes the test more maintainable and accurate.
207-209: Test assumes nil afterOrAt, but this may cause issues.The test expects
afterOrAtto be nil when target equals the newest observation (lines 207-209). However, as noted in the oracle.gno review, returning nil forafterOrAtcan cause nil pointer dereferences inobserveSingle(line 287).If the oracle.gno implementation is fixed to return
beforeOrAtfor both values when they're equal, this test assertion should be updated accordingly.After fixing the nil
afterOrAtissue in oracle.gno, update this test:uassert.NoError(t, err, "Should not error when target equals observation") uassert.Equal(t, currentTime, beforeOrAt2.BlockTimestamp(), "Should return exact observation") - if afterOrAt2 != nil { - t.Fatalf("afterOrAt should be nil when target equals newest observation") - } + uassert.NotNil(t, afterOrAt2, "afterOrAt should be the same as beforeOrAt when target equals observation") + uassert.Equal(t, beforeOrAt2.BlockTimestamp(), afterOrAt2.BlockTimestamp(), "Both should have same timestamp")
465-468: Zero liquidity test calculation is correct.The test correctly validates that when liquidity is zero, the calculation uses
max(1, liquidity) = 1, resulting insecondsPerLiquidity = 100 * 2^128 / 1. This aligns with the oracle.gno implementation.
435-439: Test now correctly expects uninitialized observations to be retrievable.The updated test (lines 435-439) correctly reflects that
observationAtreturns uninitialized observations without error, and the caller must checkInitialized(). This is a good change that aligns with the oracle.gno behavior.
Add test for repositioning when a tick changes its role from lower to upper boundary. The test demonstrates: - Position starts at [9000, 13000] with tick 9000 as lower boundary - After reposition to [5000, 9000], tick 9000 becomes upper boundary - Tick liquidityNet changes from positive (lower) to negative (upper) The test follows the proper reposition pattern: 1. Collect fees from the position 2. Decrease liquidity to make position clear 3. Reposition to new tick range
junghoon-vans
force-pushed
the
pool-codebase-maintenance-and-cleanup
branch
from
5bace90 to
6c99388
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contract/r/gnoswap/pool/oracle.gno (1)
131-139: Semantically correct change to default observation initialization.The change from
initialized: truetoinitialized: falseproperly reflects that a "default" observation is a placeholder that hasn't been written with actual data yet. This contrasts appropriately withNewObservationState(line 104-109), which creates the first valid observation withinitialized: true.Consider adding documentation to clarify the semantic difference between the constructors:
// NewDefaultObservation creates an uninitialized placeholder observation. // Used for pre-allocating observation slots before they are written with actual data. // For creating the initial valid observation, use NewObservationState instead. func NewDefaultObservation() *Observation {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contract/r/gnoswap/pool/oracle.gnocontract/r/gnoswap/pool/v1/manager.gnocontract/r/gnoswap/pool/v1/manager_test.gno
🚧 Files skipped from review as they are similar to previous changes (1)
- contract/r/gnoswap/pool/v1/manager.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/pool/oracle.gnocontract/r/gnoswap/pool/v1/manager_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/pool/oracle.gnocontract/r/gnoswap/pool/v1/manager_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). (4)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (2)
contract/r/gnoswap/pool/v1/manager_test.gno (2)
132-199: LGTM! Comprehensive boundary testing for sqrtPriceX96.The new test cases thoroughly validate the sqrtPriceX96 boundaries:
- Correctly tests MIN_SQRT_RATIO (4295128739) as inclusive
- Correctly tests MAX_SQRT_RATIO (1461446703485210103287273052203988822378723970342) as exclusive
- Tests both sides of each boundary (±1)
- Includes edge cases like sqrtPrice=1 and mid-range values
The test coverage ensures the pool creation properly rejects invalid price ranges and accepts valid ones according to the updated boundary semantics.
304-438: The test correctly focuses on boundary value validation for the fee protocol calculation logic. The access control is appropriately enforced at the public API level in theSetFeeProtocolmethod (capital S), which usesaccess.AssertIsAdminOrGovernance(caller)to restrict access. The privatesetFeeProtocolmethod being tested is a pure utility for validation and state updates, and correctly has no access control checks (as documented in its comment). This is a sound architectural pattern where the public crossing function handles permissions while helper methods handle logic. The test does not need an unauthorized access case because it's testing the internal validation, not the access control layer.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
contract/r/scenario/pool/swap_balance_mixed_positions_filetest.gno (2)
147-148: Consider handling the error fromGetPool.The error from
GetPoolis discarded. While this test would panic if the pool doesn't exist, explicit error handling improves debuggability and aligns with defensive coding practices.💡 Suggested improvement
- pools, _ := pool.GetPool(bazPath, quxPath, fee500) + pools, exists := pool.GetPool(bazPath, quxPath, fee500) + if !exists { + panic("pool not found") + }
264-275: Consider propagating transfer errors frommockSwapCallback.The callback returns
nilunconditionally even ifSafeGRC20Transfermight fail. While the conservation checks at the end would catch any discrepancy, surfacing transfer failures early provides clearer diagnostics.contract/r/scenario/pool/swap_balance_extreme_filetest.gno (1)
286-297: Callback error handling could be improved.Same observation as the other scenario files: the
mockSwapCallbackreturnsnilunconditionally. For consistency and better diagnostics, consider checking ifSafeGRC20Transferhas any failure mode that should be propagated.contract/r/scenario/pool/swap_balance_single_in_range_filetest.gno (1)
60-67: Consider extracting shared utilities across scenario files.The
BalanceSnapshotstruct,getBalanceSnapshot,validateBalanceChange, andmockSwapCallbackpatterns are nearly identical across all three scenario files (with only token names differing). While self-contained tests are valuable, extracting these into a shared test helper package could reduce maintenance burden.This could be deferred if the preference is to keep scenario tests fully self-contained.
Also applies to: 94-103, 218-247, 249-260
contract/r/scenario/position/position_reposition_tick_role_change_filetest.gno (2)
80-86: Consider documenting the sqrtPriceX96 magic number.The sqrtPriceX96 value
"130621891405341611593710811006"is not immediately obvious. A brief comment explaining this corresponds to tick 10000 would improve test readability.📝 Suggested documentation
ufmt.Println("[INFO] Creating pool at tick 10000") pool.CreatePool( cross, barPath, fooPath, fee500, + // sqrtPriceX96 corresponding to tick 10000 "130621891405341611593710811006", )
177-186: Consider documenting the full liquidity value.The hardcoded liquidity
"1183668969"should match the liquidity from the initial mint (line 244 of expected output). A brief comment would clarify this dependency.📝 Suggested documentation
ufmt.Println("[INFO] Decreasing liquidity to clear position") _, _, _, _, amount0Decreased, amount1Decreased, _ := position.DecreaseLiquidity( cross, 1, - "1183668969", // full liquidity + "1183668969", // full liquidity from initial mint in mintPosition() "0", "0", 9999999999, false, )contract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gno (1)
114-166: [MEDIUM] Realm switching in callbacks is risky; create/restore realm explicitly.Line 116 and Line 146:
testing.NewCodeRealm("gno.land/r/gnoswap/router")is recreated multiple times; plus the callback later overrides the realm (Line 195) without restoring it. This can make the scenario brittle as it grows.Fix: create a
routerRealm := testing.NewCodeRealm(...)once and restore realm inmockSwapCallback(see next comment).
-->contract/r/scenario/pool/oracle_twap_full_scenario_filetest.gno (1)
114-221: [MEDIUM] Prefer deterministic realm management across swaps/mints (avoid hidden realm state).Line 116, 165, 195: Multiple
testing.SetRealm(testing.NewCodeRealm(...))calls plus a callback that mutates realm state makes it easy to accidentally run later steps under the wrong caller.Fix: create
routerRealmonce; ensure callback restores realm (see below).
-->
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contract/r/scenario/pool/oracle_twap_basic_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_with_price_change_filetest.gnocontract/r/scenario/pool/swap_balance_extreme_filetest.gnocontract/r/scenario/pool/swap_balance_mixed_positions_filetest.gnocontract/r/scenario/pool/swap_balance_single_in_range_filetest.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gno
🚧 Files skipped from review as they are similar to previous changes (2)
- contract/r/scenario/pool/oracle_twap_with_price_change_filetest.gno
- contract/r/scenario/pool/oracle_twap_basic_filetest.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/scenario/pool/swap_balance_mixed_positions_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/swap_balance_extreme_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gnocontract/r/scenario/pool/swap_balance_single_in_range_filetest.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/pool/swap_balance_mixed_positions_filetest.gnocontract/r/scenario/pool/oracle_twap_full_scenario_filetest.gnocontract/r/scenario/pool/swap_balance_extreme_filetest.gnocontract/r/scenario/pool/oracle_twap_with_cardinality_filetest.gnocontract/r/scenario/position/position_reposition_tick_role_change_filetest.gnocontract/r/scenario/pool/swap_balance_single_in_range_filetest.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/position, contract/r/scenario/position)
- 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/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (14)
contract/r/scenario/pool/swap_balance_mixed_positions_filetest.gno (2)
60-67: LGTM! Well-structured balance tracking type.The
BalanceSnapshotstruct properly uses unexported fields, preventing external modification. The design cleanly captures all relevant balance components for conservation validation.
233-262: LGTM! Solid balance conservation validation.The
validateBalanceChangefunction correctly verifies exact conservation by summing user, pool, and protocol fee changes for each token. Usingpanicfor validation failures is appropriate for scenario tests.contract/r/scenario/pool/swap_balance_extreme_filetest.gno (2)
69-98: LGTM! Comprehensive extreme price movement scenario.The
mainfunction orchestrates a well-structured test flow covering setup, extreme downward movement, extreme upward movement, and maximum slippage. The sequential balance validation after each step ensures conservation is maintained throughout price extremes.
150-183: LGTM! Good coverage of extreme downward price movement.The test swaps 10M OBL which moves the tick from 0 to -20, validating the pool handles significant price drops correctly while maintaining balance conservation.
contract/r/scenario/pool/swap_balance_single_in_range_filetest.gno (3)
1-26: LGTM! Clean scenario test for basic in-range swaps.The file is well-organized with clear purpose documentation and proper imports. This provides a good baseline test for the simplest swap scenario.
115-140: LGTM! Pool setup is straightforward and correct.The setup creates a pool at 1:1 price with a symmetric tick range (-1000 to 1000), which is ideal for testing basic in-range swaps without edge cases.
180-216: LGTM! Exact output swap test is correctly implemented.The approval buffer of 200000 FOO for an exact output of 50000 BAR appropriately accounts for the unknown input amount plus fees.
contract/r/scenario/position/position_reposition_tick_role_change_filetest.gno (7)
1-47: Test setup and imports look well-structured.The scenario test is properly organized with clear documentation comments, appropriate imports (including side-effect imports for module registration), and well-defined constants. The global variable initialization pattern using
access.GetAddressis appropriate for test scenarios.
49-73: Well-organized test orchestration.The main function provides a clear, readable test flow with descriptive scenario labels. Each step is properly isolated into its own function, making the test easy to understand and maintain.
93-120: Position minting follows correct patterns.The function properly sets the realm context, approves tokens before the operation, and mints a position with a clearly defined tick range. The test values are appropriate for the scenario.
136-165: Swap execution correctly simulates router context.The function appropriately switches realm context to the router before executing the swap, accurately simulating how swaps would occur in production. The callback pattern with
cur realmis correctly implemented.
122-134: Tick state validation is correctly implemented.The function properly retrieves tick info with error handling via panic (appropriate for Gno), and validates the key properties (liquidityGross, liquidityNet, initialized) that demonstrate tick 9000 is functioning as a lower boundary.
223-233:mockSwapCallbackusescrosswithout being a crossing function.This function uses the
crosskeyword (lines 227, 229) to callcommon.SafeGRC20Transfer, but it lacks thecur realmparameter that defines a crossing function. According to Gno semantics,crossshould only be used within crossing functions.Since this is called from within the callback closure (line 155-157) that does have
cur realm, consider either:
- Adding
cur realmparameter to make this a proper crossing function- Inlining the transfer logic directly in the callback
♻️ Option 1: Make it a crossing function
-func mockSwapCallback(token0Path string, token1Path string, amount0Delta int64, amount1Delta int64, zeroForOne bool) error { +func mockSwapCallback(cur realm, token0Path string, token1Path string, amount0Delta int64, amount1Delta int64, zeroForOne bool) error { testing.SetRealm(adminRealm) if zeroForOne { common.SafeGRC20Transfer(cross, token0Path, poolAddr, amount0Delta) } else { common.SafeGRC20Transfer(cross, token1Path, poolAddr, amount1Delta) } return nil }And update the call site:
- return mockSwapCallback(barPath, fooPath, amount0Delta, amount1Delta, true) + return mockSwapCallback(cross, barPath, fooPath, amount0Delta, amount1Delta, true)
235-278: Expected output provides comprehensive scenario verification.The output section correctly validates the complete test scenario, including the critical assertion that tick 9000's
liquidityNettransitions from positive (as lower boundary) to negative (as upper boundary). This confirms the tick role change behavior is working as expected.
There was a problem hiding this comment.
Actionable comments posted: 3
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/pool/v1/oracle.gno (2)
322-367: [CRITICAL]binarySearchinfinite-loop and uninitialized observation handlingLines 380–415: The loop uses
for {}with no termination condition. Wheni == 0andr = i - 1executes,runderflows to65535(uint16 max), causing the search to become non-terminating. Additionally, line 390–393 checkserrbut does not validateatOrAfter.Initialized(), allowing a default/uninitialized observation to propagate and break timestamp ordering invariants.Problem:
- Infinite loop with uint16 underflow:
rwraps to65535when subtracting from0, and withl == 0, the loop repeats indefinitely.- Uninitialized observation:
atOrAftercan be uninitialized without raising an error, violating the contract that returned observations must have valid timestamps.Fix:
for l <= r { i = (l + r) / 2 beforeIndex := uint16(i % uint64(cardinality)) obs, err := observationAt(os, beforeIndex) if err != nil || !obs.Initialized() { l = i + 1 continue } beforeOrAt = obs afterIndex := uint16((i + 1) % uint64(cardinality)) atOrAfter, err = observationAt(os, afterIndex) if err != nil || !atOrAfter.Initialized() { l = i + 1 continue } targetAtOrAfter := beforeOrAt.BlockTimestamp() <= target if targetAtOrAfter && target <= atOrAfter.BlockTimestamp() { break } if !targetAtOrAfter { if i == 0 { break } r = i - 1 } else { l = i + 1 } } if beforeOrAt == nil || atOrAfter == nil { return nil, nil, errors.New("failed to find surrounding observations") } return beforeOrAt, atOrAfter, nilRationale: Adding the loop bound
l <= rterminates safely when the search space is exhausted. CheckingatOrAfter.Initialized()ensures only valid observations are used for timestamp comparisons. The guardif i == 0 { break }prevents underflow ofr. The nil-check before return ensures the function never returns uninitialized pointers.
238-317: Line 296-298: Tick cumulative interpolation truncates early, losing precision.The current formula
((after-before)/observationTimeDelta)*targetDeltaperforms integer division before multiplication, causing unnecessary precision loss. The standard interpolation form(after-before)*targetDelta/observationTimeDeltareduces truncation error and matches howsecondsPerLiquidityCumulativeX128is interpolated (lines 305-314).If overflow is a concern for int64, document it explicitly and add overflow detection similar to the
MulOverflowcheck inwriteObservation, rather than relying on divide-first to avoid overflow.Example fix
delta := afterOrAt.TickCumulative() - beforeOrAt.TickCumulative() tickCumulative := beforeOrAt.TickCumulative() + (delta*targetDelta)/observationTimeDelta
🧹 Nitpick comments (11)
contract/r/scenario/pool/oracle_twap_single_swap_filetest.gno (1)
142-156: DerivepoolPathfee fromfee500to prevent accidental mismatch.Line 144: hard-coding
"500"is brittle whenfee500exists.Proposed change
- poolPath := barPath + ":" + fooPath + ":" + "500" + poolPath := barPath + ":" + fooPath + ":" + ufmt.Sprintf("%d", fee500)contract/r/scenario/pool/oracle_twap_increased_cardinality_filetest.gno (1)
59-205: Avoid silent scenario “passes”: handle returned errors from pool APIs (GetPool/GetTWAP/Swap).Line 84, Line 118, Line 148, Line 178, Line 182, Line 186: ignoring
errrisks misleading “[EXPECTED]” logs if the call fails without panicking.contract/r/gnoswap/pool/v1/oracle.gno (1)
101-148:transform: fix error message and consider nil-liquidity guard.Line 103-105: condition is
timeDelta < 0but message says “greater than 0” (should be “>= 0”).
Line 115-116 / Line 124: ifliquiditycan ever be nil at call sites, this will panic.Proposed change
if timeDelta < 0 { - return nil, errors.New("time delta must be greater than 0") + return nil, errors.New("time delta must be >= 0") } +if liquidity == nil { + return nil, errors.New("liquidity must not be nil") +}contract/r/gnoswap/pool/v1/oracle_integration_test.gno (4)
103-112: Cardinality pre-growth logic: consider also settingCardinality(...)(or explain why limit-only is enough).Line 103-111: you
grow(...)then onlySetCardinalityLimit(neededCard). This relies onwriteObservationlater growing cardinality whenIndex == Cardinality-1. If that coupling is intentional, a short comment would help.
71-75: Fix assertion message: logic is “non-increasing as secondsAgo increases”.Line 73-75:
Gteis consistent with the query order[0, 300, 600, 900](newest → oldest), but the message says “non-decreasing forward in time”, which reads opposite.
262-287: Fix misleading test comment aboutgrowinitialization behavior.Line 262-266: comment says “initialized==true”, but Line 285-287 asserts new slots are not initialized (which matches the updated
growbehavior).
364-400: “Reject > max” coverage is currently informational-only; consider asserting the rejection via another API surface.Line 369-386: since
maxObservationCardinalityisuint16max, the test can’t pass> maxintogrow. If there’s another function that takes a wider type (or validates againstmaxObservationCardinalitywith an explicit comparison), it’d be better to assert the error path directly.contract/r/scenario/pool/oracle_twap_no_price_change_filetest.gno (2)
92-127: Avoid silent scenario “passes”: handle returned errors from GetPool/GetTWAP.Line 92 and Line 100-122: ignoring
errcan make the scenario print “expected” values even if TWAP retrieval fails (depending on the API’s failure mode).
97-102: DerivepoolPathfee fromfee500to prevent accidental mismatch.Line 99: hard-coded
"500"is brittle.contract/r/scenario/pool/oracle_twap_multiple_swaps_filetest.gno (2)
59-254: Avoid silent scenario “passes”: handle returned errors from pool APIs (GetPool/GetTWAP/Swap/Mint).This scenario is especially sensitive to silent failures because the final TWAPs are “expected” logs, not assertions.
223-241: DerivepoolPathfee fromfee500to prevent accidental mismatch.Line 225: hard-coded
"500"is brittle.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contract/r/gnoswap/pool/v1/oracle.gnocontract/r/gnoswap/pool/v1/oracle_integration_test.gnocontract/r/scenario/pool/oracle_twap_increased_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_multiple_swaps_filetest.gnocontract/r/scenario/pool/oracle_twap_no_price_change_filetest.gnocontract/r/scenario/pool/oracle_twap_single_swap_filetest.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/scenario/pool/oracle_twap_increased_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_single_swap_filetest.gnocontract/r/scenario/pool/oracle_twap_no_price_change_filetest.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/scenario/pool/oracle_twap_multiple_swaps_filetest.gnocontract/r/gnoswap/pool/v1/oracle_integration_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/scenario/pool/oracle_twap_increased_cardinality_filetest.gnocontract/r/scenario/pool/oracle_twap_single_swap_filetest.gnocontract/r/scenario/pool/oracle_twap_no_price_change_filetest.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/scenario/pool/oracle_twap_multiple_swaps_filetest.gnocontract/r/gnoswap/pool/v1/oracle_integration_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). (12)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- 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/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (2)
contract/r/scenario/pool/oracle_twap_single_swap_filetest.gno (1)
162-173: Swap callback realm switch looks intentional; keep it explicit.Line 163: setting
adminRealminsidemockSwapCallbackis easy to miss; it’s correct if the intention is “admin pays swap deltas” (vs router/user).contract/r/gnoswap/pool/v1/oracle.gno (1)
150-172: Growth optimization and cardinality==0 guard look good.
- Line 163-169: growing only
[currentCardinality, nextCardinality)is a clear improvement.- Line 425-427: explicit
cardinality == 0error avoids confusing downstream behavior.Also applies to: 416-452
… and adjust related tests
There was a problem hiding this comment.
Actionable comments posted: 0
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/pool/upgrade_test.gno (1)
1142-1156: [MEDIUM] Rule: Tests must assert success conditions (don’t ignore injected errors)Line 1143: the “success” path sets
GetTWAPto returnerrors.New(""), but the test then ignores the returned error (Line 1148). This can mask regressions and/or make the test semantically inconsistent.Fix: return a nil error from the mock, and assert
err == nil.Proposed patch
- mockPool.Response.Set("GetTWAP", tt.expectedTWAP, u256.Zero(), errors.New("")) + mockPool.Response.Set("GetTWAP", tt.expectedTWAP, u256.Zero(), error(nil)) testing.SetRealm(tt.callerRealm) // Action - twap, _, _ := GetTWAP(tt.poolPath, tt.secondsAgo) + twap, _, err := GetTWAP(tt.poolPath, tt.secondsAgo) // Assert if mockPool.Response.CallCount("GetTWAP") != 1 { t.Error("GetTWAP was not called on the implementation") } + uassert.NoError(t, err) if twap != tt.expectedTWAP { t.Errorf("GetTWAP() = %v, want %v", twap, tt.expectedTWAP) }contract/r/gnoswap/pool/v1/oracle.gno (1)
330-422: [CRITICAL] Rule: No unbounded loops / integer underflow in on-chain logicLine 388:
for {}inbinarySearchhas nol <= rguard; if the search space becomes invalid (e.g., repeated “uninitialized” hits pushlpastr), this can loop indefinitely (DoS risk).
Line 415:r = i - 1onuint64risks underflow ifi == 0.Fix: use a bounded loop (
for l <= r) with signed indices (or explicit underflow guards) and return an error when not found.Proposed patch
func binarySearch(os *pl.ObservationState, currentTime int64, target int64, index uint16, cardinality uint16, ) (*pl.Observation, *pl.Observation, error) { - l := uint64((index + 1) % cardinality) // oldest observation - r := l + uint64(cardinality) - 1 // newest observation - var i uint64 - var beforeOrAt, atOrAfter *pl.Observation - - for { - i = (l + r) / 2 - - beforeIndex := uint16(i % uint64(cardinality)) + l := int64((index + 1) % cardinality) // oldest (logical) + r := l + int64(cardinality) - 1 // newest (logical) + var beforeOrAt, atOrAfter *pl.Observation + + for l <= r { + i := (l + r) / 2 + + beforeIndex := uint16(int64(i) % int64(cardinality)) obs, err := observationAt(os, beforeIndex) if err != nil || !obs.Initialized() { // we've landed on an uninitialized tick, keep searching higher (more recently) - l = i + 1 + l = i + 1 continue } beforeOrAt = obs - afterIndex := uint16((i + 1) % uint64(cardinality)) + afterIndex := uint16((int64(i) + 1) % int64(cardinality)) atOrAfter, err = observationAt(os, afterIndex) if err != nil { - l = i + 1 + l = i + 1 continue } targetAtOrAfter := beforeOrAt.BlockTimestamp() <= target // check if we've found the answer! if targetAtOrAfter && target <= atOrAfter.BlockTimestamp() { - break + return beforeOrAt, atOrAfter, nil } if !targetAtOrAfter { - r = i - 1 + r = i - 1 } else { - l = i + 1 + l = i + 1 } } - return beforeOrAt, atOrAfter, nil + return nil, nil, errNotInitializedObservation }
🧹 Nitpick comments (5)
contract/r/gnoswap/pool/_mock_test.gno (1)
568-575: [LOW] Rule: Test helper robustnessLine 574:
res[0].(int32), res[1].(*u256.Uint), res[2].(error)will panic if a test sets an incomplete/incorrect mock tuple, producing a less actionable failure than a targeted error.Problem: Makes test failures harder to diagnose when mocks are misconfigured.
Fix:
Proposed diff
func (m *MockPool) GetTWAP(poolPath string, secondsAgo uint32) (int32, *u256.Uint, error) { res, ok := m.Response.Get("GetTWAP") if !ok { return 0, nil, errors.New("not found") } - return res[0].(int32), res[1].(*u256.Uint), res[2].(error) + if len(res) != 3 { + return 0, nil, errors.New("invalid mock response for GetTWAP") + } + tick, ok0 := res[0].(int32) + hml, ok1 := res[1].(*u256.Uint) + err, ok2 := res[2].(error) + if !ok0 || !ok1 || !ok2 { + return 0, nil, errors.New("invalid mock response types for GetTWAP") + } + return tick, hml, err }Rationale: Keeps the same behavior for correct tests, but yields clearer failures for misconfigured mocks.
contract/r/gnoswap/pool/getter.gno (1)
283-287: [MEDIUM] Rule: Avoid leaking internal mutable pointers in gettersLine 285-287:
GetTWAPreturns*u256.Uintdirectly from the implementation, unlike other getters thatClone()before returning.Problem: If the implementation returns a reference to internal state, callers can mutate it and cause hard-to-track issues.
Fix:
Proposed diff
func GetTWAP(poolPath string, secondsAgo uint32) (int32, *u256.Uint, error) { - return getImplementation().GetTWAP(poolPath, secondsAgo) + tick, harmonicMeanLiquidity, err := getImplementation().GetTWAP(poolPath, secondsAgo) + if harmonicMeanLiquidity == nil { + return tick, nil, err + } + return tick, harmonicMeanLiquidity.Clone(), err }Rationale: Matches the “return copies” pattern used elsewhere in this proxy (e.g., feeGrowth getters) and makes aliasing behavior explicit.
contract/r/scenario/upgrade/implements/mock/pool/test_impl.gno (1)
543-553: [LOW] Rule: Avoid nil-related panics in mock wrappers usingany+ type assertionsLine 552:
result[1].(*u256.Uint)andresult[2].(error)will panic if a mock function returnsnil(untyped) for either value.Fix: guard nils before asserting.
Proposed patch
func (t *TestPool) GetTWAP(poolPath string, secondsAgo uint32) (int32, *u256.Uint, error) { result := t.ExecuteFn( "GetTWAP", func(args ...any) any { r1, r2, r3 := t.instance.GetTWAP(args[0].(string), args[1].(uint32)) return []any{r1, r2, r3} }, poolPath, secondsAgo, ).([]any) - return result[0].(int32), result[1].(*u256.Uint), result[2].(error) + var liq *u256.Uint + if result[1] != nil { + liq = result[1].(*u256.Uint) + } + var err error + if result[2] != nil { + err = result[2].(error) + } + return result[0].(int32), liq, err }contract/r/gnoswap/pool/v1/getter.gno (1)
238-252: [LOW] Rule: Exported API docs must match the signatureLine 244-245: comment says “Returns TWAP tick and error” but the signature now returns
(int32, *u256.Uint, error)(harmonic mean liquidity added). Please update the doc to prevent misuse.contract/r/gnoswap/pool/v1/oracle_test.gno (1)
349-474: [LOW] Rule: Prefer explicit table fields over branching ontt.nameLine 415:
if tt.name == "correct output for liquidity overflow"couples behavior to a string. Consider adding a boolean likeexpectMaxUint128 boolin the test table.Also applies to: 496-514
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
contract/r/scenario/getter/pool_getter_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/pool/oracle_twap_increased_cardinality_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/pool/oracle_twap_multiple_swaps_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/pool/oracle_twap_no_price_change_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/pool/oracle_twap_single_swap_filetest.gnois excluded by!**/*filetest.gno
📒 Files selected for processing (10)
contract/r/gnoswap/pool/_mock_test.gnocontract/r/gnoswap/pool/getter.gnocontract/r/gnoswap/pool/types.gnocontract/r/gnoswap/pool/upgrade_test.gnocontract/r/gnoswap/pool/v1/getter.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/gnoswap/pool/v1/oracle_test.gnocontract/r/scenario/upgrade/implements/mock/pool/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/pool/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/pool/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/pool/getter.gnocontract/r/gnoswap/pool/v1/getter.gnocontract/r/gnoswap/pool/types.gnocontract/r/scenario/upgrade/implements/v2_invalid/pool/test_impl.gnocontract/r/gnoswap/pool/v1/oracle_test.gnocontract/r/gnoswap/pool/upgrade_test.gnocontract/r/gnoswap/pool/_mock_test.gnocontract/r/scenario/upgrade/implements/v3_valid/pool/test_impl.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/scenario/upgrade/implements/mock/pool/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/pool/getter.gnocontract/r/gnoswap/pool/v1/getter.gnocontract/r/gnoswap/pool/types.gnocontract/r/scenario/upgrade/implements/v2_invalid/pool/test_impl.gnocontract/r/gnoswap/pool/v1/oracle_test.gnocontract/r/gnoswap/pool/upgrade_test.gnocontract/r/gnoswap/pool/_mock_test.gnocontract/r/scenario/upgrade/implements/v3_valid/pool/test_impl.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/scenario/upgrade/implements/mock/pool/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). (10)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- 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/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/emission, contract/r/gnoswap/emission)
- GitHub Check: run-integration-test
🔇 Additional comments (7)
contract/r/gnoswap/pool/types.gno (1)
213-213: [LOW] Rule: API surface consistencyLine 213:
IPoolGetter.GetTWAPnow returns(int32, *u256.Uint, error).Problem: None in this file; aligns interface with the new TWAP API.
Fix: None.
Rationale: Propagates the harmonic-mean liquidity return through the public interface, enabling callers/tests to use it when needed.
contract/r/scenario/upgrade/implements/v2_invalid/pool/test_impl.gno (1)
415-420: [LOW] Rule: Upgrade test harness correctnessLine 415-420:
GetTWAPsignature updated to(int32, *u256.Uint, error)while preserving the activation gate and delegation.Problem: None.
Fix: None.
Rationale: Keeps upgrade-scenario behavior the same while matching the new interface/return contract.
contract/r/scenario/upgrade/implements/v3_valid/pool/test_impl.gno (1)
243-245: [LOW] Rule: Upgrade test harness correctnessLine 243-245:
GetTWAPsignature updated to(int32, *u256.Uint, error)with unchanged delegation.Problem: None.
Fix: None.
Rationale: Ensures the v3-valid upgrade scenario compiles and exercises the same underlying behavior with the new API shape.
contract/r/gnoswap/pool/v1/oracle_test.gno (1)
340-345: LGTM: UpdatedgetTWAPcall site to the new 3-return signatureLine 342:
twapTick, _, err := getTWAP(...)is the right adaptation.contract/r/gnoswap/pool/v1/oracle.gno (3)
11-69: LGTM:getTWAPnow returns arithmetic mean tick + harmonic mean liquidity with sane guardsLine 19-29: new validations for
secondsAgo == 0, nil observation state, and zero cardinality are good.
Line 59-61: returningu256.Zero()whensecondsPerLiquidityDeltais zero is a reasonable “no-liquidity” fallback.
109-156: LGTM:transformrefactor + zero-liquidity handling looks consistentLine 131-141: using
max(1, liquidity)avoids division-by-zero in seconds-per-liquidity accumulation.
171-224: LGTM: observation growth/write path + cardinality guardLine 171-177: growing from
currentCardinalityis a good micro-optimization.
Line 433-435:observenow rejectscardinality == 0, which matches the earlier invariants.Also applies to: 226-244, 264-325, 425-460
… comment for grow invariants
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contract/r/gnoswap/pool/v1/oracle.gno (2)
171-177: LGTM: Efficient optimization avoiding redundant checks.The optimization correctly skips already-initialized slots by starting the loop at
currentCardinalityrather than 0, reducing unnecessary iterations.
303-305: Integer division truncation in tick interpolation.Lines 304-305: The interpolation uses integer division which truncates:
((afterOrAt.TickCumulative()-beforeOrAt.TickCumulative())/observationTimeDelta)*targetDeltaConsideration: This truncates the quotient before multiplying by
targetDelta, potentially losing precision. The mathematically equivalent but more precise approach would be:(afterOrAt.TickCumulative()-beforeOrAt.TickCumulative())*targetDelta/observationTimeDeltaHowever, the current order may be intentional to prevent intermediate overflow. If
(after - before) * targetDeltacould exceed int64 max, the current approach is safer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contract/r/gnoswap/pool/v1/oracle.gnocontract/r/gnoswap/pool/v1/oracle_integration_test.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/pool/v1/oracle_integration_test.gnocontract/r/gnoswap/pool/v1/oracle.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/pool/v1/oracle_integration_test.gnocontract/r/gnoswap/pool/v1/oracle.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/scenario/router, contract/r/scenario/router)
- 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/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 (11)
contract/r/gnoswap/pool/v1/oracle.gno (5)
11-12: LGTM: Correct calculation of 160-bit maximum.The calculation correctly computes 2^160 - 1 for use in harmonic mean liquidity calculations.
109-156: LGTM: Safe arithmetic with proper overflow handling.The transform function correctly:
- Uses
MulOverflowfor liquidity delta calculation (line 123) with panic on overflow- Employs
max(1, liquidity)to avoid division by zero (lines 131-134)- Uses
MulDivfor safe fixed-point arithmetic (lines 137-141)- Clones values before mutation (line 143)
215-223: LGTM: Consistent with updated transform signature.The call site correctly passes
lastObservationdirectly to the refactoredtransformfunction.
433-435: LGTM: Improved error message.The cardinality check is now explicit with a clearer error message.
226-244: Behavioral change: Functions now return uninitialized observations without error.Lines 228 and 239-240: The checks (
!ok || observation == nil) meanlastObservation()andobservationAt()return observations even whenInitialized() == false, whereas they previously required initialized observations.Impact: Callers must verify
Initialized()before using returned observations. The integration test confirms this is intentional (TestObservationState_ObservationAt, line 435-440). All callers in the file already handle this correctly (lines 361, 393, 402 checkInitialized()before proceeding).contract/r/gnoswap/pool/v1/oracle_integration_test.gno (6)
103-111: LGTM: Test correctly models dynamic cardinality growth.The test properly calculates needed cardinality, grows the observation state, and sets the cardinality limit to match production usage patterns.
180-192: LGTM: Test correctly expects error for target before oldest observation.The test verifies that querying a target timestamp before the oldest observation returns the appropriate error, matching the implementation at lines 369-371 in oracle.gno.
207-209: LGTM: Test confirms nil afterOrAt behavior.The test correctly verifies that when the target equals the newest observation timestamp,
afterOrAtshould be nil, confirming the behavioral change ingetSurroundingObservations.
262-301: LGTM: Test validates grow function creates uninitialized slots.The test correctly verifies that
growcreates observation slots withinitialized=falsefor new slots, as documented in the comment at line 262.
430-440: LGTM: Test documents behavioral change in observationAt.The test correctly validates that
observationAtnow returns uninitialized observations without error, requiring callers to check theInitialized()status. The comment at line 435 clearly documents this behavior.
447-469: LGTM: Test validates transform edge cases.The test correctly:
- Uses the new
transformsignature withlastObservationparameter- Validates zero liquidity handling with the expected formula:
timeDelta * 2^128 / max(1, liquidity)- Covers negative time delta error case
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contract/r/gnoswap/pool/upgrade_test.gno (1)
1214-1214: Update the test comment to match the renamed function.The comment still references the old function name "increase observation cardinality limit" but the function has been renamed to
IncreaseObservationCardinalityNext.📝 Proposed fix
- name: "increase observation cardinality limit is success", + name: "increase observation cardinality next is success",contract/r/gnoswap/pool/v1/oracle_spec_test.gno (1)
811-826: Setup helper ignores error returns fromgrowandwriteObservation.The
setupFiveObservationshelper function callsgrowandwriteObservationwithout checking their error returns. While this may be acceptable for test setup where failures would cause subsequent assertions to fail anyway, explicit error checking would make test failures more diagnosable.♻️ Optional: Add error checking for better diagnostics
func setupFiveObservations(startTime int64) *pl.ObservationState { os := pl.NewObservationState(startTime) - grow(os, 1, 5) + if _, err := grow(os, 1, 5); err != nil { + panic("setup failed: " + err.Error()) + } os.SetCardinality(1) os.SetCardinalityNext(5) - writeObservation(os, startTime+3, -5, u256.NewUint(5)) - writeObservation(os, startTime+5, 1, u256.NewUint(2)) - writeObservation(os, startTime+9, -6, u256.NewUint(4)) - writeObservation(os, startTime+10, -2, u256.NewUint(4)) - writeObservation(os, startTime+13, -2, u256.NewUint(9)) - writeObservation(os, startTime+19, 4, u256.NewUint(2)) + // Error handling omitted for brevity - failures will cause test assertions to fail + _ = writeObservation(os, startTime+3, -5, u256.NewUint(5)) + _ = writeObservation(os, startTime+5, 1, u256.NewUint(2)) + _ = writeObservation(os, startTime+9, -6, u256.NewUint(4)) + _ = writeObservation(os, startTime+10, -2, u256.NewUint(4)) + _ = writeObservation(os, startTime+13, -2, u256.NewUint(9)) + _ = writeObservation(os, startTime+19, 4, u256.NewUint(2)) return os }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
contract/r/scenario/pool/oracle_twap_increased_cardinality_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/pool/oracle_twap_multiple_swaps_filetest.gnois excluded by!**/*filetest.gnocontract/r/scenario/pool/oracle_twap_single_swap_filetest.gnois excluded by!**/*filetest.gno
📒 Files selected for processing (15)
contract/r/gnoswap/pool/_mock_test.gnocontract/r/gnoswap/pool/oracle.gnocontract/r/gnoswap/pool/proxy.gnocontract/r/gnoswap/pool/types.gnocontract/r/gnoswap/pool/upgrade_test.gnocontract/r/gnoswap/pool/v1/_helper_test.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/gnoswap/pool/v1/oracle_integration_test.gnocontract/r/gnoswap/pool/v1/oracle_spec_test.gnocontract/r/gnoswap/pool/v1/oracle_test.gnocontract/r/gnoswap/pool/v1/pool.gnocontract/r/gnoswap/pool/v1/pool_test.gnocontract/r/scenario/upgrade/implements/mock/pool/test_impl.gnocontract/r/scenario/upgrade/implements/v2_invalid/pool/test_impl.gnocontract/r/scenario/upgrade/implements/v3_valid/pool/test_impl.gno
🚧 Files skipped from review as they are similar to previous changes (2)
- contract/r/gnoswap/pool/_mock_test.gno
- contract/r/scenario/upgrade/implements/mock/pool/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/pool/proxy.gnocontract/r/gnoswap/pool/v1/_helper_test.gnocontract/r/gnoswap/pool/v1/oracle_integration_test.gnocontract/r/gnoswap/pool/oracle.gnocontract/r/gnoswap/pool/v1/pool_test.gnocontract/r/gnoswap/pool/v1/oracle_spec_test.gnocontract/r/gnoswap/pool/v1/pool.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/scenario/upgrade/implements/v3_valid/pool/test_impl.gnocontract/r/gnoswap/pool/v1/oracle_test.gnocontract/r/gnoswap/pool/types.gnocontract/r/gnoswap/pool/upgrade_test.gnocontract/r/scenario/upgrade/implements/v2_invalid/pool/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/pool/proxy.gnocontract/r/gnoswap/pool/v1/_helper_test.gnocontract/r/gnoswap/pool/v1/oracle_integration_test.gnocontract/r/gnoswap/pool/oracle.gnocontract/r/gnoswap/pool/v1/pool_test.gnocontract/r/gnoswap/pool/v1/oracle_spec_test.gnocontract/r/gnoswap/pool/v1/pool.gnocontract/r/gnoswap/pool/v1/oracle.gnocontract/r/scenario/upgrade/implements/v3_valid/pool/test_impl.gnocontract/r/gnoswap/pool/v1/oracle_test.gnocontract/r/gnoswap/pool/types.gnocontract/r/gnoswap/pool/upgrade_test.gnocontract/r/scenario/upgrade/implements/v2_invalid/pool/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). (9)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- 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/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (41)
contract/r/gnoswap/pool/oracle.gno (3)
38-45: Clone creates an empty observations map rather than copying existing observations.The
Clone()method creates a new emptymap[uint16]*Observationinstead of copying the actual observations from the source. This may be intentional for creating a fresh state, but it differs from theObservation.Clone()which performs a deep copy.If this is intentional (e.g., for creating a template state), consider adding a comment to clarify the design choice. If observations should be preserved, the map entries need to be cloned:
♻️ Optional: Deep copy observations if needed
func (os *ObservationState) Clone() *ObservationState { + clonedObs := make(map[uint16]*Observation, len(os.observations)) + for k, v := range os.observations { + clonedObs[k] = v.Clone() + } return &ObservationState{ - observations: make(map[uint16]*Observation), + observations: clonedObs, index: os.index, cardinality: os.cardinality, cardinalityNext: os.cardinalityNext, } }
131-139: Default observation now starts uninitialized.The change from
initialized: truetoinitialized: falseis semantically correct for a "default" observation that hasn't been written yet. This aligns with the oracle pattern where uninitialized slots should be distinguishable from actual observations.
8-13: LGTM! Proper encapsulation and initialization.The struct uses unexported fields with controlled getter/setter access, following Gno security best practices. The
NewObservationStateconstructor properly initializes the first observation slot with the correct timestamp.Also applies to: 95-113
contract/r/gnoswap/pool/proxy.gno (1)
30-50: LGTM! API rename is consistent.The function rename from
IncreaseObservationCardinalityLimittoIncreaseObservationCardinalityNextis properly applied to both the proxy function and its delegation call. The documentation comment is updated accordingly.contract/r/gnoswap/pool/types.gno (2)
36-41: LGTM! Interface method rename is consistent.The
IncreaseObservationCardinalityNextmethod signature inIPoolManageris consistent with the proxy and implementation layers.
213-213: Interface signature change verified across all implementations.All implementations of
GetTWAPhave been updated to match the new return signature(int32, *u256.Uint, error). Found 6 implementations across the codebase (mock tests, v1 getter, test implementations, and main getter function), all with consistent signatures.contract/r/gnoswap/pool/v1/_helper_test.gno (1)
740-746: LGTM! Test helper rename is consistent with production API.The test helper function and parameter names are properly updated to match the renamed production API (
IncreaseObservationCardinalityNext).contract/r/gnoswap/pool/v1/pool.gno (3)
387-408: LGTM! Implementation rename with proper event emission.The function is correctly renamed to
IncreaseObservationCardinalityNext, the internal call is updated toincreaseObservationCardinalityNextByPool, and the event name matches the function name. The event usesprevAddrwhich follows the codebase convention per learnings.
258-260: LGTM! Error handling is appropriate.The inline error check with panic on failure is correct for Gno realm code where errors should abort the transaction.
157-157: The review comment is based on a misunderstanding of thegetPositionKeyfunction. The function signature isfunc getPositionKey(tickLower int32, tickUpper int32) string—it returns only astring, never an error. The function performs simple string concatenation and base64 encoding, neither of which can fail. Direct assignment at lines 157 and 211 is correct and the only way to use this function. No error handling was "removed"—it never existed.Likely an incorrect or invalid review comment.
contract/r/gnoswap/pool/v1/oracle_spec_test.gno (3)
12-57: LGTM! Comprehensive initialization tests.The initialization tests properly verify the initial state of
ObservationState: index=0, cardinality=1, cardinalityNext=1, and the first observation slot is correctly initialized.
322-326: Edge case handled: uninitialized observation slot check.Good defensive check for accessing an observation slot that may not exist, using the map lookup idiom to verify the slot is uninitialized.
328-787: Comprehensive observation interpolation test coverage.The
TestOracleObservesuite provides thorough coverage of:
- Error cases (older observation doesn't exist)
- Boundary conditions (max/min liquidity)
- Chronological and reverse-order observations
- Counterfactual calculations
- Multiple observation fetching
This aligns well with the TWAP functionality requirements.
contract/r/scenario/upgrade/implements/v3_valid/pool/test_impl.gno (2)
36-38: LGTM! Delegation updated for renamed method.The test implementation correctly delegates
IncreaseObservationCardinalityNextto the underlying v1 instance.
243-245: LGTM! GetTWAP signature updated to match interface.The delegation correctly returns all three values
(int32, *u256.Uint, error)from the underlying implementation.contract/r/gnoswap/pool/v1/pool_test.gno (4)
489-489: LGTM!The rename from
SetCardinalityLimittoSetCardinalityNextaligns with the broader API refactoring in this PR and maintains semantic clarity.
638-641: LGTM!The boundary test correctly validates that
MAX_SQRT_RATIO(1461446703485210103287273052203988822378723970342) is excluded from valid prices, matching the[minSqrtRatio, maxSqrtRatio)semantics. The error message and business context accurately reflect this boundary condition.
750-750: LGTM!The simplification from a multi-value return to a single
positionKey := getPositionKey(...)is cleaner and aligns with the position.gno changes mentioned in the AI summary.
1023-1137: Excellent test coverage forcollectTokenedge cases.The test comprehensively covers:
- Equal amounts across all inputs
- Each input being the minimum
- Zero value handling for each parameter
- MAX_UINT128 boundary conditions
- Small differences to verify correct minimum detection
This provides strong confidence in the
collectTokenfunction's correctness.contract/r/gnoswap/pool/v1/oracle_integration_test.gno (5)
103-111: LGTM!The dynamic cardinality growth pattern is cleaner than the previous static approach. Growing only when needed (
neededCard > currentCard) and then settingCardinalityNextaligns with the new observation state semantics.
180-192: LGTM!The updated test correctly expects an error when the target is before the oldest observation, with proper error code verification (
[GNOSWAP-POOL-028]). This aligns with the refined boundary condition handling.
207-209: LGTM!Good boundary assertion - when target equals the newest observation,
afterOrAtshould be nil since we're in the same block and don't need a virtual observation.
262-301: LGTM!The test correctly verifies that
growcreates observation slots but does NOT initialize them. This is the expected behavior - slots are pre-allocated but only initialized when actual observations are written.
461-468: LGTM!The zero liquidity test correctly validates the
max(1, liquidity)semantics. The expected calculation100 * 2^128 / 1usingMulDivwithq128FromDecimalproperly verifies the seconds-per-liquidity computation when liquidity is zero.contract/r/scenario/upgrade/implements/v2_invalid/pool/test_impl.gno (2)
55-60: LGTM!The rename from
IncreaseObservationCardinalityLimittoIncreaseObservationCardinalityNextis consistently applied - method name, activation key, panic message, and delegation target are all updated.
415-419: LGTM!The
GetTWAPsignature correctly updated to return(int32, *u256.Uint, error)matching the interface changes. The delegation tot.instance.GetTWAPproperly passes through all three return values.contract/r/gnoswap/pool/v1/oracle_test.gno (5)
117-152: LGTM!Test cases properly updated to use
cardinalityNextfield naming consistently. The circular buffer tests correctly verify index wrapping and cardinality behavior.
167-243: LGTM!Comprehensive test coverage for cardinality management including:
- Valid increase scenarios
- Decrease/same value rejection
- Maximum cardinality acceptance
- Uninitialized state handling
The error messages are descriptive and align with the implementation.
349-423: Excellent harmonic mean liquidity test coverage.The test cases correctly replicate Uniswap V3's
OracleLibrary.spec.tsscenarios:
- Tick = 0 verification
- Negative tick rounding (always toward negative infinity)
- Liquidity overflow capping to MAX_UINT128
The
calculateHarmonicAvgLiqhelper accurately implementsmaxUint160 * period / (delta << 32).
425-474: LGTM!Good error case coverage for
getTWAP:
secondsAgo == 0rejection- Uninitialized observation state
- Zero cardinality (no observations)
These tests ensure the function fails gracefully with descriptive errors.
496-514: LGTM!The new
newObservationStateWithLiquidityhelper properly seeds a 2-slot observation state with both tick cumulative delta and seconds-per-liquidity delta, enabling precise harmonic mean liquidity testing.contract/r/gnoswap/pool/v1/oracle.gno (10)
11-11: LGTM!The
max160constant (2^160 - 1) is correctly defined for harmonic mean liquidity calculation, matching Uniswap V3's approach.
16-69: LGTM - Solid TWAP implementation with harmonic mean liquidity.The implementation correctly:
- Validates inputs (secondsAgo > 0, observation state initialized, cardinality > 0)
- Computes arithmetic mean tick with proper negative rounding (floor toward negative infinity)
- Calculates harmonic mean liquidity using
secondsAgo * max160 / (delta << 32)- Returns zero liquidity when
secondsPerLiquidityDeltais zeroThis aligns with Uniswap V3's oracle library pattern.
89-112: LGTM!The renamed function properly validates:
- Observation state initialization
- Maximum cardinality bounds
- Strictly greater than current requirement
Error handling is clear and follows the established pattern.
114-157: LGTM!The
transformfunction correctly:
- Validates non-negative time delta
- Computes tick cumulative as
previous + tick * timeDelta- Handles liquidity overflow check
- Uses
max(1, liquidity)for seconds-per-liquidity calculation to avoid division by zero
218-220: LGTM!The cardinality growth check correctly triggers when
CardinalityNext > Cardinalityand the current index is at the buffer boundary. This ensures seamless expansion of the circular buffer.
340-389: LGTM - Well-structured surrounding observations lookup.The function correctly:
- Optimistically checks if target is at/after newest observation
- Returns
nilforatOrAfterwhen target equals newest (same block optimization)- Falls back to oldest observation and validates target is not too old
- Delegates to binary search for intermediate targets
The edge case handling for uninitialized slots at
(index+1) % cardinalityfalling back to slot 0 is appropriate.
391-436: LGTM - Correct binary search over circular buffer.The binary search correctly:
- Operates over the logical range
[oldest, newest]mapped to[l, r]- Handles uninitialized slots by searching higher (more recent)
- Uses
ltefor proper timestamp comparison in circular context- Terminates when target is bracketed by
beforeOrAtandatOrAfter
438-475: LGTM!The
observefunction correctly iterates through thesecondsAgosslice and collects cumulative values usingobserveSingle. The cardinality validation at the start prevents invalid state errors.
182-200: No issue:ltefunction safely handles positive int64 timestamps.The
ltefunction correctly handles int64 timestamps. All timestamps in this codebase come fromtime.Now().Unix(), which returns positive Unix time values. The function's wraparound logic (adding1<<32) is intentional for handling timestamp comparisons across observation array boundaries during binary search in the circular buffer. An explicit check at line 116 (if timeDelta < 0) ensures timestamps are monotonic, confirming this function receives only valid positive values.
159-180: No action required — Thegrow()function correctly handles slot initialization. It allocates storage slots and marks them as uninitialized using the booleaninitializedfield (set tofalsebyNewDefaultObservation()). TheBlockTimestamp(1)is a harmless placeholder value. Code throughout the module uses the.Initialized()method to check slot validity before reading timestamps, so the pattern is sound and poses no logic issues.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contract/r/gnoswap/pool/v1/oracle.gno (1)
155-176: Consider simplifying the zero check for uint16.The condition
currentCardinality <= 0is effectivelycurrentCardinality == 0sinceuint16cannot be negative. Consider using== 0for clarity.♻️ Suggested simplification
- if currentCardinality <= 0 { + if currentCardinality == 0 { return currentCardinality, errors.New("currentCardinality must be greater than 0") }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contract/r/gnoswap/pool/v1/oracle.gnocontract/r/gnoswap/pool/v1/oracle_test.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/pool/v1/oracle_test.gnocontract/r/gnoswap/pool/v1/oracle.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/pool/v1/oracle_test.gnocontract/r/gnoswap/pool/v1/oracle.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). (12)
- 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/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- 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/governance, contract/r/scenario/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/gnft, contract/r/gnoswap/gnft)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: run-integration-test
🔇 Additional comments (18)
contract/r/gnoswap/pool/v1/oracle.gno (12)
11-11: LGTM: max160 constant initialization.The
max160constant correctly computes2^160 - 1using uint256 operations. This is used for harmonic mean liquidity calculation.
16-65: LGTM: getTWAP implementation with harmonic mean liquidity.The TWAP calculation correctly:
- Validates inputs before processing
- Computes arithmetic mean tick with proper floor rounding for negative values
- Calculates harmonic mean liquidity using the standard formula
(secondsAgo * 2^160) / (delta << 32)
67-83: LGTM: Simple wrapper for observation writes.The function properly initializes the observation state if needed and delegates to
writeObservation.
85-108: LGTM: Cardinality management with proper validation.All edge cases are validated: uninitialized state, maximum cardinality bounds, and monotonic increase requirement.
110-153: LGTM: Transform function with proper zero-liquidity handling.The function correctly clamps liquidity to 1 when zero to prevent division by zero while still accumulating
secondsPerLiquidity. This matches the expected oracle behavior.
198-234: LGTM: Observation write with cardinality expansion.The cardinality expansion logic correctly triggers when the buffer is about to wrap and
CardinalityNext > Cardinality.
236-243: LGTM: Simple observation getter.
245-254: LGTM: Index-based observation getter.
256-334: LGTM: Single observation query with interpolation.The function correctly handles:
- Current time queries (secondsAgo == 0)
- Exact timestamp matches
- Linear interpolation between observations using safe math operations
336-385: LGTM: Surrounding observations search with early exits.The function correctly handles:
- Optimistic newest-first lookup
- Exact timestamp matches (returns nil for atOrAfter)
- Validation that target is within observable window
- Fallback to binary search for circular buffer navigation
434-471: LGTM: Batch observation query.The function correctly validates cardinality and aggregates results from
observeSinglefor multiple time points.
178-196: The1<<32wraparound constant in theltefunction handles circular buffer semantics for timestamp comparisons, not true int64 overflow. This pattern is from Uniswap V3's Oracle implementation and works correctly for the observation window's expected lifetime. No actual issue found.contract/r/gnoswap/pool/v1/oracle_test.gno (6)
12-108: LGTM: Comprehensive write operation tests.Good coverage of edge cases including same-timestamp no-op and past-timestamp error handling.
110-165: LGTM: Circular buffer behavior tests with updated API.Tests correctly use the renamed
cardinalityNextfield and verify wrap-around behavior.
245-294: LGTM: Liquidity accumulation tests with various scenarios.Test cases cover constant, changing, and zero liquidity scenarios with correct expected values.
296-347: LGTM: TWAP rounding verification.Correctly tests floor rounding behavior for both positive and negative tick deltas.
425-462: LGTM: Error case coverage for getTWAP.Tests correctly verify error conditions for zero
secondsAgoand uninitialized observation state.
464-523: LGTM: Test helper functions.Helper functions correctly seed observation states and create test pools for various test scenarios.
|
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.