docs(contract): refine public docs after #1161#1243
docs(contract): refine public docs after #1161#1243junghoon-vans wants to merge 3 commits intomainfrom
Conversation
WalkthroughThis pull request primarily updates inline documentation and docstrings across multiple Gnoswap contract modules, adding structured Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The review involves substantial breadth (40+ files), but most changes are homogeneous documentation updates (structured comment additions). However, the presence of a few new functions ( 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/gov/staker/tree.gno (1)
138-143:⚠️ Potential issue | 🟠 MajorReplace direct
strconv.ParseIntwith explicit bounds checking.Line 139 uses
strconv.ParseIntdirectly, which does not provide explicit int64 range validation. Per Audit N-05, string-to-number conversions should validate boundaries explicitly.The safe pattern already exists in this file: use
DecodeUint(s)to parse as uint64, then validate against int64 max before casting:Suggested patch
func DecodeInt64(s string) int64 { - num, err := strconv.ParseInt(s, 10, 64) - if err != nil { - panic(err) - } - return num + num := DecodeUint(s) + if num > 9223372036854775807 { + panic(ufmt.Sprintf("value overflows int64: %s", s)) + } + return int64(num) }contract/r/gnoswap/router/README.md (3)
251-292:⚠️ Potential issue | 🟠 MajorCritical documentation inconsistency: Contradictory native GNOT guidance.
This section provides extensive examples of "Native GNOT Swaps with Refunds" (lines 251-292), directly contradicting the updated documentation at lines 148-162 which states the router "does not handle native
ugnotdirectly" and "rejects native-coin handling." The code confirms native coins are rejected viaAssertIsNotHandleNativeCoin()calls in bothExactInSwapRouteandExactOutSwapRoute.This entire section (251-292) should be removed to maintain documentation consistency with the actual router implementation.
📝 Proposed fix: Remove contradictory section
Remove lines 251-292 entirely, as they describe functionality that does not exist in the current router implementation. The corrected native token guidance at lines 148-181 already provides the accurate documentation.
298-308:⚠️ Potential issue | 🟠 MajorRemove outdated developer notes referencing non-existent native GNOT features.
The "Common Integration Pitfalls" section references WUGNOT approval requirements, native GNOT send amounts, automatic refunds, and route vs token identifier confusion—all predicated on native GNOT support that the router explicitly rejects. These notes are now obsolete given the router's
AssertIsNotHandleNativeCoin()enforcement.🔧 Suggested revision
Revise or remove pitfalls 1-4 to focus on actual router integration concerns (e.g., route format vs pool format confusion, slippage protection, deadline handling) rather than non-existent native token flows.
310-317:⚠️ Potential issue | 🟡 MinorUpdate frontend checklist to remove native GNOT items.
The checklist includes multiple items for native GNOT handling (WUGNOT approval, native token amounts, automatic refunds) that are no longer applicable since the router rejects native coins.
✅ Suggested checklist revision
Remove or update checklist items referencing:
- WUGNOT approval before native GNOT swaps
- Token identifier distinction (
"ugnot"vs"gno.land/r/gnoland/wugnot")- Native token send amounts
- Automatic refund handling
Focus the checklist on actual router requirements: route formatting, slippage limits, deadline handling, and error cases.
🧹 Nitpick comments (1)
contract/r/gnoswap/gns/getter.gno (1)
142-148: Clarify the dual height getters to avoid API ambiguity.Line 146 and Line 253 both return
getCreatedHeight(), butGetEmissionCreatedHeightvsGetEmissionStartHeightsuggests different semantics. Consider explicitly marking one as an alias/deprecated for clearer external usage.✏️ Suggested doc/API clarification
-// GetEmissionStartHeight returns the block height when emission schedule was initialized. +// GetEmissionStartHeight is an alias of GetEmissionCreatedHeight. +// Deprecated: prefer GetEmissionCreatedHeight for clearer semantics. // // Returns: // - height: emission start block height func GetEmissionStartHeight() int64 { return getEmissionState().getCreatedHeight() }Also applies to: 248-254
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5697094f-28c6-4cb4-ac75-bb5100073d5e
📒 Files selected for processing (44)
contract/r/gnoswap/community_pool/community_pool.gnocontract/r/gnoswap/emission/distribution.gnocontract/r/gnoswap/emission/emission.gnocontract/r/gnoswap/emission/getter.gnocontract/r/gnoswap/gns/emission.gnocontract/r/gnoswap/gns/getter.gnocontract/r/gnoswap/gns/gns.gnocontract/r/gnoswap/gns/halving.gnocontract/r/gnoswap/gov/governance/README.mdcontract/r/gnoswap/gov/governance/proxy.gnocontract/r/gnoswap/gov/governance/upgrade.gnocontract/r/gnoswap/gov/governance/v1/README.mdcontract/r/gnoswap/gov/governance/v1/getter.gnocontract/r/gnoswap/gov/staker/README.mdcontract/r/gnoswap/gov/staker/proxy.gnocontract/r/gnoswap/gov/staker/tree.gnocontract/r/gnoswap/gov/staker/upgrade.gnocontract/r/gnoswap/gov/staker/v1/README.mdcontract/r/gnoswap/gov/xgns/xgns.gnocontract/r/gnoswap/launchpad/proxy.gnocontract/r/gnoswap/launchpad/upgrade.gnocontract/r/gnoswap/pool/upgrade.gnocontract/r/gnoswap/position/README.mdcontract/r/gnoswap/position/getter.gnocontract/r/gnoswap/position/upgrade.gnocontract/r/gnoswap/position/v1/README.mdcontract/r/gnoswap/protocol_fee/getter.gnocontract/r/gnoswap/protocol_fee/proxy.gnocontract/r/gnoswap/protocol_fee/upgrade.gnocontract/r/gnoswap/router/README.mdcontract/r/gnoswap/router/doc.gnocontract/r/gnoswap/router/proxy.gnocontract/r/gnoswap/router/upgrade.gnocontract/r/gnoswap/router/v1/doc.gnocontract/r/gnoswap/staker/README.mdcontract/r/gnoswap/staker/getter.gnocontract/r/gnoswap/staker/proxy.gnocontract/r/gnoswap/staker/tree.gnocontract/r/gnoswap/staker/upgrade.gnocontract/r/gnoswap/staker/v1/README.mdcontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/staker.gnodocs/position.mddocs/staker.md
💤 Files with no reviewable changes (2)
- docs/position.md
- contract/r/gnoswap/staker/v1/staker.gno
| // - height: block height when emission starts | ||
| // - timestamp: timestamp when emission starts |
There was a problem hiding this comment.
Parameter names in docs don’t match the function signature.
Use createdHeight / startTimestamp in the comment to avoid API confusion.
📝 Proposed doc fix
// Parameters:
-// - height: block height when emission starts
+// - createdHeight: block height when emission starts
//
-// - timestamp: timestamp when emission starts
+// - startTimestamp: timestamp when emission starts📝 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.
| // - height: block height when emission starts | |
| // - timestamp: timestamp when emission starts | |
| // - createdHeight: block height when emission starts | |
| // - startTimestamp: timestamp when emission starts |
| // CollectReward collects the rewards from the protocol fee contract based on the holdings of xGNS. | ||
| // | ||
| // Parameters: | ||
| // - cur: Pass `cross` as argument. | ||
| func CollectReward(cur realm) { | ||
| getImplementation().CollectReward() | ||
| } | ||
|
|
||
| // CollectRewardFromLaunchPad claims rewards from launchpad projects. | ||
| // CollectRewardFromLaunchPad collects the rewards from the protocol fee contract based on the holdings of xGNS in the launchpad contract (only callable by the launchpad contract). | ||
| // | ||
| // Parameters: | ||
| // - to: address to collect rewards for | ||
| // - cur: Pass `cross` as argument. | ||
| // - to: The address to delegate. | ||
| func CollectRewardFromLaunchPad(cur realm, to address) { |
There was a problem hiding this comment.
Reward source description is incomplete for CollectReward*.
Line 59 and Line 67 describe protocol-fee rewards only, but implementation-level behavior includes emission reward distribution as well. The proxy docs should reflect both components.
📝 Suggested wording update
-// CollectReward collects the rewards from the protocol fee contract based on the holdings of xGNS.
+// CollectReward collects claimable rewards for xGNS holders (including emission and protocol-fee rewards).
-// CollectRewardFromLaunchPad collects the rewards from the protocol fee contract based on the holdings of xGNS in the launchpad contract (only callable by the launchpad contract).
+// CollectRewardFromLaunchPad collects claimable launchpad-linked rewards (including emission and protocol-fee rewards) for a target address (only callable by the launchpad contract).📝 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.
| // CollectReward collects the rewards from the protocol fee contract based on the holdings of xGNS. | |
| // | |
| // Parameters: | |
| // - cur: Pass `cross` as argument. | |
| func CollectReward(cur realm) { | |
| getImplementation().CollectReward() | |
| } | |
| // CollectRewardFromLaunchPad claims rewards from launchpad projects. | |
| // CollectRewardFromLaunchPad collects the rewards from the protocol fee contract based on the holdings of xGNS in the launchpad contract (only callable by the launchpad contract). | |
| // | |
| // Parameters: | |
| // - to: address to collect rewards for | |
| // - cur: Pass `cross` as argument. | |
| // - to: The address to delegate. | |
| func CollectRewardFromLaunchPad(cur realm, to address) { | |
| // CollectReward collects claimable rewards for xGNS holders (including emission and protocol-fee rewards). | |
| // | |
| // Parameters: | |
| // - cur: Pass `cross` as argument. | |
| func CollectReward(cur realm) { | |
| getImplementation().CollectReward() | |
| } | |
| // CollectRewardFromLaunchPad collects claimable launchpad-linked rewards (including emission and protocol-fee rewards) for a target address (only callable by the launchpad contract). | |
| // | |
| // Parameters: | |
| // - cur: Pass `cross` as argument. | |
| // - to: The address to delegate. | |
| func CollectRewardFromLaunchPad(cur realm, to address) { |
| // ExactOutSwapRoute runs a exact-out direction swap through the route that offers the most favorable exchange rate. | ||
| // | ||
| // Parameters: | ||
| // - inputToken: path of input token | ||
| // - outputToken: path of output token | ||
| // - amountOut: exact output amount | ||
| // - routeArr: encoded route array | ||
| // - quoteArr: encoded quote array | ||
| // - amountInMax: maximum input amount | ||
| // - deadline: transaction deadline | ||
| // - referrer: referrer address for reward tracking | ||
| // - cur: Pass `cross` as argument. | ||
| // - inputToken: The path of the input token. | ||
| // - outputToken: The path of the output token. | ||
| // - amountOut: The amount of the output token. | ||
| // - routeArr: The route of the swap. | ||
| // - quoteArr: The share of each split of the route. | ||
| // - amountInMax: Limits the number of tokens while routing the swap. The maximum amount of tokens to pay. | ||
| // - deadline: The timestamp for transaction to be expired. | ||
| // - referrer: The referrer address for reward tracking. | ||
| // | ||
| // Returns: | ||
| // - string: actual input amount | ||
| // - string: actual output amount | ||
| // - tokenIn: The amount sent to the pool from the user (the input of the swap). | ||
| // - tokenOut: The amount sent to the user from the pool (the output of the swap). | ||
| func ExactOutSwapRoute(cur realm, inputToken string, outputToken string, amountOut string, routeArr string, quoteArr string, amountInMax string, deadline int64, referrer string) (string, string) { |
There was a problem hiding this comment.
Exact-out return docs currently miss router-fee netting behavior.
Line 57 and Line 77 imply users receive the full output amount, but exact-out in this module is net of router fee (user receives amount - routerFee). This should be explicit in the proxy docs to prevent integration/accounting mistakes.
📝 Suggested doc correction
// Returns:
-// - tokenIn: The amount sent to the pool from the user (the input of the swap).
-// - tokenOut: The amount sent to the user from the pool (the output of the swap).
+// - tokenIn: The amount paid by the user.
+// - tokenOut: The swap output amount; in exact-out flows, user-received amount is net of router fee.
// Returns:
-// - amountIn: actual input amount
-// - amountOut: actual output amount
+// - amountIn: actual input amount
+// - amountOut: swap output amount before/with router fee handling (document net user-received semantics explicitly)Also applies to: 63-79
1 participant
Summary
Notes
Summary by CodeRabbit
Release Notes
New Features
Documentation