feat: authenticated zevm -> evm call and e2e tests #2904
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe pull request introduces significant enhancements to the testing framework for the V2 smart contract functionality within the application. It adds multiple new test cases focused on authenticated calls for both ETH and ZEVM transactions, expands the capabilities of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
skosito
changed the title
| @@ -50,6 +51,56 @@ func (r *E2ERunner) V2ETHWithdrawAndCall( | |||
| return tx | |||
| } | |||
|
|
|||
| // V2ETHWithdrawAndCall calls WithdrawAndCall of Gateway with gas token on ZEVM using authenticated call | |||
| func (r *E2ERunner) V2ETHWithdrawAndAuthenticatedCall( | |||
There was a problem hiding this comment.
i added this method, but i dont see support for withdraw and call with wzeta:
function withdrawAndCall(
bytes memory receiver,
uint256 amount,
uint256 chainId,
bytes calldata message,
CallOptions calldata callOptions,
RevertOptions calldata revertOptions
)
external;
is only test missing for this?
| @@ -60,7 +60,7 @@ require ( | |||
| github.com/stretchr/testify v1.9.0 | |||
| github.com/zeta-chain/ethermint v0.0.0-20240909234716-2fad916e7179 | |||
| github.com/zeta-chain/keystone/keys v0.0.0-20231105174229-903bc9405da2 | |||
| github.com/zeta-chain/protocol-contracts v1.0.2-athens3.0.20240819143729-b8229cd7b410 | |||
| github.com/zeta-chain/protocol-contracts v1.0.2-athens3.0.20240920151810-cabe34920d52 | |||
There was a problem hiding this comment.
will have to wait for that PR to be merged before merging this one
| @@ -197,7 +196,7 @@ func (k Keeper) newWithdrawalInbound( | |||
|
|
|||
| return types.NewMsgVoteInbound( | |||
| "", | |||
| from.Hex(), | |||
| event.Sender.Hex(), | |||
There was a problem hiding this comment.
not sure why sender was not used from event here, is it intentional?
There was a problem hiding this comment.
Looks to make more sense using the event
skosito
changed the title
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
Outside diff range and nitpick comments (14)
zetaclient/chains/evm/observer/v2_inbound.go (2)
194-194: Consider using a more descriptive name for the boolean parameter.The newly added boolean parameter is named
false, which doesn't convey its purpose. Consider renaming it to something more descriptive, such asisArbitraryCall, to improve code readability and maintainability.
329-329: Consider using a more descriptive name for the boolean parameter.Similar to the previous comment, the newly added boolean parameter is named
false, which doesn't convey its purpose. Consider renaming it to something more descriptive, such asisArbitraryCall, to improve code readability and maintainability.x/crosschain/keeper/evm_hooks.go (2)
209-209: LGTM! Consider adding a comment to explain the new parameter.The additional boolean parameter
truedoes not affect the current functionality but may be used for future enhancements or clarity in the function's intent. To improve code readability and maintainability, consider adding a comment to explain the purpose of the new parameter.msg := types.NewMsgVoteInbound( "", emittingContract.Hex(), senderChain.ChainId, txOrigin, toAddr, foreignCoin.ForeignChainId, math.NewUintFromBigInt(event.Value), "", event.Raw.TxHash.String(), event.Raw.BlockNumber, gasLimit.Uint64(), foreignCoin.CoinType, foreignCoin.Asset, event.Raw.Index, types.ProtocolContractVersion_V1, + true, // TODO: Add a comment to explain the purpose of this parameter )
290-290: LGTM! Consider adding a comment to explain the new parameter.The additional boolean parameter
truedoes not affect the current functionality but may be used for future enhancements or clarity in the function's intent. To improve code readability and maintainability, consider adding a comment to explain the purpose of the new parameter.msg := types.NewMsgVoteInbound( "", emittingContract.Hex(), senderChain.ChainId, txOrigin, toAddr, receiverChain.ChainId, amount, messageString, event.Raw.TxHash.String(), event.Raw.BlockNumber, 90000, coin.CoinType_Zeta, "", event.Raw.Index, types.ProtocolContractVersion_V1, + true, // TODO: Add a comment to explain the purpose of this parameter )e2e/e2etests/test_v2_zevm_to_evm_authenticated_call_through_contract.go (2)
15-15: Consider shortening the constant name for readabilityThe constant
payloadMessageEVMAuthenticatedCallThroughContractis quite long, which may affect readability. Consider using a shorter, yet descriptive name.For example:
-const payloadMessageEVMAuthenticatedCallThroughContract = "this is a test EVM authenticated call payload through contract" +const testPayloadMessage = "this is a test EVM authenticated call payload through contract"And update its usage accordingly throughout the code.
32-32: Define transfer amount as a constant for clarityThe transfer amount
100000000000000000is hard to read and understand. Defining it as a constant with a descriptive name would improve code readability and maintainability.For example, define the constant at the top of the file:
const transferAmount = big.NewInt(100000000000000000) // 0.1 ETH in weiThen update the code:
-tx, err = r.ETHZRC20.Transfer(r.ZEVMAuth, gatewayCallerAddr, big.NewInt(100000000000000000)) +tx, err = r.ETHZRC20.Transfer(r.ZEVMAuth, gatewayCallerAddr, transferAmount)e2e/e2etests/test_v2_eth_withdraw_and_authenticated_call_through_contract.go (3)
15-15: Consider simplifying the constant name for readability.The constant
payloadMessageAuthenticatedWithdrawETHThroughContractis quite lengthy, which may affect code readability. Shortening it can make the code easier to read and maintain without losing clarity.Consider renaming it to something more concise:
-const payloadMessageAuthenticatedWithdrawETHThroughContract = "this is a test ETH withdraw and authenticated call payload through contract" +const payloadMsgAuthWithdrawETHThroughContract = "this is a test ETH withdraw and authenticated call payload through contract"
39-39: Define constants for large numerical values to enhance clarity.The literal
100000000000000000is a large numeric value that can be hard to read and prone to errors. Defining it as a constant with a descriptive name improves readability and maintainability.Consider adding a constant:
const transferAmount = 100_000_000_000_000_000 // 0.1 ETH in weiAnd update the code:
- tx, err = r.ETHZRC20.Transfer(r.ZEVMAuth, gatewayCallerAddr, big.NewInt(100000000000000000)) + tx, err = r.ETHZRC20.Transfer(r.ZEVMAuth, gatewayCallerAddr, big.NewInt(transferAmount))
62-68: Simplify the retrieval ofsenderForMsgby handling the error inline.For brevity and improved readability, you can handle the error inline when calling
SenderWithMessage, reducing the number of lines and keeping related code together.Apply this diff to streamline the code:
-senderForMsg, err := r.TestDAppV2EVM.SenderWithMessage( - &bind.CallOpts{}, - []byte(payloadMessageAuthenticatedWithdrawETHThroughContract), -) +senderForMsg, err := r.TestDAppV2EVM.SenderWithMessage(&bind.CallOpts{}, []byte(payloadMessageAuthenticatedWithdrawETHThroughContract)) require.NoError(r, err)e2e/runner/v2_zevm.go (4)
55-56: Update function comment to match method nameThe comment for
V2ETHWithdrawAndAuthenticatedCallmentionsV2ETHWithdrawAndCall, which is inconsistent with the method name. Please update the comment to accurately reflect the method name for clarity.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 authenticated call
79-81: Align function comment with method nameThe comment for
V2ETHWithdrawAndAuthenticatedCallThroughContractrefers toV2ETHWithdrawAndCall, which does not match the method name. Please update the comment to reflect the correct method name to avoid confusion.Apply this diff to correct the comment:
-// V2ETHWithdrawAndCall calls WithdrawAndCall of Gateway with gas token on ZEVM using authenticated call +// V2ETHWithdrawAndAuthenticatedCallThroughContract calls WithdrawAndCall of Gateway with gas token on ZEVM using authenticated call // through contract
158-159: Update function comment to reflect method usageThe comment for
V2ZEVMToEMVCallmentions that it callsCallof Gateway on ZEVM, but the method now usesCall0. Please update the comment to accurately describe the method being called.Apply this diff to correct the comment:
-// V2ZEVMToEMVCall calls Call of Gateway on ZEVM +// V2ZEVMToEMVCall calls Call0 of Gateway on ZEVM
10-11: Organize imports for clarityTo improve readability, consider grouping and ordering the import statements according to Go conventions: standard library packages first, then third-party packages, and finally local packages.
Apply this diff to organize the imports:
package runner +import ( + "math/big" + + ethcommon "github.com/ethereum/go-ethereum/common" + ethtypes "github.com/ethereum/go-ethereum/core/types" + "github.com/stretchr/testify/require" +) + +import ( + gatewayzevm "github.com/zeta-chain/protocol-contracts/v2/pkg/gatewayzevm.sol" + testgatewayzevmcaller "github.com/zeta-chain/node/pkg/contracts/testgatewayzevmcaller" +)e2e/e2etests/test_v2_eth_withdraw_and_authenticated_call.go (1)
17-60: Enhance Test Readability with Inline CommentsAdding explanatory comments at key steps within the
TestV2ETHWithdrawAndAuthenticatedCallfunction can improve clarity and assist future maintainers in understanding the test logic.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumpkg/contracts/testdappv2/TestDAppV2.binis excluded by!**/*.binpkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.binis excluded by!**/*.binx/crosschain/types/cross_chain_tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/crosschain/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (30)
- cmd/zetae2e/local/v2.go (1 hunks)
- e2e/e2etests/e2etests.go (3 hunks)
- e2e/e2etests/test_v2_eth_withdraw_and_authenticated_call.go (1 hunks)
- e2e/e2etests/test_v2_eth_withdraw_and_authenticated_call_through_contract.go (1 hunks)
- e2e/e2etests/test_v2_zevm_to_evm_authenticated_call.go (1 hunks)
- e2e/e2etests/test_v2_zevm_to_evm_authenticated_call_through_contract.go (1 hunks)
- e2e/e2etests/test_v2_zevm_to_evm_call.go (1 hunks)
- e2e/runner/v2_zevm.go (4 hunks)
- go.mod (2 hunks)
- pkg/contracts/testdappv2/TestDAppV2.abi (3 hunks)
- pkg/contracts/testdappv2/TestDAppV2.go (6 hunks)
- pkg/contracts/testdappv2/TestDAppV2.json (4 hunks)
- pkg/contracts/testdappv2/TestDAppV2.sol (2 hunks)
- pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.abi (1 hunks)
- pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.go (1 hunks)
- pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.json (1 hunks)
- pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.sol (1 hunks)
- pkg/contracts/testgatewayzevmcaller/bindings.go (1 hunks)
- proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1 hunks)
- proto/zetachain/zetacore/crosschain/tx.proto (1 hunks)
- x/crosschain/client/cli/tx_vote_inbound.go (1 hunks)
- x/crosschain/keeper/evm_hooks.go (2 hunks)
- x/crosschain/keeper/v2_zevm_inbound.go (7 hunks)
- x/crosschain/types/cctx.go (1 hunks)
- x/crosschain/types/message_vote_inbound.go (2 hunks)
- x/crosschain/types/message_vote_inbound_test.go (11 hunks)
- zetaclient/chains/evm/observer/v2_inbound.go (2 hunks)
- zetaclient/chains/evm/signer/v2_sign.go (2 hunks)
- zetaclient/chains/evm/signer/v2_signer.go (1 hunks)
- zetaclient/zetacore/tx.go (1 hunks)
Additional context used
Path-based instructions (23)
cmd/zetae2e/local/v2.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/e2etests.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_eth_withdraw_and_authenticated_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_eth_withdraw_and_authenticated_call_through_contract.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_zevm_to_evm_authenticated_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_zevm_to_evm_authenticated_call_through_contract.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_v2_zevm_to_evm_call.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/runner/v2_zevm.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/contracts/testdappv2/TestDAppV2.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/contracts/testgatewayzevmcaller/bindings.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.proto/zetachain/zetacore/crosschain/tx.proto (1)
Pattern
**/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/crosschain/client/cli/tx_vote_inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/evm_hooks.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/keeper/v2_zevm_inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/cctx.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/message_vote_inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/crosschain/types/message_vote_inbound_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/observer/v2_inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/v2_sign.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/evm/signer/v2_signer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/zetacore/tx.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (49)
pkg/contracts/testgatewayzevmcaller/bindings.go (1)
6-8: LGTM!The package declaration and the empty variable declaration are correct and follow the Go best practices. The empty variable declaration ensures that the package is imported and the generated bindings are included in the final binary.
e2e/e2etests/test_v2_zevm_to_evm_call.go (1)
24-24: LGTM!The change in comment from "perform the withdraw" to "perform the call" improves the clarity and consistency of the code by aligning the comment with the actual operation being performed in the
V2ZEVMToEMVCallfunction.zetaclient/chains/evm/signer/v2_signer.go (1)
30-30: Approve the change and verify the impact on the destination contract.The change correctly passes the sender's address from the inbound parameters to the
signGatewayExecutefunction, enabling authenticated calls for theOutboundTypeGasWithdrawAndCallandOutboundTypeCallcases. This aligns with the PR objective of implementing authenticated calls between zevm and evm.Please ensure that the corresponding changes have been made in the destination contract to handle the additional sender information for verification purposes.
Run the following script to verify the usage of the sender's address in the destination contract:
cmd/zetae2e/local/v2.go (1)
23-27: LGTM!The addition of these test cases significantly enhances the coverage of the V2 tests by including scenarios that involve authenticated calls for both ETH and ZEVM transactions. The test names are descriptive and follow a consistent naming convention, making the test suite more comprehensive and maintainable.
These tests will help ensure the reliability and correctness of the V2 smart contract functionality, improving the overall quality of the application.
proto/zetachain/zetacore/crosschain/cross_chain_tx.proto (1)
82-82: The new field addition looks good.The boolean field
is_arbitrary_callwith tag number 24 is a valid and backwards-compatible addition to theOutboundParamsmessage. It can be used by new clients to indicate if the outbound call is arbitrary while existing clients will simply ignore it.proto/zetachain/zetacore/crosschain/tx.proto (1)
178-179: The new fieldis_arbitrary_callis a valid addition to theMsgVoteInboundmessage.The boolean field is correctly defined with the appropriate syntax and tag number. It appears to be used to indicate the nature of the call, providing more flexibility to the message structure. The field addition maintains compatibility with the existing message definition.
x/crosschain/keeper/v2_zevm_inbound.go (2)
Line range hint
71-213: LGTM!The changes to the
newWithdrawalInboundfunction improve the correctness and clarity of the code:
- The sender's address is now correctly derived from the
eventparameter, addressing the past review comment.- The gas limit retrieval has been updated to use a more appropriate source.
- The inclusion of
event.CallOptions.IsArbitraryCallenhances the information provided by the function.Note: The function signature change is a breaking change and may require updates to existing code that relies on the old signature.
Line range hint
247-275: LGTM!The changes to the
newCallInboundfunction improve the correctness and clarity of the code:
- The sender's address is now correctly derived from the
eventparameter.- The gas limit retrieval has been updated to use a more appropriate source.
- The inclusion of
event.CallOptions.IsArbitraryCallenhances the information provided by the function.Note: The function signature change is a breaking change and may require updates to existing code that relies on the old signature.
x/crosschain/types/cctx.go (1)
244-244: LGTM!The addition of the
IsArbitraryCallfield to theoutboundParamsstruct is a valid change that captures relevant information for the cross-chain transaction context. The field is correctly set to the value ofmsg.IsArbitraryCall.x/crosschain/types/message_vote_inbound_test.go (7)
Line range hint
8-186: LGTM!The test function
TestNewMsgVoteInboundis well-structured and covers the important scenarios for testing theNewMsgVoteInboundfunction. The sub-tests are logically separated and test the behavior of theRevertOptionsfor different scenarios.
Line range hint
188-314: LGTM!The test function
TestMsgVoteInbound_ValidateBasicis well-structured and covers the important scenarios for testing theValidateBasicmethod of theMsgVoteInboundstruct. The table-driven test is a good approach for testing multiple scenarios in a concise manner.
Line range hint
316-424: LGTM!The test function
TestMsgVoteInbound_Digestis well-structured and covers all the important fields of theMsgVoteInboundstruct. The test function ensures that theDigestmethod behaves as expected when different fields of the struct are modified.
Line range hint
426-462: LGTM!The test function
TestMsgVoteInbound_GetSignersis well-structured and covers the important scenarios for testing theGetSignersmethod of theMsgVoteInboundstruct. The table-driven test is a good approach for testing multiple scenarios in a concise manner.
Line range hint
464-471: LGTM!The test function
TestMsgVoteInbound_Typeis simple and straightforward. It ensures that theTypemethod of theMsgVoteInboundstruct returns the expected result.
Line range hint
473-480: LGTM!The test function
TestMsgVoteInbound_Routeis simple and straightforward. It ensures that theRoutemethod of theMsgVoteInboundstruct returns the expected result.
Line range hint
482-489: LGTM!The test function
TestMsgVoteInbound_GetSignBytesis simple and straightforward. It ensures that theGetSignBytesmethod of theMsgVoteInboundstruct does not panic.pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.json (4)
3-18: Constructor looks good.The constructor correctly sets the
GatewayZEVMandWZETAaddresses as state variables.
91-97:depositWZETAfunction looks good.The function correctly forwards the received Ether to the
depositfunction of theWZETAtoken contract.
98-173: Avoid hardcoding the token approval amount.As mentioned in the comment on
callGatewayZEVM, approving a large hardcoded amount of tokens to theGatewayZEVMis risky.Please calculate the exact amount being withdrawn and approve only that before calling
withdrawAndCallZEVM.
175-250: Avoid hardcoding the token approval amount.This overload also has the issue of approving a hardcoded amount of tokens to the
GatewayZEVM.As suggested for the other functions, please dynamically calculate and approve the exact amount being withdrawn.
pkg/contracts/testdappv2/TestDAppV2.json (5)
64-76: LGTM!The
expectedOnCallSenderfunction is a simple getter that returns an address. The implementation is correct and follows the Solidity conventions.
128-158: LGTM!The
onCallfunction is implemented correctly and follows the Solidity conventions. The introduction of theMessageContextstruct is a good design choice that enhances the contract's message handling capabilities.
234-252: LGTM!The
senderWithMessagefunction is a simple getter that returns an address associated with a message. The implementation is correct and follows the Solidity conventions.
253-265: LGTM!The
setExpectedOnCallSenderfunction is a simple setter that updates theexpectedOnCallSenderaddress. The implementation is correct and follows the Solidity conventions.
131-137: LGTM!The introduction of the
MessageContextstruct is a good design choice that enhances the contract's message handling capabilities. The struct definition is correct and follows the Solidity conventions.go.mod (2)
63-63: Ensure the corresponding smart contract changes are finalized before merging.The version update to
github.com/zeta-chain/protocol-contractsis acknowledged. However, as mentioned in the previous review comment, please ensure that the corresponding smart contract changes have been finalized before merging this PR to maintain consistency between the node and the contracts.
338-341: Provide more context on the new dependencies and ensure they are safe to use.Two new dependencies,
github.com/showa-93/go-maskandgithub.com/tonkeeper/tongo, have been added to the project. Could you please provide more information on the purpose and necessity of these packages?Additionally, to mitigate potential security risks, I recommend running a dependency vulnerability scan using tools like
go list -json -m all | nancy sleuthto ensure that the new dependencies do not introduce any known vulnerabilities.e2e/e2etests/e2etests.go (6)
135-135: LGTM!The test name
TestV2ETHWithdrawAndAuthenticatedCallNameis descriptive and follows the established naming convention. It clearly conveys the purpose of testing Ether withdrawal from ZEVM with an authenticated call using the V2 contract.
136-136: LGTM!The test name
TestV2ETHWithdrawAndAuthenticatedCallThroughContractNameis descriptive and follows the established naming convention. It clearly conveys the purpose of testing Ether withdrawal from ZEVM with an authenticated call through an intermediary contract using the V2 contract.
148-148: LGTM!The test name
TestV2ZEVMToEVMAuthenticatedCallNameis descriptive and follows the established naming convention. It clearly conveys the purpose of testing an authenticated call from ZEVM to EVM using the V2 contract.
149-149: LGTM!The test name
TestV2ZEVMToEVMAuthenticatedCallThroughContractNameis descriptive and follows the established naming convention. It clearly conveys the purpose of testing an authenticated call from ZEVM to EVM through an intermediary contract using the V2 contract.
745-760: LGTM!The new test runner definitions for the authenticated call tests are correctly added to the
AllE2ETestsarray under the "V2 smart contract tests" section. They follow the established structure, providing the test name, description, argument definitions, and test function.
847-858: LGTM!The new test runner definitions for the authenticated call tests are correctly added to the
AllE2ETestsarray under the "V2 smart contract tests" section. They follow the established structure, providing the test name, description, argument definitions, and test function.e2e/e2etests/test_v2_zevm_to_evm_authenticated_call.go (1)
8-8: Verify the import path forgatewayzevm.sol.The import statement includes a
.solfile extension, which is unconventional in Go imports. Please ensure that the import path is correct and that the package is accessible. This might cause issues during compilation.e2e/e2etests/test_v2_zevm_to_evm_authenticated_call_through_contract.go (1)
17-83: Code implementation is well-structured and follows best practicesThe test function is well-organized with clear steps and proper error handling. The use of descriptive variable names and comments enhances readability. The code adheres to clean code principles and effectively tests the authenticated call functionality.
pkg/contracts/testdappv2/TestDAppV2.sol (2)
25-30: Definition ofMessageContextStruct is AppropriateThe
MessageContextstruct is properly defined with a clear documentation of its purpose and parameters. This enhances the clarity and maintainability of the code.
31-31: Declaration ofexpectedOnCallSenderThe addition of the public address
expectedOnCallSenderis correctly implemented, allowing the contract to store the expected sender's address for authentication purposes.e2e/runner/v2_zevm.go (3)
88-100: Ensure correct usage ofWithdrawAndCallGatewayZEVMmethodIn
V2ETHWithdrawAndAuthenticatedCallThroughContract, you are callinggatewayZEVMCaller.WithdrawAndCallGatewayZEVM. Please verify that this method exists in theTestGatewayZEVMCallercontract and that it behaves as expected.To verify the method's existence and correctness, run the following script:
#!/bin/bash # Description: Verify that WithdrawAndCallGatewayZEVM is defined in TestGatewayZEVMCaller.sol. # Test: Search for the definition of WithdrawAndCallGatewayZEVM. # Expect: The method WithdrawAndCallGatewayZEVM should be defined. ast-grep --lang solidity --pattern $'function WithdrawAndCallGatewayZEVM($_) public'
Line range hint
158-165: Verify the change fromCalltoCall0The method
V2ZEVMToEMVCallnow usesr.GatewayZEVM.Call0instead ofr.GatewayZEVM.Call. Please confirm thatCall0is the correct method to use and that this change aligns with the intended functionality.To ensure that
Call0is appropriate, run the following script:#!/bin/bash # Description: Verify the definition and usage of Call0 in GatewayZEVM. # Test: Search for the definition of Call0. # Expect: The method Call0 should be defined. ast-grep --lang solidity --pattern $'function Call0($_) public' # Test: Check for any references to Call0 in the codebase to understand its usage. rg --type solidity 'Call0'
62-74: Confirm the use ofWithdrawAndCall2methodIn
V2ETHWithdrawAndAuthenticatedCall, you are usingWithdrawAndCall2. Please ensure that this is the intended method and that it is correctly implemented in theGatewayZEVMcontract. Verify that all necessary changes have been made to support this new method.To confirm the availability and correctness of
WithdrawAndCall2, run the following script:zetaclient/chains/evm/signer/v2_sign.go (1)
20-24: Ensure all callers ofsignGatewayExecuteinclude the newsenderparameterThe function signature of
signGatewayExecutehas been updated to include a new parametersender string. Please verify that all calls to this function have been updated accordingly to include thesenderargument, ensuring consistency and preventing potential runtime errors.Run the following script to locate all usages of
signGatewayExecuteand check their parameters:Verification successful
Verification successful:
signGatewayExecuteusage is correctThe call to
signGatewayExecuteinzetaclient/chains/evm/signer/v2_signer.gohas been correctly updated to include the newsenderparameter. The function is called withcctx.InboundParams.Senderas the argument for thesenderparameter, which appears to be the appropriate source for this information.No further action is required regarding this change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'signGatewayExecute' to ensure the new 'sender' parameter is included. # Search for function calls to 'signGatewayExecute(' and display surrounding lines for context. rg --type go -A 2 -B 2 'signGatewayExecute\('Length of output: 901
pkg/contracts/testdappv2/TestDAppV2.abi (2)
63-75: Confirm Implementation ofexpectedOnCallSenderFunctionThe addition of the
expectedOnCallSenderview function in the ABI appears correct. Ensure that the corresponding implementation in the Solidity contract correctly returns the expected address and adheres to the intended logic.
127-157: Verify Struct Definition and Usage inonCallFunctionThe
onCallfunction introduces a payable function that accepts aMessageContextstruct and abytesmessage, returning abytesoutput. Ensure that:
- The
MessageContextstruct is properly defined in the contract with thesenderfield of typeaddress.- The function correctly handles the
messageinput and any associated business logic.- Proper validation and security checks are in place to prevent misuse or unexpected behavior.
To confirm the struct definition and function implementation, consider running:
Verification successful
Verification Successful: MessageContext Struct and onCall Function Implemented Correctly
The implementation of the
MessageContextstruct andonCallfunction in theTestDAppV2.solcontract aligns precisely with the ABI definition. Key observations:
- The
MessageContextstruct is correctly defined with a singleaddress senderfield.- The
onCallfunction signature matches the ABI, acceptingMessageContext calldataandbytes calldata messageas inputs.- The function is payable and returns
bytes memory, as specified.Notably, the implementation includes robust security measures:
- Sender authentication:
require(messageContext.sender == expectedOnCallSender, "unauthenticated sender")- State updates based on input:
setCalledWithMessage(string(message))andsetAmountWithMessage(string(message), msg.value)The implementation demonstrates a production-grade approach to handling cross-chain messages securely and efficiently.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of MessageContext struct and its usage in the contract. # Test: Search for the struct definition. Expect: Struct with an address field named 'sender'. rg --type solidity $'struct MessageContext {\\s*address sender;\\s*}' # Test: Search for the 'onCall' function implementation. rg --type solidity 'function onCall\\(MessageContext memory messageContext, bytes memory message\\)'Length of output: 355
Script:
#!/bin/bash # Description: Verify the definition of MessageContext struct and its usage in the contract. # Test: Search for the struct definition with simplified regex rg -C 3 "struct MessageContext" # Test: Search for the 'onCall' function implementation rg -C 5 "function onCall" # Test: Search for usage of MessageContext in function parameters rg -C 3 "MessageContext.*memory"Length of output: 4550
pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.abi (1)
169-172: Verify the duplicate definitions ofwithdrawAndCallGatewayZEVMfunctionThere are two definitions of the
withdrawAndCallGatewayZEVMfunction with different parameters at lines 169-172 and 246-249. Please confirm that this overloading is intentional and correctly handled in the ABI. Overloaded functions can cause issues if the ABI or interfacing tools do not support function overloading properly.To ensure that the ABI accurately represents these overloaded functions and to identify any unintended duplicates, you can run the following script:
Also applies to: 246-249
Verification successful
Function overloading for
withdrawAndCallGatewayZEVMverifiedThe duplicate definitions of
withdrawAndCallGatewayZEVMare confirmed to be intentional function overloads with distinct signatures:
withdrawAndCallGatewayZEVM(bytes,uint256,address,bytes,tuple,tuple)withdrawAndCallGatewayZEVM(bytes,uint256,uint256,bytes,tuple,tuple)These overloads differ in the third parameter (address vs. uint256), indicating a deliberate design choice. While this overloading is valid and correctly represented in the ABI, it is advisable to conduct a brief review of the implementation to ensure both overloads are utilized appropriately and their purposes are well-documented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify functions with duplicate names in the ABI file # Expected Result: List of function names that appear more than once jq -r '.[] | select(.type=="function") | .name' pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.abi | sort | uniq -c | awk '$1 > 1 {print $2 " appears " $1 " times"}'Length of output: 221
Script:
#!/bin/bash # Description: Extract full function signatures for 'withdrawAndCallGatewayZEVM' # Expected Result: Detailed function signatures including parameter types jq -r '.[] | select(.type=="function" and .name=="withdrawAndCallGatewayZEVM") | "\(.name)(\(.inputs | map(.type) | join(",")))"' pkg/contracts/testgatewayzevmcaller/TestGatewayZEVMCaller.abiLength of output: 329
pkg/contracts/testdappv2/TestDAppV2.go (4)
286-316: Implementation ofExpectedOnCallSenderis correctThe function
ExpectedOnCallSenderis correctly implemented and aligns with the corresponding Solidity contract method. The binding handles the return value accurately.
379-409: Implementation ofSenderWithMessageis accurateThe function
SenderWithMessagecorrectly binds to the contract methodsenderWithMessage. It appropriately handles the input parameters and return value.
452-471: Implementation ofOnCallmethod is appropriateThe
OnCallmethod is properly implemented as a payable transaction binding. It correctly passes themessageContextandmessageparameters to the contract.
515-535: Implementation ofSetExpectedOnCallSenderis correctThe method
SetExpectedOnCallSenderaccurately binds to the contract function, correctly handling the transaction and the_expectedOnCallSenderparameter.e2e/e2etests/test_v2_eth_withdraw_and_authenticated_call.go (1)
43-43: Confirm Zero Value forOnRevertGasLimitSetting
OnRevertGasLimitto zero may result in insufficient gas for revert operations. Verify that this is intentional and consider providing a minimal gas limit if necessary.
| //go:generate sh -c "solc TestGatewayZEVMCaller.sol --combined-json abi,bin | jq '.contracts.\"TestGatewayZEVMCaller.sol:TestGatewayZEVMCaller\"' > TestGatewayZEVMCaller.json" | ||
| //go:generate sh -c "cat TestGatewayZEVMCaller.json | jq .abi > TestGatewayZEVMCaller.abi" | ||
| //go:generate sh -c "cat TestGatewayZEVMCaller.json | jq .bin | tr -d '\"' > TestGatewayZEVMCaller.bin" | ||
| //go:generate sh -c "abigen --abi TestGatewayZEVMCaller.abi --bin TestGatewayZEVMCaller.bin --pkg testgatewayzevmcaller --type TestGatewayZEVMCaller --out TestGatewayZEVMCaller.go" |
There was a problem hiding this comment.
Consider using a Go-based build tool and avoiding intermediate files.
The current build process relies on shell commands and external dependencies (solc, jq), which can make it less portable and harder to maintain. Additionally, the generated intermediate files (TestGatewayZEVMCaller.json, TestGatewayZEVMCaller.abi, TestGatewayZEVMCaller.bin) can clutter the project directory.
Consider using a Go-based build tool (e.g., go-ethereum's bind package) to generate the bindings instead of shell commands. This can make the build process more reliable and easier to reproduce across different environments.
Also, consider avoiding generating intermediate files or cleaning them up after the build process to keep the project directory clean and manageable.
| @@ -83,6 +83,7 @@ func CmdVoteInbound() *cobra.Command { | |||
| argsAsset, | |||
| uint(argsEventIndex), | |||
| protocolContractVersion, | |||
| true, // TODO: do we need to provide this as arg? | |||
There was a problem hiding this comment.
Address the TODO comment and consider making the boolean argument configurable.
The new boolean argument added to the NewMsgVoteInbound function call is hardcoded to true and not configurable via CLI arguments. The accompanying TODO comment also questions the necessity of this argument.
Please consider the following suggestions:
- If the boolean argument is not needed, remove it to maintain a clean interface and avoid confusion.
- If the boolean argument is needed, make it configurable via CLI arguments for flexibility. Update the command's
ArgsandUseproperties accordingly.
| @@ -51,6 +51,7 @@ func GetInboundVoteMessage( | |||
| asset, | |||
| eventIndex, | |||
| types.ProtocolContractVersion_V1, | |||
| true, // not relevant for v1 | |||
There was a problem hiding this comment.
Consider removing the new argument or using a default value to maintain backward compatibility.
The new boolean argument added to the GetInboundVoteMessage function is labeled as "not relevant for v1". Adding a new argument to a function signature is a breaking change and could lead to compile-time errors if the function is called without providing the new argument.
If the argument is not relevant for the current version, consider removing it to avoid breaking changes. If the argument is needed for future versions, consider using a default value to maintain backward compatibility.
Additionally, add comments to clarify the purpose and relevance of the new argument for future reference.
| @@ -56,6 +56,7 @@ func NewMsgVoteInbound( | |||
| asset string, | |||
| eventIndex uint, | |||
| protocolContractVersion ProtocolContractVersion, | |||
| isArbitraryCall bool, | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Critical: NewMsgVoteInbound calls missing isArbitraryCall parameter
The verification process has revealed a significant discrepancy in the implementation of the NewMsgVoteInbound function. Despite the addition of the isArbitraryCall parameter to the function signature, none of the existing calls to NewMsgVoteInbound have been updated to include this new parameter. This oversight may lead to unexpected behavior and potential system instability.
To address this issue, the following actions are imperative:
-
Update all calls to
NewMsgVoteInboundacross the codebase to include theisArbitraryCallparameter. This includes, but is not limited to, the following files:- zetaclient/zetacore/tx.go
- zetaclient/chains/evm/observer/v2_inbound.go
- x/crosschain/types/message_vote_inbound_test.go
- x/crosschain/keeper/v2_zevm_inbound.go
- x/crosschain/keeper/evm_hooks.go
- x/crosschain/client/cli/tx_vote_inbound.go
-
Conduct a comprehensive review of the
IsArbitraryCallfield usage across the codebase to ensure consistency and correct implementation. Pay particular attention to:- zetaclient/chains/evm/signer/v2_sign.go
- x/crosschain/types/cctx.go
- x/crosschain/keeper/v2_zevm_inbound.go
- e2e/runner/v2_zevm.go
-
Update all relevant tests to include scenarios that validate the correct handling of the
isArbitraryCallparameter andIsArbitraryCallfield. -
Perform a thorough regression testing to ensure that the introduction of this new parameter does not adversely affect existing functionality.
It is crucial to address these issues promptly to maintain the integrity and reliability of the system. Failure to do so may result in inconsistent behavior and potential vulnerabilities.
Analysis chain
LGTM! Verify the usage of the new isArbitraryCall parameter.
The addition of the isArbitraryCall parameter to the NewMsgVoteInbound function and the corresponding IsArbitraryCall field to the MsgVoteInbound struct is implemented correctly.
However, please ensure that:
- All calls to
NewMsgVoteInboundacross the codebase provide the newisArbitraryCallargument. - The
IsArbitraryCallfield is correctly utilized in all relevant parts of the codebase that handleMsgVoteInboundstructs.
Run the following script to verify the usage of the new parameter and field:
Also applies to: 79-79
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new `isArbitraryCall` parameter and `IsArbitraryCall` field.
# Test 1: Search for calls to `NewMsgVoteInbound`. Expect: All calls provide the new argument.
rg --type go -A 5 $'NewMsgVoteInbound'
# Test 2: Search for usage of the `IsArbitraryCall` field. Expect: Field is utilized correctly.
rg --type go -A 5 $'IsArbitraryCall'
Length of output: 15762
| { | ||
| "inputs": [ | ||
| { | ||
| "internalType": "bytes", | ||
| "name": "receiver", | ||
| "type": "bytes" | ||
| }, | ||
| { | ||
| "internalType": "address", | ||
| "name": "zrc20", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "internalType": "bytes", | ||
| "name": "message", | ||
| "type": "bytes" | ||
| }, | ||
| { | ||
| "components": [ | ||
| { | ||
| "internalType": "uint256", | ||
| "name": "gasLimit", | ||
| "type": "uint256" | ||
| }, | ||
| { | ||
| "internalType": "bool", | ||
| "name": "isArbitraryCall", | ||
| "type": "bool" | ||
| } | ||
| ], | ||
| "internalType": "struct CallOptions", | ||
| "name": "callOptions", | ||
| "type": "tuple" | ||
| }, | ||
| { | ||
| "components": [ | ||
| { | ||
| "internalType": "address", | ||
| "name": "revertAddress", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "internalType": "bool", | ||
| "name": "callOnRevert", | ||
| "type": "bool" | ||
| }, | ||
| { | ||
| "internalType": "address", | ||
| "name": "abortAddress", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "internalType": "bytes", | ||
| "name": "revertMessage", | ||
| "type": "bytes" | ||
| }, | ||
| { | ||
| "internalType": "uint256", | ||
| "name": "onRevertGasLimit", | ||
| "type": "uint256" | ||
| } | ||
| ], | ||
| "internalType": "struct RevertOptions", | ||
| "name": "revertOptions", | ||
| "type": "tuple" | ||
| } | ||
| ], | ||
| "name": "callGatewayZEVM", | ||
| "outputs": [], | ||
| "stateMutability": "nonpayable", | ||
| "type": "function" | ||
| }, |
There was a problem hiding this comment.
Avoid hardcoding the token approval amount.
Approving the GatewayZEVM to spend a large hardcoded amount of tokens is risky. If the GatewayZEVM contract is ever compromised, it could drain the caller's token balance.
Instead, calculate the exact amount of tokens being sent in the current transaction and approve only that amount before calling callZEVM.
| // WithdrawAndCallGatewayZEVM0 is a paid mutator transaction binding the contract method 0xf66f4625. | ||
| // | ||
| // Solidity: function withdrawAndCallGatewayZEVM(bytes receiver, uint256 amount, uint256 chainId, bytes message, (uint256,bool) callOptions, (address,bool,address,bytes,uint256) revertOptions) returns() | ||
| func (_TestGatewayZEVMCaller *TestGatewayZEVMCallerTransactor) WithdrawAndCallGatewayZEVM0(opts *bind.TransactOpts, receiver []byte, amount *big.Int, chainId *big.Int, message []byte, callOptions CallOptions, revertOptions RevertOptions) (*types.Transaction, error) { | ||
| return _TestGatewayZEVMCaller.contract.Transact(opts, "withdrawAndCallGatewayZEVM0", receiver, amount, chainId, message, callOptions, revertOptions) | ||
| } | ||
|
|
||
| // WithdrawAndCallGatewayZEVM0 is a paid mutator transaction binding the contract method 0xf66f4625. | ||
| // | ||
| // Solidity: function withdrawAndCallGatewayZEVM(bytes receiver, uint256 amount, uint256 chainId, bytes message, (uint256,bool) callOptions, (address,bool,address,bytes,uint256) revertOptions) returns() | ||
| func (_TestGatewayZEVMCaller *TestGatewayZEVMCallerSession) WithdrawAndCallGatewayZEVM0(receiver []byte, amount *big.Int, chainId *big.Int, message []byte, callOptions CallOptions, revertOptions RevertOptions) (*types.Transaction, error) { | ||
| return _TestGatewayZEVMCaller.Contract.WithdrawAndCallGatewayZEVM0(&_TestGatewayZEVMCaller.TransactOpts, receiver, amount, chainId, message, callOptions, revertOptions) | ||
| } | ||
|
|
||
| // WithdrawAndCallGatewayZEVM0 is a paid mutator transaction binding the contract method 0xf66f4625. | ||
| // | ||
| // Solidity: function withdrawAndCallGatewayZEVM(bytes receiver, uint256 amount, uint256 chainId, bytes message, (uint256,bool) callOptions, (address,bool,address,bytes,uint256) revertOptions) returns() | ||
| func (_TestGatewayZEVMCaller *TestGatewayZEVMCallerTransactorSession) WithdrawAndCallGatewayZEVM0(receiver []byte, amount *big.Int, chainId *big.Int, message []byte, callOptions CallOptions, revertOptions RevertOptions) (*types.Transaction, error) { | ||
| return _TestGatewayZEVMCaller.Contract.WithdrawAndCallGatewayZEVM0(&_TestGatewayZEVMCaller.TransactOpts, receiver, amount, chainId, message, callOptions, revertOptions) | ||
| } |
There was a problem hiding this comment.
Improve method naming for overloaded functions to enhance code clarity
The methods WithdrawAndCallGatewayZEVM and WithdrawAndCallGatewayZEVM0 are intended to represent overloaded Solidity functions with different parameters. Appending a numeric suffix like 0 may not clearly convey the difference between these methods, potentially leading to confusion.
Consider adopting more descriptive method names that reflect the varying parameters. For instance, rename WithdrawAndCallGatewayZEVM0 to WithdrawAndCallGatewayZEVMWithChainID to indicate the inclusion of the chainId parameter. This approach enhances code readability and maintainability.
Apply this diff to update the method name:
func (_TestGatewayZEVMCaller *TestGatewayZEVMCallerTransactor) WithdrawAndCallGatewayZEVM0(opts *bind.TransactOpts, receiver []byte, amount *big.Int, chainId *big.Int, message []byte, callOptions CallOptions, revertOptions RevertOptions) (*types.Transaction, error) {
- return _TestGatewayZEVMCaller.contract.Transact(opts, "withdrawAndCallGatewayZEVM0", receiver, amount, chainId, message, callOptions, revertOptions)
+ return _TestGatewayZEVMCaller.contract.Transact(opts, "withdrawAndCallGatewayZEVM", receiver, amount, chainId, message, callOptions, revertOptions)
}This change would necessitate adjusting the code generation tool to support more expressive method names for overloaded functions.
Committable suggestion was skipped due to low confidence.
| @@ -29,6 +29,11 @@ var ( | |||
| _ = abi.ConvertType | |||
| ) | |||
|
|
|||
| // TestDAppV2MessageContext is an auto generated low-level Go binding around an user-defined struct. | |||
There was a problem hiding this comment.
Correct grammatical error in the comment of TestDAppV2MessageContext
In line 32, the comment contains a grammatical error. It should be "a user-defined struct" instead of "an user-defined struct". Since this file is auto-generated, consider updating the code generation tool to correct this issue.
Apply this diff to correct the comment:
-// TestDAppV2MessageContext is an auto generated low-level Go binding around an user-defined struct.
+// TestDAppV2MessageContext is an auto generated low-level Go binding around a user-defined struct.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.
| // TestDAppV2MessageContext is an auto generated low-level Go binding around an user-defined struct. | |
| // TestDAppV2MessageContext is an auto generated low-level Go binding around a user-defined struct. |
|
|
||
| r.AssertTestDAppEVMCalled(false, payloadMessageAuthenticatedWithdrawETH, amount) | ||
|
|
||
| r.ApproveETHZRC20(r.GatewayZEVMAddr) |
There was a problem hiding this comment.
Handle Errors from ApproveETHZRC20 Function Call
The call to r.ApproveETHZRC20(r.GatewayZEVMAddr) does not check for errors. To ensure robustness, handle any potential errors returned by this function.
Apply this diff to handle the error:
- r.ApproveETHZRC20(r.GatewayZEVMAddr)
+ err := r.ApproveETHZRC20(r.GatewayZEVMAddr)
+ require.NoError(r, err, "Failed to approve ETH ZRC20")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.
| r.ApproveETHZRC20(r.GatewayZEVMAddr) | |
| err := r.ApproveETHZRC20(r.GatewayZEVMAddr) | |
| require.NoError(r, err, "Failed to approve ETH ZRC20") |
| tx = r.V2ETHWithdrawAndAuthenticatedCall( | ||
| r.TestDAppV2EVMAddr, | ||
| amount, | ||
| []byte(payloadMessageAuthenticatedWithdrawETH), | ||
| gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}, | ||
| ) |
There was a problem hiding this comment.
Check for Errors in V2ETHWithdrawAndAuthenticatedCall
The function r.V2ETHWithdrawAndAuthenticatedCall may return errors that are currently unchecked. Capturing and handling these errors can prevent unexpected failures during test execution.
Apply this diff to incorporate error handling:
- tx = r.V2ETHWithdrawAndAuthenticatedCall(
+ tx, err = r.V2ETHWithdrawAndAuthenticatedCall(
r.TestDAppV2EVMAddr,
amount,
[]byte(payloadMessageAuthenticatedWithdrawETH),
gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)},
)
+ require.NoError(r, err, "Failed to perform V2 ETH withdraw and 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.
| tx = r.V2ETHWithdrawAndAuthenticatedCall( | |
| r.TestDAppV2EVMAddr, | |
| amount, | |
| []byte(payloadMessageAuthenticatedWithdrawETH), | |
| gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}, | |
| ) | |
| tx, err = r.V2ETHWithdrawAndAuthenticatedCall( | |
| r.TestDAppV2EVMAddr, | |
| amount, | |
| []byte(payloadMessageAuthenticatedWithdrawETH), | |
| gatewayzevm.RevertOptions{OnRevertGasLimit: big.NewInt(0)}, | |
| ) | |
| require.NoError(r, err, "Failed to perform V2 ETH withdraw and authenticated call") |
| require.Len(r, args, 1) | ||
|
|
||
| previousGasLimit := r.ZEVMAuth.GasLimit | ||
| r.ZEVMAuth.GasLimit = 10000000 |
There was a problem hiding this comment.
Define Gas Limit as a Constant for Clarity
The gas limit is set directly to 10000000. Defining this value as a constant with a descriptive name improves readability and maintainability.
Apply this diff to define and use a constant:
+const highGasLimit = 10000000
func TestV2ETHWithdrawAndAuthenticatedCall(r *runner.E2ERunner, args []string) {
require.Len(r, args, 1)
previousGasLimit := r.ZEVMAuth.GasLimit
- r.ZEVMAuth.GasLimit = 10000000
+ r.ZEVMAuth.GasLimit = highGasLimit
defer func() {
r.ZEVMAuth.GasLimit = previousGasLimit
}()Committable suggestion was skipped due to low confidence.
| @@ -11,6 +11,7 @@ | |||
| * [2861](https://github.com/zeta-chain/node/pull/2861) - emit events from staking precompile | |||
| * [2870](https://github.com/zeta-chain/node/pull/2870) - support for multiple Bitcoin chains in the zetaclient | |||
| * [2883](https://github.com/zeta-chain/node/pull/2883) - add chain static information for btc signet testnet | |||
| * [2904](https://github.com/zeta-chain/node/pull/2904) - authenticated zevm -> evm call | |||
There was a problem hiding this comment.
"integrate authenticated calls smart contract functionality into protocol"
| e2etests.TestV2ETHWithdrawAndAuthenticatedCallName, | ||
| e2etests.TestV2ETHWithdrawAndAuthenticatedCallThroughContractName, |
There was a problem hiding this comment.
Might be refactored in a further PR but I think we set these as the standard withdraw test and rename the current withdraw tests into WithdrawArbitraryCall
| @@ -50,6 +52,56 @@ func (r *E2ERunner) V2ETHWithdrawAndCall( | |||
| return tx | |||
| } | |||
|
|
|||
| // V2ETHWithdrawAndCall calls WithdrawAndCall of Gateway with gas token on ZEVM using authenticated call | |||
| func (r *E2ERunner) V2ETHWithdrawAndAuthenticatedCall( | |||
There was a problem hiding this comment.
Same comment as above here, rename the old function "arbitraryCall"
| function approve(address guy, uint256 wad) external returns (bool); | ||
| } | ||
|
|
||
| contract TestGatewayZEVMCaller { |
There was a problem hiding this comment.
I think we can simplify and remove the test prefix
| @@ -175,6 +175,8 @@ message MsgVoteInbound { | |||
|
|
|||
| // revert options provided by the sender | |||
| RevertOptions revert_options = 17 [ (gogoproto.nullable) = false ]; | |||
|
|
|||
| bool is_arbitrary_call = 18; | |||
There was a problem hiding this comment.
Can we maybe instead have a call_options param? And add comment for the field
| @@ -79,6 +79,7 @@ message OutboundParams { | |||
| uint64 effective_gas_limit = 22; | |||
| string tss_pubkey = 11; | |||
| TxFinalizationStatus tx_finalization_status = 12; | |||
| bool is_arbitrary_call = 24; | |||
| @@ -83,6 +83,7 @@ func CmdVoteInbound() *cobra.Command { | |||
| argsAsset, | |||
| uint(argsEventIndex), | |||
| protocolContractVersion, | |||
| true, // TODO: do we need to provide this as arg? | |||
| @@ -206,6 +206,7 @@ func (k Keeper) ProcessZRC20WithdrawalEvent( | |||
| foreignCoin.Asset, | |||
| event.Raw.Index, | |||
| types.ProtocolContractVersion_V1, | |||
| true, // not relevant for v1 | |||
There was a problem hiding this comment.
Shouldn't we rather set to false even if not used? So zero value is used
| @@ -183,7 +181,7 @@ func (k Keeper) newWithdrawalInbound( | |||
| return nil, errors.Wrapf(err, "cannot encode address %v", event.Receiver) | |||
| } | |||
|
|
|||
| gasLimit := event.GasLimit.Uint64() | |||
| gasLimit := event.CallOptions.GasLimit.Uint64() | |||
There was a problem hiding this comment.
There will be a delay between:
- processing upgrade that will use this new logic
- upgrading the gateway contract to emit the event with callOptions
In practice we should upgrade immediately after the chain upgrade, and pause cctx in between but we should still define the logic handling the event when the contract is not upgrade yet.
Do we have some mechanism to for example reject the tx when the event is not up to date (so not new cctx before contract upgraded)
| @@ -197,7 +196,7 @@ func (k Keeper) newWithdrawalInbound( | |||
|
|
|||
| return types.NewMsgVoteInbound( | |||
| "", | |||
| from.Hex(), | |||
| event.Sender.Hex(), | |||
There was a problem hiding this comment.
Looks to make more sense using the event
Description
Integrates this zeta-chain/protocol-contracts#344 into protocol, this PR should be merged after smart contract changes are merged, but this is ready for first pass.
Also e2e tests to call zevm->evm, using call and withdrawAndCall and authenticated sender, with checking if sender is properly forwarded, so destination contract can use it as authentication.
How Has This Been Tested?
Summary by CodeRabbit
New Features
TestDAppV2contract for improved message handling and sender management.TestGatewayZEVMCallercontract facilitates interactions with a gateway on the ZEVM.Bug Fixes
Documentation
Chores
go.modfor improved stability and performance.