feat: add sender to revert context

[skosito]

Description

Related protocol contracts PR: zeta-chain/protocol-contracts#361

• adds sender to RevertContext and extend e2e tests and test dapp contract to check it
• there seems to be a problem with revert gas limit, it is overwritten in PayGasAndUpdateCCTX, i fixed it by using bigger gasLimit, not sure what is intended behaviour

How Has This Been Tested?

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

Summary by CodeRabbit

• New Features

  • Integrated authenticated calls smart contract functionality into the protocol.
  • Added inbound sender to revert context for improved transaction handling.
  • Updated CLI command for vote-inbound to include a new parameter for arbitrary calls.
  • Introduced crosschainCallOptions in the API to enhance cross-chain call options.
  • Added CallOptions field to MsgVoteInbound for better parameter management.

• Bug Fixes

  • Enhanced test coverage for ETH withdrawal and arbitrary call scenarios.

• Documentation

  • Updated changelog and CLI documentation to reflect new features and changes.

• Tests

  • Added new test cases for various withdrawal and call scenarios to ensure comprehensive testing.

[coderabbitai]

Caution

Inline review comments failed to post

🛑 Comments failed to post (58)

pkg/contracts/gatewayzevmcaller/bindings.go (1)

1-4: 🛠️ Refactor suggestion

Consider improving the robustness of go:generate directives.

The go:generate directives effectively automate the process of generating Go bindings for the Solidity contract. However, there are a few points to consider for improving robustness:

1. Use of external tools: The commands rely on external tools (solc, jq, abigen) which may not be available in all environments. Consider documenting these dependencies or providing a script to install them.

2. Relative paths: The commands use relative paths, which might cause issues if the working directory changes. Consider using absolute paths or environment variables to ensure consistency.

3. Error handling: The current commands don't include error handling. Consider adding checks or using a script that can handle and report errors more gracefully.

To address these points, you might consider creating a separate shell script that handles the generation process, including dependency checks and error handling. For example:

#!/bin/bash
set -e

# Check for required tools
command -v solc >/dev/null 2>&1 || { echo >&2 "solc is required but not installed. Aborting."; exit 1; }
command -v jq >/dev/null 2>&1 || { echo >&2 "jq is required but not installed. Aborting."; exit 1; }
command -v abigen >/dev/null 2>&1 || { echo >&2 "abigen is required but not installed. Aborting."; exit 1; }

# Set working directory
cd "$(dirname "$0")"

# Generate bindings
solc GatewayZEVMCaller.sol --combined-json abi,bin | jq '.contracts."GatewayZEVMCaller.sol:GatewayZEVMCaller"' > GatewayZEVMCaller.json
jq .abi < GatewayZEVMCaller.json > GatewayZEVMCaller.abi
jq -r .bin < GatewayZEVMCaller.json > GatewayZEVMCaller.bin
abigen --abi GatewayZEVMCaller.abi --bin GatewayZEVMCaller.bin --pkg gatewayzevmcaller --type GatewayZEVMCaller --out GatewayZEVMCaller.go

echo "Bindings generated successfully."

Then, replace the current go:generate directives with a single directive:

//go:generate ./generate_bindings.sh

This approach centralizes the generation logic, improves error handling, and makes the process more maintainable.

e2e/e2etests/test_v2_eth_withdraw_and_arbitrary_call.go (3)

16-18: 🛠️ Refactor suggestion

Consider enhancing error handling for invalid arguments.

While the function checks the length of args, it doesn't provide a descriptive error message if the check fails. Consider using require.Lenf for a more informative error message.

Suggested improvement:

require.Lenf(r, args, 1, "TestV2ETHWithdrawAndArbitraryCall requires exactly one argument (amount), got %d", len(args))

---

19-20: 🛠️ Refactor suggestion

Improve error message for invalid amount.

The error message for an invalid amount could be more descriptive to aid in debugging.

Consider modifying the error message:

amount, ok := big.NewInt(0).SetString(args[0], 10)
require.Truef(r, ok, "Invalid amount '%s' specified for TestV2ETHWithdrawAndCall. Expected a valid integer.", args[0])

---

34-37: 🛠️ Refactor suggestion

Consider adding error handling for CCTX mining.

The function waits for the CCTX to be mined but doesn't handle potential errors or timeouts explicitly.

Consider adding error handling:

cctx, err := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
require.NoError(r, err, "Failed to wait for CCTX to be mined")
r.Logger.CCTX(*cctx, "withdraw")
require.Equal(r, crosschaintypes.CctxStatus_OutboundMined, cctx.CctxStatus.Status, "Unexpected CCTX status")

zetaclient/testdata/cctx/chain_8332_cctx_148.go (1)

37-44: 💡 Codebase verification

🛠️ Refactor suggestion

Identify Inconsistent GasPrice Placement in OutboundParams

The shell script results indicate that while CallOptions have been introduced to organize GasLimit, there are still instances where GasPrice fields exist outside of CallOptions. This inconsistency can lead to maintenance challenges and potential bugs.

To ensure all OutboundParams structures are uniformly refactored, please run the following script to identify OutboundParams instances where GasPrice is still defined outside of CallOptions:

#!/bin/bash
# Description: Locate OutboundParams with GasPrice outside CallOptions

echo "Searching for OutboundParams with GasPrice defined outside of CallOptions:"
rg --type go 'OutboundParams\s*:\s*\{[^}]*GasPrice\s*:' -A 5

This script utilizes ripgrep to search for OutboundParams definitions that include GasPrice fields outside the newly introduced CallOptions structure. Reviewing the output will help ensure all instances are updated accordingly.

🔗 Analysis chain

Consider further refinement of gas-related parameters

The introduction of CallOptions improves the organization of the OutboundParams structure. However, there are opportunities for further refinement:

1. For consistency, consider moving the GasPrice field into CallOptions alongside GasLimit.
2. To enhance clarity, it may be beneficial to add a comment explaining the purpose and scope of CallOptions.
3. Ensure that all parts of the codebase interacting with OutboundParams are updated to accommodate this change.

Consider applying the following changes:

 OutboundParams: []*crosschaintypes.OutboundParams{
     {
         Receiver:        "bc1qpsdlklfcmlcfgm77c43x65ddtrt7n0z57hsyjp",
         ReceiverChainId: 8332,
         CoinType:        coin.CoinType_Gas,
         Amount:          sdkmath.NewUint(12000),
         TssNonce:        148,
         CallOptions: &crosschaintypes.CallOptions{
             GasLimit: 254,
+            GasPrice: "46",
         },
-        GasPrice:               "46",
         Hash:                   "030cd813443f7b70cc6d8a544d320c6d8465e4528fc0f3410b599dc0b26753a0",
         ObservedExternalHeight: 150,
         GasUsed:                0,
         EffectiveGasPrice:      sdkmath.NewInt(0),
         EffectiveGasLimit:      0,
         TssPubkey:              "zetapub1addwnpepqtadxdyt037h86z60nl98t6zk56mw5zpnm79tsmvspln3hgt5phdc79kvfc",
         TxFinalizationStatus:   crosschaintypes.TxFinalizationStatus_Executed,
     },
 }

Additionally, consider adding a comment to explain the purpose of CallOptions:

// CallOptions encapsulates gas-related parameters for the outbound transaction
CallOptions: &crosschaintypes.CallOptions{
    // ...
},

To ensure that this change is consistent across the codebase, please run the following script:

This will help identify any areas that may need updates due to this structural change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of OutboundParams and CallOptions across the codebase

# Test: Search for OutboundParams usage
echo "Searching for OutboundParams usage:"
rg --type go -A 5 'OutboundParams'

# Test: Search for CallOptions usage
echo "Searching for CallOptions usage:"
rg --type go -A 5 'CallOptions'

Length of output: 174087

zetaclient/testdata/cctx/chain_56_cctx_68270.go (1)

37-44: 🛠️ Refactor suggestion

Consider parameterizing the gas limit and clarifying its relationship with EffectiveGasLimit.

The addition of the CallOptions field improves the structure by grouping related parameters. However, there are two points to consider:

1. The GasLimit is set to a fixed value of 21000. This might not be flexible enough for all scenarios, especially considering the variability in gas costs across different operations and network conditions.

2. There's a potential inconsistency or redundancy between GasLimit in CallOptions and EffectiveGasLimit (line 52). The relationship between these two fields should be clarified.

Consider the following improvements:

1. Parameterize the GasLimit:

CallOptions: &crosschaintypes.CallOptions{
    GasLimit: params.DefaultGasLimit, // Define this in a separate params package
},

2. Clarify the relationship between GasLimit and EffectiveGasLimit:

EffectiveGasLimit: callOptions.GasLimit, // Use the value from CallOptions

3. If EffectiveGasLimit is meant to be different, add a comment explaining the distinction:

// EffectiveGasLimit may differ from CallOptions.GasLimit due to [reason]
EffectiveGasLimit: 21000,

zetaclient/testdata/cctx/chain_1_cctx_9718.go (1)

37-44: 💡 Codebase verification

🛠️ Refactor suggestion

Discrepancy Between GasLimit and EffectiveGasLimit

The GasLimit in OutboundParams.CallOptions is set to 90,000, which is lower than the EffectiveGasLimit values used in tests (100,000 and 1,000,000). This inconsistency could lead to potential issues such as transaction failures or unexpected behavior.

Recommendations:

1. Align GasLimit with EffectiveGasLimit:

   CallOptions: &crosschaintypes.CallOptions{
       GasLimit: 100000, // Aligned with EffectiveGasLimit
       GasPrice: "112217884384",
   },
   // Remove GasPrice from this level

2. Document the Rationale:

   If the discrepancy is intentional, provide clear documentation explaining the reasoning behind the differing GasLimit values to ensure maintainability and clarity for future developers.

🔗 Analysis chain

Structural improvement with potential for refinement

The addition of the CallOptions field enhances the structure of OutboundParams, providing better organization and potential for future extensibility. This change is commendable for its forward-thinking approach.

However, there are a few points to consider:

1. Ensure that all references to OutboundParams.GasLimit throughout the codebase have been updated to OutboundParams.CallOptions.GasLimit.

2. The GasLimit (90000) is lower than the EffectiveGasLimit (100000). Consider aligning these values or documenting the reason for the discrepancy.

3. For consistency and to fully leverage the new structure, consider moving other call-related fields (e.g., GasPrice) into CallOptions as well.

Consider the following refinement:

CallOptions: &crosschaintypes.CallOptions{
    GasLimit: 100000, // Aligned with EffectiveGasLimit
    GasPrice: "112217884384",
},
// Remove GasPrice from this level

To ensure all references have been updated, run:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct references to GasLimit in OutboundParams
rg --type go 'OutboundParams.*GasLimit'

Length of output: 1137

x/crosschain/client/cli/tx_vote_inbound.go (2)

20-21: 🛠️ Refactor suggestion

Consider improving the readability of the command usage string.

The addition of the isArbitraryCall parameter has made the command usage string quite long. To enhance readability and maintainability, consider breaking it into multiple lines using string concatenation.

Here's a suggested refactor:

Use: "vote-inbound [sender] [senderChainID] [txOrigin] [receiver] [receiverChainID] " +
    "[amount] [message] [inboundHash] [inBlockHeight] [coinType] [asset] " +
    "[eventIndex] [protocolContractVersion] [isArbitraryCall]",

This approach maintains clarity while improving code readability.

---

70-74: 🛠️ Refactor suggestion

Enhance error handling and improve code conciseness.

The parsing of the isArbitraryCall parameter is correct, but we can improve the error handling and make the code more concise.

Consider refactoring the code as follows:

isArbitraryCall, err := strconv.ParseBool(args[13])
if err != nil {
    return fmt.Errorf("failed to parse isArbitraryCall: %w", err)
}

This change provides a more informative error message and reduces the number of lines, improving readability and maintainability.

zetaclient/zetacore/tx.go (1)

54-54: 🛠️ Refactor suggestion

Consider updating function signature and improving documentation

The addition of a new parameter to the types.NewMsgVoteInbound call without modifying the function signature may lead to inconsistencies and maintenance challenges. Please consider the following recommendations:

1. Update the function signature to include the new boolean parameter:

   func GetInboundVoteMessage(
       // ... existing parameters ...
       eventIndex uint,
       protocolVersion types.ProtocolContractVersion,
       newParameter bool,
   ) *types.MsgVoteInbound {
       // ... existing implementation ...
   }

2. Document the purpose of the new parameter in the function's documentation, even if it's not relevant for the current version. This will aid future developers in understanding its intended use:

   // GetInboundVoteMessage returns a new MsgVoteInbound
   // newParameter: Reserved for future use in versions beyond v1
   func GetInboundVoteMessage(
       // ... parameters ...
   ) *types.MsgVoteInbound {
       // ... implementation ...
   }

3. If this change is part of a larger upgrade, consider the implications for backwards compatibility. You may need to provide two versions of this function or use a default value for the new parameter to maintain compatibility with existing code.

x/fungible/keeper/v2_evm.go (1)

188-188: ⚠️ Potential issue

Enhance test coverage for modified functions

The static analysis tool indicates that the newly added lines in both CallExecuteRevert and CallDepositAndRevert functions are not covered by tests.

To ensure the reliability and correctness of these critical changes, it's important to expand the test suite to cover these modifications. Consider adding the following test cases:

1. Test CallExecuteRevert with a valid inboundSender address.
2. Test CallExecuteRevert with an invalid inboundSender address.
3. Test CallDepositAndRevert with a valid inboundSender address.
4. Test CallDepositAndRevert with an invalid inboundSender address.

These tests should verify that the Sender field in the RevertContext is correctly set and that invalid addresses are properly handled.

Also applies to: 242-242

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 188-188: x/fungible/keeper/v2_evm.go#L188
Added line #L188 was not covered by tests

zetaclient/chains/evm/signer/outbound_data.go (1)

194-196: ⚠️ Potential issue

Enhance error handling and validation in validateParams function.

The modification to check params.CallOptions.GasLimit instead of params.GasLimit is noted. However, there are opportunities for improvement:

1. The error message "outboundParams is empty" is now inaccurate, as it doesn't reflect the GasLimit check.
2. The function could benefit from more comprehensive validation of the OutboundParams struct.

Consider applying the following changes:

 func validateParams(params *types.OutboundParams) error {
-	if params == nil || params.CallOptions.GasLimit == 0 {
-		return errors.New("outboundParams is empty")
+	if params == nil {
+		return errors.New("outboundParams is nil")
+	}
+	if params.CallOptions == nil || params.CallOptions.GasLimit == 0 {
+		return errors.New("invalid gas limit in outboundParams")
 	}
+	// Add more validations as needed, e.g.:
+	// if params.Amount == nil || params.Amount.IsZero() {
+	//     return errors.New("invalid amount in outboundParams")
+	// }
 
 	return nil
 }

This change improves error specificity and allows for future expansion of validation checks.

Committable suggestion was skipped due to low confidence.

proto/zetachain/zetacore/crosschain/tx.proto (2)

166-166: ⚠️ Potential issue

Consider adding deprecation timeline for gas_limit field

The deprecation of the gas_limit field is noted. To ensure a smooth transition:

1. Consider adding a specific version or timeline for when this field will be removed entirely.
2. Ensure that all clients are updated to use the new CallOptions before removing this field.
3. Update any documentation or API references to reflect this change.

Suggested improvement:

-  // Deprecated (v21), use CallOptions
+  // Deprecated: Will be removed in v22. Use CallOptions instead.
   uint64 gas_limit = 11 [deprecated = true];

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  // Deprecated: Will be removed in v22. Use CallOptions instead.
   uint64 gas_limit = 11 [deprecated = true];

---

180-180: ⚠️ Potential issue

Enhance documentation for CallOptions field

The addition of the CallOptions field is noted. To improve clarity and maintainability:

1. Add a comment explaining the purpose and content of CallOptions.
2. Ensure that the CallOptions type is properly defined and imported if it's in another file.
3. Consider adding an example usage in the comment to guide implementers.

Suggested improvement:

+  // CallOptions specifies the parameters for contract calls, including gas limit.
+  // This replaces the deprecated gas_limit field.
+  // Example: { "gas_limit": 100000, "other_option": "value" }
   CallOptions call_options = 18;

Additionally, please ensure that the CallOptions type is properly defined and visible to users of this Protobuf definition.

Committable suggestion was skipped due to low confidence.

x/crosschain/keeper/v2_zevm_inbound.go (1)

246-246: 🛠️ Refactor suggestion

Consistent changes and refactoring opportunity

The modifications to newCallInbound mirror those in newWithdrawalInbound, which is commendable for maintaining consistency. However, this similarity presents an opportunity for refactoring:

1. Consider extracting the common logic for gas limit retrieval and the creation of MsgVoteInbound into a separate helper function. This would reduce code duplication and improve maintainability.

2. The addition of event.CallOptions.IsArbitraryCall is consistent with the changes in newWithdrawalInbound. Ensure that the implications of this new parameter are well-documented and handled consistently throughout the codebase.

Here's a suggested refactoring to extract common logic:

func (k Keeper) createInboundMsg(
    ctx sdk.Context,
    event interface{},
    foreignCoin fungibletypes.ForeignCoins,
    senderChain chains.Chain,
    receiverChain chains.Chain,
    txOrigin string,
    toAddr string,
) (*types.MsgVoteInbound, error) {
    gasLimit := k.getGasLimit(ctx, event, foreignCoin)
    
    // Common fields
    sender := event.(interface{ GetSender() ethcommon.Address }).GetSender().Hex()
    message := event.(interface{ GetMessage() []byte }).GetMessage()
    callOptions := event.(interface{ GetCallOptions() gatewayzevm.GatewayZEVMCallOptions }).GetCallOptions()
    revertOptions := event.(interface{ GetRevertOptions() gatewayzevm.GatewayZEVMRevertOptions }).GetRevertOptions()
    
    // Specific fields based on event type
    var value math.Uint
    var coinType coin.CoinType
    var asset string
    
    switch e := event.(type) {
    case *gatewayzevm.GatewayZEVMWithdrawn:
        value = math.NewUintFromBigInt(e.Value)
        coinType = foreignCoin.CoinType
        asset = foreignCoin.Asset
    case *gatewayzevm.GatewayZEVMCalled:
        value = math.ZeroUint()
        coinType = coin.CoinType_NoAssetCall
        asset = ""
    default:
        return nil, errors.New("unsupported event type")
    }
    
    return types.NewMsgVoteInbound(
        "",
        sender,
        senderChain.ChainId,
        txOrigin,
        toAddr,
        foreignCoin.ForeignChainId,
        value,
        hex.EncodeToString(message),
        event.(interface{ GetRaw() ethtypes.Log }).GetRaw().TxHash.String(),
        event.(interface{ GetRaw() ethtypes.Log }).GetRaw().BlockNumber,
        gasLimit,
        coinType,
        asset,
        event.(interface{ GetRaw() ethtypes.Log }).GetRaw().Index,
        types.ProtocolContractVersion_V2,
        callOptions.IsArbitraryCall,
        types.WithZEVMRevertOptions(revertOptions),
    ), nil
}

func (k Keeper) getGasLimit(ctx sdk.Context, event interface{}, foreignCoin fungibletypes.ForeignCoins) uint64 {
    gasLimit := event.(interface{ GetCallOptions() gatewayzevm.GatewayZEVMCallOptions }).GetCallOptions().GasLimit.Uint64()
    if gasLimit == 0 {
        gasLimitQueried, err := k.fungibleKeeper.QueryGasLimit(
            ctx,
            ethcommon.HexToAddress(foreignCoin.Zrc20ContractAddress),
        )
        if err != nil {
            // Handle error appropriately
            return 0
        }
        gasLimit = gasLimitQueried.Uint64()
    }
    return gasLimit
}

This refactoring would significantly reduce duplication between newWithdrawalInbound and newCallInbound, making the code more maintainable and less prone to inconsistencies in future updates.

Also applies to: 260-260, 274-274

x/crosschain/keeper/cctx_test.go (2)

40-40: ⚠️ Potential issue

Enhance test coverage for CallOptions

The addition of CallOptions to OutboundParams is noted. However, initializing it as an empty struct may not provide sufficient test coverage for various scenarios.

Consider expanding the test cases to include different CallOptions configurations. This will ensure that the system behaves correctly under various conditions. For example:

items[i].OutboundParams = []*types.OutboundParams{{
    Amount: math.ZeroUint(),
    CallOptions: &types.CallOptions{
        GasLimit: uint64(1000000 + i),
        GasPrice: sdk.NewInt64Coin("uzetaeth", 1000000000 + int64(i)),
    },
}}

---

64-70: ⚠️ Potential issue

Improve test data generation for CallOptions

The inclusion of CallOptions in OutboundParams is a positive step. However, the current implementation may not provide comprehensive test coverage.

Consider enhancing the test data generation for CallOptions to cover a wider range of scenarios:

1. Include more fields from CallOptions, such as GasPrice.
2. Use a mix of predefined values and random generation to ensure edge cases are covered.

Example implementation:

items[i].OutboundParams = []*types.OutboundParams{{
    Receiver:        fmt.Sprintf("%d", i),
    ReceiverChainId: int64(i),
    Hash:            fmt.Sprintf("%d", i),
    TssNonce:        uint64(i),
    CallOptions: &types.CallOptions{
        GasLimit: uint64(1000000 + rand.Intn(1000000)),
        GasPrice: sdk.NewInt64Coin("uzetaeth", 1000000000 + int64(rand.Intn(1000000000))),
    },
    // ... other fields ...
}}

This approach will provide more robust test cases and help identify potential issues across various CallOptions configurations.

docs/spec/crosschain/messages.md (1)

192-192: ⚠️ Potential issue

Correct indentation for consistency.

The indentation of the new field uses a hard tab, which is inconsistent with the space-based indentation used throughout the rest of the file. To maintain consistency and adhere to best practices, please replace the hard tab with spaces.

Apply this change to correct the indentation:

-	CallOptions call_options = 18;
+    CallOptions call_options = 18;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    CallOptions call_options = 18;

🧰 Tools

🪛 Markdownlint

192-192: Column: 1
Hard tabs

(MD010, no-hard-tabs)

zetaclient/chains/evm/observer/v2_inbound.go (2)

194-194: ⚠️ Potential issue

Consider removing the unused parameter or providing more context.

The addition of an unused boolean parameter set to false with a comment indicating it's "currently not relevant" may introduce unnecessary complexity. This violates the principle of clean code and could lead to confusion for other developers.

Consider one of the following options:

1. Remove the parameter if it's not needed in the current implementation:

- false, // currently not relevant since calls are not arbitrary

2. If the parameter is intended for future use, provide more context in the comment about its purpose and when it's expected to be utilized. For example:

false, // TODO: This parameter will be used to handle arbitrary calls in future implementations (Issue #XXXX)

Ensuring clarity in the code will improve maintainability and reduce potential confusion for other developers working on this codebase.

---

329-329: 🛠️ Refactor suggestion

⚠️ Potential issue

Consider removing the unused parameter or providing more context, and evaluate refactoring opportunities.

The addition of an unused boolean parameter set to false with a comment indicating it's "currently not relevant" introduces unnecessary complexity. This violates the principle of clean code and could lead to confusion for other developers.

Consider one of the following options:

1. Remove the parameter if it's not needed in the current implementation:

- false, // currently not relevant since calls are not arbitrary

2. If the parameter is intended for future use, provide more context in the comment about its purpose and when it's expected to be utilized. For example:

false, // TODO: This parameter will be used to handle arbitrary calls in future implementations (Issue #XXXX)

Additionally, since this pattern is repeated in both newDepositInboundVote and newCallInboundVote, consider refactoring these functions to use a common structure or interface that can accommodate this future functionality without adding unused parameters to multiple functions.

This refactoring would improve code maintainability and reduce duplication, adhering to the DRY (Don't Repeat Yourself) principle.

go.mod (1)

62-62: 💡 Codebase verification

Update the protocol-contracts dependency to a valid version in go.mod

The go.mod file currently references the versions v1.0.2-athens3.0.20240819143729-3a274ce7bad0 and v1.0.2-athens3.0.20240924201108-3a274ce7bad0 for the github.com/zeta-chain/protocol-contracts dependency. These versions do not exist in the repository, which may lead to build failures or unexpected behaviors. Please update the dependency to a valid version, such as v1.0.2-athens3, to ensure compatibility with the codebase.

🔗 Analysis chain

Verify compatibility with updated protocol-contracts dependency.

The github.com/zeta-chain/protocol-contracts dependency has been updated to a newer version. While this likely includes improvements or bug fixes, it's crucial to ensure compatibility with the current codebase.

To verify the impact of this change, please run the following script:

This script will help identify any breaking changes in the dependency update and locate their usage in the codebase. Please review the output carefully to ensure all changes are accounted for and no incompatibilities are introduced.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for breaking changes in the protocol-contracts dependency

# Fetch the diff between the old and new versions
OLD_VERSION="v1.0.2-athens3.0.20240819143729-b8229cd7b410"
NEW_VERSION="v1.0.2-athens3.0.20240924201108-3a274ce7bad0"
REPO_URL="https://github.com/zeta-chain/protocol-contracts"

echo "Fetching diff between $OLD_VERSION and $NEW_VERSION"
git diff $OLD_VERSION..$NEW_VERSION --name-only | while read -r file; do
    if [[ $file == *.go ]]; then
        echo "Checking $file for potential breaking changes"
        git diff $OLD_VERSION..$NEW_VERSION -- "$file" | grep '^[+-]func' || true
        git diff $OLD_VERSION..$NEW_VERSION -- "$file" | grep '^[+-]type' || true
    fi
done

# Check for usage of potentially changed functions or types
echo "Checking for usage of potentially changed functions or types"
rg -e "protocol-contracts" --type go

Length of output: 19216

---

Script:

#!/bin/bash
# Description: Verify if the specified versions exist in the repository

OLD_VERSION="v1.0.2-athens3.0.20240819143729-b8229cd7bad0"
NEW_VERSION="v1.0.2-athens3.0.20240924201108-3a274ce7bad0"

echo "Checking if versions exist in the repository..."

git rev-parse --verify "$OLD_VERSION" >/dev/null 2>&1
if [ $? -ne 0 ]; then
    echo "Old version $OLD_VERSION does not exist in the repository."
else
    echo "Old version $OLD_VERSION exists."
fi

git rev-parse --verify "$NEW_VERSION" >/dev/null 2>&1
if [ $? -ne 0 ]; then
    echo "New version $NEW_VERSION does not exist in the repository."
else
    echo "New version $NEW_VERSION exists."
fi

Length of output: 784

---

Script:

#!/bin/bash
# Description: List all available tags in the protocol-contracts repository to identify correct versions

REPO_URL="https://github.com/zeta-chain/protocol-contracts.git"

echo "Cloning the repository to list available tags..."
git clone --quiet "$REPO_URL" temp_repo
cd temp_repo || exit 1

echo "Fetching all tags..."
git fetch --tags

echo "Available tags:"
git tag -l

# Cleanup
cd ..
rm -rf temp_repo

Length of output: 742

testutil/sample/crosschain.go (1)

151-158: 💡 Codebase verification

CallOptions Not Incorporated in OutboundParams Usages

The verification process did not locate any instances where OutboundParams is invoked with the CallOptions field. This omission may lead to inconsistencies and potential runtime issues. Please ensure that all calls to OutboundParams include the CallOptions parameter to maintain consistency and functionality across the codebase.

🔗 Analysis chain

Approve changes to OutboundParams function.

The addition of the CallOptions field with GasLimit improves the structure by grouping related options. This change enhances clarity and organization in the code.

To ensure consistency across the codebase, please run the following script to verify the usage of OutboundParams:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of OutboundParams function across the codebase.

# Test: Search for OutboundParams usage. Expect: All occurrences should include CallOptions.
rg --type go -A 5 'OutboundParams\s*\('

Length of output: 6202

---

Script:

#!/bin/bash
# Description: Inspect the OutboundParams function to verify inclusion of CallOptions.

ast-grep --lang go --pattern $'func OutboundParams($_) *types.OutboundParams { 
  $$$ 
  CallOptions: $_, 
  $$$ 
}'

Length of output: 117

---

Script:

#!/bin/bash
# Description: Search for OutboundParams initializations that include CallOptions.

rg --type go 'OutboundParams\s*[:=]\s*\{[^}]*CallOptions\s*:' -A 5

Length of output: 68

e2e/e2etests/test_v2_zevm_to_evm_call.go (1)

31-31: ⚠️ Potential issue

Correct the method name typo

The method V2ZEVMToEMVAuthenticatedCall appears to have a typo. It should be V2ZEVMToEVMAuthenticatedCall to match naming conventions and ensure the correct function is invoked.

Apply this diff to correct the method name:

-tx = r.V2ZEVMToEMVAuthenticatedCall(
+tx = r.V2ZEVMToEVMAuthenticatedCall(

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	tx = r.V2ZEVMToEVMAuthenticatedCall(

e2e/e2etests/test_v2_eth_withdraw_and_call.go (1)

15-15: 🛠️ Refactor suggestion

Consider simplifying the constant name for readability.

The constant name payloadMessageAuthenticatedWithdrawETH is quite lengthy. For better readability and maintainability, consider renaming it to something more concise, such as authWithdrawETHPayload.

pkg/contracts/gatewayzevmcaller/GatewayZEVMCaller.sol (5)

58-59: 🛠️ Refactor suggestion

Consider making contract variables publicly accessible

The contract variables gatewayZEVM and wzeta are declared as private. If external contracts or users need to access these addresses, consider changing the visibility to public. This promotes transparency and allows for better integration with other contracts.

Apply this diff to adjust the visibility:

-    IGatewayZEVM private gatewayZEVM;
-    WZETA wzeta;
+    IGatewayZEVM public gatewayZEVM;
+    WZETA public wzeta;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    IGatewayZEVM public gatewayZEVM;
    WZETA public wzeta;

---

96-97: ⚠️ Potential issue

Use dynamic approval amounts and validate approval for ZRC20 tokens

The approve function is used with a hard-coded amount of 100000000000000000, which might not correspond to the necessary amount for the operation. Approving the exact required amount enhances security by limiting token exposure.

Moreover, always check the return value of approve to ensure the approval was successful.

Apply this diff to correct the issue:

-        IZRC20(zrc20).approve(address(gatewayZEVM), 100000000000000000);
+        uint256 amount = /* specify the required amount */;
+        require(IZRC20(zrc20).approve(address(gatewayZEVM), amount), "Token approval failed");

Alternatively, implement OpenZeppelin's SafeERC20 library:

+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+
+using SafeERC20 for IERC20;
+
         uint256 amount = /* specify the required amount */;
-        IZRC20(zrc20).approve(address(gatewayZEVM), amount);
+        IERC20(zrc20).safeApprove(address(gatewayZEVM), amount);

Committable suggestion was skipped due to low confidence.

---

84-85: ⚠️ Potential issue

Ensure approval success when approving WZETA tokens

The approve function on the WZETA token is invoked without verifying its success. To prevent potential issues, check the return value to confirm that the approval was successful.

Apply this diff to handle potential approval failures:

-        wzeta.approve(address(gatewayZEVM), amount);
+        require(wzeta.approve(address(gatewayZEVM), amount), "WZETA approval failed");

Or, utilize OpenZeppelin's SafeERC20 library for safer interactions:

+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+
+using SafeERC20 for IERC20;
+
-        wzeta.approve(address(gatewayZEVM), amount);
+        IERC20(address(wzeta)).safeApprove(address(gatewayZEVM), amount);

Committable suggestion was skipped due to low confidence.

---

100-102: ⚠️ Potential issue

Check for successful deposit of WZETA

When calling wzeta.deposit{value: msg.value}();, it's prudent to ensure that the deposit was successful. Although the deposit function may revert on failure, explicitly handling potential errors can improve contract robustness.

Consider wrapping the deposit in a require statement:

-        wzeta.deposit{value: msg.value}();
+        require(wzeta.deposit{value: msg.value}(), "WZETA deposit failed");

If the deposit function does not return a value, and only reverts on failure, this may not be necessary.

Committable suggestion was skipped due to low confidence.

---

72-73: ⚠️ Potential issue

Use dynamic approval amounts and handle potential approval failures

The approve function is called with a hard-coded amount of 100000000000000000, which may not align with the actual amount required for the transaction. Approving the exact amount needed enhances security by preventing over-approval of tokens.

Additionally, it's important to check the return value of the approve function to ensure the approval was successful. Ignoring a failed approval could lead to unexpected behavior in subsequent contract interactions.

Apply this diff to address the issue:

-        IZRC20(zrc20).approve(address(gatewayZEVM), 100000000000000000);
+        uint256 amount = /* specify the required amount */;
+        require(IZRC20(zrc20).approve(address(gatewayZEVM), amount), "Token approval failed");

Alternatively, consider using OpenZeppelin's SafeERC20 library for safer token interactions:

+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+
+using SafeERC20 for IERC20;
+
         uint256 amount = /* specify the required amount */;
-        IZRC20(zrc20).approve(address(gatewayZEVM), amount);
+        IERC20(zrc20).safeApprove(address(gatewayZEVM), amount);

Committable suggestion was skipped due to low confidence.

e2e/e2etests/test_v2_zevm_to_evm_call_through_contract.go (4)

71-78: ⚠️ Potential issue

Correct the method name to reflect the correct module

The method V2ZEVMToEMVAuthenticatedCallThroughContract contains the same typographical error here. Please update it to V2ZEVMToEVMAuthenticatedCallThroughContract.

Apply this diff to correct the method name:

-tx = r.V2ZEVMToEMVAuthenticatedCallThroughContract(
+tx = r.V2ZEVMToEVMAuthenticatedCallThroughContract(

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	tx = r.V2ZEVMToEVMAuthenticatedCallThroughContract(
		gatewayCaller,
		r.TestDAppV2EVMAddr,
		[]byte(payloadMessageEVMAuthenticatedCallThroughContract),
		gatewayzevmcaller.RevertOptions{
			OnRevertGasLimit: big.NewInt(0),
		},
	)

---

51-53: ⚠️ Potential issue

Handle potential errors returned by WaitCctxMinedByInboundHash

The function utils.WaitCctxMinedByInboundHash may return an error that is currently not being checked. For robust error handling, consider checking and handling any errors returned by this function.

Update the code to handle the error appropriately:

-cctx := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
+cctx, err := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
+require.NoError(r, err)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	cctx, err := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
	require.NoError(r, err)
	r.Logger.CCTX(*cctx, "call")
	require.Equal(r, crosschaintypes.CctxStatus_OutboundMined, cctx.CctxStatus.Status)

---

80-82: ⚠️ Potential issue

Handle potential errors returned by WaitCctxMinedByInboundHash

Ensure that any errors returned by utils.WaitCctxMinedByInboundHash are properly handled to prevent unexpected failures in the test.

Update the code to check and handle errors:

-cctx = utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
+cctx, err := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
+require.NoError(r, err)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	cctx, err := utils.WaitCctxMinedByInboundHash(r.Ctx, tx.Hash().Hex(), r.CctxClient, r.Logger, r.CctxTimeout)
	require.NoError(r, err)
	r.Logger.CCTX(*cctx, "call")
	require.Equal(r, crosschaintypes.CctxStatus_Reverted, cctx.CctxStatus.Status)

---

42-49: ⚠️ Potential issue

Correct the method name to reflect the correct module

The method V2ZEVMToEMVAuthenticatedCallThroughContract appears to have a typographical error in its name. It should be V2ZEVMToEVMAuthenticatedCallThroughContract to accurately represent the call from ZEVM to EVM.

Apply this diff to correct the method name:

-tx = r.V2ZEVMToEMVAuthenticatedCallThroughContract(
+tx = r.V2ZEVMToEVMAuthenticatedCallThroughContract(

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	tx = r.V2ZEVMToEVMAuthenticatedCallThroughContract(
		gatewayCaller,
		r.TestDAppV2EVMAddr,
		[]byte(payloadMessageEVMAuthenticatedCallThroughContract),
		gatewayzevmcaller.RevertOptions{
			OnRevertGasLimit: big.NewInt(0),
		},
	)

e2e/e2etests/test_v2_eth_withdraw_and_call_through_contract.go (2)

26-27: 🛠️ Refactor suggestion

Handle invalid amount input more explicitly

Currently, if args[0] does not represent a valid integer, the test will fail with a generic assertion message. Consider providing more informative feedback or handling the error explicitly to improve test diagnostics.

Example:

amount, ok := big.NewInt(0).SetString(args[0], 10)
if !ok {
    require.FailNow(r, "Invalid amount specified: %s", args[0])
}

---

59-59: 🛠️ Refactor suggestion

Enhance assertion clarity in AssertTestDAppEVMCalled

When asserting that the test DApp was called, consider adding more detailed messages or checks to provide clearer insights in case of test failures. This can aid in faster debugging and understanding of test outcomes.

pkg/contracts/testdappv2/TestDAppV2.sol (4)

105-106: 🛠️ Refactor suggestion

Ensure consistency in senderWithMessage mapping keys

The senderWithMessage mapping uses revertContext.revertMessage as the key. Since revertMessage is of type bytes, ensure that identical messages produce identical keys and consider hashing if necessary to prevent potential issues with varying data representations.

-    senderWithMessage[revertContext.revertMessage] = revertContext.sender;
+    bytes32 messageHash = keccak256(revertContext.revertMessage);
+    senderWithMessage[messageHash] = revertContext.sender;

Update the mapping declaration accordingly:

-mapping(bytes => address) public senderWithMessage;
+mapping(bytes32 => address) public senderWithMessage;

Committable suggestion was skipped due to low confidence.

---

112-116: ⚠️ Potential issue

Missing return statement in onCall function

The onCall function is declared to return bytes memory but does not include a return statement. This can lead to unexpected behavior or compilation errors.

Include a return statement to match the function signature:

 function onCall(MessageContext calldata messageContext, bytes calldata message) external payable returns (bytes memory) {
     require(messageContext.sender == expectedOnCallSender, "unauthenticated sender");
     setCalledWithMessage(string(message));
     setAmountWithMessage(string(message), msg.value);
     senderWithMessage[message] = messageContext.sender;
+    return "";
 }

If no data needs to be returned, consider changing the function signature:

-function onCall(MessageContext calldata messageContext, bytes calldata message) external payable returns (bytes memory) {
+function onCall(MessageContext calldata messageContext, bytes calldata message) external payable {

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    function onCall(MessageContext calldata messageContext, bytes calldata message) external payable returns (bytes memory) {
        require(messageContext.sender == expectedOnCallSender, "unauthenticated sender");
        setCalledWithMessage(string(message));
        setAmountWithMessage(string(message), msg.value);
        senderWithMessage[message] = messageContext.sender;
        return "";
    }

---

112-116: ⚠️ Potential issue

Ensure safe conversion from bytes to string

Converting bytes to string using string(message) can cause runtime errors if message is not valid UTF-8. This may lead to unexpected contract reverts.

Consider validating the message content or avoiding direct conversion. If the conversion is necessary, ensure that message contains valid UTF-8 sequences.

-    setCalledWithMessage(string(message));
+    string memory messageStr = string(message);
+    // Optional: Validate UTF-8 encoding if necessary
+    setCalledWithMessage(messageStr);

Alternatively, store message as bytes to prevent potential encoding issues.

 mapping(bytes32 => bool) public calledWithMessage;
 mapping(bytes32 => uint256) public amountWithMessage;
+mapping(bytes32 => bytes) public messageData;

 function onCall(MessageContext calldata messageContext, bytes calldata message) external payable returns (bytes memory) {
     require(messageContext.sender == expectedOnCallSender, "unauthenticated sender");
-    setCalledWithMessage(string(message));
+    messageData[keccak256(message)] = message;
     setAmountWithMessage(string(message), msg.value);
     senderWithMessage[message] = messageContext.sender;
+    return "";
 }

Committable suggestion was skipped due to low confidence.

---

108-110: ⚠️ Potential issue

Access control required for setExpectedOnCallSender function

The setExpectedOnCallSender function is marked as external and lacks access control. This allows any external account to modify the expectedOnCallSender state variable, potentially leading to unauthorized access and compromising the contract's integrity.

To enhance security, implement access control to restrict who can call this function. For example:

+address private owner;

+constructor() {
+    owner = msg.sender;
+}

 function setExpectedOnCallSender(address _expectedOnCallSender) external {
+    require(msg.sender == owner, "Caller is not authorized");
     expectedOnCallSender = _expectedOnCallSender;
 }

Alternatively, you can use Solidity's AccessControl or Ownable patterns for more robust permission management.

Committable suggestion was skipped due to low confidence.

e2e/runner/v2_zevm.go (4)

55-55: ⚠️ Potential issue

Update function comment to match the function name

The comment on line 55 refers to V2ETHWithdrawAndCall, but the function is named V2ETHWithdrawAndAuthenticatedCall. Please update the comment to accurately reflect the function name for clarity and consistency.

Apply this diff to correct the comment:

-// V2ETHWithdrawAndCall calls WithdrawAndCall of Gateway with gas token on ZEVM using authenticated call
+// V2ETHWithdrawAndAuthenticatedCall calls WithdrawAndCall2 of Gateway with gas token on ZEVM using an authenticated call

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

// V2ETHWithdrawAndAuthenticatedCall calls WithdrawAndCall2 of Gateway with gas token on ZEVM using an authenticated call

---

79-80: ⚠️ Potential issue

Correct function comment to reflect the accurate method name

The comments on lines 79-80 mention V2ETHWithdrawAndCall, which does not match the function V2ETHWithdrawAndAuthenticatedCallThroughContract. Please update the comments to match the function name and provide a clear description of its purpose.

Suggested changes:

-// V2ETHWithdrawAndCall calls WithdrawAndCall of Gateway with gas token on ZEVM using authenticated call
-// through contract
+// V2ETHWithdrawAndAuthenticatedCallThroughContract calls `WithdrawAndCallGatewayZEVM` using an authenticated call through a contract

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

// V2ETHWithdrawAndAuthenticatedCallThroughContract calls `WithdrawAndCallGatewayZEVM` using an authenticated call through a contract

---

88-103: ⚠️ Potential issue

Handle errors appropriately in V2ETHWithdrawAndAuthenticatedCallThroughContract

The function uses require.NoError(r, err), which will halt execution if an error occurs. If this function is intended for use outside of testing contexts, consider handling errors gracefully to enhance robustness.

Consider revising the error handling as follows:

     tx, err := gatewayZEVMCaller.WithdrawAndCallGatewayZEVM(
         r.ZEVMAuth,
         receiver.Bytes(),
         amount,
         r.ETHZRC20Addr,
         payload,
         gatewayzevmcaller.CallOptions{
             IsArbitraryCall: false,
             GasLimit:        gasLimit,
         },
         revertOptions,
     )
-    require.NoError(r, err)
+    if err != nil {
+        // Handle the error appropriately, e.g., log it or return it
+        return nil // or propagate the error
+    }

     return tx

Committable suggestion was skipped due to low confidence.

---

200-214: ⚠️ Potential issue

Ensure proper error handling in V2ZEVMToEMVAuthenticatedCallThroughContract

Similar to the previous function, require.NoError(r, err) is used, which may not be suitable outside of tests. Implementing graceful error handling can improve the function's reliability in different environments.

Suggested modification:

     tx, err := gatewayZEVMCaller.CallGatewayZEVM(
         r.ZEVMAuth,
         receiver.Bytes(),
         r.ETHZRC20Addr,
         payload,
         gatewayzevmcaller.CallOptions{
             GasLimit:        gasLimit,
             IsArbitraryCall: false,
         },
         revertOptions,
     )
-    require.NoError(r, err)
+    if err != nil {
+        // Handle the error appropriately
+        return nil // or handle the error as needed
+    }

     return tx

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	tx, err := gatewayZEVMCaller.CallGatewayZEVM(
		r.ZEVMAuth,
		receiver.Bytes(),
		r.ETHZRC20Addr,
		payload,
		gatewayzevmcaller.CallOptions{
			GasLimit:        gasLimit,
			IsArbitraryCall: false,
		},
		revertOptions,
	)
	if err != nil {
		// Handle the error appropriately
		return nil // or handle the error as needed
	}

	return tx
}

zetaclient/chains/evm/signer/v2_sign.go (5)

82-82: ⚠️ Potential issue

Increase test coverage for the modified RevertContext.

Static analysis indicates that line 82 is not covered by tests. To ensure the Sender field in RevertContext behaves as expected, add unit tests that cover scenarios involving inboundSender.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 82-82: zetaclient/chains/evm/signer/v2_sign.go#L82
Added line #L82 was not covered by tests

---

202-202: ⚠️ Potential issue

Enhance test coverage for the updated RevertContext.

Line 202 is not covered by existing tests according to static analysis. Introducing tests that specifically target the Sender field within the RevertContext will help ensure correctness and prevent future regressions.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 202-202: zetaclient/chains/evm/signer/v2_sign.go#L202
Added line #L202 was not covered by tests

---

187-187: ⚠️ Potential issue

Validate the inboundSender parameter in signERC20CustodyWithdrawRevert function.

The inboundSender parameter should be validated to confirm it's a valid Ethereum address before conversion. This prevents potential issues arising from invalid addresses affecting the transaction construction.

Apply the following diff to add validation:

 func (signer *Signer) signERC20CustodyWithdrawRevert(
 	ctx context.Context,
 	inboundSender string,
 	txData *OutboundData,
 ) (*ethtypes.Transaction, error) {
+	if !common.IsHexAddress(inboundSender) {
+		return nil, fmt.Errorf("invalid inboundSender address: %s", inboundSender)
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	inboundSender string,
) (*ethtypes.Transaction, error) {
	if !common.IsHexAddress(inboundSender) {
		return nil, fmt.Errorf("invalid inboundSender address: %s", inboundSender)
	}

---

20-24: ⚠️ Potential issue

Validate the sender parameter to ensure it's a valid Ethereum address.

The sender parameter is accepted as a string and converted to an Ethereum address using common.HexToAddress(sender). If sender is not a valid hexadecimal address, HexToAddress will return a zero address (0x000...), which may lead to unintended behavior when constructing the messageContext. It is advisable to validate the sender string before conversion.

Apply the following diff to add validation:

 func (signer *Signer) signGatewayExecute(
 	ctx context.Context,
 	sender string,
 	txData *OutboundData,
 ) (*ethtypes.Transaction, error) {
+	if !common.IsHexAddress(sender) {
+		return nil, fmt.Errorf("invalid sender address: %s", sender)
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

func (signer *Signer) signGatewayExecute(
	ctx context.Context,
	sender string,
	txData *OutboundData,
) (*ethtypes.Transaction, error) {
	if !common.IsHexAddress(sender) {
		return nil, fmt.Errorf("invalid sender address: %s", sender)
	}

---

69-69: ⚠️ Potential issue

Validate the inboundSender parameter to ensure it's a valid Ethereum address.

Similar to the signGatewayExecute function, inboundSender is a string converted to an Ethereum address. Without validation, an invalid inboundSender could result in a zero address, causing unexpected behavior during transaction signing. Implement validation to ensure robustness.

Apply the following diff to add validation:

 func (signer *Signer) signGatewayExecuteRevert(
 	ctx context.Context,
 	inboundSender string,
 	txData *OutboundData,
 ) (*ethtypes.Transaction, error) {
+	if !common.IsHexAddress(inboundSender) {
+		return nil, fmt.Errorf("invalid inboundSender address: %s", inboundSender)
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	inboundSender string,
) (*ethtypes.Transaction, error) {
	if !common.IsHexAddress(inboundSender) {
		return nil, fmt.Errorf("invalid inboundSender address: %s", inboundSender)
	}

pkg/contracts/gatewayzevmcaller/GatewayZEVMCaller.abi (1)

174-250: ⚠️ Potential issue

Resolve Potential Overload Ambiguity in Function Definitions

The ABI defines two functions named withdrawAndCallGatewayZEVM with different parameter types:

• Function 1 (lines 98-173):
  • Parameters: bytes receiver, uint256 amount, address zrc20, bytes message, struct CallOptions callOptions, struct RevertOptions revertOptions.
• Function 2 (lines 175-250):
  • Parameters: bytes receiver, uint256 amount, uint256 chainId, bytes message, struct CallOptions callOptions, struct RevertOptions revertOptions.

While Solidity allows function overloading based on parameter types, overloading can lead to issues with ABI encoding and off-chain interactions, particularly since both functions have identical names and similar parameter lists. This may cause confusion or errors when interfacing with the contract from external applications or scripts that rely on the function name.

Recommendations:

• Rename one of the functions to clearly distinguish its purpose and avoid ambiguity. For example, consider names like withdrawAndCallGatewayZEVMWithZRC20 and withdrawAndCallGatewayZEVMWithChainId.
• Ensure unique function selectors by avoiding parameter lists that could potentially lead to hash collisions in the function signatures.
• Update documentation and interfaces to reflect any changes, ensuring that developers interacting with the contract are aware of the distinct functions and their specific uses.

x/crosschain/types/cctx.go (4)

43-46: ⚠️ Potential issue

Ensure OutboundParams are fully initialized when empty

When m.OutboundParams is empty, the function returns a new OutboundParams with only CallOptions initialized. This may lead to uninitialized fields in OutboundParams, potentially causing nil pointer dereferences or unexpected behavior elsewhere in the code. Please ensure all necessary fields in OutboundParams are properly initialized.

---

132-135: ⚠️ Potential issue

Initialize all required fields in revertTxParams

When constructing revertTxParams, only a subset of fields are initialized. This might lead to incomplete data and potential errors when these fields are accessed later. Ensure that all mandatory fields of OutboundParams are properly initialized to maintain data integrity.

---

243-250: ⚠️ Potential issue

Handle potential nil CallOptions in msg

Accessing msg.CallOptions.IsArbitraryCall and msg.CallOptions.GasLimit without checking if msg.CallOptions is nil can lead to a nil pointer dereference. To ensure robustness, please add a nil check for msg.CallOptions before accessing its fields.

Apply this diff to safely handle nil CallOptions:

 outboundParams := &OutboundParams{
     Receiver:        msg.Receiver,
     ReceiverChainId: msg.ReceiverChain,
     Hash:            "",
     TssNonce:        0,
-    CallOptions: &CallOptions{
-        IsArbitraryCall: msg.CallOptions.IsArbitraryCall,
-        GasLimit:        msg.CallOptions.GasLimit,
-    },
+    CallOptions: func() *CallOptions {
+        if msg.CallOptions != nil {
+            return &CallOptions{
+                IsArbitraryCall: msg.CallOptions.IsArbitraryCall,
+                GasLimit:        msg.CallOptions.GasLimit,
+            }
+        }
+        return &CallOptions{}
+    }(),
     GasPrice:               "",
     GasPriorityFee:         "",
     BallotIndex:            "",
     ObservedExternalHeight: 0,
     Amount:                 sdkmath.ZeroUint(),
     TssPubkey:              tssPubkey,
     CoinType:               msg.CoinType,
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

		Receiver:        msg.Receiver,
		ReceiverChainId: msg.ReceiverChain,
		Hash:            "",
		TssNonce:        0,
		CallOptions: func() *CallOptions {
			if msg.CallOptions != nil {
				return &CallOptions{
					IsArbitraryCall: msg.CallOptions.IsArbitraryCall,
					GasLimit:        msg.CallOptions.GasLimit,
				}
			}
			return &CallOptions{}
		}(),

---

50-54: 🛠️ Refactor suggestion

Avoid mutating shared OutboundParams

Modifying outboundParams.CallOptions directly may have unintended side effects if outboundParams is referenced elsewhere. To maintain immutability and prevent potential bugs, consider creating a copy of outboundParams before modification.

Apply this diff to create a copy before modification:

 outboundParams := m.OutboundParams[len(m.OutboundParams)-1]
+outboundParamsCopy := *outboundParams
 if outboundParams.CallOptions == nil {
-    outboundParams.CallOptions = &CallOptions{
+    outboundParamsCopy.CallOptions = &CallOptions{
         GasLimit: outboundParams.GasLimit,
     }
 }
+return &outboundParamsCopy

Committable suggestion was skipped due to low confidence.

x/crosschain/keeper/gas_payment.go (2)

124-129: ⚠️ Potential issue

Add unit tests to cover the new RevertGasLimit logic

The introduced logic adjusts gas.GasLimit based on RevertOptions.RevertGasLimit. To ensure this functionality works as intended and to prevent future regressions, please add unit tests covering scenarios where CallOnRevert is true and RevertGasLimit is set.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 127-127: x/crosschain/keeper/gas_payment.go#L127
Added line #L127 was not covered by tests

---

148-150: ⚠️ Potential issue

Ensure CallOptions is properly initialized before assignment

Before assigning values to CallOptions.GasLimit, CallOptions.GasPrice, and CallOptions.GasPriorityFee, ensure that CallOptions is initialized to prevent potential nil pointer dereferences.

Apply this diff to initialize CallOptions if it is nil:

+    if cctx.GetCurrentOutboundParam().CallOptions == nil {
+        cctx.GetCurrentOutboundParam().CallOptions = &types.CallOptions{}
+    }
     cctx.GetCurrentOutboundParam().CallOptions.GasLimit = gas.GasLimit.Uint64()
     cctx.GetCurrentOutboundParam().GasPrice = gas.GasPrice.String()
     cctx.GetCurrentOutboundParam().GasPriorityFee = gas.PriorityFee.String()

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	if cctx.GetCurrentOutboundParam().CallOptions == nil {
		cctx.GetCurrentOutboundParam().CallOptions = &types.CallOptions{}
	}
	cctx.GetCurrentOutboundParam().CallOptions.GasLimit = gas.GasLimit.Uint64()
	cctx.GetCurrentOutboundParam().GasPrice = gas.GasPrice.String()
	cctx.GetCurrentOutboundParam().GasPriorityFee = gas.PriorityFee.String()

e2e/e2etests/e2etests.go (3)

747-747: ⚠️ Potential issue

Add missing conjunction 'and' in test descriptions for clarity

The descriptions for TestV2ETHWithdrawAndCallName and TestV2ETHWithdrawAndCallThroughContractName are missing the word "and" between "ZEVM" and "call", which improves readability and clarity.

Please apply the following changes:

For line 747:

-"withdraw Ether from ZEVM call a contract using V2 contract",
+"withdraw Ether from ZEVM and call a contract using V2 contract",

For line 755:

-"withdraw Ether from ZEVM call a contract using V2 contract through intermediary contract",
+"withdraw Ether from ZEVM and call a contract using V2 contract through intermediary contract",

Also applies to: 755-755

---

746-750: 🛠️ Refactor suggestion

Consistent Indentation for Readability

Ensure that the indentation is consistent within the arguments of runner.NewE2ETest for better readability and maintainability.

Please adjust the indentation as follows:

 runner.NewE2ETest(
 	TestV2ETHWithdrawAndCallName,
 	"withdraw Ether from ZEVM and call a contract using V2 contract",
-	[]runner.ArgDefinition{
-		{Description: "amount in wei", DefaultValue: "100000"},
-	},
+	[]runner.ArgDefinition{
+		{Description: "amount in wei", DefaultValue: "100000"},
+	},
 	TestV2ETHWithdrawAndCall,
 ),

Committable suggestion was skipped due to low confidence.

---

854-858: 🛠️ Refactor suggestion

Clarify Test Description for TestV2ZEVMToEVMCallThroughContractName

Consider expanding the test description to make it more descriptive of the test's purpose, which enhances understandability.

Suggested change:

-"zevm -> evm call using V2 contract through intermediary contract",
+"Perform a ZEVM to EVM call using V2 contract through an intermediary contract",

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

		TestV2ZEVMToEVMCallThroughContractName,
		"Perform a ZEVM to EVM call using V2 contract through an intermediary contract",
		[]runner.ArgDefinition{},
		TestV2ZEVMToEVMCallThroughContract,
	),