Skip to content
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

Merged
merged 23 commits into from
Oct 30, 2024
Merged

Conversation

fbac
Copy link
Contributor

@fbac fbac commented Oct 17, 2024

Description

  • Create a function distribute() to distribute rewards across all validators and delegators.
  • Refactor staking precompile to be divided in a similar fashion as bank.
  • Unify common logic under the precompiles/types package.
  • Minor fixes in bank emitted logs.

How Has This Been Tested?

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Flow diagram

               0. allow to spend ZRC20                                                                                              
        ┌─────────────────────────────────┐                                                                                         
        │                                 │                                                                                         
        │                                 │        4. Mint Coins              5. Send Coins        After 1 block                    
  ┌─────┴─────┐                  ┌────────▼───────┐            ┌─────────────────┐     ┌─────────────┐      ┌────────────┐          
  │           │                  │                │            │                 │     │             │      │            │          
  │   EOA     ├─────────────────►│Staking contract├────────────► Fungible Module ├─────►Fee Collector├──────► Validators │          
  │           │ 1. distribute()  │                │            │                 │     │             │      │            │          
  └───────────┘                  └────────┬───────┘            └─────────────────┘     └─────────────┘      └────────────┘          
                                          │                                                                                         
                                          │                                                                                         
                                          │ 2. LockZRC20()                                                                          
                                          │                                                                                         
                                          │                                                                                         
                                 ┌────────▼───────┐           ┌───────────────┐                                                     
                                 │                │           │               │                                                     
                                 │ ZRC20 Lock API ├───────────► Locker (bank) │                                                     
                                 │                │           │               │                                                     
                                 └────────────────┘           └───────────────┘                                                     
                                                                                                                                    
                                         3. transferFrom(spender, locker)                                                                                                                                                                                 

Summary by CodeRabbit

  • New Features

    • Introduced support for stateful precompiled contracts.
    • Added authenticated calls for smart contracts.
    • Integrated Bitcoin inscription support on testnet.
    • Added a distribution functionality for staking contracts.
    • Enhanced staking contracts with new events and methods.
  • Bug Fixes

    • Resolved voting issues for discarded keygen ballots.
    • Improved error handling for invalid Solana gateway addresses.
  • Tests

    • Expanded testing coverage for stateful precompiled contracts.
    • Added new tests for distribution and staking functionalities.
  • Documentation

    • Updated changelog to reflect new features and improvements.
  • Refactor

    • Improved code organization and removed unused code across various modules.

@fbac fbac force-pushed the feat/staking-distribute-function branch from b4c51d7 to afac1d1 Compare October 17, 2024 17:00
Copy link
Contributor

coderabbitai bot commented Oct 24, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

This 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

File Path Change Summary
changelog.md Updated to include new features like stateful precompiled contracts, a common RPC package, and Bitcoin inscription support. Refactoring and CI improvements noted.
cmd/zetae2e/local/local.go Added new test cases for precompiled contracts, enhancing testing coverage without altering existing logic.
e2e/contracts/testdistribute/*.abi Introduced new ABI for TestDistribute contract, including a constructor, event, and functions for token distribution.
e2e/contracts/testdistribute/*.go Created Go bindings for TestDistribute, including deployment and event handling functionalities.
e2e/contracts/teststaking/TestStaking.sol Reformatted function signatures for clarity without altering functionality.
e2e/e2etests/e2etests.go Added new tests for stateful precompiled contracts.
e2e/e2etests/test_precompiles_* Introduced new test files for distribution functionalities, validating various scenarios for token distribution and balance checks.
e2e/runner/runner.go Updated E2ERunner struct to include a new DistributionClient field.
pkg/rpc/clients.go Added Distribution field to Clients struct for interacting with distribution functionalities.
precompiles/bank/*.go Removed unused functions and refactored existing methods for address handling and balance checks.
precompiles/staking/*.go Introduced new staking methods and constants, enhancing staking functionalities and improving gas management.
precompiles/types/*.go Refactored address handling methods and constants for improved organization and error handling.

Possibly related issues

Possibly related PRs

Suggested labels

no-changelog

Suggested reviewers

  • kingpinXD
  • swift1337
  • skosito
  • brewmaster012
  • lumtis

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@zeta-chain zeta-chain deleted a comment from codecov bot Oct 24, 2024
@fbac fbac marked this pull request as ready for review October 24, 2024 18:21
@fbac fbac added the stateful-precompile A new stateful precompile contract that is user facing label Oct 24, 2024
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 40.28777% with 249 lines in your changes missing coverage. Please review.

Project coverage is 64.33%. Comparing base (df8b0e8) to head (14289d6).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
precompiles/staking/method_move_stake.go 0.00% 53 Missing ⚠️
precompiles/staking/method_stake.go 0.00% 49 Missing ⚠️
precompiles/staking/method_unstake.go 0.00% 46 Missing ⚠️
precompiles/staking/method_distribute.go 46.15% 28 Missing and 7 partials ⚠️
precompiles/types/address.go 37.50% 15 Missing ⚠️
precompiles/bank/method_withdraw.go 33.33% 10 Missing ⚠️
precompiles/staking/staking.go 82.60% 6 Missing and 2 partials ⚠️
precompiles/bank/method_deposit.go 46.15% 7 Missing ⚠️
precompiles/staking/logs.go 70.00% 4 Missing and 2 partials ⚠️
precompiles/staking/method_get_shares.go 79.31% 6 Missing ⚠️
... and 4 more
Additional details and impacted files

Impacted file tree graph

@@             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     
Files with missing lines Coverage Δ
pkg/rpc/clients.go 68.57% <100.00%> (+0.92%) ⬆️
precompiles/bank/logs.go 72.72% <100.00%> (ø)
precompiles/precompiles.go 100.00% <100.00%> (ø)
precompiles/staking/method_get_all_validators.go 100.00% <100.00%> (ø)
x/fungible/keeper/zrc20_cosmos_coins_mapping.go 69.38% <100.00%> (+0.31%) ⬆️
precompiles/bank/bank.go 75.00% <60.00%> (+5.00%) ⬆️
precompiles/prototype/prototype.go 69.03% <71.42%> (ø)
precompiles/bank/method_balance_of.go 22.22% <28.57%> (ø)
precompiles/types/coin.go 33.33% <54.54%> (ø)
precompiles/staking/logs.go 16.09% <70.00%> (-0.33%) ⬇️
... and 9 more

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

  1. Convert to table-driven tests to handle multiple test cases
  2. Add edge cases (zero address, max uint256)
  3. 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 to Test_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:

  1. Invalid denomination format
  2. Zero amount
  3. Negative amount
  4. 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 interface

The 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:

  1. The validation conditions could be simplified
  2. 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:

  1. RequiredTxFailed is used in multiple test files across the codebase
  2. The companion function follows the pattern RequireTx* (specifically RequireTxSuccessful)
  3. No existing usage of the proposed name RequireTxFailed exists

The original review is correct. The function should be renamed from RequiredTxFailed to RequireTxFailed to maintain consistency with the RequireTx* 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:

  1. Standardize contract initialization
  2. Centralize dependency injection
  3. 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 0
pkg/rpc/clients.go (1)

39-40: Maintain consistent documentation style

The 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 QueryClient
precompiles/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:

  1. Consider wrapping the entire operation in a transaction scope
  2. Implement compensation logic for partial failures
  3. 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 numbers

Define 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 rationale

Add 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 context

The 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:

  1. Centralize validation logic
  2. Make validation rules more maintainable
  3. Simplify testing
  4. 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,title

Length 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:

  1. Moving this code to a separate file or branch until needed
  2. Creating a GitHub issue to track the implementation
  3. 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/XXXX
precompiles/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:

  1. Add a package-level comment explaining the purpose of these tests
  2. Include a reference to the GitHub issue in a constant
  3. 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:

  1. Rename the test to better reflect its current state, e.g., Test_Stake_CurrentlyDisabled
  2. Add a TODO comment with acceptance criteria for re-enabling the tests once issue Disable stake, unstake and moveStake #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 below
changelog.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 precompile
precompiles/staking/staking_test.go (2)

198-270: Remove unused 'setup' function to improve code maintainability

The setup function appears to be unused and duplicates functionality provided by newTestSuite. 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{}),
-		&params.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 suite

The 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

📥 Commits

Files that changed from the base of the PR and between b334267 and 36e36a8.

⛔ 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 tests

precompiles/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 tests

precompiles/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 tests

precompiles/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 tests

precompiles/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 tests

precompiles/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 constants

The 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:

  1. Converting v.Status to uint8 without bounds checking
  2. 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:

  1. The actual implementation of GetAllValidators
  2. Any error handling patterns in its implementation
  3. 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:

  1. The interface definition in x/crosschain/types/expected_keepers.go shows the method signature returns only validators: GetAllValidators(ctx sdk.Context) (validators []stakingtypes.Validator)
  2. All usages in the codebase handle it as a non-error-returning function
  3. The mock implementation in testutil/keeper/mocks/crosschain/staking.go confirms this contract by only returning the validator slice

The 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 suggestion

Consider adding address validation.

While the direct usage of zrc20Addr and zrc20Token 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 functions

The 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 imports

The package structure follows Go best practices with proper grouping of standard library, external, and internal imports.


38-50: Add test coverage and documentation

The business logic implementation looks correct, but there are two important improvements needed:

  1. The code lacks test coverage as indicated by static analysis
  2. 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:

  1. Valid input with existing delegation
  2. Valid input with no delegation
  3. Invalid validator address
  4. Invalid argument types
  5. 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 tests

precompiles/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 definition

The 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 suggestion

Consider 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 to stakingContract and its propagation to NewIStakingContract 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:

  1. All contract methods consistently receive and use the sdk.Context parameter
  2. All keeper method calls properly pass the context parameter
  3. The context flows correctly from contract instantiation through method execution

The addition of the context parameter to stakingContract and its propagation to NewIStakingContract 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:

  1. Documentation explaining its purpose and parameters
  2. 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 tests

e2e/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 json

Length 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 json

Length 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:

  1. It's a test contract specifically designed to test the distribute functionality
  2. The contract has proper access controls via onlyOwner modifier for its main functionality
  3. The pattern is consistent across other test contracts (TestBank, TestStaking) which all implement empty fallback functions
  4. 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 5

Length 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:

  1. Finding related contract files
  2. 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 function

The 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 json

Length 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 conventions

The 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-structured

The 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 usage

Let'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 go

Length 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 go

Length 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:

  1. Validation that the ZRC20 contract is not paused
  2. 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 and zrc20_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:

  1. Add documentation specifying:

    • Success conditions
    • Minimum distribution amounts (if any)
    • Distribution strategy (e.g., pro-rata, equal split)
  2. Consider adding parameters for:

    • Minimum distribution threshold
    • Distribution strategy configuration
  3. 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 implementation

The 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 in e2e/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 of locker 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:

  1. Access control to restrict who can distribute rewards
  2. Input validation for the amount parameter
  3. 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:

  1. Input validation:

    • Validates number of arguments
    • Checks for valid ZRC20 address
    • Ensures amount is positive and non-nil
    • Verifies allowance through LockZRC20
  2. Access control:

    • Tracks original caller through EVM
    • Uses secure locking mechanism with dedicated locker address
    • Implements proper module-based fund transfers
  3. 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:

  1. Successful reward distribution to validators
  2. Successful reward distribution to delegators
  3. Edge cases in reward distribution (e.g., zero rewards, rounding)
  4. 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 client

The 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 client

The 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.go

Length of output: 499


Script:

#!/bin/bash
# Let's also check for any distribution client usage patterns
rg "DistributionClient" cmd/zetae2e/local/local.go

Length of output: 50


Script:

#!/bin/bash
# Check if there are any other files using the distribution client
rg "DistributionClient" --type go

Length of output: 1067


Script:

#!/bin/bash
# Let's examine the test implementation to understand the distribution client usage
cat e2e/e2etests/test_precompiles_distribute.go

Length of output: 8414


Script:

#!/bin/bash
# Let's also check the distribution types being used
rg -B 2 -A 2 "distributiontypes" --type go

Length 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 go

Length 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.*distribute

Length 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 go

Length 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 ABI

Ensure that the returned output from method.Outputs.Pack(true) aligns with the expected ABI for the distribute 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 the distribute function returns (bool success), which perfectly aligns with the implementation's method.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.abi

Length 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 and bankkeeper 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" and fungiblekeeper 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' Event

The ABI string now includes the 'Distributed' event, ensuring that the contract interface correctly reflects the new functionality.


253-273: 'Distribute' Methods Added Correctly

The 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 Correctly

The IStakingDistributed struct accurately represents the 'Distributed' event, correctly mapping event parameters to Go types.


412-490: Event Filtering and Parsing Methods Implemented Appropriately

The methods FilterDistributed, WatchDistributed, and ParseDistributed are properly defined, enabling effective event filtering and subscription for the 'Distributed' event.

precompiles/staking/const.go Outdated Show resolved Hide resolved
precompiles/staking/const.go Show resolved Hide resolved
precompiles/staking/staking.go Show resolved Hide resolved
precompiles/staking/staking.go Show resolved Hide resolved
precompiles/staking/staking_test.go Show resolved Hide resolved
precompiles/staking/staking_test.go Show resolved Hide resolved
precompiles/staking/IStaking.gen.go Show resolved Hide resolved
@fbac fbac force-pushed the feat/staking-distribute-function branch from ced9f30 to 884598c Compare October 24, 2024 20:41
@fbac fbac force-pushed the feat/staking-distribute-function branch from 82c7244 to 2f35ee7 Compare October 28, 2024 20:48
@fbac fbac requested a review from kingpinXD October 29, 2024 15:07
@lumtis
Copy link
Member

lumtis commented Oct 30, 2024

We should look into solving the lint issue

Copy link

!!!WARNING!!!
nosec detected in the following files: precompiles/staking/method_get_all_validators.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

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
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label Oct 30, 2024
@fbac fbac enabled auto-merge October 30, 2024 10:22
@fbac fbac added this pull request to the merge queue Oct 30, 2024
Merged via the queue into develop with commit 60f0f1a Oct 30, 2024
34 of 35 checks passed
@fbac fbac deleted the feat/staking-distribute-function branch October 30, 2024 10:51
@lumtis lumtis linked an issue Nov 4, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli nosec stateful-precompile A new stateful precompile contract that is user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a precompile allowing to send ZRC20 to be distributed to stakers
4 participants