docs/openapi/openapi.swagger.yaml (1)

57205-57209: Approve addition with minor title refinement.

The new is_cross_chain_call property is correctly defined and appropriately placed within the schema. The title provides valuable context for its purpose.

To enhance clarity and maintain consistency with OpenAPI best practices, consider refining the title as follows:

       is_cross_chain_call:
         type: boolean
         title: |-
-          this field describes if a smart contract call should be made for a inbound
-          with assets only used for protocol contract version 2
+          Indicates whether a smart contract call should be made for an inbound
+          transaction with assets used only in protocol contract version 2

This adjustment improves readability and aligns more closely with standard OpenAPI documentation practices.

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

1-1: Approve the EVM version specification and suggest pinning the compiler version.

The addition of --evm-version london is a prudent change, ensuring compatibility with the London hard fork features. This explicit version specification aligns with best practices in Solidity development.

To further enhance reproducibility and consistency across different environments, consider pinning the Solidity compiler version as well.

Modify the go:generate directive as follows:

-//go:generate sh -c "solc TestDAppV2.sol --evm-version london --combined-json abi,bin | jq '.contracts.\"TestDAppV2.sol:TestDAppV2\"'  > TestDAppV2.json"
+//go:generate sh -c "solc TestDAppV2.sol --evm-version london --combined-json abi,bin --version 0.8.18 | jq '.contracts.\"TestDAppV2.sol:TestDAppV2\"'  > TestDAppV2.json"

This change ensures that the contract is always compiled with a specific Solidity version, reducing the risk of unexpected behavior due to compiler differences.

e2e/e2etests/test_v2_erc20_deposit_and_call.go (1)

Line range hint 1-52: Enhance error handling and consistency in the test function.

While the function generally follows good practices, there are a few areas for improvement:

1. Error Handling: Some method calls that can return errors are not checked. For instance, the V2ERC20DepositAndCall call.

2. Magic Numbers: The OnRevertGasLimit is set to 0 directly. Consider using a named constant for better readability and maintainability.

3. Consistency: The function mixes direct error checks and the use of r. for assertions. Strive for consistency in error handling throughout the function.

Consider applying these improvements:

1. Add error checking:

tx, err := r.V2ERC20DepositAndCall(
    r.TestDAppV2ZEVMAddr,
    amount,
    []byte(payloadMessageDepositERC20),
    gatewayevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)},
)
require.NoError(r, err)

2. Use a named constant:

const defaultOnRevertGasLimit = 0

// ...

gatewayevm.RevertOptions{OnRevertGasLimit: big.NewInt(defaultOnRevertGasLimit)},

3. Consistently use r. for assertions:

r.Require().NoError(err)
r.Require().True(ok, "Invalid amount specified for TestV2ERC20DepositAndCall")
// Apply this pattern throughout the function

These changes will improve the robustness and readability of the test function.

e2e/e2etests/test_v2_eth_deposit_and_call_no_message.go (4)

15-22: Consider enhancing error messages for improved debugging.

The initial setup of the function is well-structured. However, to facilitate easier debugging, consider providing more descriptive error messages. For instance:

require.Len(r, args, 1, "TestV2ETHDepositAndCallNoMessage requires exactly one argument for the deposit amount")

amount, ok := big.NewInt(0).SetString(args[0], 10)
require.True(r, ok, "Invalid amount specified for TestV2ETHDepositAndCallNoMessage: %s", args[0])

This change would provide more context in case of failures, aiding in quicker issue resolution.

---

24-30: Document the rationale for RevertOptions configuration.

The deposit and call execution is implemented correctly. However, the reasoning behind setting OnRevertGasLimit to 0 in the RevertOptions is not immediately clear. Consider adding a comment explaining this choice:

// perform the deposit and call to the TestDAppV2ZEVMAddr
tx := r.V2ETHDepositAndCall(
    r.TestDAppV2ZEVMAddr,
    amount,
    []byte{},
    gatewayevm.RevertOptions{
        // OnRevertGasLimit is set to 0 to [explain the reason, e.g., "disable custom gas limit on revert"]
        OnRevertGasLimit: big.NewInt(0),
    },
)

This documentation would clarify the intent and help maintain the code in the future.

---

37-40: Enhance error handling for GetNoMessageIndex call.

The payload reception check is implemented correctly. However, to improve robustness, consider adding more context to the error returned by GetNoMessageIndex. This can be achieved by wrapping the error:

messageIndex, err := r.TestDAppV2ZEVM.GetNoMessageIndex(&bind.CallOpts{}, r.EVMAddress())
if err != nil {
    return fmt.Errorf("failed to get NoMessageIndex: %w", err)
}

This change would provide more context in case of failures, aiding in quicker issue resolution.

---

42-46: Enhance assertion message for balance check.

The balance update check is implemented correctly. To improve debugging in case of a failure, consider providing more context in the assertion message:

newBalance, err := r.ETHZRC20.BalanceOf(&bind.CallOpts{}, r.TestDAppV2ZEVMAddr)
require.NoError(r, err)
expectedBalance := new(big.Int).Add(oldBalance, amount)
require.Equal(r, expectedBalance, newBalance, "New balance (%s) does not match expected balance (%s)", newBalance, expectedBalance)

This change would provide more detailed information about the actual and expected balances in case of a mismatch.

x/crosschain/types/inbound_parsing_test.go (3)

13-15: Consider setting a timeline for completing the TODO items.

While TODO comments are generally discouraged in production code, they can be acceptable in test files if properly tracked. However, it would be beneficial to set a timeline for addressing these incomplete tests to ensure comprehensive test coverage.

---

32-49: Expand test coverage for NewWithdrawalInbound.

The current test case for NewWithdrawalInbound is well-structured and correctly validates the error condition for an invalid chain ID. However, to ensure comprehensive coverage, consider adding the following test cases:

1. A successful case with valid inputs.
2. Edge cases for various input parameters (e.g., zero or negative amounts).
3. Tests for different chain IDs and their corresponding behaviors.

Expanding the test suite will provide better confidence in the function's correctness across various scenarios.

---

51-68: Expand test coverage and consider refactoring for NewCallInbound.

The current test case for NewCallInbound is well-structured and correctly validates the error condition for an invalid chain ID. However, to improve the test suite:

1. Add more test cases:

   • A successful case with valid inputs.
   • Edge cases for various input parameters.
   • Tests for different chain IDs and their corresponding behaviors.

2. Consider refactoring the common setup code for TestNewWithdrawalInbound and TestNewCallInbound into a helper function to reduce duplication and improve maintainability.

Example refactor:

func setupInvalidChainIDTest(t *testing.T) (types.Context, string) {
    _, ctx, _, _ := keepertest.CrosschainKeeper(t)
    ctx = ctx.WithChainID("invalidChainID")
    return ctx, sample.EthAddress().Hex()
}

// Use in both test functions
ctx, sampleAddress := setupInvalidChainIDTest(t)

This refactoring will make the tests more concise and easier to maintain.

zetaclient/chains/evm/cctx.go (1)

65-68: Consistent logic applied to ERC20 transactions.

The changes for ERC20 transactions mirror those made for gas transactions, maintaining consistency in the codebase. This parallel structure enhances readability and addresses the PR objectives for both transaction types.

Consider refactoring the common logic for both CoinType_Gas and CoinType_ERC20 cases to reduce code duplication and improve maintainability. For example:

func determineOutboundType(isCrossChainCall bool, baseType, andCallType OutboundType) OutboundType {
    if isCrossChainCall {
        return andCallType
    }
    return baseType
}

// Usage in ParseOutboundTypeFromCCTX:
case coin.CoinType_Gas:
    return determineOutboundType(cctx.InboundParams.IsCrossChainCall, OutboundTypeGasWithdraw, OutboundTypeGasWithdrawAndCall)
case coin.CoinType_ERC20:
    return determineOutboundType(cctx.InboundParams.IsCrossChainCall, OutboundTypeERC20Withdraw, OutboundTypeERC20WithdrawAndCall)

This refactoring would centralize the logic and make future modifications easier.

zetaclient/chains/evm/cctx_test.go (1)

13-96: Comprehensive test coverage with minor improvements needed.

The test cases cover a wide range of scenarios, including different coin types, cross-chain calls, and revert conditions. This comprehensive approach ensures robust testing of the ParseOutboundTypeFromCCTX function.

However, there are two minor points for improvement:

1. There's a typo in the test case name on line 85. "Uuknown" should be "Unknown".

2. The test case for the ZETA coin type (lines 85-95) could be more explicit about why it results in an unknown outbound type. Consider adding a comment explaining this behavior or renaming the test case to better reflect the expected outcome.

Please apply the following changes:

-			name: "ZETA gives Uuknown outbound type",
+			name: "ZETA results in Unknown outbound type",

Also, consider adding a comment explaining why ZETA results in an unknown outbound type, if this is the intended behavior.

zetaclient/chains/evm/constant.go (1)

50-52: Approve the addition of TopicsGatewayDepositAndCall constant.

The new constant TopicsGatewayDepositAndCall is well-defined and consistent with the existing codebase. It enhances the system's capability to handle gateway deposit and call events.

For improved consistency, consider adding a reference to the relevant smart contract file and line number in the comment, similar to other constants in this file.

 // TopicsGatewayDepositAndCall is the number of topics for a gateway deposit and call event
 // [signature, sender, receiver]
+// https://github.com/zeta-chain/protocol-contracts/blob/main/contracts/evm/ZetaGateway.sol#L<LINE_NUMBER>
 TopicsGatewayDepositAndCall = 3

cmd/zetae2e/local/v2.go (2)

29-30: Approve addition of new test cases with a minor suggestion.

The inclusion of TestV2ETHDepositAndCallNoMessageName and TestV2ETHWithdrawAndCallNoMessageName aligns with the PR objectives and enhances test coverage for empty message scenarios.

To improve clarity, consider adding a comment above these new test cases to explicitly state their purpose:

// Test cases for deposit and withdraw with empty messages
e2etests.TestV2ETHDepositAndCallNoMessageName,
e2etests.TestV2ETHWithdrawAndCallNoMessageName,

This addition would enhance code readability and maintainability.

---

41-41: Approve addition of new test case with a suggestion for completeness.

The inclusion of TestV2ERC20DepositAndCallNoMessageName is appropriate and aligns with the PR objectives. However, for consistency with the ETH workflow, consider adding a corresponding test case for ERC20 withdrawal with an empty message:

e2etests.TestV2ERC20DepositAndCallNoMessageName,
e2etests.TestV2ERC20WithdrawAndCallNoMessageName,

This addition would ensure comprehensive coverage of both deposit and withdrawal scenarios for ERC20 tokens with empty messages, maintaining parity with the ETH workflow testing.

x/crosschain/types/message_vote_inbound.go (2)

89-89: Appropriate initialization of the new field.

The IsCrossChainCall field is correctly initialized to false, maintaining backward compatibility. To enhance code clarity, consider using a named constant for the default value:

const DefaultIsCrossChainCall = false

// In NewMsgVoteInbound
IsCrossChainCall: DefaultIsCrossChainCall,

This approach would improve readability and make it easier to update the default value if needed in the future.

---

Serialization Implications Identified for MsgVoteInbound

The IsCrossChainCall field has been added to the MsgVoteInbound struct; however, it is not reflected in the protobuf definitions or JSON marshaling. This omission can lead to serialization inconsistencies and potential runtime errors.

• Update Protobuf Definitions: Ensure that the IsCrossChainCall field is included in all relevant .proto files.
• Review JSON Marshaling: Verify and update JSON struct tags to incorporate the new field appropriately.

🔗 Analysis chain

Line range hint 41-89: Consider additional validations and serialization implications.

The introduction of the IsCrossChainCall field and its associated option function are well-implemented. However, there are two points to consider:

1. The ValidateBasic method hasn't been updated to include validation for the new field. Consider whether any validation is necessary for this field.

2. The addition of a new field may have implications for serialization and deserialization, especially if this struct is used in network communications or persistent storage. Ensure that all serialization mechanisms (e.g., protobuf definitions, JSON marshaling) are updated accordingly.

To verify the serialization implications, you may want to run the following command:

This will help identify areas where serialization might need to be updated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for protobuf definitions and JSON marshaling of MsgVoteInbound
rg -A 10 "MsgVoteInbound" | rg "(proto|json)"

Length of output: 11322

proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)

46-49: Approve the addition of is_cross_chain_call field with suggestions for improvement.

The new field is_cross_chain_call is a suitable addition to support the PR objectives. However, consider the following recommendations:

1. Enhance the comment for clarity:

// Indicates whether a smart contract call should be made for an inbound transaction.
// This field is specifically used for assets in protocol contract version 2.
bool is_cross_chain_call = 12;

2. As per the past review comment, this field might be subject to future changes. Consider adding a TODO comment to track this:

// TODO: This field may be removed or generalized for all inbound transactions
// in the future. Track progress at https://github.com/zeta-chain/node/issues/1949

3. Ensure backwards compatibility is maintained when implementing logic related to this field.

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

113-113: Approve the addition of IsCrossChainCall parameter with a suggestion for clarity.

The addition of cctx.InboundParams.IsCrossChainCall as a parameter to ZRC20DepositAndCallContract is appropriate and aligns with the PR objectives. This change enables the differentiation between standard deposits/withdrawals and those utilizing depositAndCall/withdrawAndCall functionalities.

To enhance code clarity, consider adding a brief comment explaining the purpose of this parameter:

// IsCrossChainCall indicates whether this is a cross-chain call (depositAndCall/withdrawAndCall)
cctx.InboundParams.IsCrossChainCall,

This addition will provide context for future developers working on this code.

pkg/contracts/testdappv2/TestDAppV2.abi (1)

127-145: Approval: getNoMessageIndex function addition is appropriate.

The new getNoMessageIndex function aligns with the PR objectives by providing a mechanism to retrieve information about no-message calls for a specific address. The function signature and "pure" stateMutability are consistent with Solidity best practices.

Consider adding inline documentation to clarify the purpose and expected return value of this function. For example:

/**
 * @dev Retrieves the index associated with no-message calls for a given address.
 * @param sender The address to query.
 * @return A string representing the no-message index for the given address.
 */
function getNoMessageIndex(address sender) external pure returns (string memory);

e2e/runner/v2_zevm.go (3)

Line range hint 79-102: Approve changes with minor suggestion for consistency.

The implementation of V2ETHWithdrawAndCallThroughContract is well-structured and consistent with the overall design. It correctly uses the GatewayZEVMCaller for contract interaction.

For improved consistency with other functions in this file, consider adding a comment explaining the gas limit usage:

// This function may require more gas than the default limit
previousGasLimit := r.ZEVMAuth.GasLimit
r.ZEVMAuth.GasLimit = 10000000
defer func() {
    r.ZEVMAuth.GasLimit = previousGasLimit
}()

This change would align the gas handling with other functions like V2ERC20WithdrawAndCall.

---

Line range hint 123-152: Approve changes with suggestion for improved clarity.

The renaming of V2ERC20WithdrawAndArbitraryCall and its implementation are appropriate. The function correctly handles the increased gas limit requirement.

To enhance code clarity, consider extracting the gas limit value to a named constant:

const highGasLimit = 10000000

// Usage
r.ZEVMAuth.GasLimit = highGasLimit

This change would improve readability and make it easier to update the gas limit if needed in the future.

---

Line range hint 222-245: Approve changes with suggestion for consistency.

The implementation of V2ZEVMToEMVCallThroughContract is well-structured and correctly uses the GatewayZEVMCaller for contract interaction.

For consistency with other functions like V2ETHWithdrawAndCallThroughContract, consider adding a comment explaining the purpose of this function:

// V2ZEVMToEMVCallThroughContract calls authenticated Call of Gateway on ZEVM through contract
// This function facilitates cross-chain calls from ZEVM to EVM-compatible chains

This addition would provide context and improve the overall documentation of the codebase.

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

181-183: Approve addition of is_cross_chain_call field with suggestion for improved documentation.

The addition of the is_cross_chain_call boolean field to the MsgVoteInbound message is appropriate and follows Protobuf best practices. This enhancement allows for clear indication of whether a smart contract call should be made with the asset, which is valuable for cross-chain operations.

To further improve the code, consider adding a comment that provides more context about when this field should be set to true and its implications on the system's behavior.

Consider adding a descriptive comment above the field, such as:

// Indicates whether a smart contract call should be made with the asset.
// Set to true for cross-chain contract calls, false for standard transfers.
bool is_cross_chain_call = 19;

x/fungible/keeper/v2_deposits_test.go (5)

18-50: Approve implementation with minor enhancement suggestion.

The getTestDAppNoMessageIndex function is well-structured and follows best practices for error handling and type assertion. To further enhance its robustness, consider adding a timeout mechanism to the CallEVM function call to prevent potential blocking issues in case of network delays or contract execution problems.

---

141-152: Approve test case with suggestion for additional assertions.

The test case effectively covers the basic functionality of processing a deposit without a contract call. To enhance its comprehensiveness, consider adding assertions to verify:

1. The state of other relevant contract variables or events that might be affected by the deposit.
2. The gas consumption of the operation to ensure it remains within expected limits.

These additional checks would provide a more thorough validation of the deposit process.

---

Line range hint 154-186: Enhance test case to verify message ignorance.

While the test case correctly verifies the deposit process with a non-empty message, it could be strengthened by adding explicit checks to ensure the message is truly ignored. Consider the following enhancements:

1. Add an assertion to verify that no event related to message processing is emitted.
2. If applicable, check that any contract state that might be affected by message processing remains unchanged.
3. Compare the gas consumption with the previous test case to ensure it's consistent, indicating that no additional processing occurred due to the presence of the message.

These additions would provide stronger evidence that the message is indeed ignored in this scenario.

---

Line range hint 188-222: Enhance test coverage for deposit and call scenario.

The test case effectively covers the basic flow of processing a deposit with a contract call. To improve its robustness and coverage, consider the following enhancements:

1. Add assertions to verify the emitted events, ensuring they contain the correct information about the deposit and contract call.
2. Include negative test cases, such as:
   a. Attempting a deposit with insufficient balance.
   b. Calling a non-existent contract method.
   c. Passing invalid parameters to the contract call.
3. Verify the gas consumption of the operation to ensure it's within expected limits for a contract call scenario.
4. Add a test case with a large deposit amount to check for potential overflow issues.

These additions would provide a more comprehensive test suite for the deposit and call functionality.

---

224-270: Approve new test case with suggestions for enhancement.

The test case effectively covers the new scenario of processing a deposit and call without a message, aligning well with the PR objectives. The use of getTestDAppNoMessageIndex and subsequent verification of the contract state are appropriate. To further strengthen this test, consider the following enhancements:

1. Add assertions to verify that the correct events are emitted, including the absence of a message-related event.
2. Include a check to ensure that the messageIndex is unique for each call, preventing potential conflicts.
3. Add a negative test case where isCrossChainCall is set to false, to ensure the no-message index is not generated in this scenario.
4. Verify the gas consumption difference between this case and the case with a message, to ensure efficiency in handling no-message calls.

These additions would provide a more comprehensive validation of the new functionality.

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

194-194: Add description for the new field and improve formatting.

The addition of the is_cross_chain_call field enhances the functionality for cross-chain calls, aligning with the PR objectives. However, to improve clarity and maintainability, consider the following suggestions:

1. Add a brief description of the is_cross_chain_call field, explaining its purpose and usage.
2. For consistency with the rest of the document, replace the hard tab with spaces.

Here's a suggested improvement:

-	bool is_cross_chain_call = 19;
+    bool is_cross_chain_call = 19; // Indicates whether this is a cross-chain call (e.g., depositAndCall or withdrawAndCall)

🧰 Tools

🪛 Markdownlint

194-194: Column: 1
Hard tabs

(MD010, no-hard-tabs)

---

194-194: Update surrounding documentation to provide context for the new field.

While the addition of the is_cross_chain_call field is appropriate, the surrounding documentation lacks context for this new functionality. To enhance clarity and maintainability, consider updating the documentation to:

1. Explain the purpose of the is_cross_chain_call field in the context of depositAndCall and withdrawAndCall operations.
2. Describe how this field affects the processing of inbound votes, especially when messages are empty.
3. Provide an example or use case to illustrate when this field would be set to true.

Here's a suggested addition to the documentation above the MsgVoteInbound definition:

The `MsgVoteInbound` message now supports differentiation between standard deposits/withdrawals and cross-chain calls (e.g., `depositAndCall` and `withdrawAndCall`), even when the message field is empty. This is achieved through the `is_cross_chain_call` field.

🧰 Tools

🪛 Markdownlint

194-194: Column: 1
Hard tabs

(MD010, no-hard-tabs)

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

Line range hint 1-340: Suggest comprehensive implementation of the new feature

While the addition of the IsCrossChainCall field is a step in the right direction, the implementation appears incomplete. Consider the following suggestions to fully integrate this new feature:

1. Update the Validate() method of the CrossChainTx struct to include validation for the new field.
2. Modify relevant methods such as AddRevertOutbound(), AddOutbound(), and others to utilize the IsCrossChainCall information where appropriate.
3. Add unit tests to cover the new functionality and ensure proper handling of cross-chain calls.
4. Update documentation throughout the file to reflect the new cross-chain call functionality.

Implementing these suggestions will ensure a more robust and complete integration of the new feature.

Would you like assistance in generating the code for these improvements?

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 240-240: x/crosschain/types/cctx.go#L240
Added line #L240 was not covered by tests

x/crosschain/types/message_vote_inbound_test.go (1)

188-251: Approve the new test case with a minor suggestion.

The new test case effectively verifies the functionality of setting cross-chain call options in the MsgVoteInbound message. It covers all scenarios: default value, setting to true, and setting to false.

Consider renaming the test case to be more specific:

-t.Run("can set is cross chain call options", func(t *testing.T) {
+t.Run("verify cross chain call options default and setting", func(t *testing.T) {

This name more accurately describes the test's purpose and aligns with best practices for test naming.

x/fungible/keeper/deposits_test.go (7)

48-48: Approve changes with minor suggestion for clarity

The addition of the false parameter is correct and consistent with the updated method signature. However, to enhance code readability, consider using a named parameter:

isCrossChainCall: false,

This change would make the purpose of the boolean value more explicit to readers.

---

82-82: Approve changes with suggestion for consistency

The addition of the false parameter is correct. For consistency with the previous suggestion and to improve code clarity, consider using a named parameter:

isCrossChainCall: false,

This change would maintain a uniform style across all test cases.

---

116-116: Approve changes with suggestion for consistency

The addition of the false parameter is correct. To maintain consistency with previous suggestions and enhance code readability, consider using a named parameter:

isCrossChainCall: false,

This change would ensure a uniform style across all test cases and improve overall code clarity.

---

159-159: Approve changes with suggestion for consistency

The addition of the false parameter is correct. To maintain consistency with previous suggestions and improve code clarity, consider using a named parameter:

isCrossChainCall: false,

This change would ensure a uniform style across all test cases and enhance overall readability.

---

198-198: Approve changes with suggestion for consistency

The addition of the false parameter is correct. To maintain consistency with previous suggestions and enhance code clarity, consider using a named parameter:

isCrossChainCall: false,

This change would ensure a uniform style across all test cases and improve overall readability of the test suite.

---

241-241: Approve changes with suggestion for consistency

The addition of the false parameter is correct. To maintain consistency with previous suggestions and improve code clarity, consider using a named parameter:

isCrossChainCall: false,

This change would ensure a uniform style across all test cases and enhance the overall readability of the test suite.

---

269-269: Approve changes with suggestion for consistency

The addition of the false parameter is correct. To maintain consistency with previous suggestions and enhance code clarity, consider using a named parameter:

isCrossChainCall: false,

This change would ensure a uniform style across all test cases and improve the overall readability of the test suite.

testutil/keeper/crosschain.go (2)

Line range hint 393-401: Approve addition with minor enhancement suggestion.

The MockRevertForHandleEVMDeposit function effectively simulates a failed deposit operation. Consider adding a comment explaining the purpose of this mock function for improved code documentation.

+// MockRevertForHandleEVMDeposit simulates a failed ZRC20 deposit operation for testing purposes
 func MockRevertForHandleEVMDeposit(
     m *crosschainmocks.CrosschainFungibleKeeper,
     receiver ethcommon.Address,
     amount *big.Int,
     senderChainID int64,
     errDeposit error,
 ) {

---

Line range hint 413-421: Approve changes to MockVoteOnOutboundFailedBallot with refactoring suggestion.

The modifications to MockVoteOnOutboundFailedBallot mirror those made to MockVoteOnOutboundSuccessBallot, enhancing testing flexibility. Consider refactoring these two functions to reduce code duplication, as they share similar structures and differ primarily in the ReceiveStatus parameter.

Example refactoring:

func mockVoteOnOutboundBallot(
    m *crosschainmocks.CrosschainObserverKeeper,
    ctx sdk.Context,
    cctx *types.CrossChainTx,
    senderChain chains.Chain,
    observer string,
    status chains.ReceiveStatus,
    ballotStatus observertypes.BallotStatus,
) {
    m.On("VoteOnOutboundBallot", ctx, mock.Anything, cctx.GetCurrentOutboundParam().ReceiverChainId, status, observer).
        Return(true, true, observertypes.Ballot{BallotStatus: ballotStatus}, senderChain.Name, nil).
        Once()
}

// Then use it like this:
func MockVoteOnOutboundSuccessBallot(...) {
    mockVoteOnOutboundBallot(..., chains.ReceiveStatus_success, observertypes.BallotStatus_BallotFinalized_SuccessObservation)
}

func MockVoteOnOutboundFailedBallot(...) {
    mockVoteOnOutboundBallot(..., chains.ReceiveStatus_failed, observertypes.BallotStatus_BallotFinalized_FailureObservation)
}

pkg/contracts/testdappv2/TestDAppV2.json (3)

3-15: Approve addition of NO_MESSAGE_CALL function

The new NO_MESSAGE_CALL function aligns with the PR objective of supporting depositAndCall and withdrawAndCall with empty messages. Its view state mutability ensures it doesn't modify the contract state.

Consider adding a NatSpec comment to document the purpose and return value of this function for improved clarity and maintainability.

---

128-146: Approve addition of getNoMessageIndex function

The new getNoMessageIndex function complements the NO_MESSAGE_CALL function in handling empty message scenarios. Its pure state mutability ensures it doesn't read or modify the contract state.

Consider adding a NatSpec comment to document the purpose, parameter, and return value of this function for improved clarity and maintainability.

---

Implement Comprehensive Tests for New Functions

The newly added NO_MESSAGE_CALL and getNoMessageIndex functions lack corresponding test cases. To ensure their correct integration and functionality, it is essential to develop dedicated tests covering various scenarios, including edge cases and interactions with existing functions like depositAndCall and withdrawAndCall.

• Develop test cases for NO_MESSAGE_CALL and getNoMessageIndex.
• Ensure integration tests validate their behavior within depositAndCall and withdrawAndCall operations.
• Cover edge cases such as empty messages and incorrect inputs to verify robustness.

🔗 Analysis chain

Line range hint 1-295: Approve contract modifications and suggest comprehensive testing

The additions of NO_MESSAGE_CALL and getNoMessageIndex functions enhance the contract's capability to handle empty message scenarios, aligning with the PR objectives. These view and pure functions maintain the contract's overall behavior while extending its functionality.

Ensure comprehensive testing of the new functions and their integration with existing functionality, particularly in scenarios involving depositAndCall and withdrawAndCall with empty messages. Consider the following verification steps:

1. Test NO_MESSAGE_CALL in various contexts.
2. Verify getNoMessageIndex with different sender addresses.
3. Integrate these functions with depositAndCall and withdrawAndCall operations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the integration of new functions with existing functionality

# Search for test files related to depositAndCall and withdrawAndCall
echo "Searching for relevant test files:"
fd -e js -e ts -e sol --exec grep -Hn 'depositAndCall|withdrawAndCall|NO_MESSAGE_CALL|getNoMessageIndex' {}

# Look for any TODOs or FIXMEs related to empty message handling
echo "Checking for TODOs or FIXMEs related to empty message handling:"
rg -i 'TODO|FIXME' -A 3 -B 3 | rg -i 'empty.?message|no.?message'

Length of output: 395

---

Script:

#!/bin/bash
# Broadening the search for test cases related to new functions

# Search for any test files that mention the new functions
echo "Searching for test cases covering NO_MESSAGE_CALL and getNoMessageIndex:"
fd -e js -e ts -e sol -g '*test*.js' --exec grep -Hn 'NO_MESSAGE_CALL\|getNoMessageIndex' {} \;

# Search for tests related to depositAndCall and withdrawAndCall with empty messages
echo "Searching for tests related to depositAndCall and withdrawAndCall handling empty messages:"
fd -e js -e ts -e sol -g '*test*.js' --exec grep -i -Hn 'depositAndCall.*empty message\|withdrawAndCall.*empty message' {} \;

# Verify if there are any generic tests handling empty messages
echo "Searching for generic tests handling empty messages:"
fd -e js -e ts -e sol -g '*test*.js' --exec grep -i -Hn 'empty message\|no message\|handle empty' {} \;

Length of output: 801

---

Script:

#!/bin/bash
# Expanding the search for test cases related to new functions

# Search for test files with various naming conventions and extensions
echo "Searching for test cases covering NO_MESSAGE_CALL and getNoMessageIndex with broader patterns:"
fd -e js -e ts -e sol -e jsx -e tsx -e py -g '*test*' --exec grep -Hn 'NO_MESSAGE_CALL\|getNoMessageIndex' {} \;

# Search for any usage of the new functions within the codebase
echo "Searching for any usage of NO_MESSAGE_CALL and getNoMessageIndex in the codebase:"
rg -i 'NO_MESSAGE_CALL|getNoMessageIndex'

# Use ast-grep to find implementations or references of the new functions
echo "Using ast-grep to locate implementations of NO_MESSAGE_CALL and getNoMessageIndex:"
ast-grep --pattern 'function NO_MESSAGE_CALL($_) { $$$ }' .
ast-grep --pattern 'function getNoMessageIndex($_) { $$$ }' .

Length of output: 9146

go.mod (1)

Assess Maintenance of Forked Dependencies

• github.com/bnb-chain/tss-lib last updated on 2024-01-26
• github.com/ethereum/go-ethereum last updated on 2024-07-18
• github.com/libp2p/go-libp2p last updated on 2024-07-30
• gitlab.com/thorchain/tss/go-tss last updated on 2024-09-19

The tss-lib dependency has not been updated since January 2024, which may lead to discrepancies with upstream changes and potential security vulnerabilities. It is recommended to review the maintenance status of this fork and ensure it aligns with project requirements.

🔗 Analysis chain

Line range hint 392-399: Verify maintenance of forked dependencies.

Several dependencies are being replaced with forks maintained by ZetaChain:

• github.com/bnb-chain/tss-lib
• github.com/ethereum/go-ethereum
• github.com/libp2p/go-libp2p
• gitlab.com/thorchain/tss/go-tss

While using forks allows for custom modifications, it's crucial to ensure these forks are regularly updated to incorporate upstream changes and security patches.

To verify the last update times of these forks, please run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the last commit dates of the ZetaChain forks.

echo "Checking last commit dates of ZetaChain forks:"
for repo in "tss-lib" "go-ethereum" "go-libp2p" "go-tss"; do
    echo "$repo:"
    gh repo view zeta-chain/$repo --json updatedAt --jq .updatedAt
    echo
done

Length of output: 837

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

Line range hint 102-110: LGTM. Consider adding a comment for the new parameter.

The addition of the new mock.Anything parameter aligns with the PR objectives. To enhance code readability, consider adding a comment explaining the purpose of this new parameter.

---

Line range hint 556-564: LGTM. Consider updating the comment for consistency.

The changes to this test case are consistent with the previous modifications. For improved consistency, consider updating the comment above the ZRC20DepositAndCallContract mock function call to reflect the new parameter.

testutil/keeper/mocks/crosschain/fungible.go (1)

Line range hint 700-734: Approve changes with a minor suggestion for improved readability.

The implementation of ZRC20DepositAndCallContract correctly incorporates the new isCrossChainCall parameter, aligning with the PR objectives. The changes are well-structured and consistent with the existing codebase.

To enhance readability, consider breaking down the long parameter list using line breaks and alignment:

func (_m *CrosschainFungibleKeeper) ZRC20DepositAndCallContract(
    ctx types.Context,
    from []byte,
    to common.Address,
    amount *big.Int,
    senderChainID int64,
    data []byte,
    coinType coin.CoinType,
    asset string,
    protocolContractVersion crosschaintypes.ProtocolContractVersion,
    isCrossChainCall bool,
) (*evmtypes.MsgEthereumTxResponse, bool, error) {
    // ... rest of the implementation
}

This formatting improves code readability without changing functionality.

e2e/e2etests/e2etests.go (1)

786-793: Consistent addition of new E2ETests for V2 withdrawals with no message.

The new E2ETests for V2 ETH and ERC20 withdrawal and call with no message content are well-structured and maintain consistency with existing tests. The descriptions accurately reflect their purposes.

However, for improved consistency:

Consider adding a default amount for the ERC20 withdrawal test, similar to the ETH withdrawal test:

 runner.NewE2ETest(
 	TestV2ERC20WithdrawAndCallNoMessageName,
 	"withdraw ERC20 from ZEVM and authenticated call a contract using V2 contract with no message",
-	[]runner.ArgDefinition{},
+	[]runner.ArgDefinition{
+		{Description: "amount", DefaultValue: "1000"},
+	},
 	TestV2ERC20WithdrawAndCallNoMessage,
 ),

Also applies to: 880-885

testutil/sample/crosschain.go (1)

317-318: Approve renaming of GetValidZRC20WithdrawToETH with a minor suggestion.

The renaming of GetValidZRC20WithdrawToETH to ValidZrc20WithdrawToETHReceipt improves code clarity and maintains consistency with other function names. However, there's a minor inconsistency in the capitalization of "Zrc20".

Consider updating the function name to ValidZRC20WithdrawToETHReceipt for consistent capitalization of "ZRC20".

changelog.md (1)

57-57: Consider renaming event names for consistency

The event names DepositedAndCalled and WithdrawedAndCalled are grammatically inconsistent. Consider renaming WithdrawedAndCalled to WithdrawnAndCalled for better grammatical consistency and clarity.

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

31-31: Clarify the initialization of Origin to an empty byte slice

Setting context.Origin to []byte{} may benefit from an explanatory comment. If this is intentional to reflect a specific context where the origin is undefined or irrelevant, documenting the rationale will enhance code readability and assist future developers.

x/crosschain/types/inbound_parsing.go (2)

104-107: Nitpick: Document the parameters and error cases

The inbound message constructor functions lack detailed documentation for their parameters and potential error cases. Providing comprehensive comments improves code readability and maintainability.

Enhance the function comments to include descriptions of each parameter and the conditions under which errors are returned. For example:

// NewWithdrawalInbound creates a new inbound message for a withdrawal.
//
// Parameters:
// - ctx: The context of the operation.
// - txOrigin: The original transaction sender.
// - foreignCoin: The foreign coin details.
// - event: The parsed withdrawal event.
// - receiverChain: The chain where the funds are received.
// - gasLimitQueried: The queried gas limit if not specified in the event.
//
// Returns:
// - A pointer to MsgVoteInbound if successful.
// - An error if any parameter is invalid or an operation fails.

Also applies to: 159-162, 207-210

---

3-18: Nitpick: Organize imports for clarity

The import statements can be organized into standard, third-party, and local packages to improve readability.

Group the imports as follows:

import (
	"encoding/hex"
	"math/big"

	sdkmath "cosmossdk.io/math"
	sdk "github.com/cosmos/cosmos-sdk/types"
	"github.com/pkg/errors"

	"github.com/ethereum/go-ethereum/accounts/abi/bind"
	ethcommon "github.com/ethereum/go-ethereum/common"
	ethtypes "github.com/ethereum/go-ethereum/core/types"

	"github.com/zeta-chain/node/pkg/chains"
	"github.com/zeta-chain/node/pkg/coin"
	fungibletypes "github.com/zeta-chain/node/x/fungible/types"
	"github.com/zeta-chain/protocol-contracts/v2/pkg/gatewayzevm.sol"
)

pkg/contracts/testdappv2/TestDAppV2.go (1)

225-240: Consider handling potential errors more gracefully

In the implementations of NOMESSAGECALL and GetNoMessageIndex, any errors returned from the contract call are directly returned. Evaluate the necessity of additional error handling or custom error messages to provide clearer insights during failures.

Also applies to: 380-395

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 228-234: pkg/contracts/testdappv2/TestDAppV2.go#L228-L234
Added lines #L228 - L234 were not covered by tests

---

[warning] 236-238: pkg/contracts/testdappv2/TestDAppV2.go#L236-L238
Added lines #L236 - L238 were not covered by tests

Configuration used: .coderabbit.yaml
Review profile: CHILL