feat: add DepositedAndCalled and WithdrawnAndCalled events
#403
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
=======================================
Coverage 84.23% 84.23%
=======================================
Files 8 8
Lines 387 387
Branches 122 122
=======================================
Hits 326 326
Misses 61 61 ☔ View full report in Codecov by Sentry. |
lumtis
requested review from
andresaiello,
brewmaster012,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
v2/test/GatewayEVMZEVM.t.sol (2)
Line range hint
213-224: LGTM: Comprehensive event parameters enhance traceabilityThe
WithdrawnAndCalledevent includes all necessary information about the withdrawal and the subsequent call, which greatly improves traceability and debugging capabilities. The inclusion of gas fees and protocol flat fees is particularly useful for monitoring and auditing purposes.For consistency, consider adding a comment above the event emission explaining the purpose and contents of the
WithdrawnAndCalledevent. This would help developers understand the event's significance at a glance.
Line range hint
212-224: Suggestion: Add specific test for new event emissionWhile the changes to the event emission improve clarity and align with the PR objectives, consider adding a specific test case to verify the correct emission of the new
WithdrawnAndCalledevent. This would ensure that the event is emitted with the correct parameters under various scenarios, further enhancing the test coverage of this important change.v2/contracts/zevm/interfaces/IGatewayZEVM.sol (3)
34-34: LGTM. Consider deprecation strategy for future versions.The added comment for the
messageparameter in theWithdrawnevent improves code documentation and clarity. Keeping unused parameters for compatibility is a good practice.For future versions, consider implementing a deprecation strategy for the
messageparameter, such as:
- Marking it as deprecated in the next minor version.
- Removing it in the next major version update.
This approach would help maintain backwards compatibility while signaling to developers that they should not rely on this parameter in new implementations.
50-72: LGTM. Consider adding @dev documentation.The addition of the
WithdrawnAndCalledevent improves the contract's ability to log withdraw-and-call operations precisely, aligning with the PR objectives. The event structure is consistent with existing patterns.Consider adding a
@devdocumentation comment to explain the difference between this event and theWithdrawnevent, and when each should be emitted. This would provide clarity for developers implementing or working with this interface. For example:/// @notice Emitted when a withdraw and call is made. /// @dev This event is emitted instead of the `Withdrawn` event when the withdrawal is accompanied by a contract call.
Line range hint
1-72: Overall, the changes improve cross-chain interaction logging.The modifications to this interface enhance the contract's ability to log cross-chain interactions more precisely, particularly for withdraw-and-call operations. The changes are backwards compatible and align well with the PR objectives. The code maintains good structure and follows consistent patterns.
As the contract evolves, consider:
- Implementing a deprecation strategy for unused parameters.
- Ensuring that implementations of this interface emit the appropriate events (
WithdrawnorWithdrawnAndCalled) based on the operation performed.- Keeping the documentation up-to-date, especially when adding new events or modifying existing ones.
v2/contracts/zevm/GatewayZEVM.sol (1)
Line range hint
235-275: Temporary implementation ofwithdrawAndCallfunction for ZETA tokensThe
withdrawAndCallfunction for ZETA tokens is currently disabled and reverts with aZETANotSupportederror, consistent with thewithdrawfunction for ZETA tokens.Ensure that when implementing ZETA support:
- Both
withdrawandwithdrawAndCallfunctions for ZETA are implemented and tested together.- The commented-out logic is reviewed and updated if necessary before uncommenting.
- The
WithdrawnAndCalledevent emission is consistent with the ZRC20 version of the function.v2/contracts/evm/GatewayEVM.sol (1)
Line range hint
339-354: LGTM: Newcallfunction enhances contract functionalityThe addition of the
callfunction allows for calling omnichain smart contracts without asset transfer, which aligns with the PR objectives. The function includes appropriate checks and follows the contract's conventions.For consistency with other functions, consider adding a check for a zero amount:
function call( address receiver, bytes calldata payload, RevertOptions calldata revertOptions ) external whenNotPaused nonReentrant { if (receiver == address(0)) revert ZeroAddress(); if (payload.length + revertOptions.revertMessage.length > MAX_PAYLOAD_SIZE) revert PayloadSizeExceeded(); + if (msg.value > 0) revert UnexpectedETHAmount(); emit Called(msg.sender, receiver, payload, revertOptions); }This ensures that no ETH is accidentally sent with the call, maintaining consistency with the function's purpose of calling without asset transfer.
v2/test/GatewayEVM.t.sol (1)
633-633: LGTM: Event name change improves clarityThe change from
DepositedtoDepositedAndCalledaccurately reflects the action being performed, which includes both a deposit and a call. This aligns well with the PR objectives and improves the clarity of the event emission.Consider renaming the test function from
testDepositEthToTssWithPayloadtotestDepositAndCallEthToTssWithPayloadto better reflect the action being tested. This would make the test name consistent with the new event name.-function testDepositEthToTssWithPayload() public { +function testDepositAndCallEthToTssWithPayload() public {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (56)
v2/docs/src/contracts/evm/ERC20Custody.sol/contract.ERC20Custody.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/GatewayEVM.sol/contract.GatewayEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/evm/interfaces/IGatewayEVM.sol/interface.IGatewayEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/GatewayZEVM.sol/contract.GatewayZEVM.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/IGatewayZEVM.sol/interface.IGatewayZEVMEvents.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.zContract.mdis excluded by!v2/docs/**v2/docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.zContext.mdis excluded by!v2/docs/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyupgradetest.sol/erc20custodyupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevmupgradetest.sol/gatewayzevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevm.gois excluded by!v2/pkg/**v2/pkg/igatewayevm.sol/igatewayevmevents.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevm.gois excluded by!v2/pkg/**v2/pkg/igatewayzevm.sol/igatewayzevmevents.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornativeupgradetest.sol/zetaconnectornativeupgradetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnativeupgradetest.sol/zetaconnectornonnativeupgradetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/GatewayEVM.tsis excluded by!v2/types/**v2/types/GatewayEVMUpgradeTest.tsis excluded by!v2/types/**v2/types/GatewayZEVM.tsis excluded by!v2/types/**v2/types/GatewayZEVMUpgradeTest.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVM.tsis excluded by!v2/types/**v2/types/IGatewayEVM.sol/IGatewayEVMEvents.tsis excluded by!v2/types/**v2/types/IGatewayZEVM.sol/IGatewayZEVM.tsis excluded by!v2/types/**v2/types/IGatewayZEVM.sol/IGatewayZEVMEvents.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVMEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVMEvents__factory.tsis excluded by!v2/types/**v2/types/factories/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**
📒 Files selected for processing (7)
- v2/contracts/evm/GatewayEVM.sol (2 hunks)
- v2/contracts/evm/interfaces/IGatewayEVM.sol (2 hunks)
- v2/contracts/zevm/GatewayZEVM.sol (2 hunks)
- v2/contracts/zevm/interfaces/IGatewayZEVM.sol (2 hunks)
- v2/test/GatewayEVM.t.sol (2 hunks)
- v2/test/GatewayEVMZEVM.t.sol (1 hunks)
- v2/test/GatewayZEVM.t.sol (2 hunks)
🧰 Additional context used
🔇 Additional comments (11)
v2/test/GatewayEVMZEVM.t.sol (1)
212-212: LGTM: Event name change improves clarityThe change from
WithdrawntoWithdrawnAndCalledaccurately reflects the combined action of withdrawing funds and calling a contract. This improvement in event naming enhances the clarity of the contract's behavior and aligns well with the PR's objective.v2/contracts/zevm/GatewayZEVM.sol (4)
Line range hint
290-291: Gas limit check added tocallfunctionA new check
if (callOptions.gasLimit == 0) revert InsufficientGasLimit();has been added to ensure a valid gas limit is provided for cross-chain calls. This is consistent with the check in thewithdrawAndCallfunction and improves the robustness of the contract.
Line range hint
1-474: Summary of changes in GatewayZEVM contractThe changes in this file focus on:
- Updating the
withdrawAndCallfunction to emit a more descriptiveWithdrawnAndCalledevent.- Adding temporary implementations for
withdrawandwithdrawAndCallfunctions for ZETA tokens (currently disabled).- Adding a gas limit check in the
callfunction for consistency with other functions.These changes improve the contract's clarity and prepare it for future ZETA token support without affecting existing functionality. The temporary implementations are properly isolated, and the new checks enhance the contract's robustness.
To ensure the changes are properly tested, verify the existence of relevant test cases:
#!/bin/bash # Check for test cases related to the new WithdrawnAndCalled event and gas limit checks rg --type typescript 'WithdrawnAndCalled|InsufficientGasLimit' ./test/
Line range hint
190-201: Event emission updated inwithdrawAndCallfunctionThe
Withdrawnevent has been replaced with a newWithdrawnAndCalledevent. This change improves the clarity of the event by explicitly indicating that a call was made along with the withdrawal.To ensure consistency, let's verify if this new event is properly defined in the interface:
#!/bin/bash # Check if the WithdrawnAndCalled event is defined in the IGatewayZEVM interface rg --type solidity 'event\s+WithdrawnAndCalled' ./v2/contracts/zevm/interfaces/IGatewayZEVM.sol
Line range hint
204-232: Temporary implementation ofwithdrawfunction for ZETA tokensThe
withdrawfunction for ZETA tokens is currently disabled and reverts with aZETANotSupportederror. This is a temporary measure until ZETA withdrawals are supported.To ensure this is being tracked, let's verify the existence of the mentioned issue:
Once ZETA withdrawals are supported, remember to:
- Remove the
ZETANotSupportederror.- Uncomment and implement the function logic.
- Ensure proper testing is in place for the new functionality.
✅ Verification successful
To further verify the presence of the
WithdrawnAndCalledevent in the codebase, let's run the following script:
Verification of
withdrawFunctionThe
withdrawfunction is correctly disabled and properly references the open issue #394 tracking support for ZETA withdrawals.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the issue #394 exists and is open gh issue view 394 --repo zeta-chain/protocol-contractsLength of output: 595
Script:
#!/bin/bash # Search for the 'WithdrawnAndCalled' event in the codebase rg 'event\s+WithdrawnAndCalled' .Length of output: 14003
v2/contracts/evm/GatewayEVM.sol (2)
310-310: LGTM: Event name change improves clarityThe change from
DepositedtoDepositedAndCalledaccurately reflects the function's behavior. This modification aligns with the PR objective of improving event parsing and enhances the contract's clarity.
336-336: LGTM: Consistent event naming across asset typesThe change from
DepositedtoDepositedAndCalledfor ERC20 deposits maintains consistency with the ETH deposit function. This uniform approach enhances the contract's overall clarity and aligns with the PR's objectives.v2/test/GatewayEVM.t.sol (1)
599-599: LGTM: Event name change improves clarityThe change from
DepositedtoDepositedAndCalledaccurately reflects the action being performed, which includes both a deposit and a call. This aligns well with the PR objectives and improves the clarity of the event emission.v2/test/GatewayZEVM.t.sol (2)
Line range hint
256-266: Event emission updated to 'WithdrawnAndCalled'In the
testWithdrawZRC20WithMessagetest function, the emitted event has been correctly updated toWithdrawnAndCalled, aligning with the changes in the contract's events. This ensures that the test accurately reflects the new event emission.
Line range hint
332-342: Event emission updated to 'WithdrawnAndCalled'In the
testWithdrawZRC20WithCallOptsWithMessagetest function, the event emission is correctly updated toWithdrawnAndCalled, maintaining consistency with the updated contract behavior. This accurate reflection in the test ensures reliable testing of the new event.v2/contracts/evm/interfaces/IGatewayEVM.sol (1)
46-61: TheDepositedAndCalledevent is well-defined and consistentThe new
DepositedAndCalledevent accurately captures all necessary details for deposits that involve a contract call. The parameter types, order, and indexing are consistent with existing events likeDeposited, ensuring coherence across the contract's event definitions. The documentation appropriately describes each parameter.
| @@ -32,7 +32,7 @@ interface IGatewayEVMEvents { | |||
| /// @param receiver The address of the receiver. | |||
| /// @param amount The amount of ETH or tokens deposited. | |||
| /// @param asset The address of the ERC20 token (zero address if ETH). | |||
| /// @param payload The calldata passed with the deposit. | |||
| /// @param payload The calldata passed with the deposit. No longer used. Kept to maintain compatibility. | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the deprecation of the payload parameter in the Deposited event
The comment for the payload parameter indicates it's "No longer used" but "Kept to maintain compatibility." To enhance clarity, consider marking this parameter as deprecated using a standard annotation or adding a clear deprecation notice. This will help developers understand that it should not be used in new implementations.
| @@ -32,7 +32,7 @@ interface IGatewayEVMEvents { | |||
| /// @param receiver The address of the receiver. | |||
| /// @param amount The amount of ETH or tokens deposited. | |||
| /// @param asset The address of the ERC20 token (zero address if ETH). | |||
| /// @param payload The calldata passed with the deposit. | |||
| /// @param payload The calldata passed with the deposit. No longer used. Kept to maintain compatibility. | |||
There was a problem hiding this comment.
why we need to keep unused field? we have breaking interface in v21 for other things as well
There was a problem hiding this comment.
- Keep depositAndCall emitted events valid between node upgrade -> gateway upgrade (even though it should not be used)
- I think changing the events mess up with the indexers
|
@lumtis i merged main here since conflicts were just in bindings and will merge PR when CI is green |
Change the event emitted in WithdrawAndCall and DepositAndCall to allow parsing these action with an empty message
Protocol integration associated PR: zeta-chain/node#3018
Summary by CodeRabbit
New Features
callfunction for calling omnichain smart contracts without asset transfers.WithdrawnAndCalledandDepositedAndCalledevents for improved clarity on withdrawals and deposits involving contract calls.Bug Fixes
Documentation
Tests