Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds a new package Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
Gas Report Comparison
|
junghoon-vans
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
contract/r/gnoswap/staker/v1/reward_calculation_pool_tier.gno (1)
107-108: Inconsistent use of safe arithmetic for timestamp addition.Line 101 uses
gnsmath.SafeAddInt64(currentTime, 1)for overflow-safe addition, but Line 107 uses plaincurrentTime+1. For consistency and safety, both additions should use the safe helper.♻️ Suggested fix for consistency
- pools.set(initialPoolPath, sr.NewPool(initialPoolPath, currentTime+1)) - result.changeTier(currentTime+1, pools, initialPoolPath, 1) + pools.set(initialPoolPath, sr.NewPool(initialPoolPath, gnsmath.SafeAddInt64(currentTime, 1))) + result.changeTier(gnsmath.SafeAddInt64(currentTime, 1), pools, initialPoolPath, 1)contract/r/gnoswap/pool/v1/position.gno (1)
6-6: Consider renaming the import alias for clarity.The package
gno.land/p/gnoswap/gnsmathis imported asplp, which may be a legacy alias from when the package had different contents. For consistency with other files in this PR that usegnsmathas the alias, consider updating this alias.♻️ Optional: Update alias for consistency
- plp "gno.land/p/gnoswap/gnsmath" + gnsmath "gno.land/p/gnoswap/gnsmath"Then update the call sites accordingly (lines 94-95, 106, 111).
contract/r/gnoswap/pool/v1/swap.gno (1)
14-14: LGTM — Safe conversions correctly applied in swap computations.The
SafeConvertToInt64calls at lines 341, 345, 395, and 400 properly protect against overflow when convertingu256.Uintamounts toint64for balance comparisons and protocol fee accumulation. TheSafeAddInt64calls ensure fee accumulation doesn't overflow.Note: The import alias
plpforgnsmathis unconventional (typicallyplpsuggests "pool package"). Consider renaming tognsmathfor clarity, though this is a minor stylistic point.Also applies to: 341-341, 345-345, 395-395, 400-400
contract/r/gnoswap/gov/staker/v1/protocol_fee_reward_manager.gno (1)
175-179: Note: Pre-overflow check may be redundant with SafeAddInt64.Lines 176-178 perform an explicit overflow check before calling
gnsmath.SafeAddInt64at line 179. IfSafeAddInt64panics on overflow (which is the expected behavior for safe math helpers), this pre-check is redundant. However, the current approach provides a more descriptive error message, which may be intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b05f2b9-f723-4e4c-af68-96d60657c0e3
📒 Files selected for processing (84)
contract/p/gnoswap/gnsmath/safe_math.gnocontract/p/gnoswap/gnsmath/safe_math_test.gnocontract/r/gnoswap/emission/distribution.gnocontract/r/gnoswap/emission/emission.gnocontract/r/gnoswap/emission/getter.gnocontract/r/gnoswap/emission/security_test.gnocontract/r/gnoswap/emission/utils.gnocontract/r/gnoswap/gns/emission_state.gnocontract/r/gnoswap/gns/gns.gnocontract/r/gnoswap/gns/halving.gnocontract/r/gnoswap/gns/utils.gnocontract/r/gnoswap/gns/utils_test.gnocontract/r/gnoswap/gov/governance/v1/governance_propose.gnocontract/r/gnoswap/gov/governance/v1/governance_vote.gnocontract/r/gnoswap/gov/governance/v1/proposal_schedule_status.gnocontract/r/gnoswap/gov/governance/v1/proposal_status.gnocontract/r/gnoswap/gov/governance/v1/proposal_status_test.gnocontract/r/gnoswap/gov/governance/v1/proposal_vote_status.gnocontract/r/gnoswap/gov/governance/v1/proposal_vote_status_test.gnocontract/r/gnoswap/gov/governance/v1/utils.gnocontract/r/gnoswap/gov/governance/v1/utils_test.gnocontract/r/gnoswap/gov/staker/delegation_withdraw.gnocontract/r/gnoswap/gov/staker/util.gnocontract/r/gnoswap/gov/staker/v1/delegation.gnocontract/r/gnoswap/gov/staker/v1/delegation_withdraw.gnocontract/r/gnoswap/gov/staker/v1/emission_reward_manager.gnocontract/r/gnoswap/gov/staker/v1/emission_reward_state.gnocontract/r/gnoswap/gov/staker/v1/getter.gnocontract/r/gnoswap/gov/staker/v1/launchpad_project_deposits.gnocontract/r/gnoswap/gov/staker/v1/protocol_fee_reward_manager.gnocontract/r/gnoswap/gov/staker/v1/protocol_fee_reward_state.gnocontract/r/gnoswap/gov/staker/v1/staker_delegate.gnocontract/r/gnoswap/gov/staker/v1/staker_delegate_test.gnocontract/r/gnoswap/gov/staker/v1/staker_delegation_snapshot.gnocontract/r/gnoswap/gov/staker/v1/state.gnocontract/r/gnoswap/gov/staker/v1/util.gnocontract/r/gnoswap/gov/staker/v1/util_test.gnocontract/r/gnoswap/launchpad/v1/launchpad_project.gnocontract/r/gnoswap/launchpad/v1/launchpad_reward.gnocontract/r/gnoswap/launchpad/v1/launchpad_reward_test.gnocontract/r/gnoswap/launchpad/v1/project.gnocontract/r/gnoswap/launchpad/v1/project_tier.gnocontract/r/gnoswap/launchpad/v1/reward_manager.gnocontract/r/gnoswap/launchpad/v1/reward_state.gnocontract/r/gnoswap/launchpad/v1/utils.gnocontract/r/gnoswap/launchpad/v1/utils_test.gnocontract/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/gnoswap/pool/v1/protocol_fee.gnocontract/r/gnoswap/pool/v1/swap.gnocontract/r/gnoswap/pool/v1/transfer.gnocontract/r/gnoswap/pool/v1/utils.gnocontract/r/gnoswap/pool/v1/utils_test.gnocontract/r/gnoswap/position/v1/burn.gnocontract/r/gnoswap/position/v1/position.gnocontract/r/gnoswap/position/v1/utils.gnocontract/r/gnoswap/position/v1/utils_test.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_test.gnocontract/r/gnoswap/protocol_fee/v1/utils.gnocontract/r/gnoswap/router/v1/base.gnocontract/r/gnoswap/router/v1/base_test.gnocontract/r/gnoswap/router/v1/dsl_test.gnocontract/r/gnoswap/router/v1/exact_in.gnocontract/r/gnoswap/router/v1/exact_out.gnocontract/r/gnoswap/router/v1/protocol_fee_swap.gnocontract/r/gnoswap/router/v1/router_dry.gnocontract/r/gnoswap/router/v1/router_test.gnocontract/r/gnoswap/router/v1/utils.gnocontract/r/gnoswap/router/v1/utils_test.gnocontract/r/gnoswap/staker/v1/calculate_pool_position_reward.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking.gnocontract/r/gnoswap/staker/v1/reward_calculation_incentives.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool.gnocontract/r/gnoswap/staker/v1/reward_calculation_pool_tier.gnocontract/r/gnoswap/staker/v1/reward_calculation_warmup.gnocontract/r/gnoswap/staker/v1/staker.gnocontract/r/gnoswap/staker/v1/type.gnocontract/r/gnoswap/staker/v1/utils.gnocontract/r/gnoswap/staker/v1/utils_test.gno
💤 Files with no reviewable changes (12)
- contract/r/gnoswap/emission/utils.gno
- contract/r/gnoswap/launchpad/v1/utils_test.gno
- contract/r/gnoswap/position/v1/utils.gno
- contract/r/gnoswap/gov/governance/v1/utils.gno
- contract/r/gnoswap/protocol_fee/v1/utils.gno
- contract/r/gnoswap/router/v1/utils_test.gno
- contract/r/gnoswap/gov/staker/util.gno
- contract/r/gnoswap/gov/staker/v1/util.gno
- contract/r/gnoswap/gov/governance/v1/utils_test.gno
- contract/r/gnoswap/pool/v1/utils_test.gno
- contract/r/gnoswap/staker/v1/utils.gno
- contract/r/gnoswap/pool/v1/utils.gno
Comment on lines
+220
to
223
| updatedTotalStakedAmount := gnsmath.SafeSubInt64(self.GetTotalStakedAmount(), amount) | ||
| if updatedTotalStakedAmount < 0 { | ||
| updatedTotalStakedAmount = 0 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the SafeSubInt64 implementation to verify underflow behavior
ast-grep --pattern 'func SafeSubInt64($_, $_) int64 {
$$$
}'Repository: gnoswap-labs/gnoswap
Length of output: 46
🏁 Script executed:
# Search for SafeSubInt64 definition across the codebase
rg "func SafeSubInt64" -A 10Repository: gnoswap-labs/gnoswap
Length of output: 723
🏁 Script executed:
# Also find the gnsmath package location
fd -t f "gnsmath" -o find . -type d -name "*gnsmath*" | head -20Repository: gnoswap-labs/gnoswap
Length of output: 191
Remove dead code: SafeSubInt64 panics on underflow, never returns negative values.
Lines 221-223 check if updatedTotalStakedAmount < 0 and clamp to 0. Since SafeSubInt64 panics when subtraction underflows (line in safe_math.gno: if b > 0 && a < math.MinInt64+b { panic(...) }), this result can never be negative. The defensive check is unreachable dead code and should be removed.
jinoosss
reviewed
Apr 8, 2026
Member
jinoosss
left a comment
jinoosss
left a comment
There was a problem hiding this comment.
Overall, it looks fine.
I’ll double-check to make sure nothing is missing.
Member
|
Gas costs are expected to rise in this PR, so it would be a good idea to check the details based on this PR. |
notJoon
approved these changes
Apr 12, 2026
notJoon
requested changes
Apr 12, 2026
Member
notJoon
left a comment
notJoon
left a comment
There was a problem hiding this comment.
It looks good that the duplicated utility functions have been separated into the p package. We would need to measure whether the cost of defining/creating functions within the realm is higher or whether the cost of importing the p package is higher, but at least in terms of improving cohesion, I think this is a positive change. Could you please resolve the conflicts?
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gnsmathsafe math package with coverage for common checked arithmetic helpers used across contract codeSummary by CodeRabbit
New Features
Refactor
Tests