refactor(rbac): unify contract address access #1087
Conversation
|
Warning Rate limit exceeded@junghoon-vans has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request migrates multiple smart contracts from owner-based authorization to RBAC role-based access control. Global owner variables are replaced with runtime RBAC role address lookups using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…duce new protocol fee helper test file.
Add local stakerAddr variable declarations in functions that use it: - CreateExternalIncentive: declare stakerAddr at function start - EndExternalIncentive: declare stakerAddr at function start - MintAndStake: declare stakerAddr at function start - getCurrentInfo: add missing access/prbac imports These functions were using stakerAddr without declaring it locally after the package-level variable was removed to support runtime address changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
contract/r/gnoswap/staker/v1/_helper_test.gno (1)
586-590: LGTM! Consider relocating for improved readability.The RBAC-based address initialization is correct and aligns with the PR's refactoring objectives. Go's package-level initialization handles the dependency order properly, so
adminAddrwill be available whenadminRealm(line 57) is initialized.However, for better code organization and readability, consider moving this block earlier in the file, ideally near lines 61-64 where similar RBAC-based addresses (
govGovernanceAddr,poolAddr, etc.) are already defined. This would group all role-based address initializations together and make the dependencies more apparent to readers.📋 Optional: Improve organization by grouping RBAC addresses
Move this block to appear right after line 64 to group all RBAC-derived addresses together:
govGovernanceAddr = access.MustGetAddress(prbac.ROLE_GOVERNANCE.String()) poolAddr = access.MustGetAddress(prbac.ROLE_POOL.String()) routerAddr = access.MustGetAddress(prbac.ROLE_ROUTER.String()) emissionAddr = access.MustGetAddress(prbac.ROLE_EMISSION.String()) + stakerAddr = access.MustGetAddress(prbac.ROLE_STAKER.String()) + adminAddr = access.MustGetAddress(prbac.ROLE_ADMIN.String()) + communityPoolAddr = access.MustGetAddress(prbac.ROLE_COMMUNITY_POOL.String()) posRealm = testing.NewCodeRealm("gno.land/r/gnoswap/position")Then remove lines 586-590.
contract/r/gnoswap/gnft/_helper_test.gno (1)
12-15: Consider initializing test variables in setup functions instead of at package level.While these unexported variables don't pose a security risk, initializing them within individual test setup functions would be more flexible and align with Go testing best practices. This allows tests to use different addresses if needed and makes dependencies more explicit.
💡 Alternative approach
Instead of package-level variables, consider initializing in each test or a shared setup function:
func setupTestContext() (address, *ownable.Ownable) { positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) owner := ownable.NewWithAddress(positionAddr) return positionAddr, owner }Then use in tests:
func TestSomething(t *testing.T) { positionAddr, owner := setupTestContext() // ... test logic ... }contract/r/gnoswap/gnft/gnft.gno (2)
95-96: Extract repeated position address resolution to a helper function.The pattern of retrieving
positionAddrand creating anownerinstance is duplicated acrossSetTokenURI,Mint, andBurn. Extracting this to a helper function would improve maintainability and reduce the risk of inconsistencies.🔎 Proposed helper function
// getPositionOwner retrieves the position contract address and creates an ownable instance func getPositionOwner() (address, *ownable.Ownable) { positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) owner := ownable.NewWithAddress(positionAddr) return positionAddr, owner }Then simplify the calling functions:
SetTokenURI:
func SetTokenURI(cur realm, tid grc721.TokenID, tURI grc721.TokenURI) (bool, error) { halt.AssertIsNotHaltedPosition() - - positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) - owner := ownable.NewWithAddress(positionAddr) - + _, owner := getPositionOwner() owner.AssertOwnedByPrevious() // ... }Mint:
func Mint(cur realm, to address, tid grc721.TokenID) grc721.TokenID { halt.AssertIsNotHaltedPosition() - - positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) - owner := ownable.NewWithAddress(positionAddr) + positionAddr, owner := getPositionOwner() owner.AssertOwnedByPrevious() // ... }Burn:
func Burn(cur realm, tid grc721.TokenID) { halt.AssertIsNotHaltedPosition() - - positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) - owner := ownable.NewWithAddress(positionAddr) - + _, owner := getPositionOwner() owner.AssertOwnedByPrevious() // ... }Also applies to: 221-222, 261-262
221-227: Simplify the minting logic to remove redundant variable.The code retrieves both
positionAddrandownerAddress(viaowner.Owner()), but sinceowneris constructed withpositionAddr, these variables hold the same value. Line 225 introduces unnecessary indirection that makes the code harder to follow.🔎 Proposed simplification
func Mint(cur realm, to address, tid grc721.TokenID) grc721.TokenID { halt.AssertIsNotHaltedPosition() positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) owner := ownable.NewWithAddress(positionAddr) owner.AssertOwnedByPrevious() - ownerAddress := owner.Owner() - checkErr(nft.Mint(positionAddr, tid)) // Store only the gradient parameters instead of full base64 SVG to reduce storage costs. // Parameters are converted to full SVG on read via TokenURI(). imageParams := genImageParams(generateRandInstance()) err := setTokenURI(tid, grc721.TokenURI(imageParams)) if err != nil { panic(err) } - checkErr(nft.TransferFrom(ownerAddress, to, tid)) + checkErr(nft.TransferFrom(positionAddr, to, tid)) return tid }This eliminates the redundant
ownerAddressvariable and makes the flow clearer: mint topositionAddr, then transfer frompositionAddrtoto.contract/r/gnoswap/protocol_fee/v1/_helper_test.gno (1)
8-12: Consider initialization timing for test addresses.These package-level address variables are initialized at declaration time, which means
access.MustGetAddresswill be called when the package loads. If the RBAC/access system isn't fully initialized at that point, this could cause test failures or panics.Consider moving these initializations into an
init()function or computing them lazily within each test function to ensure proper initialization order.🔎 Proposed refactor using init()
var ( - protocolFeeAddr = access.MustGetAddress(prabc.ROLE_PROTOCOL_FEE.String()) - devOpsAddr = access.MustGetAddress(prabc.ROLE_DEVOPS.String()) - govStakerAddr = access.MustGetAddress(prabc.ROLE_GOV_STAKER.String()) + protocolFeeAddr address + devOpsAddr address + govStakerAddr address ) + +func init() { + protocolFeeAddr = access.MustGetAddress(prabc.ROLE_PROTOCOL_FEE.String()) + devOpsAddr = access.MustGetAddress(prabc.ROLE_DEVOPS.String()) + govStakerAddr = access.MustGetAddress(prabc.ROLE_GOV_STAKER.String()) +}contract/r/gnoswap/staker/v1/getter.gno (1)
365-371: Repeated address resolution on every call.The
getCurrentInfofunction resolves six RBAC role addresses on every invocation. While correct, this approach adds overhead if this function is called frequently.If
getCurrentInfois called in hot paths or loops, consider caching these addresses at the package level (unexported) or accepting them as parameters to reduce repeated RBAC lookups.contract/r/gnoswap/staker/v1/external_incentive.gno (1)
249-257: Admin address resolved only for error message.The
adminAddris resolved viaaccess.MustGetAddressbut used only in the error message string. The actual authorization check happens earlier on line 249 usingaccess.IsAuthorized.This is acceptable, but note that if the ROLE_ADMIN address lookup fails (MustGetAddress panics), the error message won't be shown. Consider whether the error message needs the actual admin address or if a generic message would suffice.
🔎 Alternative approach to avoid unnecessary address resolution
// only refundee or admin can end incentive if !access.IsAuthorized(prbac.ROLE_ADMIN.String(), caller) && caller != incentiveResolver.Refundee() { - adminAddr := access.MustGetAddress(prbac.ROLE_ADMIN.String()) return nil, 0, makeErrorWithDetails( errNoPermission, ufmt.Sprintf( - "only refundee(%s) or admin(%s) can end incentive, but called from %s", - incentiveResolver.Refundee(), adminAddr.String(), caller, + "only refundee(%s) or admin can end incentive, but called from %s", + incentiveResolver.Refundee(), caller, ), ) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
contract/r/gnoswap/gnft/_helper_test.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/protocol_fee/v1/_helper_test.gnocontract/r/gnoswap/protocol_fee/v1/api.gnocontract/r/gnoswap/protocol_fee/v1/consts.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/staker/v1/_helper_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/mint_stake.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking.gnocontract/r/gnoswap/staker/v1/staker.gno
💤 Files with no reviewable changes (1)
- contract/r/gnoswap/protocol_fee/v1/consts.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/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/staker/v1/_helper_test.gnocontract/r/gnoswap/staker/v1/mint_stake.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee.gnocontract/r/gnoswap/staker/v1/staker.gnocontract/r/gnoswap/gnft/_helper_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/protocol_fee/v1/_helper_test.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/protocol_fee/v1/api.gno
🔇 Additional comments (14)
contract/r/gnoswap/staker/v1/staker.gno (4)
334-334: LGTM! Address resolution correctly moved to function scope.The refactoring successfully replaces global address variables with on-demand resolution via RBAC. This improves modularity and ensures addresses are always current at execution time.
469-469: LGTM! Community pool address resolution for penalty distribution.The address is resolved on-demand before transferring external reward penalties, ensuring the correct recipient address is used based on current RBAC configuration.
720-720: LGTM! Staker address resolution for NFT return.Address resolution happens at the point of use during unstaking, maintaining consistency with the refactored pattern across the codebase.
786-786: LGTM! Dynamic address resolution in ownership validation.The
hasTokenOwnershiphelper now resolves the staker address on-demand, allowing it to check if the owner is the staker contract without relying on global state.contract/r/gnoswap/staker/v1/protocol_fee_unstaking.gno (1)
45-47: LGTM! Protocol fee address resolution and transfer.The refactoring correctly resolves the protocol fee address on-demand before transferring fees and updating the ledger. The
crosskeyword is properly used for the realm-crossing transfer.contract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gno (2)
54-67: LGTM! DevOps distribution with on-demand address resolution.The function correctly resolves the DevOps address before distribution, updates accumulation and history, and performs the transfer. The refactoring maintains all necessary state updates while improving address management.
72-85: LGTM! GovStaker distribution with on-demand address resolution.The function mirrors the DevOps pattern, resolving the GovStaker address dynamically before performing distribution operations. This ensures consistency across the protocol fee distribution logic.
contract/r/gnoswap/staker/v1/mint_stake.gno (2)
57-57: LGTM! Staker address resolved early for multiple uses.The address is resolved once at the function start and reused for both the coin transfer (line 76) and the Mint recipient parameter (line 91), which is efficient and correct.
74-76: Coin transfer pattern is correct.Lines 74-76: The staker realm receives GNOT via
banker.OriginSend()at line 62, with validation confirming sufficient funds before theSendCoinscall. BankerTypeRealmSend provides full access to coins that the realm itself owns, including the ones sent with the transaction, making this pattern appropriate for wrapping GNOT to WUGNOT through the position contract.contract/r/gnoswap/staker/v1/external_incentive.gno (2)
39-39: LGTM! Staker address resolution for incentive creation.The address is resolved early and used multiple times throughout the function for token transfers, which is efficient and maintains consistency.
151-151: LGTM! Staker address resolution for incentive ending.The address is resolved at the function start and used for balance checks and refund calculations, following the same pattern as other refactored functions.
contract/r/gnoswap/protocol_fee/v1/api.gno (2)
58-64: LGTM! Protocol fee address resolution for balance queries.The function correctly resolves the protocol fee address on-demand before querying token balances. This ensures the API always reflects the current RBAC configuration.
101-107: LGTM! Consistent address resolution pattern.The
ApiGetUnrecordedBalancefunction follows the same pattern asApiGetActualBalance, resolving the address before computing discrepancies between recorded and actual balances.contract/r/gnoswap/protocol_fee/v1/protocol_fee.gno (1)
33-34: The refactoring to use dynamic address resolution via RBAC is properly implemented. TheROLE_PROTOCOL_FEErole is correctly defined incontract/p/gnoswap/rbac/types.gnoand the migration from the old global variable is complete. TheMustGetAddress()function properly panics when the role is not found, which is the correct pattern for Gno realms. All production code consistently uses this pattern, and tests appropriately useGetAddress()with error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/r/gnoswap/gnft/gnft.gno (1)
218-232: Refactor to mint directly to recipient address.The current implementation mints the NFT to the position contract address (line 222) and then immediately transfers it to the final recipient (line 232). This pattern introduces several issues:
Problems:
- Inefficiency: Two operations (mint + transfer) instead of one direct mint
- Reliability risk: If the transfer fails after minting, the token becomes stuck at the position address
- Increased gas costs: Extra operation increases transaction costs
- Unnecessary complexity: The intermediate state serves no functional purpose
Impact: The position contract temporarily owns every newly minted NFT, creating an unnecessary intermediate state that could be exploited if the position contract has any vulnerabilities or if the transfer operation fails.
🔎 Recommended refactor to mint directly
func Mint(cur realm, to address, tid grc721.TokenID) grc721.TokenID { halt.AssertIsNotHaltedPosition() caller := runtime.PreviousRealm().Address() access.AssertIsPosition(caller) - positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) - checkErr(nft.Mint(positionAddr, tid)) + checkErr(nft.Mint(to, tid)) // Store only the gradient parameters instead of full base64 SVG to reduce storage costs. // Parameters are converted to full SVG on read via TokenURI(). imageParams := genImageParams(generateRandInstance()) err := setTokenURI(tid, grc721.TokenURI(imageParams)) if err != nil { panic(err) } - checkErr(nft.TransferFrom(positionAddr, to, tid)) - return tid }
🧹 Nitpick comments (1)
contract/r/gnoswap/staker/v1/external_incentive.gno (1)
249-249: Consider defensive handling in error path.Calling
access.MustGetAddress()within an authorization error path could panic if the admin role is misconfigured, masking the underlying authorization failure. While this shouldn't occur in a properly configured system, it could complicate debugging during deployment or role misconfiguration scenarios.Consider either:
- Using a fallback pattern that handles missing admin addresses gracefully in error messages, or
- Accepting this pattern with the assumption that role configuration is validated at deployment
🔎 Alternative: Defensive error message construction
// only refundee or admin can end incentive if !access.IsAuthorized(prbac.ROLE_ADMIN.String(), caller) && caller != incentiveResolver.Refundee() { - adminAddr := access.MustGetAddress(prbac.ROLE_ADMIN.String()) return nil, 0, makeErrorWithDetails( errNoPermission, ufmt.Sprintf( - "only refundee(%s) or admin(%s) can end incentive, but called from %s", - incentiveResolver.Refundee(), adminAddr.String(), caller, + "only refundee(%s) or admin can end incentive, but called from %s", + incentiveResolver.Refundee(), caller, ), ) }This avoids the potential panic in the error path while still providing a clear error message.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contract/r/gnoswap/gnft/_helper_test.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/gnft/gnft_test.gnocontract/r/gnoswap/staker/v1/_helper_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/v1/mint_stake.gno
💤 Files with no reviewable changes (2)
- contract/r/gnoswap/staker/v1/getter_test.gno
- contract/r/gnoswap/staker/v1/getter.gno
🚧 Files skipped from review as they are similar to previous changes (3)
- contract/r/gnoswap/gnft/_helper_test.gno
- contract/r/gnoswap/staker/v1/_helper_test.gno
- contract/r/gnoswap/staker/v1/mint_stake.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/gnft/gnft_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/gnft/gnft.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). (48)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- 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/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/scenario/halt, contract/r/scenario/halt)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/gov/governance/v1, contract/r/gnoswap/gov/governance/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/rbac, contract/r/scenario/rbac)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/scenario/launchpad, contract/r/scenario/launchpad)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee, contract/r/gnoswap/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/scenario/gns, contract/r/scenario/gns)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/emission, contract/r/scenario/emission)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/router, contract/r/gnoswap/router)
- 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/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/pool, contract/r/gnoswap/pool)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/referral, contract/r/gnoswap/referral)
- GitHub Check: test-gnoswap (gnoswap/position, contract/r/gnoswap/position)
- GitHub Check: test-gnoswap (gnoswap/launchpad, contract/r/gnoswap/launchpad)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/halt, contract/r/gnoswap/halt)
- GitHub Check: test-gnoswap (gnoswap/gov/governance, contract/r/gnoswap/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/gnft, contract/r/gnoswap/gnft)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/r/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/xgns, contract/r/gnoswap/gov/xgns)
- GitHub Check: test-gnoswap (gnoswap/gns, contract/r/gnoswap/gns)
- GitHub Check: test-gnoswap (gnoswap/emission, contract/r/gnoswap/emission)
- GitHub Check: test-gnoswap (gnoswap/common, contract/r/gnoswap/common)
- GitHub Check: test-gnoswap (gnoswap/gov/staker, contract/r/gnoswap/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/gnsmath, contract/p/gnoswap/gnsmath)
- GitHub Check: test-gnoswap (gnoswap/access, contract/r/gnoswap/access)
- 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/int256, contract/p/gnoswap/int256)
- GitHub Check: test-gnoswap (gnoswap/version_manager, contract/p/gnoswap/version_manager)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/p/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/uint256, contract/p/gnoswap/uint256)
- GitHub Check: test-gnoswap (gnoswap/fuzz, contract/p/gnoswap/fuzz)
- GitHub Check: run-integration-test
🔇 Additional comments (7)
contract/r/gnoswap/staker/v1/external_incentive.gno (2)
54-54: LGTM: On-demand staker address retrieval.The pattern of retrieving the staker address dynamically via
access.MustGetAddress()aligns well with the PR's RBAC unification objective. The address is retrieved after all input validations and used immediately for token transfers, which is appropriate.
194-194: LGTM: Consistent address retrieval pattern.The on-demand retrieval of the staker address is consistent with the pattern established in
CreateExternalIncentiveand appropriately used for the balance check that follows.contract/r/gnoswap/gnft/gnft.gno (2)
256-257: LGTM - Access control properly migrated to RBAC pattern.The Burn function correctly validates the caller using the new RBAC pattern, consistent with SetTokenURI and Mint.
94-95: RBAC access control is properly implemented.The access control has been correctly migrated from owner-based to role-based. Lines 94-95 properly use
runtime.PreviousRealm().Address()to identify the caller andaccess.AssertIsPosition()to enforce role-based authorization using panic-based assertions, consistent with Gno security patterns. The RBAC system initializes during deployment viarbac/rbac.gno'sinit()function, which registers all default roles includingROLE_POSITIONmapped to the correct position contract address, and the access module properly maintains role-to-address mappings.contract/r/gnoswap/gnft/gnft_test.gno (3)
361-361: LGTM - Correct test isolation pattern.These tests correctly use
nft.Mint(positionAddr, ...)directly to create tokens without URIs, allowing the SetTokenURI functionality to be tested in isolation. The change fromowner.Owner()topositionAddrproperly reflects the RBAC migration.Also applies to: 372-372, 383-383
423-423: LGTM - Error message expectations correctly updated.The test assertions have been properly updated to expect the new RBAC-based error messages ("unauthorized: caller ... is not position") instead of the old owner-based messages ("ownable: caller is not owner"), maintaining consistency with the access control migration.
Also applies to: 435-435, 699-699, 756-756
1305-1397: LGTM - Comprehensive test updates for RBAC migration.All test cases in
Test_setTokenURIhave been systematically updated to usepositionAddrinstead of owner-based addresses, correctly reflecting the new position-centric authorization model. The test coverage remains comprehensive while aligning with the architectural changes.
junghoon-vans
force-pushed
the
unify-contract-address-access
branch
from
cecfc25 to
2333b56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contract/r/gnoswap/gns/_helper_test.gno (1)
17-17: Consider retrieving admin address at function call time for better test isolation.The package-level variable
adminAddris initialized when the test package loads, creating a dependency on RBAC initialization order. While the blank import on line 14 should ensure proper ordering, retrieving the address within the helper functions would provide better test isolation and make the dependency explicit.🔎 Proposed refactor for better test isolation
-var adminAddr = access.MustGetAddress(prabc.ROLE_ADMIN.String()) - func resetObject(t *testing.T) { t.Helper() resetGnsTokenObject(t) resetEmissionState(t) } func resetGnsTokenObject(t *testing.T) { t.Helper() token, privateLedger = grc20.NewToken("Gnoswap", "GNS", 6) - - privateLedger.Mint(adminAddr, INITIAL_MINT_AMOUNT) + adminAddr := access.MustGetAddress(prabc.ROLE_ADMIN.String()) + privateLedger.Mint(adminAddr, INITIAL_MINT_AMOUNT) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contract/r/gnoswap/gns/_helper_test.gnocontract/r/gnoswap/gns/gns.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/staker/v1/_helper_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gno
🚧 Files skipped from review as they are similar to previous changes (1)
- contract/r/gnoswap/protocol_fee/v1/protocol_fee_state.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/gns/gns.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/_helper_test.gnocontract/r/gnoswap/gns/_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). (48)
- GitHub Check: test-gnoswap (gnoswap/access, contract/r/gnoswap/access)
- GitHub Check: test-gnoswap (gnoswap/scenario/position, contract/r/scenario/position)
- GitHub Check: test-gnoswap (gnoswap/pool, contract/r/gnoswap/pool)
- GitHub Check: test-gnoswap (gnoswap/scenario/rbac, contract/r/scenario/rbac)
- GitHub Check: test-gnoswap (gnoswap/scenario/upgrade, contract/r/scenario/upgrade)
- GitHub Check: test-gnoswap (gnoswap/scenario/router, contract/r/scenario/router)
- GitHub Check: test-gnoswap (gnoswap/scenario/staker, contract/r/scenario/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/governance, contract/r/scenario/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/staker/v1, contract/r/gnoswap/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/gns, contract/r/scenario/gns)
- GitHub Check: test-gnoswap (gnoswap/scenario/protocol_fee, contract/r/scenario/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/referral, contract/r/gnoswap/referral)
- GitHub Check: test-gnoswap (gnoswap/scenario/pool, contract/r/scenario/pool)
- GitHub Check: test-gnoswap (gnoswap/router/v1, contract/r/gnoswap/router/v1)
- GitHub Check: test-gnoswap (gnoswap/staker, contract/r/gnoswap/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/gov/staker, contract/r/scenario/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/scenario/launchpad, contract/r/scenario/launchpad)
- GitHub Check: test-gnoswap (gnoswap/position/v1, contract/r/gnoswap/position/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/governance/v1, contract/r/gnoswap/gov/governance/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/halt, contract/r/scenario/halt)
- GitHub Check: test-gnoswap (gnoswap/protocol_fee, contract/r/gnoswap/protocol_fee)
- GitHub Check: test-gnoswap (gnoswap/version_manager, contract/p/gnoswap/version_manager)
- GitHub Check: test-gnoswap (gnoswap/launchpad/v1, contract/r/gnoswap/launchpad/v1)
- GitHub Check: test-gnoswap (gnoswap/scenario/emission, contract/r/scenario/emission)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/r/gnoswap/rbac)
- 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/test/fuzz, contract/r/gnoswap/test/fuzz)
- GitHub Check: test-gnoswap (gnoswap/gov/governance, contract/r/gnoswap/gov/governance)
- GitHub Check: test-gnoswap (gnoswap/router, contract/r/gnoswap/router)
- GitHub Check: test-gnoswap (gnoswap/position, contract/r/gnoswap/position)
- GitHub Check: test-gnoswap (gnoswap/launchpad, contract/r/gnoswap/launchpad)
- GitHub Check: test-gnoswap (gnoswap/gns, contract/r/gnoswap/gns)
- GitHub Check: test-gnoswap (gnoswap/pool/v1, contract/r/gnoswap/pool/v1)
- GitHub Check: test-gnoswap (gnoswap/gov/xgns, contract/r/gnoswap/gov/xgns)
- GitHub Check: test-gnoswap (gnoswap/gov/staker, contract/r/gnoswap/gov/staker)
- GitHub Check: test-gnoswap (gnoswap/rbac, contract/p/gnoswap/rbac)
- GitHub Check: test-gnoswap (gnoswap/community_pool, contract/r/gnoswap/community_pool)
- GitHub Check: test-gnoswap (gnoswap/gnsmath, contract/p/gnoswap/gnsmath)
- GitHub Check: test-gnoswap (gnoswap/emission, contract/r/gnoswap/emission)
- GitHub Check: test-gnoswap (gnoswap/gov/staker/v1, contract/r/gnoswap/gov/staker/v1)
- GitHub Check: test-gnoswap (gnoswap/store, contract/p/gnoswap/store)
- GitHub Check: test-gnoswap (gnoswap/fuzz, contract/p/gnoswap/fuzz)
- GitHub Check: test-gnoswap (gnoswap/gnft, contract/r/gnoswap/gnft)
- GitHub Check: test-gnoswap (gnoswap/uint256, contract/p/gnoswap/uint256)
- GitHub Check: test-gnoswap (gnoswap/common, contract/r/gnoswap/common)
- GitHub Check: test-gnoswap (gnoswap/int256, contract/p/gnoswap/int256)
- GitHub Check: run-integration-test
🔇 Additional comments (4)
contract/r/gnoswap/gns/gns.gno (1)
37-38: No action required. The initialization order is properly managed via the blank import ofrbacon line 16, which ensures all roles are populated in the access module beforegns.init()executes. TheMustGetAddress()call is safe and uses appropriate error handling (panic).contract/r/gnoswap/staker/v1/_helper_test.gno (1)
65-67: Fix initialization order: defineadminAddrbefore using it on line 57The code has a forward reference error. Line 57 uses
adminAddrintesting.NewUserRealm(adminAddr), butadminAddris not defined until line 66. In Go/Gno var() blocks, variables must be defined before use. Move theadminAddrdefinition (line 66) before theadminRealminitialization (line 57).Note:
stakerAddrandcommunityPoolAddrare unused and could be removed if not needed elsewhere in the test suite.Likely an incorrect or invalid review comment.
contract/r/gnoswap/staker/v1/external_incentive.gno (2)
249-256: LGTM - Error message enhancement.Retrieving the admin address for the error message provides clearer feedback to callers. The authorization check via
access.IsAuthorized(line 248) is the actual security gate, and ifMustGetAddressfails, the panic would still block unauthorized access.
54-55: No actionable issues identified. The code properly initializes RBAC roles at deployment via theinit()function, andaccess.MustGetAddress()correctly panics if a role is missing, preventing any fund transfer to invalid addresses.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contract/r/gnoswap/gnft/_helper_test.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/gnft/gnft_test.gnocontract/r/gnoswap/gns/_helper_test.gnocontract/r/gnoswap/gns/gns.gnocontract/r/gnoswap/protocol_fee/v1/api.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/staker/v1/getter.gno
💤 Files with no reviewable changes (1)
- contract/r/gnoswap/staker/v1/getter.gno
🚧 Files skipped from review as they are similar to previous changes (6)
- contract/r/gnoswap/protocol_fee/v1/api.gno
- contract/r/gnoswap/gns/gns.gno
- contract/r/gnoswap/gnft/gnft_test.gno
- contract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gno
- contract/r/gnoswap/gnft/_helper_test.gno
- contract/r/gnoswap/gns/_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/gnft/gnft.gno
🧠 Learnings (1)
📚 Learning: 2026-01-05T03:20:38.341Z
Learnt from: notJoon
Repo: gnoswap-labs/gnoswap PR: 1118
File: contract/r/gnoswap/access/access.gno:37-43
Timestamp: 2026-01-05T03:20:38.341Z
Learning: In the gnoswap codebase, enforce consistency where event attribute names use prevAddr to refer to the caller/previous realm address in emitted events, as this is the established convention. When reviewing or adding new contracts, prefer using prevAddr over caller for event fields; if a semantic renaming is ever considered, document the rationale and ensure all related events across contracts are updated to maintain consistency.
Applied to files:
contract/r/gnoswap/gnft/gnft.gno
🔇 Additional comments (3)
contract/r/gnoswap/gnft/gnft.gno (3)
16-16: LGTM: Global variable properly scoped.The unexported
nftglobal follows Gno best practices for realm state management with controlled access through exported functions.
78-80: LGTM: Access control correctly implemented.The position-based authorization properly replaces owner-based checks using runtime caller derivation and role assertion.
230-232: LGTM: Position-based authorization correctly applied.The Burn function properly implements the unified RBAC pattern, replacing owner-based checks with position-based authorization consistent with SetTokenURI and Mint.
| caller := runtime.PreviousRealm().Address() | ||
| access.AssertIsPosition(caller) | ||
|
|
||
| checkErr(nft.Mint(ownerAddress, tid)) | ||
| positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) | ||
| checkErr(nft.Mint(positionAddr, tid)) | ||
|
|
||
| // Store only the gradient parameters instead of full base64 SVG to reduce storage costs. | ||
| // Parameters are converted to full SVG on read via TokenURI(). | ||
| imageParams := genImageParamsString(generateRandInstance()) | ||
| checkErr(setTokenURI(tid, grc721.TokenURI(imageParams))) | ||
|
|
||
| checkErr(nft.TransferFrom(ownerAddress, to, tid)) | ||
| checkErr(nft.TransferFrom(positionAddr, to, tid)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, understand the file structure
echo "=== Repository structure ==="
git ls-files | grep -E '(gnft|position|access)' | head -20
# Check the exact implementation
echo -e "\n=== Full Mint function context ==="
fd -e gno -path '*/gnft/gnft.gno' --exec cat -n {} \; | head -250
# Look for the access control logic and role definitions
echo -e "\n=== Searching for ROLE_POSITION definition ==="
rg -n "ROLE_POSITION" -g '*.gno' -C 3
# Search for similar patterns in the codebase
echo -e "\n=== Searching for similar nft.Mint patterns ==="
rg -n 'nft\.Mint' -g '*.gno' -B 2 -A 2
# Check for position-related contracts
echo -e "\n=== Looking for position contract files ==="
fd -e gno | grep -i positionRepository: gnoswap-labs/gnoswap
Length of output: 50562
🏁 Script executed:
#!/bin/bash
# Verify the relationship between caller and positionAddr
echo "=== Checking AssertIsPosition function ==="
rg -n "AssertIsPosition" -g '*.gno' -B 3 -A 5
# Check access control module structure
echo -e "\n=== Checking access module for role management ==="
fd -e gno -path '*/access*' --exec basename {} \;
# Look for all uses of nft.TransferFrom in gnft context
echo -e "\n=== Searching for TransferFrom usage patterns ==="
rg -n "TransferFrom" -g '*gnft*.gno' -B 3 -A 1
# Check if MustGetAddress returns caller or separate position address
echo -e "\n=== Checking MustGetAddress usage ==="
rg -n "MustGetAddress.*ROLE_POSITION" -g '*.gno' -B 5 -A 5Repository: gnoswap-labs/gnoswap
Length of output: 19588
🏁 Script executed:
#!/bin/bash
# Get full AssertIsAuthorized implementation
echo "=== AssertIsAuthorized implementation ==="
rg -n "func AssertIsAuthorized" -g '*.gno' -A 10
# Check IsAuthorized to understand how role checking works
echo -e "\n=== IsAuthorized implementation ==="
rg -n "func IsAuthorized" -g '*.gno' -A 8
# Check if caller could be different from positionAddr
echo -e "\n=== Looking for caller vs address discrepancies ==="
rg -n "AssertIsPosition\(" -g '*.gno' -B 5 -A 5 | head -80
# See if there's documentation about the mint pattern
echo -e "\n=== Searching for comments about Mint pattern ==="
rg -n "nft\.Mint.*positionAddr" -g '*.gno' -B 5 -A 5Repository: gnoswap-labs/gnoswap
Length of output: 10807
🏁 Script executed:
#!/bin/bash
# Check the complete Mint function to see if there's any special handling
echo "=== Full Mint function context (lines 190-220) ==="
sed -n '190,220p' contract/r/gnoswap/gnft/gnft.gno
# Look for why the position contract needs intermediate ownership
echo -e "\n=== Position contract structure ==="
fd -path '*position*' -name '*.gno' | head -5
# Check if there's any event emission or logging around this pattern
echo -e "\n=== Event/emission patterns in Mint ==="
sed -n '190,220p' contract/r/gnoswap/gnft/gnft.gno | grep -i "chain\|emit\|event"Repository: gnoswap-labs/gnoswap
Length of output: 1277
Optimize address lookup in Mint function.
After access.AssertIsPosition(caller) on line 201 validates that caller is authorized for ROLE_POSITION, the caller address is guaranteed to equal the position contract address stored in the access registry. Line 203's lookup of positionAddr is therefore redundant and can be eliminated by using caller directly:
func Mint(cur realm, to address, tid grc721.TokenID) grc721.TokenID {
halt.AssertIsNotHaltedPosition()
caller := runtime.PreviousRealm().Address()
access.AssertIsPosition(caller)
// caller already verified as position contract, use it directly
checkErr(nft.Mint(caller, tid))
imageParams := genImageParamsString(generateRandInstance())
checkErr(setTokenURI(tid, grc721.TokenURI(imageParams)))
checkErr(nft.TransferFrom(caller, to, tid))
return tid
}The mint-then-transfer pattern is intentional (creates token at position, sets metadata, transfers to recipient), but eliminating the redundant lookup removes unnecessary storage access and clarifies intent.
Signed-off-by: √ans <[email protected]>
junghoon-vans
force-pushed
the
unify-contract-address-access
branch
from
c3479d6 to
16fb624
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contract/r/gnoswap/gov/staker/v1/staker_reward.gno (1)
176-204: Consider usingcallerdirectly after launchpad assertion.Line 185 retrieves the launchpad address via
access.MustGetAddress(), but line 183 already verified thatcalleris the launchpad. Since the assertion guaranteescallerholds the launchpad address, you could usecallerdirectly on lines 194 and 202 instead of retrieving it again.♻️ Optional refactor to eliminate redundant lookup
func (gs *govStakerV1) SetAmountByProjectWallet(addr address, amount int64, add bool) { halt.AssertIsNotHaltedGovStaker() halt.AssertIsNotHaltedWithdraw() caller := runtime.PreviousRealm().Address() currentTimestamp := time.Now().Unix() access.AssertIsLaunchpad(caller) - launchpadAddr := access.MustGetAddress(prbac.ROLE_LAUNCHPAD.String()) - if add { // Add stake for the project wallet and mint xGNS to launchpad err := gs.addStakeFromLaunchpad(addr.String(), amount, currentTimestamp) if err != nil { panic(err) } - xgns.Mint(cross, launchpadAddr, amount) + xgns.Mint(cross, caller, amount) } else { // Remove stake for the project wallet and burn xGNS from launchpad err := gs.removeStakeFromLaunchpad(addr.String(), amount, currentTimestamp) if err != nil { panic(err) } - xgns.Burn(cross, launchpadAddr, amount) + xgns.Burn(cross, caller, amount) } }contract/r/gnoswap/gnft/gnft.gno (1)
200-211: Consider simplifying the mint-then-transfer pattern.The current implementation:
- Verifies caller is the position contract
- Retrieves position address from RBAC
- Mints NFT to position address
- Immediately transfers to the recipient
Since the caller is already verified to be the position contract, this creates unnecessary complexity with two operations (mint + transfer) when a direct mint to the recipient would suffice. The mint-then-transfer pattern also doubles gas costs and emits redundant events.
♻️ Proposed simplification
func Mint(cur realm, to address, tid grc721.TokenID) grc721.TokenID { halt.AssertIsNotHaltedPosition() caller := runtime.PreviousRealm().Address() access.AssertIsPosition(caller) - positionAddr := access.MustGetAddress(prabc.ROLE_POSITION.String()) - checkErr(nft.Mint(positionAddr, tid)) + checkErr(nft.Mint(to, tid)) // Store only the gradient parameters instead of full base64 SVG to reduce storage costs. // Parameters are converted to full SVG on read via TokenURI(). imageParams := genImageParamsString(generateRandInstance()) checkErr(setTokenURI(tid, grc721.TokenURI(imageParams))) - checkErr(nft.TransferFrom(positionAddr, to, tid)) - return tid }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
contract/r/gnoswap/gnft/_helper_test.gnocontract/r/gnoswap/gnft/assert_test.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/gnft/gnft_test.gnocontract/r/gnoswap/gns/_helper_test.gnocontract/r/gnoswap/gns/gns.gnocontract/r/gnoswap/gov/staker/v1/staker_reward.gnocontract/r/gnoswap/position/v1/position.gnocontract/r/gnoswap/protocol_fee/v1/_helper_test.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee.gnocontract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gnocontract/r/gnoswap/staker/v1/_helper_test.gnocontract/r/gnoswap/staker/v1/external_incentive.gnocontract/r/gnoswap/staker/v1/getter.gnocontract/r/gnoswap/staker/v1/getter_test.gnocontract/r/gnoswap/staker/v1/mint_stake.gnocontract/r/gnoswap/staker/v1/protocol_fee_unstaking.gnocontract/r/gnoswap/staker/v1/staker.gno
💤 Files with no reviewable changes (3)
- contract/r/gnoswap/gnft/assert_test.gno
- contract/r/gnoswap/staker/v1/getter_test.gno
- contract/r/gnoswap/staker/v1/getter.gno
🚧 Files skipped from review as they are similar to previous changes (10)
- contract/r/gnoswap/protocol_fee/v1/protocol_fee_state.gno
- contract/r/gnoswap/staker/v1/external_incentive.gno
- contract/r/gnoswap/protocol_fee/v1/_helper_test.gno
- contract/r/gnoswap/gnft/_helper_test.gno
- contract/r/gnoswap/staker/v1/mint_stake.gno
- contract/r/gnoswap/gns/gns.gno
- contract/r/gnoswap/staker/v1/staker.gno
- contract/r/gnoswap/protocol_fee/v1/protocol_fee.gno
- contract/r/gnoswap/staker/v1/protocol_fee_unstaking.gno
- contract/r/gnoswap/staker/v1/_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/position/v1/position.gnocontract/r/gnoswap/gns/_helper_test.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/gnft/gnft_test.gnocontract/r/gnoswap/gov/staker/v1/staker_reward.gno
🧠 Learnings (1)
📚 Learning: 2026-01-05T03:20:38.341Z
Learnt from: notJoon
Repo: gnoswap-labs/gnoswap PR: 1118
File: contract/r/gnoswap/access/access.gno:37-43
Timestamp: 2026-01-05T03:20:38.341Z
Learning: In the gnoswap codebase, enforce consistency where event attribute names use prevAddr to refer to the caller/previous realm address in emitted events, as this is the established convention. When reviewing or adding new contracts, prefer using prevAddr over caller for event fields; if a semantic renaming is ever considered, document the rationale and ensure all related events across contracts are updated to maintain consistency.
Applied to files:
contract/r/gnoswap/position/v1/position.gnocontract/r/gnoswap/gns/_helper_test.gnocontract/r/gnoswap/gnft/gnft.gnocontract/r/gnoswap/gnft/gnft_test.gnocontract/r/gnoswap/gov/staker/v1/staker_reward.gno
🔇 Additional comments (7)
contract/r/gnoswap/position/v1/position.gno (1)
474-495: LGTM! RBAC migration improves address management.The migration from direct address lookups to RBAC role-based retrieval (
access.MustGetAddress) centralizes address management and improves maintainability. The use ofMustGetAddresscorrectly enforces fail-fast behavior if roles are not configured, which is appropriate for these critical infrastructure addresses.The token transfer logic remains unchanged:
- poolAddr (line 474): Used as the source for token transfers to the caller and protocol fee recipient
- protocolFeeAddr (line 488): Used as the recipient for withdrawal fees
Note: Ensure that
ROLE_POOLandROLE_PROTOCOL_FEEare properly configured in the RBAC system before deployment, asMustGetAddresswill panic if these roles are not found.contract/r/gnoswap/gnft/gnft.gno (2)
79-80: LGTM: Position-based authorization implemented correctly.The migration from owner-based to position-based authorization is properly implemented using
runtime.PreviousRealm().Address()andaccess.AssertIsPosition().
231-232: LGTM: Position-based authorization for Burn.The authorization check correctly migrates from owner-based to position-based access control.
contract/r/gnoswap/gnft/gnft_test.gno (2)
423-423: LGTM: Error messages correctly updated to position-based authorization.The expected error messages have been properly updated from owner-based ("ownable: caller is not owner") to position-based ("unauthorized: caller ... is not position"), consistent with the RBAC migration.
Also applies to: 435-435, 699-699, 757-757
1320-1394: Verify positionAddr definition before using in test cases.These test cases use
positionAddrextensively, but as noted in the earlier comment, this variable is not defined in the test file. Once the missing declaration is added (see previous comment), these test cases should work correctly.The test logic correctly migrates from owner-based to position-based semantics, which aligns with the RBAC refactor objectives.
contract/r/gnoswap/gns/_helper_test.gno (2)
31-31: LGTM: Clean migration to RBAC-based address.The minting operation correctly uses
adminAddrretrieved via RBAC instead of the deprecated ownable pattern. This aligns with the PR objective to unify contract address access through role-based lookups.
17-18: No issue. The package-level variable initialization is safe.In Gno, all import statements (including blank imports) complete their
init()functions before the importing package's package-level variables are initialized. The blank import at line 14 ensuresrbac.init()runs first, which populatesroleAddressesin the access package viaSetRoleAddress()calls. By the time line 17 executesMustGetAddress(), the role will already exist in the map.
| // Mint token without URI by directly calling nft.Mint | ||
| ownerAddress := owner.Owner() | ||
| checkErr(nft.Mint(ownerAddress, tid(tokenId))) | ||
| checkErr(nft.Mint(positionAddr, tid(tokenId))) |
There was a problem hiding this comment.
[CRITICAL] Undefined variable: positionAddr.
The test file references positionAddr in multiple locations (lines 361, 372, 383, and throughout the Test_setTokenURI test cases starting at line 1320), but this variable is not defined in the test file's variable declarations (lines 20-32).
Problem: This will cause compilation failures as positionAddr is undefined.
Fix: Add the missing variable declaration at the top of the test file.
🐛 Proposed fix
Add this declaration in the variable block at lines 20-32:
var (
positionPath = "gno.land/r/gnoswap/position"
positionRealm = testing.NewCodeRealm(positionPath)
+ positionAddr = positionRealm.Address()
stakerPath = "gno.land/r/gnoswap/staker"
stakerRealm = testing.NewCodeRealm(stakerPath)Also applies to: 372-372, 383-383
|
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.