-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: distribute ZRC20 rewards function #3019
Conversation
b4c51d7
to
afac1d1
Compare
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📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant updates across multiple files, primarily focusing on enhancements to staking and distribution functionalities within the ZetaChain ecosystem. Key changes include the addition of new methods for distributing tokens, improvements to existing smart contracts, and the introduction of new test cases to ensure robustness. The changelog reflects these updates, detailing new features, refactoring efforts, and fixes. Additionally, the integration of new constants and modifications to error handling are noted, contributing to an overall improvement in code quality and functionality. Changes
Possibly related issues
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3019 +/- ##
===========================================
- Coverage 64.46% 64.33% -0.14%
===========================================
Files 412 419 +7
Lines 28986 29102 +116
===========================================
+ Hits 18687 18723 +36
- Misses 9516 9597 +81
+ Partials 783 782 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 66
🧹 Outside diff range and nitpick comments (35)
precompiles/staking/method_get_all_validators.go (1)
8-11
: Add GoDoc comments for the GetAllValidators method.Consider adding documentation that explains the purpose of this method, its parameters, and return values. This will improve code maintainability and help other developers understand the method's role in the staking contract.
+// GetAllValidators retrieves all validators from the staking keeper and returns their +// information in a format compatible with the staking precompile contract. +// Parameters: +// - ctx: The SDK context +// - method: The ABI method definition used for output encoding +// Returns encoded validator data or an error if the operation fails. func (c *Contract) GetAllValidators(precompiles/types/coin_test.go (3)
Line range hint
10-16
: Consider enhancing test coverage with table-driven tests.While the current test is functional, consider these improvements for better maintainability and coverage:
- Convert to table-driven tests to handle multiple test cases
- Add edge cases (zero address, max uint256)
- Document the significance of the test value 12345
Here's a suggested implementation:
func Test_ZRC20ToCosmosDenom(t *testing.T) { tests := []struct { name string address *big.Int want string }{ { name: "standard address", address: big.NewInt(12345), // 0x3039 want: "zrc20/0x0000000000000000000000000000000000003039", }, { name: "zero address", address: big.NewInt(0), want: "zrc20/0x0000000000000000000000000000000000000000", }, // Add more test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := ZRC20ToCosmosDenom(common.BigToAddress(tt.address)) require.Equal(t, tt.want, got, "ZRC20ToCosmosDenom(%v)", tt.address) }) } }
Line range hint
18-21
: Update test function name to match the tested function.The test function name
Test_createCoinSet
should be updated toTest_CreateCoinSet
to maintain consistency with the exported function being tested.-func Test_createCoinSet(t *testing.T) { +func Test_CreateCoinSet(t *testing.T) {
Line range hint
22-29
: Enhance test coverage with edge cases and error scenarios.The current test only covers the happy path. Consider adding test cases for:
- Invalid denomination format
- Zero amount
- Negative amount
- Maximum uint256 amount
Here's a suggested implementation:
func Test_CreateCoinSet(t *testing.T) { tests := []struct { name string tokenDenom string amount *big.Int wantErr bool }{ { name: "valid input", tokenDenom: "zrc20/0x0000000000000000000000000000000000003039", amount: big.NewInt(100), wantErr: false, }, { name: "invalid denom", tokenDenom: "invalid", amount: big.NewInt(100), wantErr: true, }, // Add more test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { coinSet, err := CreateCoinSet(tt.tokenDenom, tt.amount) if tt.wantErr { require.Error(t, err) return } require.NoError(t, err) require.Len(t, coinSet, 1) require.Equal(t, tt.tokenDenom, coinSet[0].Denom) require.Equal(t, tt.amount, coinSet[0].Amount.BigInt()) }) } }e2e/contracts/testdistribute/TestDistribute.sol (1)
4-10
: Add comprehensive NatSpec documentation to the interfaceThe interface would benefit from detailed documentation explaining its purpose and function parameters.
// @dev Interface to interact with distribute. interface IDistribute { + /// @notice Distributes ZRC20 tokens to validators and delegators + /// @param zrc20 The address of the ZRC20 token to distribute + /// @param amount The amount of tokens to distribute + /// @return success True if the distribution was successful function distribute( address zrc20, uint256 amount ) external returns (bool success); }precompiles/types/coin.go (2)
11-11
: Add documentation for the ZRC20DenomPrefix constant.Consider adding a documentation comment explaining the purpose and usage of this prefix constant.
+// ZRC20DenomPrefix is the prefix used to identify ZRC20 token denominations in the system. const ZRC20DenomPrefix = "zrc20/"
Line range hint
19-41
: Enhance CreateCoinSet implementation and documentation.Consider the following improvements:
- The validation conditions could be simplified
- The documentation could be more comprehensive
+// CreateCoinSet creates a new sdk.Coins set containing a single coin. +// It performs validation to ensure the coin and resulting coin set are valid. +// Returns an error if the coin or coin set fails validation checks. +// Parameters: +// - tokenDenom: the denomination of the token +// - amount: the amount of tokens +// Returns: +// - sdk.Coins: a valid coin set containing the single coin +// - error: if validation fails func CreateCoinSet(tokenDenom string, amount *big.Int) (sdk.Coins, error) { coin := sdk.NewCoin(tokenDenom, math.NewIntFromBigInt(amount)) if !coin.IsValid() { return nil, &ErrInvalidCoin{ Got: coin.GetDenom(), Negative: coin.IsNegative(), Nil: coin.IsNil(), } } - // A sdk.Coins (type []sdk.Coin) has to be created because it's the type expected by MintCoins - // and SendCoinsFromModuleToAccount. - // But sdk.Coins will only contain one coin, always. + // Create a single-coin set as required by MintCoins and SendCoinsFromModuleToAccount coinSet := sdk.NewCoins(coin) - if !coinSet.IsValid() || coinSet.Empty() || coinSet.IsAnyNil() || coinSet == nil { + // Validate the coin set + if coinSet == nil || !coinSet.IsValid() || coinSet.Empty() || coinSet.IsAnyNil() { return nil, &ErrInvalidCoin{ Got: coinSet.String(), Negative: coinSet.IsAnyNegative(),precompiles/types/address_test.go (1)
39-45
: Add documentation for the helper type.The contractRef implementation is clean, but would benefit from documentation explaining its purpose in the test context.
Consider adding these comments:
+// contractRef implements the ContractRef interface for testing purposes. type contractRef struct { address common.Address } +// Address returns the contract's address as required by the ContractRef interface. func (c contractRef) Address() common.Address { return c.address }precompiles/types/address.go (1)
10-14
: Documentation could be more precise.The comment should follow Go's documentation conventions and clearly state the return values.
Consider updating the documentation:
-// GetEVMCallerAddress returns the caller address. -// Usually the caller is the contract.CallerAddress, which is the address of the contract that called the precompiled contract. -// If contract.CallerAddress != evm.Origin is true, it means the call was made through a contract, -// on which case there is a need to set the caller to the evm.Origin. +// GetEVMCallerAddress returns the effective caller address for an EVM transaction. +// If the contract caller differs from the EVM origin (indicating a contract-initiated call), +// it returns the origin address. Otherwise, it returns the contract caller address. +// Returns an error if the address determination fails.e2e/utils/require.go (1)
Function name inconsistency requires correction
The verification confirms that:
RequiredTxFailed
is used in multiple test files across the codebase- The companion function follows the pattern
RequireTx*
(specificallyRequireTxSuccessful
)- No existing usage of the proposed name
RequireTxFailed
existsThe original review is correct. The function should be renamed from
RequiredTxFailed
toRequireTxFailed
to maintain consistency with theRequireTx*
pattern. This will require updating all 23 usage locations identified in the e2e test files.🔗 Analysis chain
Line range hint
40-48
: Function name should follow consistent naming pattern.The function name
RequiredTxFailed
contains a typo and breaks the naming convention used by other functions in this file (e.g.,RequireTxSuccessful
).Apply this diff to fix the function name:
-func RequiredTxFailed(t require.TestingT, receipt *ethtypes.Receipt, msgAndArgs ...any) { +func RequireTxFailed(t require.TestingT, receipt *ethtypes.Receipt, msgAndArgs ...any) {Let's verify if this function is used elsewhere in the codebase:
Let's analyze the naming patterns of similar functions to ensure our suggestion aligns with the codebase's conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of both the old and new function names echo "Current function name usage:" rg "RequiredTxFailed" echo -e "\nProposed function name conflicts:" rg "RequireTxFailed"Length of output: 2972
Script:
#!/bin/bash # Let's check for similar function names in the utils package to verify naming conventions echo "Similar function names in utils package:" rg "func Require.*Tx" e2e/utils/Length of output: 350
precompiles/bank/method_balance_of.go (1)
Line range hint
8-26
: Consider enhancing function documentation with error conditions.The function documentation is good but could be improved by explicitly listing the error conditions that callers should handle. This would make the contract's behavior more predictable for integrators.
Add error conditions to the documentation:
// balanceOf returns the balance of cosmos coins minted by the bank's deposit function, // for a given cosmos account calculated with toAddr := sdk.AccAddress(addr.Bytes()). // The denomination of the cosmos coin will be "zrc20/0x12345" where 0x12345 is the ZRC20 address. // Call this function using solidity with the following signature: // From IBank.sol: function balanceOf(address zrc20, address user) external view returns (uint256 balance); +// +// Returns error if: +// - Invalid number of arguments +// - Invalid address format +// - Token is not a whitelisted ZRC20 +// - Resulting coin is invalid (negative or nil)precompiles/precompiles.go (1)
52-53
: Consider implementing a contract factory pattern.The current implementation directly instantiates contracts within the
StatefulContracts
function. As the system grows with more precompiled contracts, consider implementing a contract factory pattern to:
- Standardize contract initialization
- Centralize dependency injection
- Simplify testing and mocking
Example factory pattern:
type ContractFactory interface { NewContract(ctx sdktypes.Context, params ContractParams) vm.PrecompiledContract } type ContractParams struct { Keepers KeeperSet Codec codec.Codec GasConfig storetypes.GasConfig } type KeeperSet struct { StakingKeeper *stakingkeeper.Keeper FungibleKeeper *fungiblekeeper.Keeper BankKeeper bankkeeper.Keeper }e2e/contracts/testdistribute/TestDistribute.json (1)
37-60
: Function signature is well-defined.The
distributeThroughContract
function has clear parameters and appropriate return type. Consider adding natspec documentation in the implementation to clarify:
- Expected behavior on success/failure
- Requirements for input parameters
- Potential revert conditions
precompiles/staking/method_get_shares_test.go (1)
12-54
: Enhance documentation for the disabled test case.While the commented-out test is justified with a reference to issue #3009, consider adding more context about the expected timeline or conditions for re-enabling it.
Consider adding a TODO comment with the following structure:
// Disabled temporarily because the staking functions were disabled. // Issue: https://github.com/zeta-chain/node/issues/3009 +// TODO: Re-enable this test when staking functions are restored in the referenced issue.
precompiles/staking/IStaking.sol (1)
118-125
: Consider adding input validation requirements.While the method signature is well-defined, consider documenting the following validation requirements in the NatSpec comments:
- Expected behavior for zero amounts
- Expected behavior for invalid/unsupported ZRC20 tokens
- Any minimum/maximum amount constraints
Here's a suggested enhancement to the documentation:
/// @notice Distribute a ZRC20 token as staking rewards. /// @param zrc20 The ZRC20 token address to be distributed. /// @param amount The amount of ZRC20 tokens to distribute. /// @return success Boolean indicating whether the distribution was successful. +/// @dev Requirements: +/// - `zrc20` must be a valid and supported ZRC20 token address +/// - `amount` must be greater than 0pkg/rpc/clients.go (1)
39-40
: Maintain consistent documentation styleThe comment format for the Distribution field differs from other fields. Consider aligning it with the established pattern:
- // Distribution is a "github.com/cosmos/cosmos-sdk/x/distribution/types" QueryClient + // Distribution is a github.com/cosmos/cosmos-sdk/x/distribution/types QueryClientprecompiles/bank/method_withdraw.go (2)
82-85
: Consider enhancing error context in the coin set creation chain.While the migration to
ptypes.CreateCoinSet
is appropriate, consider wrapping the error with additional context to aid in debugging:coinSet, err := ptypes.CreateCoinSet(ptypes.ZRC20ToCosmosDenom(zrc20Addr), amount) if err != nil { - return nil, err + return nil, &ptypes.ErrUnexpected{ + When: "CreateCoinSet", + Got: err.Error(), + } }
Line range hint
19-124
: Consider implementing explicit transaction boundaries.The
withdraw
function performs multiple state-changing operations. While the current implementation follows a clear pattern (validation -> effect -> interactions), consider implementing explicit transaction boundaries to ensure atomicity:
- Consider wrapping the entire operation in a transaction scope
- Implement compensation logic for partial failures
- Add detailed logging for each critical operation
This would enhance the robustness of the financial operations and make debugging easier.
precompiles/bank/method_deposit.go (1)
Line range hint
8-19
: Enhance documentation with security considerations.The documentation effectively explains the deposit process, but consider adding explicit security notes about:
- Reentrancy protection via Check-Effects-Interactions pattern
- Potential implications of permissionless minting
- Importance of proper allowance setting
e2e/e2etests/test_precompiles_distribute_through_contract.go (3)
31-34
: Consider using named constants for magic numbersDefine these values as package-level constants with descriptive names to improve maintainability and make the test's intent clearer.
+const ( + initialBalance = 1000 + overAllowance = 1001 + halfBalance = 500 + overHalf = 501 +) - oneThousand = big.NewInt(1e3) - oneThousandOne = big.NewInt(1001) - fiveHundred = big.NewInt(500) - fiveHundredOne = big.NewInt(501) + oneThousand = big.NewInt(initialBalance) + oneThousandOne = big.NewInt(overAllowance) + fiveHundred = big.NewInt(halfBalance) + fiveHundredOne = big.NewInt(overHalf)
51-52
: Document the gas limit modification rationaleAdd a comment explaining why this specific gas limit (10_000_000) was chosen and what operations require such a high limit.
112-122
: Enhance error handling with contextThe error handling could be more descriptive to aid in debugging failures.
func distributeThroughContract( r *runner.E2ERunner, dstr *testdistribute.TestDistribute, zrc20Address common.Address, amount *big.Int, ) *types.Receipt { tx, err := dstr.DistributeThroughContract(r.ZEVMAuth, zrc20Address, amount) - require.NoError(r, err) + require.NoError(r, err, "failed to distribute through contract: amount %s, zrc20 %s", + amount.String(), zrc20Address.Hex()) receipt := utils.MustWaitForTxReceipt(r.Ctx, r.ZEVMClient, tx, r.Logger, r.ReceiptTimeout) return receipt }x/fungible/keeper/zrc20_cosmos_coins_mapping.go (2)
Line range hint
1-180
: Consider refactoring validation logic for better maintainability.The validation logic is currently spread across multiple functions (
CheckZRC20Allowance
,CheckZRC20Balance
,IsValidZRC20
,IsValidDepositAmount
). Consider consolidating these into a dedicated validator struct to improve maintainability and reusability.Here's a suggested approach:
// ZRC20Validator encapsulates validation logic type ZRC20Validator struct { keeper *Keeper } func NewZRC20Validator(k *Keeper) *ZRC20Validator { return &ZRC20Validator{keeper: k} } func (v *ZRC20Validator) ValidateTransferContext(ctx sdk.Context, params TransferParams) error { // Combine all validation logic in one place if err := v.validateAddresses(params); err != nil { return err } if err := v.validateAmounts(ctx, params); err != nil { return err } return v.validateContractState(ctx, params) }This pattern would:
- Centralize validation logic
- Make validation rules more maintainable
- Simplify testing
- Enable easier addition of new validation rules
Security-critical TODO regarding whitelisted locker addresses requires attention
The issue (#2991) about implementing whitelisted locker addresses is still open. This verification confirms that the security measure mentioned in the TODO comment remains unimplemented. Given the security implications of unrestricted locker addresses in ZRC20 token operations, this requires immediate attention.
x/fungible/keeper/zrc20_cosmos_coins_mapping.go
: Implement address whitelist validation before performing balance checks- Consider implementing an admin-controlled whitelist registry in the keeper
- Add validation middleware to ensure only whitelisted addresses can act as lockers
🔗 Analysis chain
Line range hint
40-41
: Address TODO comment regarding whitelisted locker addresses.The TODO comment references an open issue for implementing whitelisted locker addresses. This is a security-critical feature that should be prioritized.
Let's check if this issue is still open:
Would you like help implementing the whitelisted locker address functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the issue about whitelisted locker addresses is still open gh issue view 2991 --json state,titleLength of output: 123
precompiles/staking/IStaking.json (1)
3-27
: LGTM! Consider standardizing parameter naming convention.The event structure follows best practices with appropriate parameter indexing. However, for consistency with Solidity naming conventions, consider using camelCase for parameter names.
- "name": "zrc20_distributor", + "name": "zrc20Distributor", - "name": "zrc20_token", + "name": "zrc20Token",precompiles/staking/method_distribute_test.go (1)
191-220
: Enhance success test case with additional assertions.The success test case should verify the complete state changes after distribution:
t.Run("should distribute and lock ZRC20", func(t *testing.T) { // ... existing setup ... // Capture initial state initialBalance := s.fungibleKeeper.GetBalance(s.ctx, s.zrc20Address, s.defaultCaller) success, err := s.contract.Run(s.mockEVM, s.mockVMContract, false) require.NoError(t, err) // Verify result res, err := s.methodID.Outputs.Unpack(success) require.NoError(t, err) require.True(t, res[0].(bool)) // Verify state changes finalBalance := s.fungibleKeeper.GetBalance(s.ctx, s.zrc20Address, s.defaultCaller) require.Equal(t, big.NewInt(0), finalBalance) // Verify events were emitted // TODO: Add event verification // Verify tokens were locked // TODO: Add lock verification })e2e/e2etests/test_precompiles_bank_through_contract.go (2)
61-61
: LGTM! Consider enhancing test readability with test case grouping.The balance check calls are well-placed throughout the test flow, verifying the state after each operation. The renaming to
checkCosmosBalanceThroughBank
better reflects the function's purpose.Consider using test tables or subtests to group related test scenarios (e.g., "insufficient allowance", "insufficient balance", "successful deposit") for better organization and readability:
testCases := []struct { name string depositAmount *big.Int allowance *big.Int shouldSucceed bool }{ { name: "deposit without allowance", depositAmount: oneThousand, allowance: big.NewInt(0), shouldSucceed: false, }, // ... more cases }Also applies to: 70-70, 83-83, 96-96, 105-105, 121-121, 130-130
159-163
: Add function documentation to improve code clarity.While the function name is descriptive, adding documentation would help explain its purpose, parameters, and return value.
Consider adding documentation:
+// checkCosmosBalanceThroughBank queries the balance of a ZRC20 token for a target address +// through the bank contract. This is used in E2E tests to verify the state of deposits +// and withdrawals. +// Parameters: +// - r: E2E test runner instance +// - bank: TestBank contract instance +// - zrc20: Address of the ZRC20 token contract +// - target: Address whose balance is being queried +// Returns: Balance as a big.Int func checkCosmosBalanceThroughBank( r *runner.E2ERunner, bank *testbank.TestBank, zrc20, target common.Address, ) *big.Int {e2e/e2etests/test_precompiles_distribute.go (2)
19-43
: Add documentation for the test function.Consider adding a function comment that describes:
- The purpose of the test
- The test scenarios covered
- The expected outcomes
Example:
// TestPrecompilesDistribute tests the distribution functionality of the staking contract. // It verifies: // - Distribution fails without proper allowance // - Distribution fails when amount exceeds allowance // - Distribution fails when amount exceeds balance // - Successful distribution with proper allowance and balance // - Proper event emission // - Balance updates after distribution
120-166
: Consider removing commented-out code.Large blocks of commented code can make maintenance more difficult. Consider:
- Moving this code to a separate file or branch until needed
- Creating a GitHub issue to track the implementation
- Adding a TODO comment with just the issue reference
Example:
// TODO(#ISSUE): Implement validator reward withdrawal functionality // See https://github.com/zeta-chain/node/issues/XXXXprecompiles/staking/method_unstake_test.go (1)
15-46
: Consider enhancing test documentation and error validation.While the test correctly verifies the disabled state, consider these improvements:
- Add a package-level comment explaining the purpose of these tests
- Include a reference to the GitHub issue in a constant
- Enhance error message validation
+// Package staking_test provides comprehensive testing for staking precompile functionality. package staking +const ( + unstakeDisabledIssueURL = "https://github.com/zeta-chain/node/issues/3005" +) func Test_Unstake(t *testing.T) { - // Disabled until further notice, check https://github.com/zeta-chain/node/issues/3005. + // Disabled until further notice, see unstakeDisabledIssueURL t.Run("should fail with error disabled", func(t *testing.T) { // ... require.Error(t, err) + require.Equal(t, "method unstake is disabled", err.Error()) require.ErrorIs(t, err, ptypes.ErrDisabledMethod{ Method: UnstakeMethodName, })precompiles/staking/method_stake_test.go (1)
15-46
: Enhance test documentation and naming.While the test is well-structured, consider these improvements:
- Rename the test to better reflect its current state, e.g.,
Test_Stake_CurrentlyDisabled
- Add a TODO comment with acceptance criteria for re-enabling the tests once issue Disable
stake
,unstake
andmoveStake
#3005 is resolved-func Test_Stake(t *testing.T) { +func Test_Stake_CurrentlyDisabled(t *testing.T) { // Disabled until further notice, check https://github.com/zeta-chain/node/issues/3005. + // TODO: Re-enable these tests when: + // 1. Issue #3005 is resolved + // 2. Staking functionality is fully implemented + // 3. All edge cases are covered by the commented test cases belowchangelog.md (1)
24-24
: Fix typo in PR title reference.There is a typo in the PR title: "ditribute" should be "distribute".
Apply this diff to fix the typo:
-* [3019](https://github.com/zeta-chain/node/pull/3019) - add ditribute functions to staking precompile +* [3019](https://github.com/zeta-chain/node/pull/3019) - add distribute functions to staking precompileprecompiles/staking/staking_test.go (2)
198-270
: Remove unused 'setup' function to improve code maintainabilityThe
setup
function appears to be unused and duplicates functionality provided bynewTestSuite
. Removing it will keep the codebase clean and reduce redundancy.Apply this diff to remove the unused function:
-func setup(t *testing.T) (sdk.Context, *Contract, abi.ABI, keeper.SDKKeepers, *vm.EVM, *vm.Contract) { - // Initialize state. - // Get sdk keepers initialized with this state and the context. - cdc := keeper.NewCodec() - db := tmdb.NewMemDB() - stateStore := store.NewCommitMultiStore(db) - keys, memKeys, tkeys, allKeys := keeper.StoreKeys() - - sdkKeepers := keeper.NewSDKKeepersWithKeys(cdc, keys, memKeys, tkeys, allKeys) - - for _, key := range keys { - stateStore.MountStoreWithDB(key, storetypes.StoreTypeIAVL, db) - } - for _, key := range tkeys { - stateStore.MountStoreWithDB(key, storetypes.StoreTypeTransient, nil) - } - for _, key := range memKeys { - stateStore.MountStoreWithDB(key, storetypes.StoreTypeMemory, nil) - } - - require.NoError(t, stateStore.LoadLatestVersion()) - - ctx := keeper.NewContext(stateStore) - - // Initialize codecs and gas config. - encoding := ethermint.MakeEncodingConfig() - appCodec := encoding.Codec - gasConfig := storetypes.TransientGasConfig() - - stakingGenesisState := stakingtypes.DefaultGenesisState() - stakingGenesisState.Params.BondDenom = config.BaseDenom - sdkKeepers.StakingKeeper.InitGenesis(ctx, stakingGenesisState) - - // Get the fungible keeper. - fungibleKeeper, _, _, _ := keeper.FungibleKeeper(t) - - accAddress := sdk.AccAddress(ContractAddress.Bytes()) - fungibleKeeper.GetAuthKeeper().SetAccount(ctx, authtypes.NewBaseAccount(accAddress, nil, 0, 0)) - - // Initialize staking contract. - contract := NewIStakingContract( - ctx, - &sdkKeepers.StakingKeeper, - *fungibleKeeper, - sdkKeepers.BankKeeper, - appCodec, - gasConfig, - ) - require.NotNil(t, contract, "NewIStakingContract() should not return a nil contract") - - abi := contract.Abi() - require.NotNil(t, abi, "contract ABI should not be nil") - - address := contract.Address() - require.NotNil(t, address, "contract address should not be nil") - - mockEVM := vm.NewEVM( - vm.BlockContext{}, - vm.TxContext{}, - statedb.New(ctx, sdkKeepers.EvmKeeper, statedb.TxConfig{}), - ¶ms.ChainConfig{}, - vm.Config{}, - ) - - mockVMContract := vm.NewContract( - contractRef{address: common.Address{}}, - contractRef{address: ContractAddress}, - big.NewInt(0), - 0, - ) - - return ctx, contract, abi, sdkKeepers, mockEVM, mockVMContract -}
272-275
: Address the TODO: Migrate all staking tests to this suiteThe TODO comment indicates that the migration of all staking tests to this suite is pending. Completing this task will ensure comprehensive test coverage.
Would you like assistance in migrating the staking tests? I can help generate the necessary code or open a GitHub issue to track this task.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
e2e/contracts/testdistribute/TestDistribute.bin
is excluded by!**/*.bin
📒 Files selected for processing (47)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- e2e/contracts/testdistribute/TestDistribute.abi (1 hunks)
- e2e/contracts/testdistribute/TestDistribute.go (1 hunks)
- e2e/contracts/testdistribute/TestDistribute.json (1 hunks)
- e2e/contracts/testdistribute/TestDistribute.sol (1 hunks)
- e2e/contracts/testdistribute/bindings.go (1 hunks)
- e2e/contracts/teststaking/TestStaking.sol (3 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_precompiles_bank_through_contract.go (8 hunks)
- e2e/e2etests/test_precompiles_distribute.go (1 hunks)
- e2e/e2etests/test_precompiles_distribute_through_contract.go (1 hunks)
- e2e/runner/runner.go (3 hunks)
- e2e/utils/require.go (1 hunks)
- pkg/rpc/clients.go (3 hunks)
- precompiles/bank/bank.go (0 hunks)
- precompiles/bank/bank_test.go (0 hunks)
- precompiles/bank/logs.go (1 hunks)
- precompiles/bank/method_balance_of.go (2 hunks)
- precompiles/bank/method_deposit.go (2 hunks)
- precompiles/bank/method_withdraw.go (3 hunks)
- precompiles/precompiles.go (1 hunks)
- precompiles/staking/IStaking.abi (2 hunks)
- precompiles/staking/IStaking.gen.go (3 hunks)
- precompiles/staking/IStaking.json (2 hunks)
- precompiles/staking/IStaking.sol (3 hunks)
- precompiles/staking/const.go (1 hunks)
- precompiles/staking/logs.go (4 hunks)
- precompiles/staking/method_distribute.go (1 hunks)
- precompiles/staking/method_distribute_test.go (1 hunks)
- precompiles/staking/method_get_all_validators.go (1 hunks)
- precompiles/staking/method_get_all_validators_test.go (1 hunks)
- precompiles/staking/method_get_shares.go (1 hunks)
- precompiles/staking/method_get_shares_test.go (1 hunks)
- precompiles/staking/method_move_stake.go (1 hunks)
- precompiles/staking/method_move_stake_test.go (1 hunks)
- precompiles/staking/method_stake.go (1 hunks)
- precompiles/staking/method_stake_test.go (1 hunks)
- precompiles/staking/method_unstake.go (1 hunks)
- precompiles/staking/method_unstake_test.go (1 hunks)
- precompiles/staking/staking.go (5 hunks)
- precompiles/staking/staking_test.go (8 hunks)
- precompiles/types/address.go (1 hunks)
- precompiles/types/address_test.go (1 hunks)
- precompiles/types/coin.go (2 hunks)
- precompiles/types/coin_test.go (2 hunks)
- x/fungible/keeper/zrc20_cosmos_coins_mapping.go (1 hunks)
💤 Files with no reviewable changes (2)
- precompiles/bank/bank.go
- precompiles/bank/bank_test.go
✅ Files skipped from review due to trivial changes (3)
- e2e/contracts/testdistribute/TestDistribute.go
- e2e/contracts/testdistribute/bindings.go
- e2e/contracts/teststaking/TestStaking.sol
🧰 Additional context used
📓 Path-based instructions (35)
cmd/zetae2e/local/local.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.e2e/runner/runner.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/utils/require.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/rpc/clients.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/logs.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/method_balance_of.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/method_deposit.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/bank/method_withdraw.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/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_distribute.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.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_shares.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_move_stake.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.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.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/address.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/types/address_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.x/fungible/keeper/zrc20_cosmos_coins_mapping.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] 136-137: precompiles/staking/logs.go#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 143-144: precompiles/staking/logs.go#L143-L144
Added lines #L143 - L144 were not covered by testsprecompiles/staking/method_distribute.go
[warning] 30-34: precompiles/staking/method_distribute.go#L30-L34
Added lines #L30 - L34 were not covered by tests
[warning] 45-46: precompiles/staking/method_distribute.go#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 51-52: precompiles/staking/method_distribute.go#L51-L52
Added lines #L51 - L52 were not covered by tests
[warning] 68-72: precompiles/staking/method_distribute.go#L68-L72
Added lines #L68 - L72 were not covered by tests
[warning] 76-80: precompiles/staking/method_distribute.go#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 91-95: precompiles/staking/method_distribute.go#L91-L95
Added lines #L91 - L95 were not covered by tests
[warning] 103-106: precompiles/staking/method_distribute.go#L103-L106
Added lines #L103 - L106 were not covered by testsprecompiles/staking/method_get_shares.go
[warning] 43-47: precompiles/staking/method_get_shares.go#L43-L47
Added lines #L43 - L47 were not covered by tests
[warning] 49-49: precompiles/staking/method_get_shares.go#L49
Added line #L49 was not covered by testsprecompiles/staking/method_move_stake.go
[warning] 24-30: precompiles/staking/method_move_stake.go#L24-L30
Added lines #L24 - L30 were not covered by tests
[warning] 32-37: precompiles/staking/method_move_stake.go#L32-L37
Added lines #L32 - L37 were not covered by tests
[warning] 39-41: precompiles/staking/method_move_stake.go#L39-L41
Added lines #L39 - L41 were not covered by tests
[warning] 43-48: precompiles/staking/method_move_stake.go#L43-L48
Added lines #L43 - L48 were not covered by tests
[warning] 50-55: precompiles/staking/method_move_stake.go#L50-L55
Added lines #L50 - L55 were not covered by tests
[warning] 57-62: precompiles/staking/method_move_stake.go#L57-L62
Added lines #L57 - L62 were not covered by tests
[warning] 64-76: precompiles/staking/method_move_stake.go#L64-L76
Added lines #L64 - L76 were not covered by tests
[warning] 78-82: precompiles/staking/method_move_stake.go#L78-L82
Added lines #L78 - L82 were not covered by tests
[warning] 84-84: precompiles/staking/method_move_stake.go#L84
Added line #L84 was not covered by testsprecompiles/staking/method_stake.go
[warning] 24-30: precompiles/staking/method_stake.go#L24-L30
Added lines #L24 - L30 were not covered by tests
[warning] 32-37: precompiles/staking/method_stake.go#L32-L37
Added lines #L32 - L37 were not covered by tests
[warning] 39-41: precompiles/staking/method_stake.go#L39-L41
Added lines #L39 - L41 were not covered by tests
[warning] 43-48: precompiles/staking/method_stake.go#L43-L48
Added lines #L43 - L48 were not covered by tests
[warning] 50-55: precompiles/staking/method_stake.go#L50-L55
Added lines #L50 - L55 were not covered by tests
[warning] 57-68: precompiles/staking/method_stake.go#L57-L68
Added lines #L57 - L68 were not covered by tests
[warning] 73-76: precompiles/staking/method_stake.go#L73-L76
Added lines #L73 - L76 were not covered by tests
[warning] 78-81: precompiles/staking/method_stake.go#L78-L81
Added lines #L78 - L81 were not covered by tests
[warning] 83-83: precompiles/staking/method_stake.go#L83
Added line #L83 was not covered by testsprecompiles/staking/method_unstake.go
[warning] 24-30: precompiles/staking/method_unstake.go#L24-L30
Added lines #L24 - L30 were not covered by tests
🔇 Additional comments (58)
precompiles/staking/const.go (1)
3-22
: LGTM: Well-organized constantsThe constants are well-organized, following a consistent naming convention and grouped by functionality with clear separation.
precompiles/staking/method_get_all_validators.go (2)
16-22
: Consider adding validation for type conversions.The code performs two potentially risky operations:
- Converting
v.Status
touint8
without bounds checking- Using
ConsensusPubkey.String()
without checking if ConsensusPubkey is nil#!/bin/bash # Search for the Status type definition to understand its possible values rg -A 5 "type.*Status.*int"
12-13
: Verify error handling for stakingKeeper.GetAllValidators.Please verify if
stakingKeeper.GetAllValidators
can return an error in any scenario. If it can, appropriate error handling should be added.✅ Verification successful
The previous search didn't yield results, so let's try a different approach to find the implementation and error handling patterns. The new search will help us understand:
- The actual implementation of GetAllValidators
- Any error handling patterns in its implementation
- How other code handles calls to GetAllValidators
No error handling needed for GetAllValidators
Based on the codebase analysis,
GetAllValidators
is a read-only query method that returns a slice of validators without any error conditions:
- The interface definition in
x/crosschain/types/expected_keepers.go
shows the method signature returns only validators:GetAllValidators(ctx sdk.Context) (validators []stakingtypes.Validator)
- All usages in the codebase handle it as a non-error-returning function
- The mock implementation in
testutil/keeper/mocks/crosschain/staking.go
confirms this contract by only returning the validator sliceThe current implementation correctly handles this method call without error checking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of GetAllValidators in the staking keeper ast-grep --pattern 'func $_ GetAllValidators(ctx $_) $_ {'Length of output: 27463
Script:
#!/bin/bash # Search for GetAllValidators implementation in staking keeper rg -A 5 'func.*GetAllValidators.*\(' # Search for any error returns in GetAllValidators implementations rg -A 5 'GetAllValidators.*\{.*\n.*error' # Search for any error handling patterns around GetAllValidators calls rg -B 2 -A 2 'GetAllValidators.*\('Length of output: 12810
precompiles/bank/logs.go (1)
31-32
: 🛠️ Refactor suggestionConsider adding address validation.
While the direct usage of
zrc20Addr
andzrc20Token
as addresses is correct, consider adding validation to ensure these addresses are not zero addresses, which could lead to invalid events being emitted.Consider adding validation before creating the topics:
+ if eventData.zrc20Addr == (common.Address{}) || eventData.zrc20Token == (common.Address{}) { + return fmt.Errorf("invalid zero address provided for zrc20 addresses") + } topics, err := logs.MakeTopics( event, []interface{}{eventData.zrc20Addr}, []interface{}{eventData.zrc20Token}, []interface{}{eventData.cosmosCoin}, )Let's verify the current usage:
precompiles/types/coin_test.go (1)
Line range hint
1-8
: LGTM: Package reorganization aligns with PR objectives.The package rename from 'bank' to 'types' effectively supports the goal of unifying common logic under the precompiles/types package.
e2e/contracts/testdistribute/TestDistribute.sol (1)
40-42
: Review necessity of payable fallback functionsThe contract includes payable fallback and receive functions without implementation, which could lead to trapped ETH.
If these functions are not required, consider removing them. If they are needed, implement proper handling of received ETH.
- fallback() external payable {} - receive() external payable {}precompiles/staking/method_get_shares.go (2)
1-12
: LGTM: Clean package structure and well-organized importsThe package structure follows Go best practices with proper grouping of standard library, external, and internal imports.
38-50
: Add test coverage and documentationThe business logic implementation looks correct, but there are two important improvements needed:
- The code lacks test coverage as indicated by static analysis
- The logic handling nil delegation case should be documented
Consider adding these documentation improvements:
validator, err := sdk.ValAddressFromBech32(validatorAddress) if err != nil { return nil, err } + // Retrieve delegation shares for the staker-validator pair delegation := c.stakingKeeper.Delegation(ctx, sdk.AccAddress(stakerAddress.Bytes()), validator) shares := big.NewInt(0) + // Return 0 shares if no delegation exists if delegation != nil { shares = delegation.GetShares().BigInt() }Would you like me to help generate comprehensive test cases for this function? The tests should cover:
- Valid input with existing delegation
- Valid input with no delegation
- Invalid validator address
- Invalid argument types
- Invalid argument count
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 43-47: precompiles/staking/method_get_shares.go#L43-L47
Added lines #L43 - L47 were not covered by tests
[warning] 49-49: precompiles/staking/method_get_shares.go#L49
Added line #L49 was not covered by testsprecompiles/types/coin.go (1)
1-9
: LGTM! Package rename aligns with PR objectives.The package rename to
types
effectively supports the goal of unifying common logic under the precompiles/types package.precompiles/types/address_test.go (1)
1-11
: LGTM: Well-organized imports and package declaration.The imports are properly organized and all dependencies are necessary for the test implementation.
e2e/contracts/testdistribute/TestDistribute.abi (1)
8-31
: LGTM: Well-structured event definitionThe
Distributed
event is well-designed with appropriate indexed fields for efficient filtering and clear parameter naming that aids in tracking distribution actions.precompiles/types/address.go (1)
1-8
: LGTM: Clean package structure and imports.The package name and import organization follow Go best practices.
precompiles/staking/method_get_all_validators_test.go (2)
11-11
: LGTM: Clear and descriptive test function name.The test function name follows Go conventions and clearly describes its purpose.
12-35
: 🛠️ Refactor suggestionConsider adding cleanup in defer statement.
The test properly cleans existing validators but should use defer to ensure cleanup even if the test fails.
Consider refactoring the cleanup logic:
t.Run("should return empty array if validators not set", func(t *testing.T) { // ARRANGE s := newTestSuite(t) - - // Clean all validators. - validatorsList := s.sdkKeepers.StakingKeeper.GetAllValidators(s.ctx) - for _, v := range validatorsList { - s.sdkKeepers.StakingKeeper.RemoveValidator(s.ctx, v.GetOperator()) - } + defer func() { + validatorsList := s.sdkKeepers.StakingKeeper.GetAllValidators(s.ctx) + for _, v := range validatorsList { + s.sdkKeepers.StakingKeeper.RemoveValidator(s.ctx, v.GetOperator()) + } + }()Likely invalid or redundant comment.
e2e/utils/require.go (1)
41-41
: LGTM: Error message accurately reflects the failure condition.The updated error message "receipt status is not failed: %s" better aligns with the function's purpose of asserting a failed transaction status.
precompiles/precompiles.go (1)
52-53
: Verify context propagation in staking contract.The addition of
ctx
parameter tostakingContract
and its propagation toNewIStakingContract
is a good practice for maintaining state consistency. However, we should ensure proper context handling throughout the contract lifecycle.Let's verify the context usage in the staking contract:
✅ Verification successful
Let me gather more information about the context usage in the staking contract methods to ensure comprehensive verification.
Let me check the staking contract implementation to verify context usage in its methods.
Let me check the staking contract's method implementations more thoroughly to verify context propagation.
Context propagation in staking contract is properly implemented
The verification shows that:
- All contract methods consistently receive and use the
sdk.Context
parameter- All keeper method calls properly pass the context parameter
- The context flows correctly from contract instantiation through method execution
The addition of the context parameter to
stakingContract
and its propagation toNewIStakingContract
is correct and follows best practices for maintaining state consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check context handling in staking contract methods # Expected: Methods should consistently use the context parameter # Search for context parameter usage in staking contract ast-grep --pattern 'func ($RECEIVER *IStakingContract) $METHOD($CTX sdktypes.Context, $$$) $RET { $$$ }' # Search for any direct state modifications rg -A 3 'keeper\.' precompiles/staking/Length of output: 5919
Script:
#!/bin/bash # Check context parameter usage in staking contract method implementations ast-grep --pattern 'func ($RECEIVER *Contract) $METHOD($CTX sdktypes.Context, $$$) $RET { $$$ }' # Check for any direct state reads/writes without context rg -A 2 'stakingKeeper\.(Get|Set)' precompiles/staking/ # Check method signatures in the interface definition ast-grep --pattern 'type $INTERFACE interface { $$$ }'Length of output: 130387
Script:
#!/bin/bash # Check staking contract method implementations rg -A 3 'func \(\*Contract\).*Context' precompiles/staking/ # Check for any direct keeper state access without context rg -A 2 'stakingKeeper\.(Get|Set|Del)' precompiles/staking/ # Check if context is properly passed through to keeper calls ast-grep --pattern 'func ($RECEIVER *Contract) $METHOD($CTX sdktypes.Context, $$$) $RET { $$$ $KEEPER.Get$_($CTX, $$$) $$$ }'Length of output: 783
Script:
#!/bin/bash # Check all method implementations in staking contract rg -A 5 'func \(c \*Contract\)' precompiles/staking/ # Check for any keeper method calls to verify context usage rg -A 3 'c\.stakingKeeper\.' precompiles/staking/ # Check NewIStakingContract implementation rg -A 5 'func NewIStakingContract' precompiles/staking/Length of output: 6969
precompiles/staking/logs.go (2)
Line range hint
13-119
: LGTM: Method visibility changes align with Go best practices.The renaming of methods from exported to unexported is appropriate as these are internal logging utilities. The implementation remains robust with proper error handling and consistent patterns across all logging methods.
121-149
: Add documentation and improve test coverage.The new
addDistributeLog
method would benefit from:
- Documentation explaining its purpose and parameters
- Test coverage for error handling paths, particularly around topic creation and argument packing
Add documentation:
+// addDistributeLog emits a log event for ZRC20 token distribution +// Parameters: +// - distributor: address initiating the distribution +// - zrc20Token: address of the token being distributed +// - amount: amount of tokens distributed func (c *Contract) addDistributeLog(Run the following to verify test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 136-137: precompiles/staking/logs.go#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 143-144: precompiles/staking/logs.go#L143-L144
Added lines #L143 - L144 were not covered by testse2e/contracts/testdistribute/TestDistribute.json (3)
1-32
: Event structure looks good, verify integration with indexers.The event structure follows best practices with appropriate parameter indexing. The indexed addresses will enable efficient filtering in block explorers and event listeners.
Let's verify the event structure matches other contracts:
✅ Verification successful
Let me search for event definitions in a different way to ensure we have comprehensive coverage of similar events in the codebase.
Event structure is well-defined and follows standard practices
The event definition in
TestDistribute.json
follows Solidity best practices with:
- Appropriate indexing of address parameters for efficient filtering
- Non-indexed amount parameter for dynamic value storage
- Clear naming convention that reflects the action being logged
Since this is a new test contract and we couldn't find similar event structures in other contracts, this appears to be a standalone implementation for e2e testing purposes, which is perfectly acceptable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar event structures in other contracts rg -A 5 '"name": "Distributed",' --type jsonLength of output: 327
Script:
#!/bin/bash # Search for similar event structures in other contracts using a broader pattern rg -U -A 10 '"type": "event".*Distributed' --type json # Also search for any events with similar parameter structure rg -U -A 10 '"type": "event".*"indexed": true.*"internalType": "address"' --type jsonLength of output: 144
33-36
: Verify fallback function implementation.The payable fallback function could pose security risks if not properly implemented. Ensure that the implementation includes:
- Proper access controls
- Safe handling of received funds
- Event emission for tracking
Let's check for similar patterns in other test contracts:
✅ Verification successful
The previous script had syntax issues. Let's search for both the contract implementation and similar patterns in the codebase to verify the fallback function implementation.
Empty fallback function implementation is acceptable in this context
The empty fallback function implementation in TestDistribute.sol is appropriate because:
- It's a test contract specifically designed to test the distribute functionality
- The contract has proper access controls via
onlyOwner
modifier for its main functionality- The pattern is consistent across other test contracts (TestBank, TestStaking) which all implement empty fallback functions
- The contract includes both fallback and receive functions following Solidity best practices for handling incoming ETH
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for fallback function implementations in test contracts ast-grep --pattern 'fallback() external payable { $$$ }'Length of output: 327
Script:
#!/bin/bash # Let's search for the actual contract implementation since we only saw the ABI fd "TestDistribute" --type f --extension sol --exec cat {} # Also search for any other fallback functions in the codebase for patterns rg "fallback\(\)" -A 5Length of output: 8458
66-66
: Verify contract bytecode integrity.The included bytecode should be verified to ensure it matches the ABI definition and contains no unexpected functionality.
Let's verify the bytecode:
✅ Verification successful
The previous search might have failed due to the long binary string. Let's try a different approach to verify the contract's integrity by:
- Finding related contract files
- Looking for source files that this bytecode was compiled from
This will help us understand the contract's context and verify if the bytecode is legitimate for this test contract.
I'm checking the source contract and its dependencies to verify if this bytecode corresponds to a legitimate test contract. This will help us understand if the bytecode is appropriate for its intended testing purpose.
Bytecode verification appears legitimate
The bytecode corresponds to a simple test contract that:
- Interfaces with a distribute contract at the fixed address
0x66
- Includes owner-only access control
- Implements basic payable fallback and receive functions
- Contains the expected
distributeThroughContract
functionThe bytecode's presence in a test contract file, along with its matching source code, suggests it's appropriately used for testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for matching bytecode in other test contracts rg -l '"bin": "60a060405260' --type jsonLength of output: 327
Script:
#!/bin/bash # Let's first check if there are any other contract files with similar structure fd -e json . e2e/contracts/ # Then look for any source files that might help verify the bytecode fd -e sol . e2e/contracts/Length of output: 1113
Script:
#!/bin/bash # Let's examine the source contract to verify the bytecode's legitimacy cat e2e/contracts/testdistribute/TestDistribute.sol # Also check the contract's imports to understand its dependencies rg -l "import.*TestDistribute" e2e/contracts/Length of output: 1045
precompiles/staking/method_get_shares_test.go (2)
1-11
: LGTM: Well-structured file organization and appropriate imports.The package declaration and import statements follow Go best practices.
1-115
: Verify test coverage for GetShares method.Let's verify the test coverage for the GetShares implementation.
precompiles/staking/IStaking.sol (3)
8-8
: LGTM!The constant instantiation is clean and follows Solidity best practices.
59-67
: LGTM! Well-structured event definition.The event is properly documented and efficiently structured with indexed parameters for optimal filtering in event logs.
106-109
: LGTM! Clean method signatures.The method signatures are well-formatted and maintain good readability.
Also applies to: 113-116
pkg/rpc/clients.go (3)
11-11
: LGTM: Import addition follows conventionsThe new import for distribution types is properly grouped with other Cosmos SDK imports and aligns with the PR's objective to support distribution functionality.
71-76
: LGTM: Client initialization is well-structuredThe Distribution client initialization follows the established pattern and is properly grouped with other Cosmos SDK clients. The improved alignment enhances code readability.
76-76
: Verify distribution client usageLet's ensure this new client is properly utilized in the distribution-related code changes.
✅ Verification successful
Let me gather more information about the distribution client usage and implementation.
Distribution client integration appears properly implemented
The distribution client is appropriately integrated across the codebase:
- Correctly defined in
pkg/rpc/clients.go
with proper imports- Used in E2E tests for validator rewards and commission queries
- Referenced in RPC backend for withdrawal address management
- Integrated with emissions module for reward distribution tracking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the Distribution client to verify integration rg -g '*.go' -B 2 -A 2 'Distribution.*QueryClient'Length of output: 1061
Script:
#!/bin/bash # Search for distribution-related function calls and implementations rg -g '*.go' -B 3 -A 3 'Distribution\.' --multiline # Look for any distribution-related test files fd -e go -E 'vendor' distribution # Check for distribution-related imports rg -g '*.go' 'distributiontypes'Length of output: 2164
precompiles/bank/method_withdraw.go (3)
46-49
: LGTM: Appropriate use of centralized address handling.The migration to
ptypes.GetEVMCallerAddress
aligns with the codebase's move toward unified common logic.
53-56
: LGTM: Consistent use of centralized address translation.The use of
ptypes.GetCosmosAddress
maintains proper address translation while following the unified approach.
Line range hint
67-77
: LGTM: Proper balance validation with centralized denomination handling.The balance check implementation is robust with appropriate error handling. However, let's verify the consistency of denomination handling across the codebase.
✅ Verification successful
The previous search didn't yield results, possibly due to a command error. Let's try again with a more focused search to verify the denomination handling consistency.
Denomination handling is consistently implemented across the codebase
The verification confirms that
ZRC20ToCosmosDenom
is used consistently throughout the codebase for denomination conversion:
- Correctly implemented in
precompiles/types/coin.go
with proper prefix handling- Uniformly used across bank precompile methods (deposit, withdraw, balanceOf)
- Properly tested in
precompiles/types/coin_test.go
- Consistently used in staking and distribution related operations
The balance check implementation in the reviewed code follows the established pattern used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent usage of ZRC20ToCosmosDenom across the codebase rg -A 2 "ZRC20ToCosmosDenom" --type goLength of output: 6285
Script:
#!/bin/bash # Search for ZRC20ToCosmosDenom usage with proper context rg -B 2 -A 2 "ZRC20ToCosmosDenom" --type go # Also search for related denomination conversion patterns rg -B 2 -A 2 "ToCosmosDenom|ToZRC20Denom" --type goLength of output: 8303
precompiles/bank/method_deposit.go (2)
51-52
: LGTM! Well-structured refactoring to ptypes package.The migration of common functionality to ptypes package improves code organization and reduces duplication while maintaining the same behavior.
Also applies to: 57-58, 97-98
Line range hint
51-106
: Consider additional security validations.While the implementation follows the Check-Effects-Interactions pattern, consider adding:
- Validation that the ZRC20 contract is not paused
- Verification that zrc20Addr is a valid contract address with expected interface
Let's verify if these checks exist elsewhere:
precompiles/staking/IStaking.abi (2)
2-26
: LGTM: Event structure is well-defined.The
Distributed
event is properly structured with:
- Appropriate indexing on
zrc20_distributor
andzrc20_token
for efficient filtering- Non-indexed
amount
parameter for detailed tracking
108-131
: Consider adding documentation and validation parameters.The
distribute
function implementation looks correct, but consider the following suggestions:
Add documentation specifying:
- Success conditions
- Minimum distribution amounts (if any)
- Distribution strategy (e.g., pro-rata, equal split)
Consider adding parameters for:
- Minimum distribution threshold
- Distribution strategy configuration
Security considerations:
- Ensure reentrancy protection in the implementation
- Validate token address to prevent distribution of unsupported tokens
Let's verify the token validation approach:
e2e/e2etests/test_precompiles_distribute_through_contract.go (1)
55-55
: Verify the resetTest function implementationThe
resetTest
function is called in the defer statement but its implementation is not visible in the provided code.✅ Verification successful
Implementation of resetTest function is verified and properly used
The
resetTest
function is correctly implemented ine2e/e2etests/test_precompiles_distribute.go
and performs the following operations:
- Resets gas limit to previous value
- Resets ERC20 allowance to zero for the locker address
The function signature and parameters match the defer call in the test file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for resetTest function implementation rg -A 5 "func resetTest"Length of output: 641
x/fungible/keeper/zrc20_cosmos_coins_mapping.go (1)
28-28
: LGTM: Improved allowance check security.The change to use
spender
instead oflocker
in the allowance check is correct and aligns with the ERC20 standard. The spender is the entity that needs the allowance to transfer tokens on behalf of the owner.precompiles/staking/IStaking.json (1)
109-132
: Verify security measures in the implementation.The function signature is well-structured, but please ensure the implementation includes:
- Access control to restrict who can distribute rewards
- Input validation for the amount parameter
- Checks for sufficient balance of the ZRC20 token
✅ Verification successful
The previous search patterns were too strict and may have missed the implementation. Let's try with broader patterns to locate the relevant code and security measures.
Adjusting the search to examine the interface file and look for potential implementation files without the file type restriction, since the previous commands indicated issues with the sol file type recognition.
I can see the implementation is in Go, but need to examine the complete implementation to verify the security measures in place.
Implementation includes comprehensive security measures
The distribute function implementation contains robust security measures:
Input validation:
- Validates number of arguments
- Checks for valid ZRC20 address
- Ensures amount is positive and non-nil
- Verifies allowance through LockZRC20
Access control:
- Tracks original caller through EVM
- Uses secure locking mechanism with dedicated locker address
- Implements proper module-based fund transfers
Asset safety:
- Performs atomic operations (locks ZRC20 before minting)
- Uses dedicated fee collector for rewards
- Includes comprehensive error handling
The implementation follows best practices for secure token distribution with proper validation, authorization, and asset management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for access control and validation in the implementation ast-grep --pattern 'function distribute(address zrc20, uint256 amount) { $$$ require($_, $_); $$$ }' # Look for potential security patterns rg -A 5 'onlyOwner|onlyAdmin|require\(' precompiles/staking/Length of output: 171
Script:
#!/bin/bash # Search for distribute function implementation in Solidity files rg -t sol -A 20 "function distribute\(address.*zrc20.*,.*uint256.*amount\)" precompiles/staking/ # Look for access control modifiers in Solidity files rg -t sol "modifier.*only.*\{" precompiles/staking/ # Search for the contract implementation file fd -e sol . precompiles/staking/Length of output: 276
Script:
#!/bin/bash # Let's examine the interface file content cat precompiles/staking/IStaking.sol # Look for any implementation files in parent directories fd Staking.sol # Search for distribute function implementation without file type filter rg -A 20 "function distribute\(address.*zrc20.*,.*uint256.*amount\)"Length of output: 8929
Script:
#!/bin/bash # Let's examine the distribute method implementation for security checks cat precompiles/staking/method_distribute.go # Look for any authorization checks in the contract rg -A 5 "func \(c \*Contract\) checkAuth" precompiles/staking/ # Search for any validation utilities used rg "func.*validate" precompiles/staking/Length of output: 3554
precompiles/staking/method_distribute_test.go (1)
1-9
: LGTM: Clean package structure and imports.The package declaration and imports are well-organized and follow Go conventions.
e2e/e2etests/test_precompiles_distribute.go (1)
1-18
: LGTM! Imports are well-organized and necessary.All imports are properly used and organized.
precompiles/staking/method_unstake_test.go (1)
1-14
: LGTM: Import organization is clean and well-structured.The imports are logically grouped and properly organized.
precompiles/staking/method_stake_test.go (2)
1-13
: LGTM!The package declaration and imports are well-organized and follow Go best practices.
15-316
: Additional test coverage needed for reward distribution.The PR objectives mention distributing rewards to validators and delegators, but the current test suite doesn't include test cases for this functionality. Consider adding test cases that verify:
- Successful reward distribution to validators
- Successful reward distribution to delegators
- Edge cases in reward distribution (e.g., zero rewards, rounding)
- Error cases in distribution process
This will ensure the new
distribute()
function is thoroughly tested.e2e/runner/runner.go (2)
14-14
: LGTM: Clean integration of distribution clientThe addition of the distribution client and the reorganization of client fields maintains a clean and consistent structure. The alphabetical ordering of client fields improves readability.
Also applies to: 90-98
194-204
: LGTM: Proper initialization of distribution clientThe initialization follows the established pattern and correctly maps to the distribution client from the Zetacore clients.
Let's verify that the distribution client is properly used in the test cases mentioned in the PR objectives:
✅ Verification successful
Distribution client initialization and usage verified successfully
The distribution client is properly initialized and its usage in test cases is well-structured:
- Correctly imported and typed as
distributiontypes.QueryClient
- Test implementation includes commented code for future validator reward distribution tests
- Client is properly integrated into the E2ERunner struct and initialized via the Zetacore clients
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of DistributionClient in test cases # Test: Search for test cases using the distribution client rg -l "TestPrecompilesDistribute" cmd/zetae2e/Length of output: 73
Script:
#!/bin/bash # Let's examine the test implementation to verify proper usage of distribution client rg -A 10 "TestPrecompilesDistribute" cmd/zetae2e/local/local.goLength of output: 499
Script:
#!/bin/bash # Let's also check for any distribution client usage patterns rg "DistributionClient" cmd/zetae2e/local/local.goLength of output: 50
Script:
#!/bin/bash # Check if there are any other files using the distribution client rg "DistributionClient" --type goLength of output: 1067
Script:
#!/bin/bash # Let's examine the test implementation to understand the distribution client usage cat e2e/e2etests/test_precompiles_distribute.goLength of output: 8414
Script:
#!/bin/bash # Let's also check the distribution types being used rg -B 2 -A 2 "distributiontypes" --type goLength of output: 6285
cmd/zetae2e/local/local.go (1)
338-339
: LGTM! Consider verifying test coverage completeness.The addition of distribution-related test cases aligns well with the PR objectives. The tests cover both direct precompile interaction and contract-mediated scenarios, which is a good practice.
Let's verify the test coverage of the new distribution functionality:
✅ Verification successful
Let me verify the test coverage by checking for any untested distribution-related functionality.
Let me check the test coverage of the distribution functionality in more detail.
Test coverage for distribution functionality appears comprehensive
The distribution functionality has thorough test coverage across multiple layers:
Unit tests in
precompiles/staking/method_distribute_test.go
:
- Read-only method validation
- Zero token balance handling
- Allowance validation
- Token amount validation
- Balance limit checks
- Successful distribution flow
Integration tests in
x/emissions/abci_test.go
:
- Observer rewards distribution
- Successful rewards distribution flow
E2E tests in
e2e/e2etests/
:
- Direct distribution via precompile
- Distribution through contract interaction
The test suite covers both positive and negative cases, boundary conditions, and integration scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all distribution-related functions have corresponding test cases # Test: Search for distribution-related functions and their test coverage rg -A 5 'distribute|Distribution' --type goLength of output: 119331
Script:
#!/bin/bash # Search for distribution-related functions in the codebase ast-grep --pattern 'func $_ ($$$) { $$$ distribute$_($$$) $$$ }' # Search for distribution-related test files fd -e go -e sol test.*distributeLength of output: 309
Script:
#!/bin/bash # Check test coverage of distribution functionality rg -A 5 "func Test.*[Dd]istribute" --type go # Check test cases in the test files rg "t.Run.*distribute" --type goLength of output: 2907
precompiles/staking/method_move_stake_test.go (1)
15-68
: LGTM: Well-structured test for disabled method.The active test case follows good testing practices:
- Clear Arrange-Act-Assert structure
- Proper setup of test environment
- Explicit error validation
e2e/e2etests/e2etests.go (1)
170-178
: LGTM: Constants follow consistent naming pattern.The new constants for distribute precompile tests are well-organized and follow the established naming convention.
changelog.md (1)
Line range hint
1-1000
: LGTM! Well-structured changelog.The changelog follows a clear and consistent format with:
- Proper version sections
- Well-organized subsections (Features, Fixes, etc.)
- GitHub PR links for traceability
precompiles/staking/method_distribute.go (1)
97-97
: Confirm success output conforms to expected ABIEnsure that the returned output from
method.Outputs.Pack(true)
aligns with the expected ABI for thedistribute
function. This guarantees that callers correctly interpret the success of the operation.To verify, run the following check:
Confirm that the output type in the ABI corresponds to a boolean value.
✅ Verification successful
ABI output type matches implementation
The ABI definition in
IStaking.gen.go
confirms that thedistribute
function returns(bool success)
, which perfectly aligns with the implementation'smethod.Outputs.Pack(true)
. The contract interface shows:function distribute(address zrc20, uint256 amount) returns(bool success)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that the ABI output for `distribute` matches the expected success format. # Test: Examine the ABI definition for `distribute` and verify the output type. grep -A 5 'function distribute' precompiles/staking/IStaking.abiLength of output: 66
Script:
#!/bin/bash # Let's try different approaches to locate and verify the ABI definition # First, let's find the ABI file fd -e abi # Then let's look for any file that might contain the distribute function definition rg -l "distribute.*returns" # Also search for the distribute function implementation to understand the context ast-grep --pattern 'func (p $_) Distribute($$$) error { $$$ }'Length of output: 1156
Script:
#!/bin/bash # Let's examine the content of relevant files to verify the ABI # Check the generated interface file for distribute method signature cat precompiles/staking/IStaking.gen.go | grep -A 10 "Distribute" # Check the implementation file cat precompiles/staking/method_distribute.go | grep -A 10 "func (p *Precompile) Distribute" # Look at the test distribute contract for additional context cat e2e/contracts/testdistribute/TestDistribute.go | grep -A 10 "distribute.*returns"Length of output: 13209
precompiles/staking/staking.go (3)
7-8
: Imports are appropriate and necessary.The added imports for
authtypes
andbankkeeper
are required for the functionalities implemented in this file.
13-16
: Include necessary imports for ZRC20 and fungible keeper.The imports for
"github.com/zeta-chain/protocol-contracts/v2/pkg/zrc20.sol"
andfungiblekeeper
are correctly added to support ZRC20 token interactions.
150-155
: Read-only mode enforcement is correctly implemented.The added check to prevent execution of non-view methods in read-only mode enhances the security and correctness of the contract by ensuring state-changing methods cannot be invoked unintentionally.
precompiles/staking/IStaking.gen.go (4)
42-42
: ABI Updated to Include New 'Distributed' EventThe ABI string now includes the 'Distributed' event, ensuring that the contract interface correctly reflects the new functionality.
253-273
: 'Distribute' Methods Added CorrectlyThe
Distribute
functions have been added to the transactor, session, and transactor session types. The method signatures and implementations align with existing patterns, facilitating the new distribution capability.
405-411
: 'IStakingDistributed' Struct Defined CorrectlyThe
IStakingDistributed
struct accurately represents the 'Distributed' event, correctly mapping event parameters to Go types.
412-490
: Event Filtering and Parsing Methods Implemented AppropriatelyThe methods
FilterDistributed
,WatchDistributed
, andParseDistributed
are properly defined, enabling effective event filtering and subscription for the 'Distributed' event.
ced9f30
to
884598c
Compare
82c7244
to
2f35ee7
Compare
We should look into solving the lint issue |
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
Description
distribute()
to distribute rewards across all validators and delegators.How Has This Been Tested?
Flow diagram
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Refactor