refactor: renaming origin and sender in MessageContext
#474
Conversation
📝 WalkthroughWalkthroughThe changes update the cross-chain interaction interface by replacing the old Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)`test/**`: Review the test files for proper coverage, edge cases, and best practices.
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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
🧹 Nitpick comments (1)
test/utils/TestUniversalContract.sol (1)
48-48: Rename event parameter and doc references.You're now passing
context.senderas the first event argument, yet the event parameter remains namedorigin, and the documentation refers to it as "The origin address on the external chain." This may cause confusion since it's now a chain-agnosticbytes sender. Consider renaming the parameter and updating the related doc comments for clarity.Here is a possible diff to illustrate the rename:
- event ContextData(bytes origin, address sender, uint256 chainID, address msgSender, string message); + event ContextData(bytes sender, address senderEVM, uint256 chainID, address msgSender, string message);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
docs/src/contracts/zevm/interfaces/UniversalContract.sol/interface.UniversalContract.mdis excluded by!docs/**docs/src/contracts/zevm/interfaces/UniversalContract.sol/struct.MessageContext.mdis excluded by!docs/**pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!pkg/**pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!pkg/**pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!pkg/**pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!pkg/**pkg/gatewayzevmupgradetest.sol/gatewayzevmupgradetest.gois excluded by!pkg/**pkg/igatewayzevm.sol/igatewayzevm.gois excluded by!pkg/**pkg/senderzevm.sol/senderzevm.gois excluded by!pkg/**pkg/systemcontract.sol/systemcontract.gois excluded by!pkg/**pkg/systemcontractmock.sol/systemcontractmock.gois excluded by!pkg/**pkg/testuniversalcontract.sol/testuniversalcontract.gois excluded by!pkg/**pkg/universalcontract.sol/universalcontract.gois excluded by!pkg/**pkg/zrc20.t.sol/zrc20test.gois excluded by!pkg/**types/GatewayZEVM.tsis excluded by!types/**types/GatewayZEVMUpgradeTest.tsis excluded by!types/**types/IGatewayZEVM.sol/IGatewayZEVM.tsis excluded by!types/**types/TestUniversalContract.tsis excluded by!types/**types/UniversalContract.sol/UniversalContract.tsis excluded by!types/**types/factories/GatewayZEVMUpgradeTest__factory.tsis excluded by!types/**types/factories/GatewayZEVM__factory.tsis excluded by!types/**types/factories/IGatewayZEVM.sol/IGatewayZEVM__factory.tsis excluded by!types/**types/factories/SenderZEVM__factory.tsis excluded by!types/**types/factories/SystemContract.sol/SystemContract__factory.tsis excluded by!types/**types/factories/SystemContractMock.sol/SystemContractMock__factory.tsis excluded by!types/**types/factories/TestUniversalContract__factory.tsis excluded by!types/**types/factories/UniversalContract.sol/UniversalContract__factory.tsis excluded by!types/**
📒 Files selected for processing (3)
contracts/zevm/interfaces/UniversalContract.sol(1 hunks)test/GatewayZEVM.t.sol(20 hunks)test/utils/TestUniversalContract.sol(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`test/**`: Review the test files for proper coverage, edge c...
test/**: Review the test files for proper coverage, edge cases, and best practices.
test/utils/TestUniversalContract.soltest/GatewayZEVM.t.sol
`contracts/**`: Review the Solidity contracts for security v...
contracts/**: Review the Solidity contracts for security vulnerabilities and best practices.
contracts/zevm/interfaces/UniversalContract.sol
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: generate
- GitHub Check: slither
- GitHub Check: test
🔇 Additional comments (22)
contracts/zevm/interfaces/UniversalContract.sol (2)
26-27: Documentation looks clear.These docstrings accurately convey the purpose of the struct and highlight its chain-agnostic design.
40-46: New interface and doc block are well-structured.The reworked
onCallmethod with the revisedMessageContextparameter is consistent with the described functionality and documentation. No immediate concerns.test/GatewayZEVM.t.sol (20)
799-800: Same concern as above.
809-810: Same concern as above.
819-820: Same concern as above.
829-830: Same concern as above.
859-860: Same concern as above.
869-870: Same concern as above.
879-880: Same concern as above.
892-893: Same concern as above.
906-907: Same concern as above.
916-917: Same concern as above.
926-927: Same concern as above.
985-986: Same concern as above.
995-996: Same concern as above.
1005-1006: Same concern as above.
1015-1016: Same concern as above.
1029-1030: Same concern as above.
1050-1051: Same concern as above.
1061-1062: Same concern as above.
1072-1073: Same concern as above.
789-790:✅ Verification successful
Potential mismatch in
sendervs.senderEVM.Here,
MessageContext.senderis set toabi.encodePacked(address(gateway)), whileMessageContext.senderEVMis set toprotocolAddress. Verify that this difference is intentional. If this chain is EVM-based, you might want both to represent the same address or document the reason for using distinct addresses.
🏁 Script executed:
#!/bin/bash # Searching the codebase for all test context initializations to ensure consistency rg -A 5 "MessageContext({ sender:"Length of output: 151
Below is a revised next step to verify the intended behavior:
It appears the first run failed due to unescaped curly braces in the regex. To continue gathering context on the initialization pattern for
MessageContext—especially the assignments forsenderandsenderEVM—please run the following script using a fixed-string search so that the curly braces are treated literally:The output from this search should help confirm whether:
- The use of
abi.encodePacked(address(gateway))forsenderandprotocolAddressforsenderEVMis unique to this test case.- There exists a consistent pattern elsewhere in the codebase that supports using different addresses.
Once you have validated that the discrepancy is intentional (or identified a mistake if similar initializations are meant to use the same value), please update the test code accordingly or add documentation that clarifies the difference.
🏁 Script executed:
#!/bin/bash # Search for the MessageContext initialization in the specific file with correct escaping rg --fixed-strings -A 5 "MessageContext({ sender:" test/GatewayZEVM.t.solLength of output: 6331
Attention: Confirm Intentional Use of Distinct Values for sender and senderEVM
We verified that every instantiation of
MessageContextintest/GatewayZEVM.t.solconsistently assigns:
senderasabi.encodePacked(address(gateway))senderEVMasprotocolAddressSince the same pattern is used throughout the test cases, it appears the distinction is intentional. However, to avoid any future confusion, please confirm that this design choice is deliberate. If so, consider adding a brief comment/documentation that explains why the gateway’s address is encoded for
senderwhile a different address (i.e.protocolAddress) is used forsenderEVM.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
=======================================
Coverage 86.22% 86.22%
=======================================
Files 9 9
Lines 559 559
Branches 129 129
=======================================
Hits 482 482
Misses 77 77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changes in smart contract discussed from zeta-chain/node#3689
These changes are aimed to be backward compatible, keep same signature, but rename the field for refactoring in the chain.
originwas initially intended to be set fromtx.originand represent the signer address on each non-EVM chains but the field has never been used.Summary by CodeRabbit