add example of decoding event on-chain #286
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
npm warn config production Use WalkthroughThe pull request introduces significant updates to the Venus contract and its associated bindings. Key changes include the addition of new methods and events in 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/go/venus/Venus.go(4 hunks)contracts/examples/Venus.sol(2 hunks)package.json(1 hunks)src/evm/contracts/Venus.ts(9 hunks)src/evm/contracts/factories/Venus__factory.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/evm/contracts/Venus.ts
[error] 220-220: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 270-270: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (4)
src/evm/contracts/factories/Venus__factory.ts (2)
28-32: Ensure constructor parameters are correctly typed and documented
The constructor now includes the '_counterParty' parameter. Verify that all usages of the constructor pass the correct address and that the parameter is properly documented.
284-297: Update deployment functions to include '_counterParty'
The 'getDeployTransaction' and 'deploy' methods have been updated to include '_counterParty'. Ensure that any scripts or tools that deploy this contract are updated to provide this new parameter.
bindings/go/venus/Venus.go (1)
34-34: Go bindings updated to reflect new contract interface
The Go bindings have been correctly updated to include new methods and events corresponding to the contract changes.
Also applies to: 183-244, 318-338, 609-742, 744-877
src/evm/contracts/Venus.ts (1)
185-188: Methods appropriately added for new contract functions
The methods 'counterParty' and 'lastReceivedTransmission' have been correctly added with proper types.
| if (address(uint160(uint256(bytes32(emittingAddress)))) != counterParty) { | ||
| // If this triggers, we have received a valid event from the source chain with a valid proof, | ||
| // but it was emitted from a wrong address. This would likely be someone trying to spoof another contract's | ||
| // event. | ||
| // This validation is important since any contract can emit any event. | ||
| revert invalidEventSender(); | ||
| } |
There was a problem hiding this comment.
Potential incorrect address conversion in 'emittingAddress' comparison
The conversion of 'emittingAddress' to an address may not handle all cases correctly. Casting bytes to address should ensure proper length and formatting to avoid incorrect comparisons.
Update the address conversion to safely decode 'emittingAddress':
- if (address(uint160(uint256(bytes32(emittingAddress)))) != counterParty) {
+ if (abi.decode(emittingAddress, (address)) != counterParty) {
// ...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (address(uint160(uint256(bytes32(emittingAddress)))) != counterParty) { | |
| // If this triggers, we have received a valid event from the source chain with a valid proof, | |
| // but it was emitted from a wrong address. This would likely be someone trying to spoof another contract's | |
| // event. | |
| // This validation is important since any contract can emit any event. | |
| revert invalidEventSender(); | |
| } | |
| if (abi.decode(emittingAddress, (address)) != counterParty) { | |
| // If this triggers, we have received a valid event from the source chain with a valid proof, | |
| // but it was emitted from a wrong address. This would likely be someone trying to spoof another contract's | |
| // event. | |
| // This validation is important since any contract can emit any event. | |
| revert invalidEventSender(); | |
| } |
| constructor(ICrossL2Prover _prover, address _counterParty) { | ||
| if (address(_prover) == address(0)) { | ||
| revert invalidProverAddress(); | ||
| } | ||
| prover = _prover; | ||
| counterParty = _counterParty; |
There was a problem hiding this comment.
Missing validation for '_counterParty' address in constructor
The '_counterParty' parameter is not checked for the zero address. Assigning a zero address to 'counterParty' could lead to unexpected behavior or security risks.
Consider adding a validation check for '_counterParty':
constructor(ICrossL2Prover _prover, address _counterParty) {
if (address(_prover) == address(0)) {
revert invalidProverAddress();
}
prover = _prover;
+ if (_counterParty == address(0)) {
+ revert invalidCounterPartyAddress();
+ }
counterParty = _counterParty;
}Also, define the new error:
+ error invalidCounterPartyAddress();Committable suggestion skipped: line range outside the PR's diff.
| [void], | ||
| "nonpayable" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'void' with 'undefined' in method output types
Using 'void' outside of return type positions can be confusing in TypeScript. It's recommended to use 'undefined' for output tuple types.
Apply this change:
receiveTransmissionEvent: TypedContractMethod<
[
receiptIndex: BytesLike,
receiptRLPEncodedBytes: BytesLike,
logIndex: BigNumberish,
proof: BytesLike
],
- [void],
+ [undefined],
"nonpayable"
>;Similarly, update other instances where '[void]' is used as an output type.
Also applies to: 270-271
🧰 Tools
🪛 Biome (1.9.4)
[error] 220-220: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Summary by CodeRabbit
New Features
Bug Fixes
Chores