feat: add function to withdraw and check delegator rewards #3088
Conversation
fbac
added
the
stateful-precompile
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the ZetaChain application, particularly enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
fbac
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3088 +/- ##
===========================================
- Coverage 62.85% 62.55% -0.30%
===========================================
Files 424 427 +3
Lines 30138 30405 +267
===========================================
+ Hits 18942 19019 +77
- Misses 10358 10533 +175
- Partials 838 853 +15
|
fbac
force-pushed
the
feat/claim-zrc20-rewards
branch
3 times, most recently
from
654e153
to
f2e421b
Compare
fbac
force-pushed
the
feat/claim-zrc20-rewards
branch
from
f2e421b
to
6afbaec
Compare
fbac
force-pushed
the
feat/claim-zrc20-rewards
branch
from
910c75c
to
af1360f
Compare
fbac
force-pushed
the
feat/claim-zrc20-rewards
branch
from
8eac446
to
3502cea
Compare
fbac
force-pushed
the
feat/claim-zrc20-rewards
branch
from
7d3383b
to
d95dd4b
Compare
fbac
requested review from
brewmaster012,
kingpinXD,
ws4charlie,
skosito and
swift1337
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🧹 Outside diff range and nitpick comments (59)
precompiles/staking/const.go (1)
25-29: Consider adding method documentation.The query method constants are well-organized and properly named. The addition of
GetRewardsMethodNameandGetValidatorsMethodNamealigns with the PR objectives for delegator reward functionality.Consider adding documentation comments for each method constant to describe their purpose and any specific requirements or behaviors.
Example documentation format:
// Query methods. +// GetAllValidatorsMethodName is the method name for retrieving all validators GetAllValidatorsMethodName = "getAllValidators" +// GetSharesMethodName is the method name for retrieving delegation shares GetSharesMethodName = "getShares" +// GetRewardsMethodName is the method name for retrieving delegator rewards for a validator GetRewardsMethodName = "getRewards" +// GetValidatorsMethodName is the method name for retrieving all validators a delegator has delegated to GetValidatorsMethodName = "getDelegatorValidators"precompiles/precompiles_test.go (1)
28-35: Consider adding distribution-specific test cases.Given that this PR introduces delegator rewards functionality, consider adding test cases that specifically verify:
- Contract initialization with distribution-related parameters
- Proper handling of distribution keeper in the contract context
- Error cases for invalid distribution keeper scenarios
Example test structure:
func Test_StatefulContractsDistribution(t *testing.T) { k, ctx, sdkk, _ := keeper.FungibleKeeper(t) contracts := StatefulContracts( k, &sdkk.StakingKeeper, sdkk.BankKeeper, sdkk.DistributionKeeper, appCodec, gasConfig, ) // Test distribution-specific contract initialization for _, customContractFn := range contracts { contract := customContractFn(ctx, ethparams.Rules{}) // Verify distribution-related methods are properly initialized // Add assertions for reward-related functionality } // Test with nil distribution keeper // Test with invalid distribution keeper state }precompiles/staking/method_get_all_validators_test.go (2)
22-26: LGTM! Consider enhancing test readability.The switch to
stkContractABIandstkContract.Runis correct. However, the test readability could be improved by extracting the common contract execution logic into a helper function.Consider refactoring like this:
+func (s *PrecompileTestSuite) executeGetAllValidators() ([]byte, error) { + methodID := s.stkContractABI.Methods[GetAllValidatorsMethodName] + s.mockVMContract.Input = methodID.ID + return s.stkContract.Run(s.mockEVM, s.mockVMContract, false) +} func Test_GetAllValidators(t *testing.T) { t.Run("should return empty array if validators not set", func(t *testing.T) { // ... setup code ... - methodID := s.stkContractABI.Methods[GetAllValidatorsMethodName] - s.mockVMContract.Input = methodID.ID - validators, err := s.stkContract.Run(s.mockEVM, s.mockVMContract, false) + validators, err := s.executeGetAllValidators()
40-47: LGTM! Consider improving test data generation.The switch to staking contract and the use of deterministic random number generator is good. However, the test could benefit from more explicit validator data generation.
Consider creating a helper function that generates validators with specific attributes for better test clarity:
+func createTestValidator(t *testing.T) stakingtypes.Validator { + r := rand.New(rand.NewSource(42)) + validator := sample.Validator(t, r) + // Optionally set specific attributes for testing + return validator +} t.Run("should return validators if set", func(t *testing.T) { // ARRANGE s := newTestSuite(t) methodID := s.stkContractABI.Methods[GetAllValidatorsMethodName] s.mockVMContract.Input = methodID.ID - r := rand.New(rand.NewSource(42)) - validator := sample.Validator(t, r) + validator := createTestValidator(t) s.sdkKeepers.StakingKeeper.SetValidator(s.ctx, validator)e2e/contracts/testdistribute/TestDistribute.sol (2)
16-28: Consider memory optimization and parameter validationWhile the interface functions are well-structured, consider these improvements:
- Use
calldataforvalidatorparameter inclaimRewardsandgetRewardsto optimize gas usage- Document parameter validation responsibilities (e.g., empty validator string, zero address delegator)
function claimRewards( address delegator, - string memory validator + string calldata validator ) external returns (bool success); function getRewards( address delegator, - string memory validator + string calldata validator ) external view returns (DecCoin[] calldata rewards);
46-64: Consider flexible contract address configurationThe wrapper functions are well-implemented, but the hardcoded distribute contract address (0x66) might cause issues across different networks or during testing.
Consider:
- Making the contract address configurable via constructor
- Adding an admin function to update the address if needed
- Adding events to track address changes
Example implementation:
contract TestDistribute { address public distrAddress; IDistribute public distr; event DistrAddressUpdated(address indexed oldAddr, address indexed newAddr); constructor(address _distrAddress) { distrAddress = _distrAddress; distr = IDistribute(_distrAddress); } }precompiles/staking/method_get_validators.go (2)
38-48: Add context timeout handlingThe querier operation could potentially be long-running. Consider adding context timeout handling for better reliability.
Consider wrapping the context with a timeout:
+ queryCtx, cancel := context.WithTimeout(ctx.Context(), 10*time.Second) + defer cancel() + res, err := dstrQuerier.DelegatorValidators( - ctx, + sdk.WrapSDKContext(queryCtx), &distrtypes.QueryDelegatorValidatorsRequest{ DelegatorAddress: delegatorCosmosAddr.String(), }, )
50-52: Enhance comment clarity for empty validator caseThe current comment could be more descriptive about the business logic behind returning an empty array.
Consider updating the comment to:
- // Return immediately, no need to check the slice. - // If there are no validators we simply return an empty array to calling contracts. + // Return the validators list without additional validation. + // An empty array is returned when the delegator has no validators, + // allowing calling contracts to handle this as a valid state.cmd/zetae2e/local/precompiles.go (2)
37-51: Enhance error handling and documentationWhile the initialization is functionally correct, consider these improvements:
- Add more context to the error return
- Document security considerations for private key handling
- Simplify single-element slice initialization
Consider applying these changes:
// Initialize a ZetaTxServer with the precompile user account. -// It's needed to send messages on behalf of the precompile user. +// It's needed to send messages on behalf of the precompile user. +// Security: Private keys are only used in test environments. zetaTxServer, err := txserver.NewZetaTxServer( conf.RPCs.ZetaCoreRPC, - []string{ - sdk.AccAddress(conf.AdditionalAccounts.UserPrecompile.EVMAddress().Bytes()).String(), - }, - []string{ - conf.AdditionalAccounts.UserPrecompile.RawPrivateKey.String(), - }, + []string{sdk.AccAddress(conf.AdditionalAccounts.UserPrecompile.EVMAddress().Bytes()).String()}, + []string{conf.AdditionalAccounts.UserPrecompile.RawPrivateKey.String()}, conf.ZetaChainID, ) if err != nil { - return err + return fmt.Errorf("failed to initialize ZetaTxServer: %w", err) }
59-59: Consider using a named constant for the token amountWhile using scientific notation (1e7) improves readability, consider declaring this as a named constant to document its purpose and make future adjustments easier.
Consider adding at the package level:
+const ( + // TestERC20Amount represents the amount of ERC20 tokens to send in precompile tests + TestERC20Amount = 1e7 +) // In the function: -txERC20Send := deployerRunner.SendERC20OnEvm(account.EVMAddress(), 1e7) +txERC20Send := deployerRunner.SendERC20OnEvm(account.EVMAddress(), TestERC20Amount)precompiles/staking/method_get_validators_test.go (3)
26-27: Fix incorrect commentThe comment refers to "getRewards" but the test is actually calling "getValidators".
- // Call getRewards. + // Call getValidators.
19-20: Consider extracting magic numberThe random seed (42) should be defined as a constant at the package level for better maintainability.
+const ( + defaultTestSeed = 42 +) + - validator := sample.Validator(t, rand.New(rand.NewSource(42))) + validator := sample.Validator(t, rand.New(rand.NewSource(defaultTestSeed)))
72-73: Fix incorrect commentSimilar to the first test case, the comment refers to "getRewards" instead of "getValidators".
- // Call getRewards. + // Call getValidators.precompiles/staking/method_get_rewards_test.go (2)
16-43: Consider extracting common setup code into helper functions.The validator creation and contract method setup appear in both test cases. Consider extracting these into helper functions to improve maintainability and reduce duplication.
+func setupValidatorAndStaker(t *testing.T, s *PrecompileTestSuite) (*stakingtypes.Validator, common.Address) { + validator := sample.Validator(t, rand.New(rand.NewSource(42))) + s.sdkKeepers.StakingKeeper.SetValidator(s.ctx, validator) + stakerEVMAddr := sample.EthAddress() + return validator, stakerEVMAddr +} + +func callGetRewards(t *testing.T, s *PrecompileTestSuite, stakerEVMAddr common.Address, validatorAddr string) ([]interface{}, error) { + getRewardsMethod := s.stkContractABI.Methods[GetRewardsMethodName] + s.mockVMContract.Input = packInputArgs( + t, + getRewardsMethod, + []interface{}{stakerEVMAddr, validatorAddr}..., + ) + bytes, err := s.stkContract.Run(s.mockEVM, s.mockVMContract, false) + if err != nil { + return nil, err + } + return getRewardsMethod.Outputs.Unpack(bytes) +}
99-99: Remove commented debug print statement.Debug print statements should be removed before committing the code.
-//fmt.Println("getRewards response: ", res)precompiles/staking/method_get_shares_test.go (1)
59-67: Enhance error test cases with specific assertionsWhile the error test cases cover various invalid scenarios, they could be improved in several ways:
- Assert specific error messages instead of just checking for error presence
- Consider using table-driven tests to reduce code duplication
- Use consistent random seed initialization across test cases that need it
Here's a suggested refactor using table-driven tests:
func Test_GetShares(t *testing.T) { tests := []struct { name string args []interface{} wantErr string }{ { name: "wrong args amount", args: []interface{}{sample.Bech32AccAddress()}, wantErr: "invalid number of arguments", }, { name: "invalid staker arg", args: []interface{}{42, sample.Validator(t, rand.New(rand.NewSource(42))).OperatorAddress}, wantErr: "invalid staker address", }, // Add other test cases... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := newTestSuite(t) methodID := s.stkContractABI.Methods[GetSharesMethodName] _, err := s.stkContract.GetShares(s.ctx, &methodID, tt.args) require.Error(t, err) require.Contains(t, err.Error(), tt.wantErr) }) } }Also applies to: 74-83, 89-98, 104-114
precompiles/staking/logs.go (1)
Line range hint
1-179: Consider refactoring to reduce code duplicationThe logging functions follow a consistent pattern but contain duplicated logic. Consider introducing a helper function to handle the common logging pattern:
+func (c *Contract) addEventLog( + ctx sdk.Context, + stateDB vm.StateDB, + eventName string, + indexedParams [][]interface{}, + amount *big.Int, +) error { + event := c.Abi().Events[eventName] + topics, err := logs.MakeTopics(event, indexedParams...) + if err != nil { + return err + } + + data, err := logs.PackArguments([]logs.Argument{ + {Type: "uint256", Value: amount}, + }) + if err != nil { + return err + } + + logs.AddLog(ctx, c.Address(), stateDB, topics, data) + return nil +}This would simplify the implementation of individual logging functions. For example:
func (c *Contract) addClaimRewardsLog(...) error { return c.addEventLog( ctx, stateDB, ClaimRewardsEventName, [][]interface{}{{delegator}, {zrc20Token}}, amount, ) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 157-167: precompiles/staking/logs.go#L157-L167
Added lines #L157 - L167 were not covered by tests
[warning] 169-174: precompiles/staking/logs.go#L169-L174
Added lines #L169 - L174 were not covered by tests
[warning] 176-178: precompiles/staking/logs.go#L176-L178
Added lines #L176 - L178 were not covered by testsprecompiles/staking/IStaking.sol (2)
144-166: Enhance function documentation for better clarityWhile the function signatures are well-structured, consider enhancing the documentation:
For
claimRewards:
- Add
@param delegatordescription- Clarify if there are any restrictions on who can call this function
- Document any potential failure conditions
For
getDelegatorValidators:
- Remove
@devtag as this is a public interface- Add details about the returned validator format
For
getRewards:
- Add information about potential empty returns
- Clarify if rewards are automatically claimed or just viewed
Example improvement for
claimRewards:/// @notice Claim ZRC20 staking rewards from a specific validator. /// @param delegator The address of the delegator claiming rewards /// @param validator The validator address to claim rewards from /// @return success Boolean indicating whether the claim was successful /// @dev Only the delegator or an authorized contract can call this function
Solidity version inconsistency detected across the codebase
The codebase currently uses three different Solidity versions:
- 0.8.10 (5 files)
- ^0.8.7 (15 files)
- ^0.8.26 (5 files, including this interface)
This version disparity could lead to:
- Inconsistent behavior due to different compiler optimizations
- Security implications from varying language features
- Maintenance overhead when dealing with version-specific syntax
Recommend standardizing to a single version across all Solidity files, preferably ^0.8.7 which is most widely used in the codebase, unless there are specific features from 0.8.26 that are required.
🔗 Analysis chain
Line range hint
2-2: Verify Solidity version compatibilityThe interface uses Solidity ^0.8.26. While this version includes important security features, verify that it aligns with the rest of the codebase and deployment requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Solidity version usage across the codebase echo "Checking Solidity version patterns..." rg -g "*.sol" "pragma solidity" --no-filename | sort | uniq -cLength of output: 246
precompiles/staking/IStaking.abi (1)
216-234: Consider pagination for getDelegatorValidators.The function returns an unbounded array of validators, which could potentially cause out-of-gas errors if a delegator has staked with many validators.
Consider implementing pagination to handle large validator sets efficiently:
- function getDelegatorValidators(address delegator) external view returns (string[] memory validators); + function getDelegatorValidators( + address delegator, + uint256 offset, + uint256 limit + ) external view returns ( + string[] memory validators, + uint256 total + );e2e/e2etests/test_precompiles_bank_through_contract.go (1)
20-29: Consider package-level constants for monetary valuesWhile the variable grouping improves readability, consider:
- Moving monetary constants to package level with descriptive names
- Using more descriptive names that indicate the unit (e.g.,
oneThousandTokens)+const ( + TokensOneThousand = 1000 + TokensOneThousandOne = 1001 + TokensFiveHundred = 500 + TokensFiveHundredOne = 501 +) var ( spender = r.EVMAddress() bankAddress = bank.ContractAddress zrc20Address = r.ERC20ZRC20Addr - oneThousand = big.NewInt(1e3) - oneThousandOne = big.NewInt(1001) - fiveHundred = big.NewInt(500) - fiveHundredOne = big.NewInt(501) + oneThousand = big.NewInt(TokensOneThousand) + oneThousandOne = big.NewInt(TokensOneThousandOne) + fiveHundred = big.NewInt(TokensFiveHundred) + fiveHundredOne = big.NewInt(TokensFiveHundredOne) zero = big.NewInt(0) )precompiles/staking/method_distribute_test.go (2)
113-114: Improve error handling in testsThe current error checking mechanism using string contains is fragile and may break if error messages change. Consider using error types or constants:
// In the main package +const ( + ErrInvalidAllowanceMsg = "invalid allowance" + ErrInvalidTokenAmountMsg = "invalid token amount" +) // In tests -require.Contains(t, err.Error(), "invalid allowance") +require.ErrorIs(t, err, ErrInvalidAllowance) // or +require.Contains(t, err.Error(), ErrInvalidAllowanceMsg)Also applies to: 156-158, 199-201, 241-243
Line range hint
77-83: Refactor: Extract common verification logicThe result unpacking and verification logic is duplicated across test cases. Consider extracting this into a helper function:
func (s *testSuite) verifyDistributeResult(t *testing.T, success []byte, expectedOK bool) { res, err := s.distributeMethod.Outputs.Unpack(success) require.NoError(t, err) ok := res[0].(bool) require.Equal(t, expectedOK, ok) }Also applies to: 117-123, 160-166, 203-209, 245-251
precompiles/staking/IStaking.json (2)
217-235: Consider implementing pagination for scalability.While the current implementation works, returning all validators in a single call might become inefficient as the number of validators grows. Consider implementing pagination to handle large validator sets efficiently.
236-271: Consider implementing caching for frequently accessed reward data.The function correctly returns reward information using the
DecCoinstruct. However, for frequently accessed reward data, consider implementing a caching strategy to reduce computational overhead and improve response times.e2e/contracts/testdistribute/TestDistribute.json (1)
4-5: Consider removing unnecessary payable modifier from fallback function.The fallback function is marked as payable, but based on the contract's purpose (managing delegator rewards), it might not need to receive ETH directly. Consider removing the payable modifier unless there's a specific requirement for the contract to receive ETH.
- "stateMutability": "payable", + "stateMutability": "nonpayable",precompiles/staking/method_unstake_test.go (2)
Line range hint
48-289: Re-enable comprehensive test coverageThe commented-out test cases cover essential scenarios that should be validated:
- Read-only method validation
- Non-existent validator handling
- Successful unstaking flow
- Caller authorization
- Previous staking requirement
- Input validation
- Address format validation
These test cases should be uncommented and updated to use the new contract references (
stkContractandstkContractABI).
Line range hint
1-289: Add test coverage for new delegator reward functionsThe PR objectives mention new functions (
getDelegatorValidators,getRewards,claimRewards) that require test coverage. Please add test cases for:
getDelegatorValidators:
- Empty validator list
- Multiple validators
- Invalid delegator address
getRewards:
- Zero rewards
- Multiple rewards
- Invalid validator/delegator
claimRewards:
- Successful claim
- Authorization checks
- Zero rewards scenario
Would you like me to help generate the test cases for these new functions?
precompiles/staking/method_stake_test.go (1)
Line range hint
1-260: Add test cases for new delegator reward functionsThe test file focuses on staking functionality but lacks test coverage for the new delegator reward functions mentioned in the PR objectives:
getDelegatorValidatorsgetRewardsclaimRewardsThese new functions require comprehensive test coverage including:
- Success scenarios
- Error cases
- Permission checks
- Input validation
- Integration with the distribution keeper
Would you like me to help generate test cases for the new delegator reward functions?
precompiles/staking/method_move_stake_test.go (1)
Line range hint
71-394: Consider managing commented test casesThe file contains extensive commented-out test cases that appear to provide comprehensive coverage for the
MoveStakefunctionality. While these tests are valuable for when the feature is re-enabled, having large blocks of commented code can lead to technical debt.Consider either:
- Moving these test cases to a separate file with a clear "disabled" prefix until the feature is re-enabled
- Using test skip markers (
t.Skip()) with clear skip messages instead of commenting out the code- Creating a separate branch to preserve these tests until the feature is ready to be re-enabled
This would help maintain code cleanliness while preserving the test coverage for future use.
precompiles/staking/staking_test.go (1)
404-432: Consider enhancing error messages in stakeThroughCosmosAPIWhile the function implementation is correct, the error messages could be more descriptive to aid in debugging test failures.
Consider applying this improvement:
func stakeThroughCosmosAPI(...) { err := bankKeeper.MintCoins(ctx, fungibletypes.ModuleName, coins) - require.NoError(t, err) + require.NoError(t, err, "failed to mint coins: %v", err) err = bankKeeper.SendCoinsFromModuleToAccount(ctx, fungibletypes.ModuleName, staker, coins) - require.NoError(t, err) + require.NoError(t, err, "failed to send coins to account: %v", err) shares, err := stakingKeeper.Delegate(...) - require.NoError(t, err) + require.NoError(t, err, "failed to delegate tokens: %v", err)cmd/zetae2e/local/local.go (1)
364-371: Review disabled test and track its re-enablement.The
TestPrecompilesStakingThroughContractNametest is currently disabled with a reference to issue #3005. While the comment provides good context, we should:
- Ensure this is tracked for re-enablement
- Consider adding a TODO comment with more context about the conditions required for re-enabling
Add more context to the comment:
-// Disabled until further notice, check https://github.com/zeta-chain/node/issues/3005. +// TODO: Test disabled due to [brief reason] - track re-enablement in https://github.com/zeta-chain/node/issues/3005 +// Required conditions for re-enabling: +// - [condition 1] +// - [condition 2]testutil/keeper/keeper.go (2)
156-170: Remove commented permissions instead of keeping them as commentsThe permissions map contains commented-out entries for
ibctransfertypes.ModuleNameandibccrosschaintypes.ModuleName. If these permissions are no longer needed, remove the comments entirely rather than keeping them as commented code.maccPerms = map[string][]string{ authtypes.FeeCollectorName: nil, distrtypes.ModuleName: nil, stakingtypes.BondedPoolName: {authtypes.Burner, authtypes.Staking}, stakingtypes.NotBondedPoolName: {authtypes.Burner, authtypes.Staking}, govtypes.ModuleName: {authtypes.Burner}, - //ibctransfertypes.ModuleName: {authtypes.Minter, authtypes.Burner}, crosschaintypes.ModuleName: {authtypes.Minter, authtypes.Burner}, - //ibccrosschaintypes.ModuleName: nil, evmtypes.ModuleName: {authtypes.Minter, authtypes.Burner}, fungibletypes.ModuleName: {authtypes.Minter, authtypes.Burner}, emissionstypes.ModuleName: nil, emissionstypes.UndistributedObserverRewardsPool: nil, emissionstypes.UndistributedTSSRewardsPool: nil, }
Line range hint
422-552: Add documentation for keeper initialization orderThe
NewSDKKeepersWithKeysfunction handles complex keeper initialization with interdependencies. Consider adding documentation that explains the initialization order and dependencies between keepers to help maintainers understand the critical path.Add a comment block like this at the start of the function:
// NewSDKKeepersWithKeys initializes SDK keepers in a specific order to handle dependencies: // 1. Authority Keeper (no dependencies) // 2. Account Keeper (depends on Authority) // 3. Params Keeper (no dependencies) // ...e2e/contracts/testdistribute/TestDistribute.abi (1)
54-71: Consider returning addresses instead of strings for validator identifiersIn the
getDelegatorValidatorsThroughContractfunction, the output is defined asstring[]. Returning validator addresses (address[]) orbytes32[]can be more efficient and reduce gas costs.Apply this diff to adjust the return type:
"outputs": [ { - "internalType": "string[]", + "internalType": "bytes32[]", "name": "", - "type": "string[]" + "type": "bytes32[]" } ],Update the function implementation accordingly to return the validator identifiers in the new format.
precompiles/staking/method_get_rewards.go (1)
70-71: Preallocate slice capacity forrewardsto improve performanceTo avoid multiple allocations, consider preallocating the capacity of the
rewardsslice based on the length ofcoins.Apply this diff to preallocate slice capacity:
- rewards := make([]DecCoin, 0) + rewards := make([]DecCoin, 0, len(coins))precompiles/staking/method_claim_rewards.go (6)
39-53: Re-evaluate authorization logic for delegator rewards claimThe current implementation allows the reward claim if either the
contract.CallerAddressorevm.Originmatches thedelegatorAddr. This permits contracts called by the delegator to claim rewards on their behalf. Consider whether this aligns with the intended security model. If the goal is to ensure that only the delegator directly initiates reward claims, you might restrict the check toevm.Origin == delegatorAddrto prevent unauthorized contract interactions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-53: precompiles/staking/method_claim_rewards.go#L39-L53
Added lines #L39 - L53 were not covered by tests
70-76: Enhance error context when withdrawing delegation rewardsWhen handling errors from
WithdrawDelegationRewards, providing specific context can aid in debugging. Instead of generalizing the error asErrUnexpected, consider mapping specific errors to more descriptive precompile errors or including additional details about the failure.Apply this diff to improve error handling:
if err != nil { - return nil, precompiletypes.ErrUnexpected{ - When: "WithdrawDelegationRewards", - Got: err.Error(), - } + return nil, &precompiletypes.ErrWithdrawRewardsFailed{ + Delegator: delegatorCosmosAddr.String(), + Validator: validatorCosmosAddr.String(), + Reason: err.Error(), + } }This change introduces a more specific error type, enhancing clarity for users and developers.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 70-76: precompiles/staking/method_claim_rewards.go#L70-L76
Added lines #L70 - L76 were not covered by tests
82-85: Log skipped coins for improved transparencyCurrently, invalid coins, zero amounts, or non-ZRC20 coins are silently skipped in the rewards processing loop. For better observability and debugging, consider logging a warning or emitting an event for each coin that is skipped. This provides insight into which coins were not processed and why.
Apply this diff to add logging:
if !coin.IsValid() || !coin.Amount.IsPositive() || !precompiletypes.CoinIsZRC20(coin.Denom) { + ctx.Logger().Warn("Skipped coin during rewards claim", + "coin", coin.String(), + "reason", "invalid coin or not a ZRC20 denomination", + ) continue }Ensure that logging is appropriately configured to avoid performance overhead.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-85: precompiles/staking/method_claim_rewards.go#L82-L85
Added lines #L82 - L85 were not covered by tests
96-100: Review necessity of redundant balance checkThe balance check using
CheckZRC20Balancebefore callingUnlockZRC20may be redundant sinceUnlockZRC20performs this check internally. Evaluate whether this extra validation is necessary, or if it can be safely removed to streamline the code.Apply this diff if you decide to remove the redundant check:
- if err := c.fungibleKeeper.CheckZRC20Balance(ctx, zrc20Addr, bank.ContractAddress, zrc20Amount); err != nil { - continue - }This simplification reduces code duplication without compromising functionality.
131-137: Improve error messages in argument unpackingWhen type assertion for
delegatorfails, the error message includesdelegator.String(), which may not provide meaningful information sincedelegatoris uninitialized in this case. To enhance clarity, include the actual value fromargs[0]in the error.Apply this diff to enhance the error message:
func unpackClaimRewardsArgs(args []interface{}) (delegator common.Address, validator string, err error) { delegator, ok := args[0].(common.Address) if !ok { - return common.Address{}, "", &precompiletypes.ErrInvalidAddr{ - Got: delegator.String(), - } + return common.Address{}, "", &precompiletypes.ErrInvalidAddr{ + Got: fmt.Sprintf("%v", args[0]), + } }Remember to import the
fmtpackage:import ( "strings" + "fmt"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 131-137: precompiles/staking/method_claim_rewards.go#L131-L137
Added lines #L131 - L137 were not covered by tests
139-144: Enhance error reporting for validator addressSimilar to the previous comment, when the type assertion for
validatorfails, the error message uses the variablevalidator, which may be empty. Including the actual value fromargs[1]will make the error more informative.Apply this diff:
validator, ok = args[1].(string) if !ok { - return common.Address{}, "", &precompiletypes.ErrInvalidAddr{ - Got: validator, - } + return common.Address{}, "", &precompiletypes.ErrInvalidAddr{ + Got: fmt.Sprintf("%v", args[1]), + } }Ensure that the
fmtpackage is imported as noted previously.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-144: precompiles/staking/method_claim_rewards.go#L139-L144
Added lines #L139 - L144 were not covered by testsprecompiles/staking/staking.go (1)
269-276: Ensure consistent error handling and output packingIn the
ClaimRewardsMethodNamecase, whenexecErr != nil, you pack the outputs before returning an error. In contrast, other methods directly return the error without packing outputs. For consistency and clarity, consider standardizing the error handling approach across all methods. If output packing on errors is necessary for this method, ensure that it's clearly documented and justified to maintain consistency.e2e/e2etests/test_precompiles_distribute_through_contract.go (7)
39-40: Avoid Hardcoding the Carryover AmountThe
carryvariable is hardcoded with a specific value. To enhance maintainability and ensure accuracy, consider retrieving the carryover balance programmatically. This approach reduces the risk of errors if the underlying balance changes in the future.
52-53: Define Stake Amount as a Named ConstantThe stake amount is set using a hardcoded string
"1000000000000000000000". For better readability and maintainability, define this value as a named constant with a clear, descriptive name. This practice makes the code easier to understand and modify.Apply this change:
const stakeAmountValue = "1000000000000000000000" // ... _, ok := stakeAmt.SetString(stakeAmountValue, 10)
91-92: Enhance Error Handling for Stake OperationWhen calling
stakeThroughCosmosAPI, the error is checked usingrequire.NoError(r, err). For better debugging and clarity, include an error message to provide context if the staking operation fails. This practice aids in identifying issues during test failures.Apply this change:
require.NoError(r, err, "failed to stake through Cosmos API")
95-134: Refactor Repetitive Balance Checks and Approvals into Helper FunctionsThe code contains multiple instances of balance checks and allowance approvals with similar logic. To improve code readability and reduce duplication, consider refactoring these repetitive blocks into reusable helper functions. This will make the test easier to maintain and extend.
Example refactoring:
func attemptDistribution(r *runner.E2ERunner, contract *testdistribute.TestDistribute, amount *big.Int, expectedSuccess bool, errorMessage string) { tx, err := contract.DistributeThroughContract(r.ZEVMAuth, zrc20Address, amount) require.NoError(r, err) receipt := utils.MustWaitForTxReceipt(r.Ctx, r.ZEVMClient, tx, r.Logger, r.ReceiptTimeout) if expectedSuccess { utils.RequireTxSuccessful(r, receipt, errorMessage) } else { utils.RequiredTxFailed(r, receipt, errorMessage) } // Add balance checks here if needed }Then replace the distribution attempts with:
attemptDistribution(r, testDstrContract, oneThousand, false, "distribute should fail when there's no allowance")
183-187: Calculate Expected Rewards DynamicallyThe expected rewards amount
14239697290875601808is hardcoded. To enhance maintainability and clarity, calculate this value dynamically based on the distribution amount, number of validators, and commission rates. This approach ensures that changes in the underlying logic are accurately reflected in the test.Example calculation:
// Assuming there are two validators and a commission rate numValidators := big.NewInt(2) // Adjust for commission if applicable commissionRate := big.NewInt(0) // Replace with actual commission rate if any // Calculate expected rewards amount expectedRewards := big.NewInt(0).Div(zrc20DistrAmt, numValidators) // Apply commission if necessary // expectedRewards = expectedRewards.Sub(expectedRewards, commissionAmount) zrc20RewardsAmt := expectedRewards
189-194: Verify Event Data ConsistencyEnsure that the parsed event data matches the expected values. While the checks are in place, consider adding assertions for all relevant fields to guarantee the event's integrity. This practice strengthens the test by validating that all aspects of the event are correct.
Extend the assertions:
require.Equal(r, expectedEventField, eventClaimed.OtherField)
195-198: Clarify Calculation of Locker Final BalanceThe calculation of
lockerFinalBalancesubtractszrc20RewardsAmtfromzrc20DistrAmtCarry. To improve code readability, add comments or refactor the calculation to clearly explain the reasoning behind this computation. This will help future maintainers understand the logic without having to infer it.Add explanatory comments:
// Locker final balance = Total distributed amount with carryover - Claimed rewards amount lockerFinalBalance := big.NewInt(0).Sub(zrc20DistrAmtCarry, zrc20RewardsAmt)e2e/e2etests/test_precompiles_distribute.go (2)
327-329: Remove commented-out code to enhance code clarityThe commented-out code in lines 327-329 is no longer needed and can be removed to improve code readability. Keeping obsolete code can lead to confusion and hinders maintainability.
Apply this diff to remove the unnecessary commented code:
- // distrContract, err := staking.NewIStaking(staking.ContractAddress, r.ZEVMClient) - // require.NoError(r, err, "failed to create distribute contract caller")
23-200: RefactorTestPrecompilesDistributeAndClaiminto smaller functions for better maintainabilityThe
TestPrecompilesDistributeAndClaimfunction spans approximately 180 lines and encompasses multiple logical steps, including setup, validation checks, failed attempts, successful distribution, and claiming rewards. Refactoring these logical sections into well-named helper functions will enhance readability and maintainability of the test code.Consider extracting sections like allowance approvals, distribution attempts, and reward claims into separate functions.
e2e/contracts/testdistribute/TestDistribute.go (3)
32-32: Correct the grammatical error in the comment for 'DecCoin' structThe comment has a grammatical error: "an user-defined struct" should be "a user-defined struct".
Apply this diff to correct the comment:
-// DecCoin is an auto generated low-level Go binding around an user-defined struct. +// DecCoin is an auto generated low-level Go binding around a user-defined struct.
218-220: Return nil slice instead of empty slice when an error occursIt is more idiomatic in Go to return
nil, errwhen an error occurs, rather than an empty slice.Apply this diff to improve error handling:
if err != nil { - return *new([]string), err + return nil, err }
249-251: Return nil slice instead of empty slice when an error occursIt is more idiomatic in Go to return
nil, errwhen an error occurs, rather than an empty slice.Apply this diff to improve error handling:
if err != nil { - return *new([]DecCoin), err + return nil, err }precompiles/staking/IStaking.gen.go (4)
32-32: Correct the grammatical error in the comment.The comment on line 32 should use "a" instead of "an" before "user-defined" for correct grammar.
Apply this diff to fix the comment:
-// DecCoin is an auto generated low-level Go binding around an user-defined struct. +// DecCoin is an auto generated low-level Go binding around a user-defined struct.
236-237: Return a nil slice when an error occurs to adhere to Go conventions.In the error handling at lines 236-237, it's more idiomatic in Go to return a
nilslice when an error occurs rather than an empty slice. This distinction helps callers differentiate between an error state and a valid empty result.Apply this diff to modify the return statement:
if err != nil { - return *new([]string), err + return nil, err }
267-268: Return a nil slice when an error occurs to adhere to Go conventions.In the error handling at lines 267-268, returning a
nilslice instead of an empty slice when an error occurs is more idiomatic in Go. This practice clarifies the distinction between an error and a successful call that returns no results.Apply this diff to modify the return statement:
if err != nil { - return *new([]DecCoin), err + return nil, err }
426-578: Refactor duplicated iterator code for improved maintainability.The iterator implementations for events such as
IStakingClaimedRewardsIterator,IStakingDistributedIterator,IStakingMoveStakeIterator,IStakingStakeIterator, andIStakingUnstakeIteratorcontain duplicated code. Refactoring by abstracting common logic into a generic iterator or leveraging helper functions would enhance maintainability and reduce code duplication.Consider creating a generic iterator structure or function that can handle the common aspects of event iteration, thereby simplifying the codebase and making future updates more manageable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/contracts/testdistribute/TestDistribute.binis excluded by!**/*.bin
📒 Files selected for processing (36)
app/app.go(1 hunks)changelog.md(1 hunks)cmd/zetae2e/local/local.go(3 hunks)cmd/zetae2e/local/precompiles.go(2 hunks)e2e/contracts/testdistribute/TestDistribute.abi(2 hunks)e2e/contracts/testdistribute/TestDistribute.go(2 hunks)e2e/contracts/testdistribute/TestDistribute.json(2 hunks)e2e/contracts/testdistribute/TestDistribute.sol(1 hunks)e2e/e2etests/e2etests.go(2 hunks)e2e/e2etests/test_precompiles_bank_through_contract.go(6 hunks)e2e/e2etests/test_precompiles_distribute.go(3 hunks)e2e/e2etests/test_precompiles_distribute_through_contract.go(2 hunks)precompiles/precompiles.go(3 hunks)precompiles/precompiles_test.go(1 hunks)precompiles/staking/IStaking.abi(3 hunks)precompiles/staking/IStaking.gen.go(5 hunks)precompiles/staking/IStaking.json(3 hunks)precompiles/staking/IStaking.sol(3 hunks)precompiles/staking/const.go(1 hunks)precompiles/staking/logs.go(1 hunks)precompiles/staking/method_claim_rewards.go(1 hunks)precompiles/staking/method_distribute_test.go(13 hunks)precompiles/staking/method_get_all_validators_test.go(2 hunks)precompiles/staking/method_get_rewards.go(1 hunks)precompiles/staking/method_get_rewards_test.go(1 hunks)precompiles/staking/method_get_shares_test.go(4 hunks)precompiles/staking/method_get_validators.go(1 hunks)precompiles/staking/method_get_validators_test.go(1 hunks)precompiles/staking/method_move_stake_test.go(3 hunks)precompiles/staking/method_stake_test.go(2 hunks)precompiles/staking/method_unstake_test.go(2 hunks)precompiles/staking/staking.go(5 hunks)precompiles/staking/staking_test.go(14 hunks)precompiles/types/coin.go(2 hunks)precompiles/types/coin_test.go(1 hunks)testutil/keeper/keeper.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (29)
app/app.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/local.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
cmd/zetae2e/local/precompiles.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/contracts/testdistribute/TestDistribute.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/e2etests.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_precompiles_bank_through_contract.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_precompiles_distribute.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
e2e/e2etests/test_precompiles_distribute_through_contract.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/precompiles.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/precompiles_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/IStaking.gen.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/const.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/logs.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_claim_rewards.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_distribute_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_get_all_validators_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_get_rewards.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_get_rewards_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_get_shares_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_get_validators.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_get_validators_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_move_stake_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_stake_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_unstake_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/staking.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/staking_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/types/coin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/types/coin_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/keeper.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
precompiles/staking/logs.go
[warning] 157-167: precompiles/staking/logs.go#L157-L167
Added lines #L157 - L167 were not covered by tests
[warning] 169-174: precompiles/staking/logs.go#L169-L174
Added lines #L169 - L174 were not covered by tests
[warning] 176-178: precompiles/staking/logs.go#L176-L178
Added lines #L176 - L178 were not covered by tests
precompiles/staking/method_claim_rewards.go
[warning] 26-32: precompiles/staking/method_claim_rewards.go#L26-L32
Added lines #L26 - L32 were not covered by tests
[warning] 34-37: precompiles/staking/method_claim_rewards.go#L34-L37
Added lines #L34 - L37 were not covered by tests
[warning] 39-53: precompiles/staking/method_claim_rewards.go#L39-L53
Added lines #L39 - L53 were not covered by tests
[warning] 56-59: precompiles/staking/method_claim_rewards.go#L56-L59
Added lines #L56 - L59 were not covered by tests
[warning] 62-65: precompiles/staking/method_claim_rewards.go#L62-L65
Added lines #L62 - L65 were not covered by tests
[warning] 70-76: precompiles/staking/method_claim_rewards.go#L70-L76
Added lines #L70 - L76 were not covered by tests
[warning] 82-85: precompiles/staking/method_claim_rewards.go#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 91-99: precompiles/staking/method_claim_rewards.go#L91-L99
Added lines #L91 - L99 were not covered by tests
[warning] 102-106: precompiles/staking/method_claim_rewards.go#L102-L106
Added lines #L102 - L106 were not covered by tests
[warning] 109-110: precompiles/staking/method_claim_rewards.go#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 114-115: precompiles/staking/method_claim_rewards.go#L114-L115
Added lines #L114 - L115 were not covered by tests
[warning] 120-125: precompiles/staking/method_claim_rewards.go#L120-L125
Added lines #L120 - L125 were not covered by tests
[warning] 128-128: precompiles/staking/method_claim_rewards.go#L128
Added line #L128 was not covered by tests
[warning] 131-137: precompiles/staking/method_claim_rewards.go#L131-L137
Added lines #L131 - L137 were not covered by tests
[warning] 139-144: precompiles/staking/method_claim_rewards.go#L139-L144
Added lines #L139 - L144 were not covered by tests
[warning] 146-146: precompiles/staking/method_claim_rewards.go#L146
Added line #L146 was not covered by tests
precompiles/staking/method_get_rewards.go
[warning] 22-26: precompiles/staking/method_get_rewards.go#L22-L26
Added lines #L22 - L26 were not covered by tests
[warning] 30-31: precompiles/staking/method_get_rewards.go#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 36-37: precompiles/staking/method_get_rewards.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 56-59: precompiles/staking/method_get_rewards.go#L56-L59
Added lines #L56 - L59 were not covered by tests
[warning] 64-68: precompiles/staking/method_get_rewards.go#L64-L68
Added lines #L64 - L68 were not covered by tests
[warning] 72-76: precompiles/staking/method_get_rewards.go#L72-L76
Added lines #L72 - L76 were not covered by tests
[warning] 84-87: precompiles/staking/method_get_rewards.go#L84-L87
Added lines #L84 - L87 were not covered by tests
[warning] 91-94: precompiles/staking/method_get_rewards.go#L91-L94
Added lines #L91 - L94 were not covered by tests
precompiles/staking/method_get_validators.go
[warning] 20-24: precompiles/staking/method_get_validators.go#L20-L24
Added lines #L20 - L24 were not covered by tests
[warning] 28-29: precompiles/staking/method_get_validators.go#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 34-35: precompiles/staking/method_get_validators.go#L34-L35
Added lines #L34 - L35 were not covered by tests
🔇 Additional comments (46)
precompiles/staking/const.go (2)
21-24: LGTM! Proper reorganization of state-changing methods.
The moveStake constants are appropriately relocated to maintain a clear grouping of state-changing methods while preserving their original values and naming conventions.
4-8: LGTM! Gas costs align with similar operations.
The new claimRewards constants follow consistent naming patterns and are appropriately grouped under state-changing methods. The gas cost (10000) matches other similar state-changing operations like distribute and stake.
Let's verify the gas costs are consistent across similar operations:
✅ Verification successful
Gas cost for ClaimRewards aligns with other staking operations
The verification confirms that the gas cost of 10000 for ClaimRewardsMethodGas is consistent with other state-changing staking operations:
DistributeMethodGas: 10000StakeMethodGas: 10000MoveStakeMethodGas: 10000
Only UnstakeMethodGas has a lower cost at 1000, which is reasonable given its simpler nature. Bank operations have higher costs (200,000) due to their different scope and complexity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check gas costs for similar operations across the codebase
# Expect: Similar gas costs for state-changing operations related to staking
rg -A 1 "MethodGas\s+=\s+\d+" --type go
Length of output: 723
precompiles/precompiles_test.go (1)
28-35: LGTM: Parameter addition aligns with PR objectives.
The addition of DistributionKeeper as a parameter to StatefulContracts is correct and necessary for supporting the new delegator rewards functionality.
precompiles/types/coin.go (1)
5-5: LGTM: Import is correctly placed and necessary
e2e/contracts/testdistribute/TestDistribute.sol (1)
4-7: LGTM: Well-structured reward representation
The DecCoin struct provides a clean representation for denominated coins with appropriate field types.
cmd/zetae2e/local/precompiles.go (2)
7-7: LGTM: Imports are well-organized
The new imports follow the standard Go import organization pattern (standard lib → external → internal packages).
Also applies to: 13-13
53-53: LGTM: Clean assignment
The assignment is clear and follows the initialization sequence correctly.
precompiles/staking/method_get_validators_test.go (3)
1-11: LGTM: Clean imports and proper package declaration
The imports are well-organized and necessary for the test implementation.
13-93: Well-structured test implementation with comprehensive coverage
The test file effectively covers both the non-staker and staker scenarios with proper assertions and error handling. The use of a test suite setup promotes clean and maintainable tests.
61-69: Verify helper function implementation
The test uses stakeThroughCosmosAPI helper function. Let's verify its implementation.
✅ Verification successful
Helper function implementation verified and appears correct
The stakeThroughCosmosAPI helper function in precompiles/staking/staking_test.go is correctly implemented for testing purposes. It:
- Mints test coins with the "stake" denomination
- Sends the minted coins to the staker's account
- Delegates the coins to the validator using the staking keeper
- Verifies the delegation shares match the input amount
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stakeThroughCosmosAPI implementation
ast-grep --pattern 'func stakeThroughCosmosAPI($$$)'
Length of output: 3451
precompiles/precompiles.go (2)
8-8: LGTM: Import follows established patterns
The new distribution keeper import is properly organized with other Cosmos SDK imports and follows the project's import aliasing conventions.
55-63: LGTM: Well-structured contract initialization
The updated NewIStakingContract initialization is well-organized with:
- Clear parameter separation and alignment
- Logical parameter ordering (context → keepers → utilities)
- Proper integration of the new distribution keeper
precompiles/staking/method_get_shares_test.go (1)
Line range hint 13-57: Verify if the disabled test can be re-enabled
The main positive test case for GetShares is currently disabled with reference to issue #3009. Given that this PR adds new delegator rewards functionality, we should verify if this test can be re-enabled to ensure proper test coverage.
✅ Verification successful
Disabled test should remain commented out due to legal constraints
The test is correctly disabled as the underlying staking functions (stake, unstake, moveStake) are intentionally disabled for legal reasons, as confirmed by issue #3009 which remains open. The comment and link to the issue provide sufficient context for other developers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the status of issue #3009 and related staking function tests
# Check if the issue is still open
gh issue view 3009
# Look for other disabled staking tests that might need re-enabling
rg -l "Disabled temporarily because the staking functions were disabled" "precompiles/staking/"
Length of output: 527
precompiles/staking/logs.go (1)
151-179: Implementation follows established patterns
The addClaimRewardsLog function is well-implemented, following the established patterns for event logging in the codebase. It correctly handles:
- Event lookup using the constant
- Topic creation with indexed parameters
- Data packing for the amount
- Error handling
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 157-167: precompiles/staking/logs.go#L157-L167
Added lines #L157 - L167 were not covered by tests
[warning] 169-174: precompiles/staking/logs.go#L169-L174
Added lines #L169 - L174 were not covered by tests
[warning] 176-178: precompiles/staking/logs.go#L176-L178
Added lines #L176 - L178 were not covered by tests
precompiles/staking/IStaking.sol (2)
26-31: LGTM: Well-documented struct following Cosmos SDK standards
The DecCoin struct correctly implements the Cosmos SDK's coin representation with appropriate field types and clear documentation reference.
76-84: LGTM: Well-structured event with appropriate indexing
The ClaimedRewards event is properly defined with:
- Clear documentation
- Appropriate indexing on address fields for efficient filtering
- Essential parameters for tracking reward claims
precompiles/staking/IStaking.abi (2)
2-26: Well-structured event definition with appropriate indexing.
The ClaimedRewards event is well-designed with:
- Indexed fields (
claim_address,zrc20_token) for efficient filtering - Non-indexed
amountfield to avoid unnecessary gas costs
133-156: Verify access control for claimRewards function.
The function signature looks correct, but ensure that the implementation includes proper access control to prevent unauthorized claims.
✅ Verification successful
I need to examine the actual implementation to verify the access control mechanisms. The previous results only showed the function signature, but we need to see the complete implementation to ensure proper authorization checks are in place.
Access control for claimRewards function is properly implemented
The implementation includes robust access control mechanisms:
- Only the delegator can claim their own rewards, either directly or through a contract
- Authorization is enforced by checking both
callerIsDelegatorandoriginIsDelegatorconditions - Unauthorized attempts are rejected with a clear error message
- Additional safety checks are implemented for address validation and coin operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for access control implementations in the staking precompile
rg -A 5 "func.*claimRewards" precompiles/staking/
Length of output: 2700
Script:
#!/bin/bash
# Let's check the implementation of claimRewards to verify access control
cat precompiles/staking/method_claim_rewards.go
Length of output: 4811
precompiles/staking/method_distribute_test.go (1)
Line range hint 1-292: Well-structured and comprehensive test coverage
The test suite demonstrates good practices:
- Comprehensive coverage of error cases
- Clear test case organization
- Descriptive test names
- Consistent verification patterns
precompiles/staking/IStaking.json (2)
3-27: Well-structured event definition for reward claims.
The ClaimedRewards event follows best practices by appropriately indexing address fields for efficient filtering while keeping the amount parameter unindexed. The parameter naming and types are clear and suitable for tracking reward claims.
134-157: Consider adding validator address validation.
The function correctly uses nonpayable for state modifications. However, consider adding validation for the validator parameter to ensure it matches the expected format of a validator address.
Run the following script to check the validator address format across the codebase:
e2e/contracts/testdistribute/TestDistribute.json (4)
11-29: LGTM: claimRewardsThroughContract implementation.
The function signature and return type are well-defined and align with the PR objective of allowing delegators to claim their rewards. The boolean return value provides clear success/failure feedback.
55-73: LGTM: getDelegatorValidatorsThroughContract implementation.
The function is correctly marked as view and returns an array of validator strings, perfectly matching the PR objective of retrieving a list of validators for a specific delegator.
74-109: LGTM: getRewardsThroughContract implementation.
The function implementation is robust with:
- Proper view modifier for read-only operation
- Well-structured DecCoin tuple return type containing denom and amount
- Clear parameter naming
115-115: LGTM: Updated binary code.
The binary code has been properly updated to include all new function selectors and implementations.
precompiles/staking/method_unstake_test.go (1)
20-20: LGTM: Contract reference updates are correct
The changes to use stkContractABI and stkContract are consistent with the staking-specific contract implementation.
Also applies to: 39-39
precompiles/staking/method_stake_test.go (2)
20-20: LGTM: Contract reference updates are consistent
The changes from contractABI/contract to stkContractABI/stkContract align with the specialized staking contract implementation. The naming is more precise and better reflects the component under test.
Also applies to: 38-38
Line range hint 46-260: Re-enable comprehensive test coverage
A significant portion of test cases are commented out, including critical scenarios such as:
- Read-only mode validation
- Validator existence checks
- Successful staking flow
- Input validation
- Error handling for insufficient funds
- Parameter validation
Given that this PR introduces new delegator reward functionality, comprehensive test coverage is essential for ensuring system stability.
Let's verify the test coverage:
precompiles/staking/method_move_stake_test.go (2)
Line range hint 15-16: Well-documented test case disablement
The test case disablement is properly documented with a reference to the tracking issue #3005, which is a good practice for maintaining code clarity.
20-20: Contract reference changes are consistent
The changes from contractABI to stkContractABI and from contract.Run to stkContract.Run are consistent throughout the file, properly aligning with the staking precompile enhancements.
Also applies to: 44-46, 61-61
precompiles/staking/staking_test.go (2)
251-251: LGTM: Distribution keeper integration aligns with PR objectives
The addition of DistributionKeeper to the contract initialization is consistent with the PR's goal of adding delegator rewards functionality.
Also applies to: 324-324
288-289: LGTM: Improved variable naming clarity
The renaming from contract to stkContract enhances code readability by making the contract type explicit.
cmd/zetae2e/local/local.go (2)
163-167: LGTM: Improved readability of policy names configuration.
The policy names are now properly formatted across multiple lines, enhancing code readability.
258-258: Verify the impact of increased minting amount.
The minting amount has been significantly increased from a previous value to 1e10. While this change might be necessary for testing scenarios, we should verify that:
- This large amount doesn't cause overflow issues in downstream calculations
- The increase is justified by test requirements
testutil/keeper/keeper.go (1)
Line range hint 1-770: Implementation aligns well with delegator rewards management
The keeper setup provides a solid foundation for managing delegator rewards, with proper initialization of all necessary components including staking, distribution, and emissions keepers.
e2e/e2etests/e2etests.go (1)
1147-1147: LGTM! Test names accurately reflect enhanced functionality.
The renaming of test functions to include "AndClaim" better represents their complete functionality, aligning with the PR's objective of adding delegator reward claiming capabilities.
Also applies to: 1159-1159
changelog.md (1)
58-58: LGTM: Changelog entry accurately reflects PR objectives.
The changelog entry properly documents the addition of delegator reward functions under the Features section, aligning with the PR's primary objective.
precompiles/staking/method_claim_rewards.go (1)
128-129: Confirm correctness of output packing
The function returns method.Outputs.Pack(true), indicating a successful execution. Ensure that this aligns with the expected ABI definition and that the returned value correctly informs the caller of the method's success. If more detailed information is desired, consider extending the output to include relevant data.
Please review the method's ABI to confirm that the output adheres to the expected format.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-128: precompiles/staking/method_claim_rewards.go#L128
Added line #L128 was not covered by tests
e2e/e2etests/test_precompiles_distribute_through_contract.go (1)
56-71: Ensure Gas Limit is Restored After Test Execution
The gas limit is modified at line 56 to avoid out-of-gas errors. While you save the previous gas limit in previousGasLimit, ensure it is properly restored after the test to prevent side effects on other tests. Review the resetDistributionTest function to confirm that it resets the gas limit accordingly.
e2e/e2etests/test_precompiles_distribute.go (2)
318-319: Verify unit conversion when setting the undelegation Amount
In the undelegation message, the Amount is calculated by dividing amount by 1e18. Ensure that this division accurately converts the unit from wei to the cosmos native token denomination ("azeta"). This verification is crucial to prevent incorrect token amounts during undelegation.
145-146: Ensure correct recipient addresses in balance checks
In the balance checks after distribution, verify that the recipient addresses are correct. The code checks the balance of lockerAddress and r.FeeCollectorAddress, both expecting zrc20DistrAmt. Confirm that this aligns with the intended flow of funds in the test scenario.
If both addresses are intended to hold the same balance after distribution, consider clarifying this in code comments for better understanding.
e2e/contracts/testdistribute/TestDistribute.go (3)
211-226: Implementation of 'GetDelegatorValidatorsThroughContract' is correct
The method correctly binds to the contract function and handles data retrieval appropriately.
243-258: Implementation of 'GetRewardsThroughContract' is correct
The method accurately retrieves reward data by binding to the contract function as expected.
273-293: Implementation of 'ClaimRewardsThroughContract' is correct
The method properly binds to the contract function for claiming rewards and handles the transaction correctly.
app/app.go (1)
544-544: Integration of app.DistrKeeper into EVM precompiles is appropriate.
Adding app.DistrKeeper to the precompiles.StatefulContracts function ensures that the Ethereum Virtual Machine (EVM) precompiles have access to the distribution module. This integration is necessary for supporting the new functions related to delegator rewards, such as getRewards and claimRewards, enhancing the functionality of the staking mechanism within the EVM context.
precompiles/staking/IStaking.gen.go (1)
321-326: Implementation of the ClaimRewards function is correct.
The ClaimRewards function correctly binds to the contract method claimRewards and passes the appropriate parameters, ensuring proper transaction invocation.
| @@ -1,22 +1,30 @@ | |||
| package staking | |||
|
|
|||
| const ( | |||
| // State changing methods. | |||
There was a problem hiding this comment.
Consider using Tx and Query to the method names to clarify if they modify the state or query it.
There was a problem hiding this comment.
This is at the smart contract level, this is a Cosmos convention
|
|
||
| // Emit an event per ZRC20 coin unlocked. | ||
| // This keeps events as granular and deterministic as possible. | ||
| if err := c.addClaimRewardsLog(ctx, evm.StateDB, delegatorAddr, zrc20Addr, zrc20Amount); err != nil { |
There was a problem hiding this comment.
Can we add the validator address to this log
| /* ASSERT */ | ||
| _, err = getRewardsMethod.Outputs.Unpack(bytes) | ||
| require.NoError(t, err) | ||
| //fmt.Println("getRewards response: ", res) |
There was a problem hiding this comment.
Can we remove the print statement
| ) | ||
|
|
||
| func Test_GetRewards(t *testing.T) { | ||
| t.Run("should return empty rewards list to a non staker", func(t *testing.T) { |
There was a problem hiding this comment.
Consider adding a test for invalid coins
| rewards := make([]DecCoin, 0) | ||
| for _, coin := range coins { | ||
| rewards = append(rewards, DecCoin{ | ||
| Denom: coin.Denom, | ||
| Amount: coin.Amount.BigInt(), | ||
| }) |
There was a problem hiding this comment.
Does this return even if one of the coins is invalid ?
Would it make sense to show the rewards for the valid coins instead?
| ) | ||
|
|
||
| // getValidators queries the list of validators for a given delegator. | ||
| func (c *Contract) getDelegatorValidators( |
There was a problem hiding this comment.
I feel like getValidatorListForDelegatorAddress might be clearer, but I don't feel strongly about it, though
| } | ||
|
|
||
| // Get the cosmos address of the caller. | ||
| delegatorCosmosAddr, err := precompiletypes.GetCosmosAddress(c.bankKeeper, delegatorAddr) |
There was a problem hiding this comment.
Not required for this PR , but we should add unit tests for this function
|
|
||
| // Return immediately, no need to check the slice. | ||
| // If there are no validators we simply return an empty array to calling contracts. | ||
| return method.Outputs.Pack(res.Validators) |
There was a problem hiding this comment.
Please add a test for a large number of Validators.
Maybe 100 or 120 validators.
Also, is there any way to add pagination to these queries?
| if (zrc20address == common.Address{}) { | ||
| return nil, &ErrInvalidAddr{ | ||
| Got: zrc20address.String(), | ||
| Reason: "empty address", | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add a unit test for this
| {"zrc20/0xCa14007Eff0dB1f8135f4C25B34De49AB0d42766", true}, // Valid ZRC20 address. | ||
| {"zrc200xabcdef", false}, // Malformed prefix. | ||
| {"foo/0x0123456789", false}, // Invalid prefix. | ||
| {"ZRC20/0x0123456789abcdef", false}, // Invalid prefix. |
There was a problem hiding this comment.
| {"ZRC20/0x0123456789abcdef", false}, // Invalid prefix. | |
| {"ZRC20/0x0123456789abcdef", false}, // Invalid prefix. | |
| {"zrc20/0x0123456789abcdef", false, // valid prefix invalid address |
| amount math.Int, | ||
| ) { | ||
| // Coins to stake with default cosmos denom. | ||
| coins := sdk.NewCoins(sdk.NewCoin("stake", amount)) |
There was a problem hiding this comment.
Not required for the pr, but might be good to explore the option of setting the default denom to azeta .
I think it can be set using the staking params
| shares, err := stakingKeeper.Delegate( | ||
| ctx, | ||
| staker, | ||
| coins.AmountOf(coins.Denoms()[0]), |
There was a problem hiding this comment.
Why not just use the string , can define stake as a constant ?
| ) | ||
|
|
||
| /* ACT */ | ||
| // Call getRewards. |
There was a problem hiding this comment.
The comment needs to be adjusted
There was a problem hiding this comment.
same for other instances in the file
| @@ -55,6 +55,7 @@ | |||
| * [3028](https://github.com/zeta-chain/node/pull/3028) - whitelist connection gater | |||
| * [3019](https://github.com/zeta-chain/node/pull/3019) - add ditribute functions to staking precompile | |||
| * [3020](https://github.com/zeta-chain/node/pull/3020) - add support for TON withdrawals | |||
| * [3088](https://github.com/zeta-chain/node/pull/3088) - add functions to check and withdraw delegator rewards | |||
There was a problem hiding this comment.
| * [3088](https://github.com/zeta-chain/node/pull/3088) - add functions to check and withdraw delegator rewards | |
| * [3088](https://github.com/zeta-chain/node/pull/3088) - add functions to check and withdraw delegator zrc20 rewards |
| @@ -1,22 +1,30 @@ | |||
| package staking | |||
|
|
|||
| const ( | |||
| // State changing methods. | |||
There was a problem hiding this comment.
This is at the smart contract level, this is a Cosmos convention
| callerIsDelegator = contract.CallerAddress == delegatorAddr | ||
|
|
||
| // This represents the delegator calling the precompile through a contract. | ||
| originIsDelegator = evm.Origin == delegatorAddr |
There was a problem hiding this comment.
I'm not sure I understand this one. If you build a smart contract application to stake, the smart contract would be the delegator, then the smart contract would have the responsibility to distribute stake, etc..
I don't think we should ever use Origin in any case.
Right now since stake function is disable a smart contract can't stake so we consider only EoA will be able to use this function
| zrc20Amount = coin.Amount.BigInt() | ||
| ) | ||
|
|
||
| // Check if bank address has enough ZRC20 balance. |
There was a problem hiding this comment.
// Check if bank address has enough ZRC20 balance.
This is supposed to always be the case? We could have an invariant in the future but for now we should at least have a log here if it must not happen
| coinSet := sdk.NewCoins(coin) | ||
|
|
||
| // Send the coins to the fungible module to burn them. | ||
| if err := c.bankKeeper.SendCoinsFromAccountToModule(ctx, delegatorCosmosAddr, fungibletypes.ModuleName, coinSet); err != nil { |
| } | ||
|
|
||
| if err := c.bankKeeper.BurnCoins(ctx, fungibletypes.ModuleName, coinSet); err != nil { | ||
| continue |
There was a problem hiding this comment.
It seems we should return error here to cancel the entire operation
If we use continue here, there will be a non completed side effect SendCoinsFromAccountToModule.
The entire action should be atomic
| } | ||
|
|
||
| // Finally, unlock the ZRC20 coins. | ||
| if err := c.fungibleKeeper.UnlockZRC20(ctx, zrc20Addr, delegatorAddr, bank.ContractAddress, zrc20Amount); err != nil { |
There was a problem hiding this comment.
Same as above, should return error
| expected bool | ||
| }{ | ||
| {"", false}, // Empty string. | ||
| {"zrc20/", false}, // Missing address. |
There was a problem hiding this comment.
If the right part should be address, it seems we're missing a test case zrc20/<invalid address>
zrc20/foo
Description
Add distribute functions to the staking precompile:
getDelegatorValidators: returns all the validators where a delegator has delegated to.getRewards: returns all the outstanding rewards in a validator for an specific delegator.claimRewards: claims all the delegation rewards. The caller has to be the delegator, or a contract called by the delegator.How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor