ci: parallelize integration and fuzz tests for faster CI execution #1094
Conversation
WalkthroughReorganizes fuzz tests from a single "fuzz" package into category-specific packages (math, pool, position, router, staker); adds many mock stores and helper utilities per category; updates CI workflows to build and pass a compiled Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
contract/r/gnoswap/test/fuzz/staker/unstake_token_params_test.gno (1)
8-14: Fix misleading comment.The comment references "ExactOutSwapRoute fuzz test" but this struct is for unstaking token parameters, not swap routing.
🔎 Suggested fix
-// unStakeTokenParams is the parameters for the ExactOutSwapRoute fuzz test. +// unStakeTokenParams is the parameters for the UnstakeToken fuzz test. type unStakeTokenParams struct { positionID uint64 unwrapResult boolcontract/r/gnoswap/test/fuzz/math/int256_params_fuzz_test.gno (1)
17-20: Unexport global variables to follow Gno best practices.The variables
maxInt256AbsandminInt256Absare exported, which allows any importing package to reassign them. While the impact is limited to test code, this violates Gno guidelines that discourage exported globals to prevent unintended state mutations.🔎 Proposed fix
var ( - maxInt256Abs = intAbs(i256.MaxInt256()) - minInt256Abs = intAbs(i256.MinInt256()) + maxInt256Abs = intAbs(i256.MaxInt256()) + minInt256Abs = intAbs(i256.MinInt256()) )contract/r/gnoswap/test/fuzz/pool/pool_utils_test.gno (1)
131-133: Typo in fuzz Draw label: both tick diffs use "tickLowerDiff".Same issue as in
staker/_helper_test.gno: Line 132 uses"tickLowerDiff"as the label fortickUpperDiff.Suggested fix
tickLowerDiff := fuzz.Int32Range(1, 1000).Draw(f, "tickLowerDiff").(int32) * tickSpacing - tickUpperDiff := fuzz.Int32Range(1, 1000).Draw(f, "tickLowerDiff").(int32) * tickSpacing + tickUpperDiff := fuzz.Int32Range(1, 1000).Draw(f, "tickUpperDiff").(int32) * tickSpacingcontract/r/gnoswap/test/fuzz/router/exact_out_swap_route_params_test.gno (1)
120-137: Logic error in route validation.The conditions on lines 126 and 130 are confusing and likely incorrect. The intent seems to be:
- On the first pool (
i == 0), capture the first input token- On the last pool, capture the last output token
But the current conditions (
firstToken != p.inputTokenandlastToken != p.outputToken) don't serve that purpose sincefirstTokenstarts as"".🔎 Proposed fix
for i, routePool := range routePools { inputToken, outputToken, feeInt64 := parsePoolPath(routePool) - if i == 0 && firstToken != p.inputToken { + if i == 0 { firstToken = inputToken } - if lastToken != p.outputToken { - lastToken = outputToken - } + lastToken = outputToken if feeInt64 != 100 && feeInt64 != 500 && feeInt64 != 3000 && feeInt64 != 10000 { return false } }
♻️ Duplicate comments (4)
contract/r/gnoswap/test/fuzz/position/_mock_test.gno (1)
1-765: Duplicate mock implementation - same as pool package.This entire file is nearly identical to
contract/r/gnoswap/test/fuzz/pool/_mock_test.gno. The same issues (nil map access inApprove, incorrect error inOwnerOf,oblPathreference) apply here.Consider extracting shared mock implementations to a common test utilities location.
contract/r/gnoswap/test/fuzz/staker/_mock_test.gno (2)
1-765: Duplicate mock implementation - same pattern across all fuzz packages.This file follows the same pattern as pool, position, and router mocks. Same issues and refactoring opportunity apply.
256-259: Same nil map access issue inApprove.Consistent with the other mock files, this needs the same fix.
contract/r/gnoswap/test/fuzz/router/_mock_test.gno (1)
256-259: Same nil map access issue inApprove.Consistent with the other mock files.
🧹 Nitpick comments (22)
contract/r/gnoswap/test/fuzz/position/position_reposition_test.gno (1)
56-57: Inconsistent error handling forstrconv.ParseInt—adopt the safer pattern used in lines 300–305.Lines 56–57 and 137–138 ignore parse errors with
_, while the later implementation (lines 300–305) properly validates the error and amount. The second pattern is more defensive and prevents issues ifrepositionParamscontains invalid numeric strings.Adopt the check-and-validate approach consistently across all test functions.
🔎 Proposed fix for consistent error handling
- amount0Int64, _ := strconv.ParseInt(repositionParams.amount0Desired, 10, 64) - amount1Int64, _ := strconv.ParseInt(repositionParams.amount1Desired, 10, 64) - mintTestToken(token0Path, amount0Int64) - mintTestToken(token1Path, amount1Int64) + if amount0Int64, err := strconv.ParseInt(repositionParams.amount0Desired, 10, 64); err == nil && amount0Int64 > 0 { + mintTestToken(token0Path, amount0Int64) + } + if amount1Int64, err := strconv.ParseInt(repositionParams.amount1Desired, 10, 64); err == nil && amount1Int64 > 0 { + mintTestToken(token1Path, amount1Int64) + }Also applies to: 137-138
contract/r/gnoswap/test/fuzz/math/int256_bitwise_fuzz_params_test.gno (1)
10-340: Optional: Consider adding doc comments for exported types and functions.While the code is well-structured and correct, the exported types (e.g.,
int256LshParams,int256RshParams) and factory functions (e.g.,NewValidInt256LshParams) lack documentation comments. For test infrastructure that may be used across the codebase, brief doc comments would improve maintainability.Example:
// int256LshParams holds parameters for testing Int256 left shift operations. type int256LshParams struct { ... } // NewValidInt256LshParams generates valid parameters for Int256 left shift fuzz tests. func NewValidInt256LshParams(t *fuzz.T) *int256LshParams { ... }This is a low-priority suggestion given that this is test infrastructure code.
contract/r/gnoswap/test/fuzz/position/position_increase_liquidity_params_test.gno (2)
52-54: Unnecessary comparison: unsigned integers cannot be negative.
u256.Uintis an unsigned 256-bit integer and can never be less than zero. TheLt(u256.Zero())check is always false and serves no purpose.Proposed fix
- if amount0.Lt(u256.Zero()) || amount1.Lt(u256.Zero()) { - return false - }
74-76: Same issue: redundant negative check on unsigned type.This check can also be removed for the same reason as above.
Proposed fix
- if amount0Min.Lt(u256.Zero()) || amount1Min.Lt(u256.Zero()) { - return false - }contract/r/gnoswap/test/fuzz/position/position_collect_fee_params_test.gno (1)
61-64: Redundant negative check:Int64Range(1, 1024)always returns positive values.The range is explicitly
[1, 1024], sooffsetcan never be negative.Proposed fix
offset := fuzz.Int64Range(1, 1024).Draw(t, "positionOffset").(int64) - if offset < 0 { - offset = -offset - } randomPositionId := existingPositionId + uint64(offset) - if randomPositionId < existingPositionId { - randomPositionId = existingPositionId - }Note: The overflow check on lines 66-68 is also redundant since
offsetis at most 1024 andexistingPositionId + 1024won't wrap around auint64.contract/r/gnoswap/test/fuzz/position/position_collect_fee_test.gno (1)
179-179: Hardcoded iteration count should use the constant.The log message uses
100but the actual iteration count isFUZZ_ITERATIONS.Proposed fix
- t.Logf("Total successful fee collections: %d out of 100", successfulCollections) + t.Logf("Total successful fee collections: %d out of %d", successfulCollections, FUZZ_ITERATIONS)contract/r/gnoswap/test/fuzz/staker/stake_token_test.gno (1)
47-55: Misleading comment: function is for staking, not swapping.Line 47 comment says "runs the test swap route" but the function
runTestStakeTokenperforms staking viastaker.StakeToken. Update the comment to accurately describe the function's purpose.Suggested fix
-// runTestStakeToken runs the test swap route. +// runTestStakeToken runs the stake token test. func runTestStakeToken(ft *fuzz.T, params *stakeTokenParams) {contract/r/gnoswap/test/fuzz/math/_helper_test.gno (1)
52-55: Shared test constants duplicated across multiple helper files.The constants
VALID_FEE_TIERSandMAX_UINT128(and many others in this file) are duplicated instaker/_helper_test.gnoandposition/_helper_test.gno. Consider extracting these into a shared test utilities package to reduce maintenance burden and ensure consistency.contract/r/gnoswap/test/fuzz/math/uint256_arithmetic_fuzz_test.gno (1)
260-304: Local umul helper functions duplicate library internals.
umulBaseline,umulStepLocal, andumulHopLocalappear to duplicate internal implementations from the uint256 package. If these are intended as reference implementations for verification, consider adding a comment explaining this purpose. If the original functions could be exported for testing, that would reduce duplication.contract/r/gnoswap/test/fuzz/staker/_helper_test.gno (1)
314-326: Non-deterministic map iteration affects fuzz test reproducibility.Iterating over
map[string]boolwithfor tokenPath := range tokenPathsproduces non-deterministic ordering. This means the sametokenPathIndexmay select different tokens across runs, making fuzz test failures harder to reproduce.Consider converting to a sorted slice for deterministic selection:
Suggested fix
func generateTokenPathByList(f *fuzz.T, tokenPaths map[string]bool) string { - tokenPathIndex := fuzz.IntRange(0, len(tokenPaths)-1).Draw(f, "tokenPath").(int) - - currentIndex := 0 - for tokenPath := range tokenPaths { - if currentIndex == tokenPathIndex { - return tokenPath - } - currentIndex++ + // Convert to sorted slice for deterministic selection + paths := make([]string, 0, len(tokenPaths)) + for path := range tokenPaths { + paths = append(paths, path) } - - return "" + sort.Strings(paths) + tokenPathIndex := fuzz.IntRange(0, len(paths)-1).Draw(f, "tokenPath").(int) + return paths[tokenPathIndex] }contract/r/gnoswap/test/fuzz/position/_helper_test.gno (2)
315-328: Non-deterministic map iteration affects fuzz test reproducibility.Same issue as in
staker/_helper_test.gno: iterating over maps produces non-deterministic ordering, affecting reproducibility of fuzz tests. Apply the same fix to use a sorted slice.
340-341: Unused initial tick values are immediately overwritten.
tickLowerandtickUpperare assigned values on lines 340-341, but all branches of the switch statement (lines 357-367) overwrite both values. These initial assignments are dead code.Suggested fix
func generateRandomTicks(f *fuzz.T, currentTick int32, feeTier uint32) (int32, int32) { tickSpacing := defaultTickSpacings[feeTier] - - tickLower := currentTick - (currentTick % tickSpacing) - tickUpper := currentTick + (currentTick % tickSpacing) + var tickLower, tickUpper int32 inRangeType := fuzz.IntRange(0, 2).Draw(f, "inRangeType").(int)contract/r/gnoswap/test/fuzz/pool/pool_utils_test.gno (1)
26-31: Unnecessary blank identifier in map iteration.The
_infor tokenPath, _ := range defaultTokenPathsis unnecessary since you're only using the key. Same applies to line 43.Suggested fix
- for tokenPath, _ := range defaultTokenPaths { + for tokenPath := range defaultTokenPaths {contract/r/gnoswap/test/fuzz/router/exact_out_swap_route_params_test.gno (1)
81-109:MustFromDecimalwill panic on invalid input.If
p.amountOut,p.amountInMax, orp.amountIncontain invalid decimal strings,MustFromDecimalwill panic instead of returning false for invalid params. Consider usingFromDecimalwith error handling for robustness in fuzz testing.🔎 Example fix for amountOut validation
func (p *exactOutSwapRouteParams) isValidAmount() bool { - amountOut := u256.MustFromDecimal(p.amountOut) + amountOut, err := u256.FromDecimal(p.amountOut) + if err != nil { + return false + } if amountOut.Cmp(u256.Zero()) <= 0 { return false }contract/r/gnoswap/test/fuzz/pool/pool_swap_params_test.gno (1)
45-59: Silent error handling inSwapAmount().The
ParseInterror is discarded, which means invalidamountSpecifiedstrings will silently return 0. This could mask bugs during fuzz testing where the goal is to find edge cases.🔎 Consider logging or handling the error
func (p *poolSwapParams) SwapAmount() int64 { - amountInt, _ := strconv.ParseInt(p.amountSpecified, 10, 64) + amountInt, err := strconv.ParseInt(p.amountSpecified, 10, 64) + if err != nil { + return 0 // or handle differently + }scripts/all-integration-tests.sh (1)
62-62: Consider quoting variables to prevent word splitting.While the values are controlled, quoting
$SKIP_FLAGand$CATEGORY_FLAGwould be more robust against potential edge cases.🔎 Safer quoting
-done < <(cd "$CONTRACT_PATH" && python3 setup.py --list-tests $SKIP_FLAG $CATEGORY_FLAG) +done < <(cd "$CONTRACT_PATH" && python3 setup.py --list-tests ${SKIP_FLAG:+"$SKIP_FLAG"} ${CATEGORY_FLAG:+"$CATEGORY_FLAG"})Note: The current approach works fine since flags are controlled strings without spaces.
.github/workflows/run_test.yml (1)
50-55: Artifact upload includes entire gno directory.Uploading the entire
gno/directory (line 54) may include unnecessary files. Consider uploading only the built binary to reduce artifact size and upload/download time.🔎 Upload only the binary
- name: Upload gno artifacts uses: actions/upload-artifact@v4 with: name: gno-build - path: gno/ + path: gno/gno-binary retention-days: 1Then adjust the download and install steps accordingly.
contract/r/gnoswap/test/fuzz/router/_helper_test.gno (1)
59-66: Initialization flags may cause test pollution.The
initialized*flags are set once and never reset, even wheninitStates()is called. If tests share state across runs or if tests run in parallel, this could lead to unexpected behavior where later tests use stale mock registrations.Consider either:
- Resetting the flags in
initStates()to ensure fresh initialization per test suite- Documenting that these flags are intentionally persistent across tests
func initStates(t *testing.T) { burnTestTokens(t) + // Reset initialization flags for fresh state + initializedPool = false + initializedPosition = false + initializedRouter = false + initializedStaker = false + defaultNFTAccessor = newMockNFTAccessor() defaultPoolAccessor = newMockPoolAccessor() defaultEmissionAccessor = newMockEmissionAccessor()Also applies to: 107-119
contract/r/gnoswap/test/fuzz/pool/_helper_test.gno (1)
57-64: Same initialization flag concern as router helper.The
initialized*flags are never reset ininitStates(), which could cause test pollution. Consider resetting these flags as suggested for the router helper.Also applies to: 105-117
setup.py (1)
472-475: Consider filtering empty strings from category parsing.If the user provides
--category "base, ,gov"(with extra commas or spaces), this could result in empty strings in the categories list.🔎 Proposed fix
# Parse categories if provided categories = None if args.category: - categories = [c.strip() for c in args.category.split(",")] + categories = [c.strip() for c in args.category.split(",") if c.strip()]contract/r/gnoswap/test/fuzz/pool/_mock_test.gno (1)
1-766: Large amount of duplicated mock code across fuzz packages.This file is nearly identical to the mock files in
position,staker, androuterfuzz packages. Consider extracting common mock implementations into a shared test utilities package to reduce maintenance burden and ensure consistency.contract/r/gnoswap/test/fuzz/router/_mock_test.gno (1)
1-765: Duplicate mock implementation across fuzz packages.All four fuzz mock files (pool, position, staker, router) contain nearly identical code (~766 lines each). This significant duplication could be addressed by:
- Creating a shared
contract/r/gnoswap/test/fuzz/commonpackage with the mock implementations- Importing and potentially extending/configuring these mocks in each fuzz package
This would reduce ~3000 lines of duplicated code to a single source of truth, making future maintenance and bug fixes much simpler.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
.github/workflows/run_integration_test.yml.github/workflows/run_test.ymlcontract/r/gnoswap/test/fuzz/gnomod.tomlcontract/r/gnoswap/test/fuzz/math/_helper_test.gnocontract/r/gnoswap/test/fuzz/math/_mock_test.gnocontract/r/gnoswap/test/fuzz/math/gnomod.tomlcontract/r/gnoswap/test/fuzz/math/int256_arithmetic_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_arithmetic_fuzz_test.gnocontract/r/gnoswap/test/fuzz/math/int256_bitwise_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_bitwise_fuzz_test.gnocontract/r/gnoswap/test/fuzz/math/int256_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_fuzz_test.gnocontract/r/gnoswap/test/fuzz/math/int256_params_fuzz_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_arithmetic_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_arithmetic_fuzz_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_fuzz_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_math_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_math_fuzz_test.gnocontract/r/gnoswap/test/fuzz/pool/_helper_test.gnocontract/r/gnoswap/test/fuzz/pool/_mock_test.gnocontract/r/gnoswap/test/fuzz/pool/gnomod.tomlcontract/r/gnoswap/test/fuzz/pool/pool_create_params_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_create_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_swap_params_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_swap_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_utils_test.gnocontract/r/gnoswap/test/fuzz/pool/tick_math_params_test.gnocontract/r/gnoswap/test/fuzz/pool/tick_math_test.gnocontract/r/gnoswap/test/fuzz/position/_helper_test.gnocontract/r/gnoswap/test/fuzz/position/_mock_test.gnocontract/r/gnoswap/test/fuzz/position/gnomod.tomlcontract/r/gnoswap/test/fuzz/position/position_collect_fee_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_collect_fee_test.gnocontract/r/gnoswap/test/fuzz/position/position_decrease_liquidity_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_decrease_liquidity_test.gnocontract/r/gnoswap/test/fuzz/position/position_increase_liquidity_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_increase_liquidity_test.gnocontract/r/gnoswap/test/fuzz/position/position_mint_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_mint_test.gnocontract/r/gnoswap/test/fuzz/position/position_reposition_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_reposition_test.gnocontract/r/gnoswap/test/fuzz/router/_helper_test.gnocontract/r/gnoswap/test/fuzz/router/_mock_test.gnocontract/r/gnoswap/test/fuzz/router/exact_in_swap_route_params_test.gnocontract/r/gnoswap/test/fuzz/router/exact_in_swap_route_test.gnocontract/r/gnoswap/test/fuzz/router/exact_out_swap_route_params_test.gnocontract/r/gnoswap/test/fuzz/router/exact_out_swap_route_test.gnocontract/r/gnoswap/test/fuzz/router/gnomod.tomlcontract/r/gnoswap/test/fuzz/router/router_utils_test.gnocontract/r/gnoswap/test/fuzz/staker/_helper_test.gnocontract/r/gnoswap/test/fuzz/staker/_mock_test.gnocontract/r/gnoswap/test/fuzz/staker/collect_reward_params_test.gnocontract/r/gnoswap/test/fuzz/staker/collect_reward_test.gnocontract/r/gnoswap/test/fuzz/staker/gnomod.tomlcontract/r/gnoswap/test/fuzz/staker/stake_token_params_test.gnocontract/r/gnoswap/test/fuzz/staker/stake_token_test.gnocontract/r/gnoswap/test/fuzz/staker/staker_utils_test.gnocontract/r/gnoswap/test/fuzz/staker/unstake_token_params_test.gnocontract/r/gnoswap/test/fuzz/staker/unstake_token_test.gnoscripts/all-integration-tests.shsetup.py
💤 Files with no reviewable changes (1)
- contract/r/gnoswap/test/fuzz/gnomod.toml
🧰 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/test/fuzz/math/int256_arithmetic_fuzz_test.gnocontract/r/gnoswap/test/fuzz/math/int256_params_fuzz_test.gnocontract/r/gnoswap/test/fuzz/position/position_increase_liquidity_params_test.gnocontract/r/gnoswap/test/fuzz/staker/staker_utils_test.gnocontract/r/gnoswap/test/fuzz/position/position_mint_test.gnocontract/r/gnoswap/test/fuzz/position/position_decrease_liquidity_test.gnocontract/r/gnoswap/test/fuzz/position/position_reposition_test.gnocontract/r/gnoswap/test/fuzz/position/position_reposition_params_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_math_fuzz_test.gnocontract/r/gnoswap/test/fuzz/router/router_utils_test.gnocontract/r/gnoswap/test/fuzz/pool/tick_math_test.gnocontract/r/gnoswap/test/fuzz/router/exact_out_swap_route_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_fuzz_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_swap_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_bitwise_fuzz_test.gnocontract/r/gnoswap/test/fuzz/pool/tick_math_params_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/staker/collect_reward_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_mint_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_collect_fee_params_test.gnocontract/r/gnoswap/test/fuzz/math/_mock_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_create_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_arithmetic_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_arithmetic_fuzz_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_swap_test.gnocontract/r/gnoswap/test/fuzz/staker/stake_token_test.gnocontract/r/gnoswap/test/fuzz/math/int256_bitwise_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/staker/collect_reward_test.gnocontract/r/gnoswap/test/fuzz/router/exact_in_swap_route_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_create_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_decrease_liquidity_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_fuzz_test.gnocontract/r/gnoswap/test/fuzz/position/position_increase_liquidity_test.gnocontract/r/gnoswap/test/fuzz/staker/unstake_token_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_arithmetic_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/router/_helper_test.gnocontract/r/gnoswap/test/fuzz/math/uint256_math_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/position/position_collect_fee_test.gnocontract/r/gnoswap/test/fuzz/router/exact_in_swap_route_params_test.gnocontract/r/gnoswap/test/fuzz/pool/_helper_test.gnocontract/r/gnoswap/test/fuzz/staker/stake_token_params_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_utils_test.gnocontract/r/gnoswap/test/fuzz/math/_helper_test.gnocontract/r/gnoswap/test/fuzz/router/exact_out_swap_route_params_test.gnocontract/r/gnoswap/test/fuzz/staker/unstake_token_test.gnocontract/r/gnoswap/test/fuzz/pool/_mock_test.gnocontract/r/gnoswap/test/fuzz/staker/_helper_test.gnocontract/r/gnoswap/test/fuzz/position/_mock_test.gnocontract/r/gnoswap/test/fuzz/staker/_mock_test.gnocontract/r/gnoswap/test/fuzz/position/_helper_test.gnocontract/r/gnoswap/test/fuzz/router/_mock_test.gno
🔇 Additional comments (65)
contract/r/gnoswap/test/fuzz/position/position_reposition_test.gno (2)
1-16: Package reorganization and imports look good.The change from
package fuzztopackage positionis consistent with the PR objectives. All imports are properly structured and necessary for the test suite.
19-97: Realm context and crossing patterns are correct.All test functions properly set up realm context with
testing.SetRealm()and use thecrosskeyword for realm-crossing calls. Test assertions correctly usepanic()rather than returning errors. This aligns with Gno best practices.Also applies to: 100-159, 162-269, 271-322
contract/r/gnoswap/test/fuzz/math/int256_bitwise_fuzz_test.gno (1)
1-631: LGTM! Well-structured fuzz tests with comprehensive coverage.The package reorganization from
fuzztomathaligns with the PR's objectives to enable parallel test execution. The test suite is thorough:
- Comprehensive coverage of bitwise operations (Lsh, Rsh, And, Or)
- Proper verification of mathematical properties (commutativity, associativity, distributivity, absorption, idempotence)
- Good edge case testing (zero shifts, sign preservation, large shifts, sparse patterns)
- Consistent test structure using the fuzzutils framework
The tests properly use panic for assertions, which is standard practice in Gno test contexts.
contract/r/gnoswap/test/fuzz/math/uint256_math_fuzz_test.gno (1)
1-1: LGTM! Package reorganization aligns with PR objectives.The package rename from
fuzztomathcorrectly implements the test suite reorganization for parallel CI execution. The new package name follows Go conventions (matches directory structure) and improves test modularity by grouping math-related fuzz tests together.contract/r/gnoswap/test/fuzz/staker/unstake_token_params_test.gno (1)
1-1: LGTM! Package reorganization aligns with PR objectives.The package name change from
fuzztostakeris consistent with the PR's goal to split fuzz tests into category-specific submodules (math, pool, position, router, staker) for parallel execution.contract/r/gnoswap/test/fuzz/math/int256_bitwise_fuzz_params_test.gno (1)
1-1: LGTM: Package name change aligns with test reorganization.The package declaration change from
fuzztomathis appropriate for this test infrastructure file, which specifically contains parameter definitions for int256 bitwise operations. This aligns with the PR's objective to reorganize fuzz tests into category-specific submodules for parallel execution.contract/r/gnoswap/test/fuzz/position/position_reposition_params_test.gno (1)
1-1: LGTM! Package reorganization aligns with PR objectives.The package rename from
fuzztopositionis part of the broader restructuring to split fuzz tests into category-specific submodules (math, pool, position, router, staker) for parallel execution. The change is minimal and correct—no functional logic is affected.The parameter validation and generation logic in this test infrastructure file remains intact and appears comprehensive.
contract/r/gnoswap/test/fuzz/position/position_decrease_liquidity_test.gno (2)
34-34: Correct use ofcrosskeyword.The
crosskeyword is properly used when callingposition.DecreaseLiquidity, which is a realm-crossing function. This follows Gno best practices for explicit realm boundary crossing.Also applies to: 74-74, 101-101, 136-136
1-1: Package rename topositionaligns with PR objectives.The package rename from
fuzztopositionfollows the strategy to reorganize fuzz tests into category-specific submodules for parallel execution. Code follows Gno patterns correctly:
- Proper use of
crosskeyword for realm boundary crossing- Correct realm setup with
testing.SetRealm()- No security issues in test structure
Ensure all helper functions referenced in this test file are present in the reorganized
positionpackage before merging:
initStates(t)createTestPosition(t, adminAddr, poolAddr)NewValidPositionDecreaseLiquidityParams(ft, tokenId)NewRandomizedPositionDecreaseLiquidityParams(ft, tokenId)FUZZ_ITERATIONScontract/r/gnoswap/test/fuzz/position/position_increase_liquidity_params_test.gno (1)
1-1: Package namespace change aligns with the fuzz test reorganization.The package rename from
fuzztopositioncorrectly reflects the new modular structure for parallel test execution.contract/r/gnoswap/test/fuzz/staker/gnomod.toml (1)
1-2: Manifest file correctly defines module path for parallel fuzz test execution.The module path
gno.land/r/gnoswap/test/fuzz/stakerand Gno version0.9are consistent with other fuzz subdirectory manifests in this PR.contract/r/gnoswap/test/fuzz/math/int256_fuzz_test.gno (2)
1-1: Package namespace change correctly reflects the math test category.
90-93: Randomized tests should expect overflow as a valid outcome.In
TestFuzzInt256AddOverflow_RandomizedParams_Stateless, the test panics when overflow occurs. However, randomized inputs can legitimately trigger overflow conditions. Consider either:
- Accepting overflow as a valid test result (no panic, just record)
- Constraining randomized inputs to avoid overflow
The same pattern applies to
SubOverflow(lines 171-173) andMulOverflow(lines 252-254).contract/r/gnoswap/test/fuzz/math/uint256_arithmetic_fuzz_params_test.gno (2)
1-1: Package namespace change aligns with math test category.
74-132: Good coverage of edge cases for umul testing.The sparse pattern generation covers important code paths including zero operands, maximum values, asymmetric lengths, and alternating zeros. This thorough approach helps ensure robust testing of the multiplication implementation.
contract/r/gnoswap/test/fuzz/position/position_collect_fee_params_test.gno (1)
1-1: Package namespace change correctly reflects the position test category.contract/r/gnoswap/test/fuzz/pool/gnomod.toml (1)
1-2: Manifest file correctly defines module path for parallel fuzz test execution.Consistent with other fuzz subdirectory manifests.
contract/r/gnoswap/test/fuzz/position/position_collect_fee_test.gno (2)
1-1: Package namespace change correctly reflects the position test category.
82-86: Randomized test may crash on invalid parameters.
NewRandomizedPositionCollectFeeParamscan generate invalid parameters (e.g., non-existent position IDs in scenarios 1 and 2). WhenCollectFeereceives invalid parameters, it will panic and crash the test rather than recording the failure gracefully.Consider wrapping with panic recovery if the intent is to test error handling:
defer func() { if r := recover(); r != nil { // Record as expected failure for invalid params fuzzResult.AddError(index, r) } }()contract/r/gnoswap/test/fuzz/router/gnomod.toml (1)
1-2: Manifest file correctly defines module path for parallel fuzz test execution.Consistent with other fuzz subdirectory manifests.
contract/r/gnoswap/test/fuzz/math/int256_arithmetic_fuzz_test.gno (1)
1-1: LGTM! Package reorganization improves test modularity.The package rename from
fuzztomathaligns with the PR objective to enable parallel fuzz test execution by category. This organizational change has no functional impact on the test logic.contract/r/gnoswap/test/fuzz/position/position_decrease_liquidity_params_test.gno (1)
1-1: LGTM! Package reorganization aligns with category-based testing.The package rename from
fuzztopositionsupports the parallel test execution strategy outlined in the PR objectives. No functional changes to the parameter validation logic.contract/r/gnoswap/test/fuzz/position/gnomod.toml (1)
1-2: LGTM! Module manifest correctly defines the position fuzz test module.The manifest file properly specifies the module path and Gno version, enabling the position fuzz tests to run as a separate module for parallel execution.
contract/r/gnoswap/test/fuzz/math/int256_arithmetic_fuzz_params_test.gno (1)
1-1: LGTM! Package reorganization improves categorization.The package rename from
fuzztomathis consistent with the broader refactoring to organize fuzz tests by category. No functional changes to parameter definitions.contract/r/gnoswap/test/fuzz/math/uint256_fuzz_test.gno (1)
1-1: LGTM! Package namespace refactoring supports parallel test execution.The package rename from
fuzztomathenables category-based test isolation for parallel CI execution. No functional changes to the uint256 test logic.contract/r/gnoswap/test/fuzz/math/uint256_math_fuzz_params_test.gno (1)
1-1: LGTM! Package reorganization maintains consistency.The package rename from
fuzztomathis consistent with the category-based test organization. Parameter validation logic remains unchanged and correct.contract/r/gnoswap/test/fuzz/position/position_increase_liquidity_test.gno (1)
1-1: LGTM! Package reorganization with proper Gno patterns.The package rename from
fuzztopositionenables category-based parallel testing. The test implementation correctly follows Gno patterns:
- Proper use of
testing.SetRealm()for realm context- Correct
crosskeyword usage for realm crossing- Appropriate
panic()for test assertionscontract/r/gnoswap/test/fuzz/staker/stake_token_params_test.gno (1)
1-1: LGTM! Package reorganization completes the category-based refactoring.The package rename from
fuzztostakercompletes the reorganization of fuzz tests into category-specific packages (math, pool, position, router, staker) to enable parallel CI execution per the PR objectives.contract/r/gnoswap/test/fuzz/math/int256_fuzz_params_test.gno (1)
1-1: LGTM - Package reorganization aligns with parallel test execution goals.The package rename from
fuzztomathcorrectly categorizes these int256 parameter generators for the restructured fuzz test architecture.contract/r/gnoswap/test/fuzz/math/_mock_test.gno (1)
1-1: LGTM - Comprehensive mock infrastructure for fuzz testing.The package rename to
mathis appropriate, and the extensive mock implementations correctly use Gno realm patterns including propercur realmparameters andcrosskeyword usage throughout.contract/r/gnoswap/test/fuzz/position/position_mint_params_test.gno (1)
1-1: LGTM - Well-structured position mint parameter validation.The package rename to
positioncorrectly categorizes this test infrastructure. The comprehensive validation logic includes proper overflow checks and panic recovery for edge cases.contract/r/gnoswap/test/fuzz/staker/collect_reward_params_test.gno (1)
1-1: LGTM - Clean parameter structure for reward collection testing.The package rename to
stakerappropriately categorizes these reward collection test parameters.contract/r/gnoswap/test/fuzz/math/uint256_fuzz_params_test.gno (1)
1-1: LGTM - Comprehensive uint256 parameter structures.The package rename to
mathcorrectly categorizes these uint256 arithmetic test parameters. The low-level arithmetic helpers are properly implemented for fuzz testing.contract/r/gnoswap/test/fuzz/math/gnomod.toml (1)
1-2: LGTM - Standard module manifest for math fuzz tests.The module declaration correctly identifies the math fuzz test category and specifies Gno version 0.9, enabling this test suite to run as an independent module.
contract/r/gnoswap/test/fuzz/staker/collect_reward_test.gno (1)
1-1: LGTM - Well-structured reward collection fuzz tests.The package rename to
stakercorrectly categorizes these tests. The test implementation properly uses realm crossing with thecrosskeyword and sets up appropriate test contexts.contract/r/gnoswap/test/fuzz/position/position_mint_test.gno (1)
1-1: LGTM - Comprehensive position mint fuzz tests.The package rename to
positioncorrectly categorizes these tests. The implementation provides both stateless and stateful test variants with proper realm crossing patterns and state management.contract/r/gnoswap/test/fuzz/math/_helper_test.gno (1)
57-64: Global initialization flags may cause test isolation issues.These package-level boolean flags (
initializedPool,initializedPosition, etc.) are set once and never reset between test runs. If tests within this package run in parallel or in different orders, the initialization state may not be as expected.Since
initStatesis called at the start of stateless tests, this appears intentional for performance. However, document this behavior to clarify that tests share initialization state.contract/r/gnoswap/test/fuzz/math/uint256_arithmetic_fuzz_test.gno (1)
1-10: LGTM!The package rename aligns with the PR objective to organize fuzz tests by category. The arithmetic property tests (associativity, commutativity, distributive law, identity) are well-structured and comprehensive.
contract/r/gnoswap/test/fuzz/pool/pool_swap_test.gno (2)
1-16: LGTM!The package rename to
poolaligns with the PR objective for parallel test execution. Imports are correctly organized.
117-171: Well-structured swap callback with proper realm handling.The
callPoolSwapfunction correctly:
- Sets up token approvals from user realm
- Defines a swap callback that handles realm crossing properly
- Executes the swap from a mock contract realm to bypass EOA restrictions
This follows the coding guidelines for realm crossing in tests.
contract/r/gnoswap/test/fuzz/router/exact_out_swap_route_test.gno (2)
92-98: Silently setting negative mint amount to 0 may mask bugs.When
params.amountInproduces a negativeint64, settingmintedAmount = 0silently proceeds without minting tokens. If the fuzz test generates such values, this could hide issues in the parameter generation logic.Consider either:
- Logging/panicking when a negative amount is encountered
- Ensuring the parameter generator (
NewValidExactOutSwapRouteParams/NewRandomizedExactOutSwapRouteParams) never produces negative amounts
1-17: LGTM!The package rename to
routeraligns with the PR objective. The test structure follows established patterns with proper realm context setup.contract/r/gnoswap/test/fuzz/pool/pool_utils_test.gno (1)
1-16: LGTM!The package rename to
poolaligns with the PR objective for organizing fuzz tests by category.contract/r/gnoswap/test/fuzz/staker/unstake_token_test.gno (1)
1-1: LGTM! Package reorganization supports parallel test execution.The package namespace change from
fuzztostakeraligns with the PR's objective to enable parallel execution of fuzz tests by category. The test logic remains unchanged and follows proper Gno patterns.contract/r/gnoswap/test/fuzz/pool/tick_math_test.gno (1)
1-1: LGTM! Package reorganization aligns with parallel execution strategy.The package namespace change from
fuzztopoolis consistent with the broader reorganization to enable category-based parallel test execution.contract/r/gnoswap/test/fuzz/pool/pool_create_test.gno (1)
1-1: LGTM! Package namespace change follows proper Gno testing patterns.The package declaration update from
fuzztopoolsupports the parallel execution objective. The tests correctly usetesting.SetRealmand thecrosskeyword for realm interactions.contract/r/gnoswap/test/fuzz/staker/staker_utils_test.gno (1)
1-1: LGTM! Test utilities properly organized under staker namespace.The package change from
fuzztostakeris consistent with the PR's reorganization strategy. All utility functions correctly usetesting.SetRealmand thecrosskeyword.contract/r/gnoswap/test/fuzz/pool/tick_math_params_test.gno (1)
1-1: LGTM! Parameter definitions properly scoped to pool namespace.The package namespace update from
fuzztopoolaligns with the organizational restructuring for parallel test execution.contract/r/gnoswap/test/fuzz/router/exact_in_swap_route_params_test.gno (1)
1-1: LGTM! Router parameter definitions properly organized.The package namespace change from
fuzztoroutersupports the parallel execution strategy. The parameter validation logic is well-structured and comprehensive.contract/r/gnoswap/test/fuzz/router/router_utils_test.gno (1)
1-1: LGTM! Router utilities properly scoped to router namespace.The package declaration update from
fuzztorouteris consistent with the organizational restructuring. The utility functions include sophisticated route generation logic with proper duplicate pair prevention.contract/r/gnoswap/test/fuzz/router/exact_in_swap_route_test.gno (1)
1-1: LGTM! Router tests properly organized with correct Gno patterns.The package namespace change from
fuzztoroutercompletes the reorganization strategy. The tests correctly usetesting.SetRealmand thecrosskeyword for realm interactions.contract/r/gnoswap/test/fuzz/router/exact_out_swap_route_params_test.gno (1)
1-1: Package rename looks good.The package rename from
fuzztorouteraligns with the PR's objective to reorganize fuzz tests into category-specific packages for parallel execution.contract/r/gnoswap/test/fuzz/pool/pool_swap_params_test.gno (1)
1-1: Package rename aligns with restructuring goals.The change from
fuzztopoolpackage is consistent with the PR's objective to organize fuzz tests by category.contract/r/gnoswap/test/fuzz/pool/pool_create_params_test.gno (1)
1-106: Well-structured fuzz test parameter implementation.The package rename and validation logic are well-implemented. The
IsValid()method properly validates all constraints including token ordering, fee tiers, price range bounds, and pool existence checks.scripts/all-integration-tests.sh (1)
25-41: Argument parsing implementation looks good.The parsing loop correctly handles
--skip,--category=*, and positional arguments. The logic properly differentiates between flags and path arguments..github/workflows/run_test.yml (1)
17-17: Good: Ruby action now uses pinned commit hash.The change from unpinned
v1toruby/setup-ruby@d354de180d0c9e813cfddfcbdc079945d4be589b # v1.275.0addresses the security concern about using unpinned 3rd party actions..github/workflows/run_integration_test.yml (2)
40-55: Matrix strategy effectively parallelizes integration tests.The categorization into "base", "upgradable", "gov-launchpad", and "others" enables parallel execution, which should significantly reduce CI time. The
fail-fast: falseensures all categories run even if one fails.
32-38: Consider uploading only the necessary gno artifacts.Similar to
run_test.yml, the entiregno/directory is uploaded. Since integration tests need thegno.land/pkg/integrationpath (line 86), this may be intentional. However, verify if the full directory is needed or if a subset would suffice.contract/r/gnoswap/test/fuzz/router/_helper_test.gno (1)
34-105: Comprehensive test infrastructure with well-defined constants.The constants for price ratios, tick bounds, token paths, and fee tiers are well-organized. The unexported package-level variables follow good practice for test utilities.
contract/r/gnoswap/test/fuzz/pool/_helper_test.gno (2)
1-103: Helper infrastructure mirrors router package structure.The constants, globals, and initialization patterns are consistent with the router helper, which is good for maintainability across fuzz test packages.
190-216: Missing helper function dependency.
setupRouterMintPositionByRoutePathcallsparsePoolPathAlign,setupCreatePool,generateInRangeTick, andsetupMintPosition, but their definitions could not be verified in this file or the package. Ensure these functions exist elsewhere in the pool package or are properly imported.setup.py (2)
243-257: LGTM - Category filtering logic is well-implemented.The filtering correctly handles both root-level files (via the "root" pseudo-category) and subdirectory-based categories. The use of
rel_path.parts[0]to extract the top-level subdirectory is appropriate.
384-402: LGTM -list_categoriesfunction is clean and functional.The function correctly discovers both subdirectory categories and detects root-level
.txtarfiles. The "root" pseudo-category is appended last for consistency.contract/r/gnoswap/test/fuzz/pool/_mock_test.gno (2)
688-734: Consider whether real realm calls are intended in mock accessors.The
mockPoolAccessormethods call the actualpoolpackage functions (e.g.,pool.ExistsPoolPath,pool.GetSlot0Tick). This makes these not true mocks but rather wrappers that set up test realm context. If the intent is to have fully isolated mocks for unit testing, these should return test-controlled values instead.If the intent is integration-style fuzz testing where real pool interactions are needed, this is appropriate.
663-663: VerifyoblPathis defined in the package.The variable
oblPathis referenced on line 663 but its definition is not visible in this file. Ensure it's defined or properly imported within thepooltest package.
| func (n *mockNFTAccessor) Approve(approved address, tid grc721.TokenID) error { | ||
| n.approved[tid][approved] = true | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Potential nil map access panic in Approve.
If Approve is called with a token ID that hasn't been minted first, n.approved[tid] will be nil, and accessing n.approved[tid][approved] will panic.
🔎 Proposed fix
func (n *mockNFTAccessor) Approve(approved address, tid grc721.TokenID) error {
+ if n.approved[tid] == nil {
+ n.approved[tid] = make(map[address]bool)
+ }
n.approved[tid][approved] = true
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (n *mockNFTAccessor) Approve(approved address, tid grc721.TokenID) error { | |
| n.approved[tid][approved] = true | |
| return nil | |
| } | |
| func (n *mockNFTAccessor) Approve(approved address, tid grc721.TokenID) error { | |
| if n.approved[tid] == nil { | |
| n.approved[tid] = make(map[address]bool) | |
| } | |
| n.approved[tid][approved] = true | |
| return nil | |
| } |
| func (n *mockNFTAccessor) OwnerOf(tid grc721.TokenID) (address, error) { | ||
| if _, ok := n.owners[tid]; !ok { | ||
| return "", grc721.ErrCallerIsNotOwner | ||
| } | ||
|
|
||
| return n.owners[tid], nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find grc721 error definitions
find . -type f -name "*.gno" | xargs grep -l "ErrCallerIsNotOwner\|ErrInvalidTokenId" | head -20Repository: gnoswap-labs/gnoswap
Length of output: 257
🏁 Script executed:
# Look for the grc721 package files
fd -e gno "grc721" | head -20Repository: gnoswap-labs/gnoswap
Length of output: 257
🏁 Script executed:
# Check the specific file and surrounding context
cat -n contract/r/gnoswap/test/fuzz/pool/_mock_test.gno | sed -n '275,295p'Repository: gnoswap-labs/gnoswap
Length of output: 257
🏁 Script executed:
# Search for all error definitions in grc721
fd -e gno grc721 | xargs grep -h "^var.*Err" 2>/dev/null | sort -uRepository: gnoswap-labs/gnoswap
Length of output: 257
Incorrect error returned for non-existent token.
OwnerOf returns grc721.ErrCallerIsNotOwner when the token doesn't exist. This is semantically incorrect—the issue is the token doesn't exist, not that the caller isn't the owner. Use grc721.ErrInvalidTokenId instead.
func (n *mockNFTAccessor) OwnerOf(tid grc721.TokenID) (address, error) {
if _, ok := n.owners[tid]; !ok {
- return "", grc721.ErrCallerIsNotOwner
+ return "", grc721.ErrInvalidTokenId
}
return n.owners[tid], nil
}| func (n *mockNFTAccessor) Approve(approved address, tid grc721.TokenID) error { | ||
| n.approved[tid][approved] = true | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Same nil map access issue as in pool mock.
The Approve method has the same potential panic if called before Mint for a given token ID.
🔎 Proposed fix
func (n *mockNFTAccessor) Approve(approved address, tid grc721.TokenID) error {
+ if n.approved[tid] == nil {
+ n.approved[tid] = make(map[address]bool)
+ }
n.approved[tid][approved] = true
return nil
}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
contract/r/gnoswap/test/fuzz/staker/_helper_test.gno (2)
54-57: Consider making test variables unexported unless needed externally.The exported variables
VALID_FEE_TIERSandMAX_UINT128are accessible from other packages. If these are only used within the staker fuzz tests, making them unexported (validFeeTiers,maxUint128) would better encapsulate test helpers and prevent unintended external dependencies.
267-279: Inconsistent error handling between similar parsing functions.
parsePoolKey(lines 267-279) panics on invalid input, whileparsePoolPath(lines 335-347) silently returns zero values. Both parse the same"token0:token1:fee"format, which could confuse callers about expected error behavior.Consider either consolidating these into a single function with a
strictparameter, or adding comments explaining when to use each variant.Also applies to: 335-347
contract/r/gnoswap/test/fuzz/router/_helper_test.gno (2)
54-57: Consider making test variables unexported unless needed externally.The exported variables
VALID_FEE_TIERSandMAX_UINT128are accessible from other packages. If these are only used within the router fuzz tests, making them unexported (validFeeTiers,maxUint128) would better encapsulate test helpers and prevent unintended external dependencies.
267-279: Inconsistent error handling between similar parsing functions.
parsePoolKey(lines 267-279) panics on invalid input, whileparsePoolPath(lines 335-347) silently returns zero values. Both parse the same"token0:token1:fee"format, which could confuse callers about expected error behavior.Consider either consolidating these into a single function with a
strictparameter, or adding comments explaining when to use each variant.Also applies to: 335-347
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contract/r/gnoswap/test/fuzz/router/_helper_test.gnocontract/r/gnoswap/test/fuzz/staker/_helper_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/test/fuzz/router/_helper_test.gnocontract/r/gnoswap/test/fuzz/staker/_helper_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). (43)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/pool, contract/r/gnoswap/test/fuzz/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/protocol_fee, contract/r/scenario/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/launchpad, contract/r/scenario/launchpad)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/staker, contract/r/gnoswap/test/fuzz/staker)
- 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/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/position, contract/r/gnoswap/test/fuzz/position)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/gns, contract/r/scenario/gns)
- GitHub Check: test-gnoswap (gnoswap/scenario/emission, contract/r/scenario/emission)
- GitHub Check: test-gnoswap (gnoswap/referral, contract/r/gnoswap/referral)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/router, contract/r/gnoswap/test/fuzz/router)
- GitHub Check: test-gnoswap (gnoswap/position, contract/r/gnoswap/position)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/math, contract/r/gnoswap/test/fuzz/math)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee/v1, contract/r/gnoswap/protocol_fee/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/governance, contract/r/gnoswap/gov/governance)
- 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/rbac, contract/r/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- 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/community_pool, contract/r/gnoswap/community_pool)
- GitHub Check: test-gnoswap (gnoswap/store, contract/p/gnoswap/store)
- GitHub Check: test-gnoswap (gnoswap/gov/xgns, contract/r/gnoswap/gov/xgns)
- GitHub Check: test-gnoswap (gnoswap/gov/governance/v1, contract/r/gnoswap/gov/governance/v1)
- 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/gnft, contract/r/gnoswap/gnft)
- GitHub Check: run-integration-test (base, base)
- GitHub Check: run-integration-test (others, router,pool,position,staker,root)
- GitHub Check: run-integration-test (gov-launchpad, gov,launchpad)
- GitHub Check: run-integration-test (upgradable, upgradable)
🔇 Additional comments (2)
contract/r/gnoswap/test/fuzz/staker/_helper_test.gno (1)
364-366: Previously reported typo has been fixed.The fuzz label for
tickUpperDiffon line 365 is now correct ("tickUpperDiff"). The issue mentioned in the previous review has been resolved.contract/r/gnoswap/test/fuzz/router/_helper_test.gno (1)
364-366: Previously reported typo has been fixed.The fuzz label for
tickUpperDiffon line 365 is now correct ("tickUpperDiff"). The issue mentioned in the previous review has been resolved.
| func generateTokenPathByList(f *fuzz.T, tokenPaths map[string]bool) string { | ||
| tokenPathIndex := fuzz.IntRange(0, len(tokenPaths)-1).Draw(f, "tokenPath").(int) | ||
|
|
||
| currentIndex := 0 | ||
| for tokenPath := range tokenPaths { | ||
| if currentIndex == tokenPathIndex { | ||
| return tokenPath | ||
| } | ||
| currentIndex++ | ||
| } | ||
|
|
||
| return "" | ||
| } |
There was a problem hiding this comment.
Map iteration order affects fuzz test reproducibility.
Go map iteration is non-deterministic, so the same fuzz seed could select different token paths across runs. This makes it harder to reproduce fuzz failures.
🔎 Suggested fix for deterministic token selection
func generateTokenPathByList(f *fuzz.T, tokenPaths map[string]bool) string {
tokenPathIndex := fuzz.IntRange(0, len(tokenPaths)-1).Draw(f, "tokenPath").(int)
- currentIndex := 0
+ // Convert to sorted slice for deterministic iteration
+ paths := make([]string, 0, len(tokenPaths))
for tokenPath := range tokenPaths {
- if currentIndex == tokenPathIndex {
- return tokenPath
- }
- currentIndex++
+ paths = append(paths, tokenPath)
}
+ // Sort for deterministic ordering
+ sort.Strings(paths)
+ return paths[tokenPathIndex]
-
- return ""
}Note: Add "sort" to imports.
| func generateTokenPathByList(f *fuzz.T, tokenPaths map[string]bool) string { | ||
| tokenPathIndex := fuzz.IntRange(0, len(tokenPaths)-1).Draw(f, "tokenPath").(int) | ||
|
|
||
| currentIndex := 0 | ||
| for tokenPath := range tokenPaths { | ||
| if currentIndex == tokenPathIndex { | ||
| return tokenPath | ||
| } | ||
| currentIndex++ | ||
| } | ||
|
|
||
| return "" | ||
| } |
There was a problem hiding this comment.
Map iteration order affects fuzz test reproducibility.
Go map iteration is non-deterministic, so the same fuzz seed could select different token paths across runs. This makes it harder to reproduce fuzz failures.
🔎 Suggested fix for deterministic token selection
func generateTokenPathByList(f *fuzz.T, tokenPaths map[string]bool) string {
tokenPathIndex := fuzz.IntRange(0, len(tokenPaths)-1).Draw(f, "tokenPath").(int)
- currentIndex := 0
+ // Convert to sorted slice for deterministic iteration
+ paths := make([]string, 0, len(tokenPaths))
for tokenPath := range tokenPaths {
- if currentIndex == tokenPathIndex {
- return tokenPath
- }
- currentIndex++
+ paths = append(paths, tokenPath)
}
+ // Sort for deterministic ordering
+ sort.Strings(paths)
+ return paths[tokenPathIndex]
-
- return ""
}Note: Add "sort" to imports.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contract/r/gnoswap/test/fuzz/pool/pool_swap_params_test.gno (1)
46-58: Consider handling parse errors explicitly.The function ignores errors from
strconv.ParseIntat lines 46, 53, and 57. If parsing fails, the function silently returns 0, which may not accurately represent the intended swap amount in test scenarios.🔎 Proposed improvement with error handling
func (p *poolSwapParams) SwapAmount() int64 { amountInt, _ := strconv.ParseInt(p.amountSpecified, 10, 64) if amountInt > 0 { return amountInt } if p.zeroForOne { - expectedAmount0Int, _ := strconv.ParseInt(p.expectedAmount0, 10, 64) + expectedAmount0Int, err := strconv.ParseInt(p.expectedAmount0, 10, 64) + if err != nil { + panic(ufmt.Sprintf("failed to parse expectedAmount0: %v", err)) + } return expectedAmount0Int } - expectedAmount1Int, _ := strconv.ParseInt(p.expectedAmount1, 10, 64) + expectedAmount1Int, err := strconv.ParseInt(p.expectedAmount1, 10, 64) + if err != nil { + panic(ufmt.Sprintf("failed to parse expectedAmount1: %v", err)) + } return expectedAmount1Int }Note: Line 46 could also benefit from error handling, but since it checks
amountInt > 0immediately after, a parse error resulting in 0 flows to the fallback logic, which may be acceptable for test fuzzing scenarios.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contract/r/gnoswap/test/fuzz/math/int256_fuzz_params_test.gnocontract/r/gnoswap/test/fuzz/pool/pool_swap_params_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/test/fuzz/pool/pool_swap_params_test.gnocontract/r/gnoswap/test/fuzz/math/int256_fuzz_params_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). (54)
- 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/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/staker, contract/r/gnoswap/test/fuzz/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/rbac, contract/r/scenario/rbac)
- GitHub Check: test-gnoswap (gnoswap/scenario/protocol_fee, contract/r/scenario/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/math, contract/r/gnoswap/test/fuzz/math)
- GitHub Check: test-gnoswap (gnoswap/scenario/halt, contract/r/scenario/halt)
- GitHub Check: test-gnoswap (gnoswap/scenario/emission, contract/r/scenario/emission)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/gns, contract/r/scenario/gns)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/pool, contract/r/gnoswap/test/fuzz/pool)
- GitHub Check: test-gnoswap (gnoswap/test/fuzz/router, contract/r/gnoswap/test/fuzz/router)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- 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/position, contract/r/gnoswap/test/fuzz/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/launchpad, contract/r/scenario/launchpad)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/referral, contract/r/gnoswap/referral)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee/v1, contract/r/gnoswap/protocol_fee/v1)
- GitHub Check: test-gnoswap (gnoswap/halt, contract/r/gnoswap/halt)
- GitHub Check: test-gnoswap (gnoswap/pool, contract/r/gnoswap/pool)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee, contract/r/gnoswap/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/router, contract/r/gnoswap/router)
- GitHub Check: test-gnoswap (gnoswap/uint256, contract/p/gnoswap/uint256)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/r/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/gov/governance, contract/r/gnoswap/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: test-gnoswap (gnoswap/version_manager, contract/p/gnoswap/version_manager)
- GitHub Check: test-gnoswap (gnoswap/position, contract/r/gnoswap/position)
- 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/emission, contract/r/gnoswap/emission)
- 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/gns, contract/r/gnoswap/gns)
- GitHub Check: test-gnoswap (gnoswap/launchpad, contract/r/gnoswap/launchpad)
- GitHub Check: test-gnoswap (gnoswap/int256, contract/p/gnoswap/int256)
- GitHub Check: test-gnoswap (gnoswap/community_pool, contract/r/gnoswap/community_pool)
- GitHub Check: test-gnoswap (gnoswap/gov/staker, contract/r/gnoswap/gov/staker)
- 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/rbac, contract/p/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/gnsmath, contract/p/gnoswap/gnsmath)
- GitHub Check: run-integration-test (others, router,pool,position,staker,root)
- GitHub Check: run-integration-test (gov-launchpad, gov,launchpad)
- GitHub Check: run-integration-test (base, base)
- GitHub Check: run-integration-test (upgradable, upgradable)
🔇 Additional comments (5)
contract/r/gnoswap/test/fuzz/math/int256_fuzz_params_test.gno (3)
1-1: LGTM! Package reorganization aligns with PR objectives.The package rename from
fuzztomathis consistent with the PR's goal of splitting fuzz tests into category-specific submodules to enable parallel test execution.
70-86: LGTM! Formatting change preserves logic.The refactoring from an else-if chain to separate if statements is purely cosmetic. The conditions are mutually exclusive (non-negative signs vs. negative signs), and each branch returns immediately, so the behavior remains identical.
169-185: LGTM! Consistent formatting change with no behavioral impact.Similar to the AddOverflow changes, this refactoring splits the else-if chain into separate if statements. The conditions remain mutually exclusive with immediate returns, preserving the original logic.
contract/r/gnoswap/test/fuzz/pool/pool_swap_params_test.gno (2)
1-1: LGTM: Package reorganization aligns with PR objectives.The package rename from
fuzztopoolis appropriate and consistent with the PR's goal to split fuzz tests into category-specific submodules for parallel execution.
48-59: Cleaner control flow with flattened conditionals.The refactoring removes nested else blocks and uses early returns, improving readability while maintaining equivalent behavior.
|
Summary
Changes
--categoryand--list-categoriesoptions tosetup.pyall-integration-tests.shto support category filteringrun_integration_test.yml(4 parallel jobs)Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.