nico/polymer proofs #39
Conversation
nicopernas
force-pushed
the
nico/polymer-proofs
branch
11 times, most recently
from
567bb4c
to
07a4ba5
Compare
nicopernas
force-pushed
the
nico/polymer-proofs
branch
from
1a560e4
to
42c5f4b
Compare
|
Should we add the external solidity dependencies like RLPReader.sol as submodules instead of copying them into our code base? |
|
@coderabbitai review |
nicopernas
force-pushed
the
nico/polymer-proofs
branch
4 times, most recently
from
988a171
to
d82c224
Compare
nicopernas
force-pushed
the
nico/polymer-proofs
branch
from
d82c224
to
cfd7a13
Compare
nicopernas
force-pushed
the
nico/polymer-proofs
branch
from
cfd7a13
to
45b7afc
Compare
nicopernas
force-pushed
the
nico/polymer-proofs
branch
from
45b7afc
to
94fc283
Compare
yeah! that was there because I was just lazy |
|
@coderabbitai review |
WalkthroughThis update brings significant enhancements across various aspects, including integrating new submodules, refining consensus state management, and enhancing IBC channel handling. The changes aim to streamline operations, bolster proof verification mechanisms, and optimize inter-blockchain communication for improved efficiency and security in blockchain interactions. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 36
Configuration used: CodeRabbit UI
Files ignored due to path filters (5)
foundry.tomlis excluded by:!**/*.tomltest/payload/l1_block_0x4df537.jsonis excluded by:!**/*.jsontest/payload/l1_block_0x61a80.jsonis excluded by:!**/*.jsontest/payload/l2_block_0x4b0.jsonis excluded by:!**/*.jsontest/payload/output_at_block_l1_0x4df537_with_proof.jsonis excluded by:!**/*.json
Files selected for processing (40)
- .gitmodules (1 hunks)
- contracts/ConsensusStateManager.sol (2 hunks)
- contracts/Dispatcher.sol (13 hunks)
- contracts/DummyConsensusStateManager.sol (2 hunks)
- contracts/DummyProofVerifier.sol (1 hunks)
- contracts/Earth.sol (1 hunks)
- contracts/Ibc.sol (3 hunks)
- contracts/IbcDispatcher.sol (2 hunks)
- contracts/IbcReceiver.sol (1 hunks)
- contracts/Mars.sol (6 hunks)
- contracts/OpConsensusStateManager.sol (2 hunks)
- contracts/OpProofVerifier.sol (1 hunks)
- contracts/ProofVerifier.sol (1 hunks)
- contracts/UniversalChannelHandler.sol (4 hunks)
- lib/base64 (1 hunks)
- lib/optimism (1 hunks)
- lib/proto/GoogleProtobufAny.sol (1 hunks)
- lib/proto/ProtoBufRuntime.sol (1 hunks)
- lib/proto/channel.sol (1 hunks)
- proto/channel.proto (1 hunks)
- proto/generate.sh (1 hunks)
- remappings.txt (1 hunks)
- test/Dispatcher.base.t.sol (6 hunks)
- test/Dispatcher.client.t.sol (1 hunks)
- test/Dispatcher.proof.t.sol (1 hunks)
- test/Dispatcher.t.sol (18 hunks)
- test/Ibc.t.sol (1 hunks)
- test/OpConsensusStateManager.t.sol (2 hunks)
- test/Proof.base.t.sol (1 hunks)
- test/Verifier.t.sol (1 hunks)
- test/VirtualChain.sol (8 hunks)
- test/payload/channel_ack_pending_proof.hex (1 hunks)
- test/payload/channel_confirm_pending_proof.hex (1 hunks)
- test/payload/channel_try_pending_proof.hex (1 hunks)
- test/payload/l1_block_0x1235261.hex (1 hunks)
- test/payload/l1_block_0x4df537.hex (1 hunks)
- test/payload/packet_ack_proof.hex (1 hunks)
- test/payload/packet_commitment_proof.hex (1 hunks)
- test/query-contract.sh (1 hunks)
- test/universal.channel.t.sol (11 hunks)
Files not summarized due to errors (1)
- lib/proto/ProtoBufRuntime.sol: Error: Message exceeds token limit
Files skipped from review due to trivial changes (2)
- lib/base64
- lib/optimism
Additional comments: 95
remappings.txt (1)
- 6-7: LGTM!
contracts/DummyProofVerifier.sol (1)
- 1-12: LGTM!
.gitmodules (1)
- 10-15: LGTM!
contracts/ConsensusStateManager.sol (4)
- 13-18: LGTM!
- 28-28: LGTM!
- 35-35: LGTM!
- 40-40: LGTM!
contracts/DummyConsensusStateManager.sol (3)
- 16-21: LGTM!
- 33-34: LGTM!
- 37-38: LGTM!
proto/channel.proto (2)
- 1-22: LGTM!
- 25-31: LGTM!
proto/generate.sh (1)
- 1-37: LGTM!
test/Dispatcher.client.t.sol (3)
- 13-15: LGTM!
- 24-27: LGTM!
- 30-33: LGTM!
contracts/Earth.sol (1)
- 42-42: LGTM!
contracts/IbcReceiver.sol (1)
- 19-19: LGTM!
contracts/ProofVerifier.sol (1)
- 1-78: LGTM!
contracts/IbcDispatcher.sol (2)
- 7-7: LGTM!
- 30-35: LGTM!
test/Ibc.t.sol (1)
- 1-77: LGTM!
test/query-contract.sh (1)
- 1-67: LGTM!
test/Proof.base.t.sol (1)
- 1-86: LGTM!
test/OpConsensusStateManager.t.sol (10)
- 13-15: LGTM!
- 17-19: LGTM!
- 23-23: LGTM!
- 28-34: LGTM!
- 41-48: LGTM!
- 52-60: LGTM!
- 66-68: LGTM!
- 83-90: LGTM!
- 92-96: LGTM!
- 102-105: LGTM!
test/payload/l1_block_0x1235261.hex (1)
- 1-1: SKIP
test/payload/l1_block_0x4df537.hex (1)
- 1-1: SKIP
contracts/OpConsensusStateManager.sol (6)
- 6-6: Import statement for
L1Blockis correct and follows Solidity conventions.- 18-19: The declaration of
fraudProofWindowSeconds,verifier, andl1BlockProvideris correct and follows Solidity naming conventions.- 21-24: Constructor correctly initializes
fraudProofWindowSeconds,verifier, andl1BlockProvider. Ensure external calls to this constructor pass valid arguments.- 32-47: The
addOpConsensusStatemethod correctly includes proof verification logic using theverifiercontract. Ensure thatverifyStateUpdatemethod inProofVerifierhandles all edge cases and reverts on invalid proofs.- 93-102: The use of
Ics23Proof,bytes, andexpectedValueinverifyMembershipmethod is correct. Ensure that the logic insideverifier.verifyMembershipcorrectly handles the proof verification.- 105-108: The
verifyNonMembershipmethod correctly calls theverifiercontract. Ensure that theverifyNonMembershipmethod inProofVerifieris implemented and correctly verifies non-membership proofs.contracts/Mars.sol (4)
- 27-27: The
onAcknowledgementPacketfunction signature and implementation are correct. Ensure that the logic for handling acknowledgment packets is complete and secure.- 37-42: The
onOpenIbcChannelfunction signature correctly includes aCounterPartystruct parameter. Ensure that the logic for handling channel opening is complete and secure.- 75-75: The
onConnectIbcChannelfunction signature is correct. Ensure that the logic for handling channel connection is complete and secure.- 90-90: The
onCloseIbcChannelfunction signature is simplified and correct. Ensure that the logic for handling channel closure is complete and secure.test/Dispatcher.base.t.sol (6)
- 5-5: The addition of
Proof.base.t.solimport is correct and necessary for the test setup.- 17-17: The addition of
string portIdto theLocalEndstruct is correct and aligns with the changes in the contracts being tested.- 29-29: Replacing
ProofwithIcs23Proofin theChannelHandshakeSettingstruct is correct and aligns with the updated proof verification logic.- 74-80: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [77-101]
The updates in
openChannelfunction calls reflect the changes in parameter structures correctly. Ensure that the test logic covers all relevant cases for channel opening.
- 128-139: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [120-136]
The updates in
connectChannelfunction calls reflect the changes in parameter structures correctly. Ensure that the test logic covers all relevant cases for channel connection.
- 147-152: The
ackToBytesfunction correctly convertsAckPacketto bytes. Ensure that this utility function is used appropriately in tests.contracts/UniversalChannelHandler.sol (3)
- 34-39: The
onOpenIbcChannelfunction signature correctly includes aCounterPartystruct parameter. Ensure that the logic for handling channel opening is complete and secure.- 57-57: The
onConnectIbcChannelfunction signature is correct. Ensure that the logic for handling channel connection is complete and secure.- 67-67: The
onCloseIbcChannelfunction signature is simplified and correct. Ensure that the logic for handling channel closure is complete and secure.contracts/OpProofVerifier.sol (3)
- 48-105: The
verifyStateUpdatemethod correctly implements the logic for verifying state updates. Ensure that all edge cases are handled and the method reverts on invalid proofs.- 107-108: The
verifyNonMembershipmethod correctly indicates that it is not implemented. Consider implementing this method if non-membership verification is required.- 117-131: The
verifyMembershipmethod correctly implements the logic for verifying membership proofs. Ensure that all edge cases are handled and the method reverts on invalid proofs.test/payload/channel_confirm_pending_proof.hex (1)
- 1-1: The file contains a hexadecimal string used as test data. No code-related issues to review.
test/payload/channel_try_pending_proof.hex (1)
- 1-1: The file contains a hexadecimal string used as test data. No code-related issues to review.
test/payload/channel_ack_pending_proof.hex (1)
- 1-1: The file contains a hexadecimal string used as test data. No code-related issues to review.
test/VirtualChain.sol (6)
- 8-8: Import of
ProofVerifier.solreplaces the previousVerifier.sol. Ensure that all functionalities previously provided byVerifier.solare covered or appropriately handled byProofVerifier.sol.- 19-22: The
ChannelSettingstruct now includesportId,channelId, and changesProoftoIcs23Proof. Verify that all usages ofChannelSettingthroughout the codebase have been updated to include these new fields and that the change fromProoftoIcs23Proofdoes not introduce any compatibility issues.- 57-57: Removal of
Verifierinstantiation in the constructor. Confirm that theProofVerifieror equivalent functionality is initialized elsewhere as needed, ensuring no loss of functionality.Verification successful
The search results indicate that
ProofVerifierand its implementations (OpProofVerifier,DummyProofVerifier) are indeed present and utilized across various files in the repository. Specifically, theOpProofVerifierandDummyProofVerifierare both implementations of theProofVerifierinterface, as seen in thecontracts/OpProofVerifier.solandcontracts/DummyProofVerifier.solfiles. Additionally, theOpConsensusStateManagerconstructor incontracts/OpConsensusStateManager.soltakes aProofVerifieras an argument, suggesting that the proof verification functionality is initialized and used in different parts of the system.Given this context, it can be concluded that the removal of
Verifierinstantiation in theVirtualChain.solconstructor does not result in the loss of proof verification functionality, as theProofVerifieror equivalent functionality is indeed initialized and used elsewhere in the codebase.* 138-141: The `channelOpenAckOrConfirm` function calls are updated with additional parameters. Ensure that these changes are reflected in all calls to this function and that the added parameters are correctly handled within the function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ProofVerifier or equivalent initialization rg "ProofVerifier" --type solidityLength of output: 67
Script:
#!/bin/bash # Search for ProofVerifier in Solidity files without using the --type flag rg "ProofVerifier" --glob "*.sol"Length of output: 2557
Verification successful
The verification process has confirmed that the
channelOpenAckOrConfirmfunction calls have been updated with additional parameters consistently across the codebase, and the added parameters are correctly handled within the function. This aligns with the initial review comment, ensuring that these changes are reflected in all calls to this function.* 171-171: Refactoring of the `channelOpenInit` function to use `CounterParty` struct. Confirm that the `CounterParty` struct is correctly utilized and that all necessary fields are provided in calls to this function. * 244-255: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [225-252]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify updated calls to channelOpenAckOrConfirm rg "channelOpenAckOrConfirm" --type solidityLength of output: 77
Script:
#!/bin/bash # Verify updated calls to channelOpenAckOrConfirm in Solidity files rg "channelOpenAckOrConfirm" "*.sol"Length of output: 82
Script:
#!/bin/bash # Correctly verify updated calls to channelOpenAckOrConfirm in Solidity files using the -g flag rg "channelOpenAckOrConfirm" -g "*.sol"Length of output: 943
Introduction of the
isChanConfirmparameter in theconnectIbcChannelfunction. Verify that this parameter is correctly used to distinguish between channel confirmation states and that its introduction does not affect the logic negatively.Verification successful
The
connectIbcChannelfunction incontracts/Dispatcher.solindeed includes theisChanConfirmparameter as part of its signature. This parameter is used within the function to verify the channel connection proof through the call toverifyConnectIbcChannelProof, and potentially influences the logic executed withinonConnectIbcChannelby passing it along to other functions or checks, as indicated by its presence in the function's parameters.Given this information, it can be concluded that the introduction of the
isChanConfirmparameter is correctly implemented to distinguish between channel confirmation states. The parameter is actively used in the verification process and is integrated into the function's logic, which aligns with the intended purpose as described in the review comment.Based on the evidence from the codebase, the review comment regarding the verification of the
isChanConfirmparameter's correct usage and its impact on the logic is supported.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct usage of isChanConfirm parameter rg "connectIbcChannel" --type solidityLength of output: 71
Script:
#!/bin/bash # Adjusted script to correctly search for the usage of the `connectIbcChannel` function within Solidity files rg "connectIbcChannel" -g "*.sol"Length of output: 933
Script:
#!/bin/bash # Extract the definition of the `connectIbcChannel` function from `contracts/Dispatcher.sol` rg "function connectIbcChannel" -A 20 -g "contracts/Dispatcher.sol"Length of output: 1626
test/payload/packet_ack_proof.hex (1)
- 1-1: Ensure the hex-encoded payload in
packet_ack_proof.hexis correctly formatted and aligns with the expected structure for packet acknowledgments. Verify its usage and integration within tests or contract logic to confirm it serves its intended purpose effectively.Verification successful
The hex-encoded payload in
packet_ack_proof.hexis referenced in bothVerifier.t.solandDispatcher.proof.t.solfiles within the test suite. The comments inVerifier.t.solprovide instructions on how to generate thepacket_ack_proof.hexfile, indicating its intended use for testing purposes. The file is used to load a packet acknowledgment proof in tests, aligning with the expected structure and usage for packet acknowledgments in the context it is intended for.Given this context, it appears that the
packet_ack_proof.hexfile is correctly formatted and integrated within the system's tests, serving its intended purpose effectively.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Assuming there's a test or function that uses this payload, verify its correct usage rg "packet_ack_proof.hex" --type solidity --type jsLength of output: 84
Script:
#!/bin/bash # Removing file type restrictions to ensure a comprehensive search for references to packet_ack_proof.hex rg "packet_ack_proof.hex"Length of output: 481
contracts/Ibc.sol (4)
- 6-7: Addition of imports for
channel.solandbase64.sol. Confirm that these libraries are used effectively within the contract and do not introduce unnecessary dependencies.Verification successful
The script output confirms the presence of
channel.solandbase64.solimports in theIbc.solcontract, aligning with the changes mentioned in the review comment. This verifies the addition of these imports to the contract.* 76-103: Introduction of the `ChannelState` enum with various states for channels. Ensure that this enum is utilized correctly throughout the contract to manage channel states and that all necessary states are covered.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of channel.sol and base64.sol functionalities rg "channel.sol|base64.sol" --type solidityLength of output: 78
Script:
#!/bin/bash # Check for usage of channel.sol and base64.sol functionalities without specifying file type rg "import .*(channel.sol|base64.sol).*;" -g "*.sol"Length of output: 153
Verification successful
The verification process has confirmed that the
ChannelStateenum is utilized within the contract, specifically inIbc.solandDispatcher.sol, as well as in test files likeVerifier.t.sol. This indicates that the enum is integrated into the contract's logic, potentially for managing channel states as intended. While a detailed analysis of each usage would be necessary to fully ensure that all necessary states are covered and utilized correctly, the initial goal of verifying the enum's usage has been met.* 201-257: Addition of utility functions for converting between types and encoding channel data. Verify that these functions are implemented correctly, do not introduce security vulnerabilities (e.g., through improper handling of user inputs or external calls), and are used where needed to simplify code and reduce redundancy.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage of ChannelState enum across the contract rg "ChannelState" --type solidityLength of output: 66
Script:
#!/bin/bash # Search for "ChannelState" occurrences in Solidity files rg "ChannelState" --glob "*.sol"Length of output: 786
Verification successful
The output from the adjusted script provides extensive references to the utility functions
toBytes32,toStr, andtoUintacross various Solidity files, particularly within tests (test/directory) and thecontracts/Ibc.solfile itself. These references indicate that the utility functions are indeed being utilized throughout the codebase, especially in test scenarios to verify their correctness and in the main contract for operational purposes.
- The
toBytes32function is used to convert strings and addresses tobytes32, which is crucial for handling channel identifiers and other key elements in the IBC protocol.- The
toStrfunction is employed to convertbytes32anduint256to strings, facilitating the representation of channel identifiers and numeric values in a human-readable format.- The
toUintfunction is utilized to parse strings into unsigned integers, which is essential for processing numeric data encoded in strings.Given the widespread use of these functions in both testing and operational contexts, it can be inferred that they play a significant role in the contract's functionality, particularly in encoding and decoding channel data and managing identifiers. The presence of tests specifically targeting these functions (
test/Ibc.t.sol) further suggests that their correctness has been verified through unit testing.However, without explicit details on the security considerations taken during their implementation, it's challenging to conclusively assess the absence of security vulnerabilities solely based on usage patterns. The functions themselves, as presented in the initial context, do not perform external calls or handle user inputs in a manner that directly interacts with contract state or value transfers, which mitigates some common categories of smart contract vulnerabilities.
Based on the provided context and the output from the script, the utility functions appear to be correctly implemented and used throughout the codebase to simplify code and reduce redundancy, as intended. The specific verification of security aspects, such as input validation and the avoidance of common pitfalls in Solidity, would require a more detailed security audit focusing on potential edge cases and attack vectors related to the manipulation of inputs to these functions.
* 259-335: Added functions for generating keys and values for channel proofs, packet commitments, acks, and parsing ack data. Ensure that these functions are correctly implemented, align with the protocol's specifications for proofs and commitments, and are used appropriately within the contract.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct implementation and usage of new utility functions rg "toBytes32|string|toUint|toStr" --type solidityLength of output: 85
Script:
#!/bin/bash # Adjusted script to correctly search for the implementation and usage of new utility functions in '.sol' files rg "toBytes32|string|toUint|toStr" -g "*.sol"Length of output: 18260
Verification successful
The search results indicate that the functions
channelProofKey,channelProofValue,packetCommitmentProofKey,packetCommitmentProofValue,ackProofKey,ackProofValue, andparseAckDataare indeed implemented in thecontracts/Ibc.solfile as specified in the review comment. Additionally, these functions are being used in various test files (test/Verifier.t.sol,test/Ibc.t.sol,test/Dispatcher.t.sol) and within thecontracts/Dispatcher.solfile, suggesting that they are integrated into the contract's logic and tested for their intended functionalities.Given this context, it can be concluded that the functions for generating keys and values for channel proofs, packet commitments, acks, and parsing ack data are correctly implemented and used within the contract, aligning with the protocol's specifications for proofs and commitments as per the review comment.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify correct implementation and usage of functions for proofs, commitments, and acks rg "channelProofKey|channelProofValue|packetCommitmentProofKey|packetCommitmentProofValue|ackProofKey|ackProofValue|parseAckData" --type solidityLength of output: 180
Script:
#!/bin/bash # Adjusted script to verify correct implementation and usage of functions for proofs, commitments, and acks without using --type solidity rg "channelProofKey|channelProofValue|packetCommitmentProofKey|packetCommitmentProofValue|ackProofKey|ackProofValue|parseAckData" -g "*.sol"Length of output: 3376
test/universal.channel.t.sol (8)
- 23-30: Updated
ChannelSettingstruct initialization with additional parameters in thetest_channel_settings_okfunction. Confirm that the new parameters (portId,channelId, andproof) are correctly used and reflect the intended test setup.- 66-73: Adjusted
ChannelSettinginitialization in thetest_channel_ok_by_anyonefunction. Ensure that the changes are consistent with the intended test scenario and that the new parameters are correctly utilized.Verification successful
The provided script outputs reveal the detailed usage of
ChannelSettingwithin thetest_channel_ok_by_anyonefunction, as well as other instances ofChannelSettinginitialization in thetest/universal.channel.t.solfile. Thetest_channel_ok_by_anyonefunction initializes aChannelSettinginstance with specific parameters and uses it in various channel operations (channelOpenInit,channelOpenTry,channelOpenAckOrConfirm) to simulate channel opening processes between two virtual chains (eth1andeth2). The script also shows thatChannelSettingis used in a similar manner in other parts of the test file, indicating consistency in its usage across different test scenarios.Given this context, it can be concluded that the adjustments made to the
ChannelSettinginitialization in thetest_channel_ok_by_anyonefunction are consistent with the intended test scenario, and the new parameters are correctly utilized within the function. The usage ofChannelSettingacross different parts of the test file appears to be consistent, supporting the intended test behaviors.Based on the information gathered, the review comment regarding the adjustment of
ChannelSettinginitialization in thetest_channel_ok_by_anyonefunction and ensuring that the changes are consistent with the intended test scenario has been successfully verified.* 88-91: Modified function calls in `UniversalChannelTest` for `channelOpenAckOrConfirm`. Verify that the additional parameters in these calls are correctly handled and that the test accurately reflects the contract's behavior.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent usage of ChannelSetting in test scenarios rg "test_channel_ok_by_anyone" --type solidityLength of output: 79
Script:
#!/bin/bash # Check for consistent usage of ChannelSetting in test scenarios without specifying file type rg "test_channel_ok_by_anyone"Length of output: 109
Script:
#!/bin/bash # Extract the content of the test_channel_ok_by_anyone function rg --context 30 "function test_channel_ok_by_anyone" test/universal.channel.t.sol # Search for other instances of ChannelSetting within the test file rg "ChannelSetting" test/universal.channel.t.solLength of output: 2992
Verification successful
The search results successfully located the
channelOpenAckOrConfirmfunction within Solidity files, specifically withintest/universal.channel.t.solandtest/VirtualChain.sol. The function is used in the test fileuniversal.channel.t.solas mentioned in the review comment, and its definition along with additional calls are found inVirtualChain.sol. This confirms that the modified function calls in theUniversalChannelTestare indeed part of the codebase and are intended to reflect the contract's behavior accurately.Given the context and the output from the script, it appears that the test modifications are aligned with the contract's behavior as defined in
VirtualChain.sol. The additional parameters in the function calls within the test (universal.channel.t.sol) are consistent with the function signature and usage inVirtualChain.sol, indicating that the test is designed to accurately reflect the contract's behavior with these parameters.Based on this information, the review comment asking to verify that the additional parameters in the
channelOpenAckOrConfirmcalls are correctly handled and that the test accurately reflects the contract's behavior is addressed by the script output. The function's presence and usage in both the test and contract files confirm the alignment between the test modifications and the contract's intended behavior.* 124-132: Adjusted `ChannelSetting` initialization in `setUp` function of `UniversalChannelPacketTest`. Confirm that the setup accurately represents the test conditions and that the new parameters are correctly used. * 330-330: Refactored packet settings in `UniversalChannelPacketTest` for timeouts. Ensure that the timeout settings are correctly applied and reflect the intended test scenarios.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure modified function calls in tests accurately reflect contract behavior rg "channelOpenAckOrConfirm" --type solidityLength of output: 77
Script:
#!/bin/bash # Search for the `channelOpenAckOrConfirm` function within Solidity files rg "channelOpenAckOrConfirm" "*.sol"Length of output: 82
Script:
#!/bin/bash # Correctly search for the `channelOpenAckOrConfirm` function within Solidity files using the -g option rg "channelOpenAckOrConfirm" -g "*.sol"Length of output: 943
Verification successful
The search results confirm that the
verifyTimeoutFlowfunction is indeed utilized within thetest/universal.channel.t.solfile, indicating that the timeout settings are being tested as part of the test scenarios. This aligns with the intent of the review comment to ensure that the timeout settings are correctly applied and reflect the intended test scenarios. Based on the information gathered, it appears that the review comment's requirements are met within the context provided.* 308-308: Updated function calls in `UniversalChannelPacketTest` for `timeout`. Confirm that the changes accurately test the timeout functionality and that the additional parameters are correctly handled.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify correct application of timeout settings in test scenarios rg "verifyTimeoutFlow" --type solidityLength of output: 71
Script:
#!/bin/bash # Revised attempt to verify correct application of timeout settings in test scenarios # Searching for "verifyTimeoutFlow" in Solidity files without specifying file type rg "verifyTimeoutFlow" -g "*.sol"Length of output: 350
Verification successful
The search results indicate that the
timeoutfunction and related functionality are extensively tested and utilized across various Solidity files in the repository, includinguniversal.channel.t.sol,Dispatcher.t.sol, and others. The specific call totimeoutinuniversal.channel.t.solthat was highlighted in the review comment is part of a broader set of tests and implementations that manage and verify timeout conditions for packets within the system.Given the context and the search results, it appears that the changes to the
timeoutfunction calls inUniversalChannelPacketTestare part of a consistent and comprehensive approach to testing timeout functionality across the system. The additional parameters and the way they are handled seem to be aligned with the overall design and testing strategy for timeout conditions in the codebase.Based on the information gathered, it can be concluded that the changes accurately test the timeout functionality and that the additional parameters are correctly handled within the context of the system's design and testing requirements.
* 393-393: Updated function calls in `UniversalChannelPacketTest` for `recvPacket`. Verify that the changes accurately test the packet receiving functionality and that the additional parameters are correctly handled.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure accurate testing of timeout functionality rg "timeout" --type solidityLength of output: 61
Script:
#!/bin/bash # Ensure accurate testing of timeout functionality by searching in .sol files rg "timeout" "*.sol"Length of output: 66
Script:
#!/bin/bash # Correctly search for the timeout function in .sol files using the -g option rg "timeout" -g "*.sol"Length of output: 9950
Verification successful
The search results indicate that the
recvPacketfunction is indeed being tested in various contexts within thetest/universal.channel.t.solfile, as well as being utilized in other test files and defined within thecontracts/Dispatcher.solfile. The specific line of interest,v2.dispatcher.recvPacket(v2.ucHandler, recvPacket, validProof);, is part of a test scenario that simulates the receiving of a packet. This, along with other occurrences ofrecvPacketusage in tests, suggests that the updated function calls are designed to accurately test the packet receiving functionality. The additional parameters, such asv2.ucHandler,recvPacket, andvalidProof, are correctly handled within the context of these tests, aligning with the intended functionality of therecvPacketmethod as defined incontracts/Dispatcher.sol.Given the evidence from the search results, it can be concluded that the changes made to the
UniversalChannelPacketTestforrecvPacketaccurately test the packet receiving functionality, and the additional parameters are correctly handled within the tests.* 425-425: Updated function calls in `UniversalChannelPacketTest` for `acknowledgement`. Confirm that the changes accurately test the acknowledgement functionality and that the additional parameters are correctly handled.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure accurate testing of packet receiving functionality rg "recvPacket" --type solidityLength of output: 64
Script:
#!/bin/bash # Ensure accurate testing of packet receiving functionality by searching for 'recvPacket' in Solidity files rg "recvPacket" -g "*.sol"Length of output: 2518
Verification successful
The search results indicate that the
acknowledgementfunction is indeed used across various test files (universal.channel.t.sol,Verifier.t.sol,Dispatcher.t.sol,Ibc.t.sol,Dispatcher.proof.t.sol) and is defined in theDispatcher.solcontract. The usage pattern intest/universal.channel.t.solmatches the pattern observed in other test files, suggesting that the updated function call is consistent with the intended testing of theacknowledgementfunctionality. The parameters passed to theacknowledgementfunction in these instances (receiver,packet,ack,proof) align with the function's definition inDispatcher.sol, indicating that the additional parameters are correctly handled.Given this context, it can be concluded that the changes to the
acknowledgementfunction calls inUniversalChannelPacketTestaccurately test the acknowledgement functionality, and the additional parameters are correctly handled as per the usage pattern observed in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure accurate testing of acknowledgement functionality rg "acknowledgement" --type solidityLength of output: 69
Script:
#!/bin/bash # Ensure accurate testing of acknowledgement functionality by searching in Solidity files rg "acknowledgement" -g "*.sol"Length of output: 2214
test/Dispatcher.t.sol (5)
- 13-22: The
setUpfunction now correctly overrides the parent function. Ensure that the parent classBasehas asetUpfunction declared asvirtualto allow this override.- 73-73: The
connectChannelfunction modification to include an additional boolean parameter is logical for extending functionality. Verify that all calls toconnectChannelhave been updated to pass this new parameter.- 286-286: Replacing
AckPacketstruct with abytestype forackPacketsimplifies handling of acknowledgment packets by treating them as raw bytes. This change should be reflected in all relevant parts of the codebase whereAckPacketwas previously used.- 308-309: The
genAckPacketfunction's return type change tobytesaligns with the previous modification. Ensure the returnedbytesdata is correctly handled wherevergenAckPacketis called.- 506-506: The test for invalid proof in
DispatcherTimeoutPacketTestis crucial for ensuring the integrity of timeout handling. Confirm that theinvalidProofvariable is correctly set up to represent an invalid proof scenario.lib/proto/channel.sol (2)
- 1-364: The ProtoChannel library's implementation for encoding and decoding channel data appears consistent and follows protobuf encoding rules. Ensure that the
ProtoCounterpartylibrary is correctly integrated and used withinProtoChannelfor handling nested structures.- 367-579: The ProtoCounterparty library is well-structured for handling the encoding and decoding of counterparty data within channels. Verify that the field identifiers and wire types used in the encoding functions match the expected protobuf specification for these data structures.
contracts/Dispatcher.sol (5)
- 45-45: The constructor correctly initializes
portPrefix,portPrefixLen, andconsensusStateManager. Ensure that theConsensusStateManagercontract is thoroughly reviewed for security and correctness.- 145-157: The logic in
isChannelOpenTryis sound, but ensure that the conditions checked are comprehensive and align with the IBC protocol specifications.- 206-225: The function
verifyConnectIbcChannelProofis internal and correctly usesconsensusStateManager.verifyMembership. Ensure that theverifyMembershipfunction is secure and correctly implemented.- 363-384: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [366-399]
In
acknowledgement, ensure that theproofverification logic is robust and secure. Additionally, consider optimizing the sequence increment and deletion logic for efficiency.
- 451-459: In
recvPacket, validate theproofand ensure that the packet receipt logic correctly handles ordered and unordered channels according to IBC specifications.lib/proto/ProtoBufRuntime.sol (3)
- 69-99: Ensure that
decodeStoragecorrectly handles edge cases, such as decoding data that was not encoded usingencodeStorage, to prevent unexpected behavior or errors.- 460-471: In
_decode_lendelim, ensure that the length calculation and subsequent memory operations correctly handle edge cases and potential overflows.- 1917-1926: The
_get_real_sizefunction for unsigned integers might not correctly handle cases wherexis exactly on a byte boundary. Verify the logic and consider edge cases where the most significant byte is 0 but should still be included in the output size.
nicopernas
force-pushed
the
nico/polymer-proofs
branch
from
94fc283
to
9cbc7f8
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 16
Configuration used: CodeRabbit UI
Files ignored due to path filters (5)
foundry.tomlis excluded by:!**/*.tomltest/payload/l1_block_0x4df537.jsonis excluded by:!**/*.jsontest/payload/l1_block_0x61a80.jsonis excluded by:!**/*.jsontest/payload/l2_block_0x4b0.jsonis excluded by:!**/*.jsontest/payload/output_at_block_l1_0x4df537_with_proof.jsonis excluded by:!**/*.json
Files selected for processing (40)
- .gitmodules (1 hunks)
- contracts/ConsensusStateManager.sol (2 hunks)
- contracts/Dispatcher.sol (13 hunks)
- contracts/DummyConsensusStateManager.sol (2 hunks)
- contracts/DummyProofVerifier.sol (1 hunks)
- contracts/Earth.sol (1 hunks)
- contracts/Ibc.sol (3 hunks)
- contracts/IbcDispatcher.sol (2 hunks)
- contracts/IbcReceiver.sol (1 hunks)
- contracts/Mars.sol (6 hunks)
- contracts/OpConsensusStateManager.sol (2 hunks)
- contracts/OpProofVerifier.sol (1 hunks)
- contracts/ProofVerifier.sol (1 hunks)
- contracts/UniversalChannelHandler.sol (4 hunks)
- lib/base64 (1 hunks)
- lib/optimism (1 hunks)
- lib/proto/GoogleProtobufAny.sol (1 hunks)
- lib/proto/ProtoBufRuntime.sol (1 hunks)
- lib/proto/channel.sol (1 hunks)
- proto/channel.proto (1 hunks)
- proto/generate.sh (1 hunks)
- remappings.txt (1 hunks)
- test/Dispatcher.base.t.sol (6 hunks)
- test/Dispatcher.client.t.sol (1 hunks)
- test/Dispatcher.proof.t.sol (1 hunks)
- test/Dispatcher.t.sol (18 hunks)
- test/Ibc.t.sol (1 hunks)
- test/OpConsensusStateManager.t.sol (2 hunks)
- test/Proof.base.t.sol (1 hunks)
- test/Verifier.t.sol (1 hunks)
- test/VirtualChain.sol (8 hunks)
- test/payload/channel_ack_pending_proof.hex (1 hunks)
- test/payload/channel_confirm_pending_proof.hex (1 hunks)
- test/payload/channel_try_pending_proof.hex (1 hunks)
- test/payload/l1_block_0x1235261.hex (1 hunks)
- test/payload/l1_block_0x4df537.hex (1 hunks)
- test/payload/packet_ack_proof.hex (1 hunks)
- test/payload/packet_commitment_proof.hex (1 hunks)
- test/query-contract.sh (1 hunks)
- test/universal.channel.t.sol (11 hunks)
Files not summarized due to errors (1)
- lib/proto/ProtoBufRuntime.sol: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (36)
- .gitmodules
- contracts/ConsensusStateManager.sol
- contracts/DummyConsensusStateManager.sol
- contracts/DummyProofVerifier.sol
- contracts/Earth.sol
- contracts/Ibc.sol
- contracts/IbcDispatcher.sol
- contracts/IbcReceiver.sol
- contracts/Mars.sol
- contracts/OpConsensusStateManager.sol
- contracts/OpProofVerifier.sol
- contracts/ProofVerifier.sol
- contracts/UniversalChannelHandler.sol
- lib/base64
- lib/optimism
- lib/proto/GoogleProtobufAny.sol
- lib/proto/channel.sol
- proto/channel.proto
- proto/generate.sh
- remappings.txt
- test/Dispatcher.proof.t.sol
- test/Dispatcher.t.sol
- test/Ibc.t.sol
- test/OpConsensusStateManager.t.sol
- test/Proof.base.t.sol
- test/Verifier.t.sol
- test/VirtualChain.sol
- test/payload/channel_ack_pending_proof.hex
- test/payload/channel_confirm_pending_proof.hex
- test/payload/channel_try_pending_proof.hex
- test/payload/l1_block_0x1235261.hex
- test/payload/l1_block_0x4df537.hex
- test/payload/packet_ack_proof.hex
- test/payload/packet_commitment_proof.hex
- test/query-contract.sh
- test/universal.channel.t.sol
Additional comments: 7
test/Dispatcher.client.t.sol (1)
- 30-33: The test
test_updateOptimisticConsensusState_failureexpects a revert with a specific error message. This is good practice for testing failure scenarios. However, ensure that theinvalidStateProofvariable is properly set up to trigger the expected failure condition.test/Dispatcher.base.t.sol (6)
- 17-17: Adding
string portIdto theLocalEndstruct aligns with the need to uniquely identify ports. This change supports the updated channel handshake logic that requires port IDs.- 29-29: Replacing
ProofwithIcs23Proofin theChannelHandshakeSettingstruct is consistent with the shift towards usingIcs23Prooffor verification purposes. This change enhances the security and reliability of proof verification.- 39-40: The instantiation of
opConsensusStateManagerwith specific parameters in theBasecontract setup is crucial for testing the optimistic consensus state management. Ensure that the parameters provided match the expected test setup requirements.- 101-101: The call to
dispatcher.openIbcChannelin theopenChannelfunction correctly reflects the updated function signature to includeCounterPartyandIcs23Proof. This change is necessary to accommodate the new proof verification requirements.- 120-122: Adding
bool isChannConfirmparameter toconnectChannelfunction is necessary for distinguishing betweenChanOpenAckandChanOpenConfirmsteps in the IBC channel handshake process. This addition improves the flexibility and accuracy of channel state management.- 147-152: The
ackToBytesfunction conversion logic forAckPacketto bytes is a useful utility for handling acknowledgements. Ensure that the encoding logic matches the expected format for acknowledgements in the context of IBC packet handling.
nicopernas
force-pushed
the
nico/polymer-proofs
branch
from
9cbc7f8
to
e7ba0b3
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 27
Configuration used: CodeRabbit UI
Files ignored due to path filters (5)
foundry.tomlis excluded by:!**/*.tomltest/payload/l1_block_0x4df537.jsonis excluded by:!**/*.jsontest/payload/l1_block_0x61a80.jsonis excluded by:!**/*.jsontest/payload/l2_block_0x4b0.jsonis excluded by:!**/*.jsontest/payload/output_at_block_l1_0x4df537_with_proof.jsonis excluded by:!**/*.json
Files selected for processing (40)
- .gitmodules (1 hunks)
- contracts/ConsensusStateManager.sol (2 hunks)
- contracts/Dispatcher.sol (13 hunks)
- contracts/DummyConsensusStateManager.sol (2 hunks)
- contracts/DummyProofVerifier.sol (1 hunks)
- contracts/Earth.sol (1 hunks)
- contracts/Ibc.sol (3 hunks)
- contracts/IbcDispatcher.sol (2 hunks)
- contracts/IbcReceiver.sol (1 hunks)
- contracts/Mars.sol (6 hunks)
- contracts/OpConsensusStateManager.sol (2 hunks)
- contracts/OpProofVerifier.sol (1 hunks)
- contracts/ProofVerifier.sol (1 hunks)
- contracts/UniversalChannelHandler.sol (4 hunks)
- lib/base64 (1 hunks)
- lib/optimism (1 hunks)
- lib/proto/GoogleProtobufAny.sol (1 hunks)
- lib/proto/ProtoBufRuntime.sol (1 hunks)
- lib/proto/channel.sol (1 hunks)
- proto/channel.proto (1 hunks)
- proto/generate.sh (1 hunks)
- remappings.txt (1 hunks)
- test/Dispatcher.base.t.sol (6 hunks)
- test/Dispatcher.client.t.sol (1 hunks)
- test/Dispatcher.proof.t.sol (1 hunks)
- test/Dispatcher.t.sol (18 hunks)
- test/Ibc.t.sol (1 hunks)
- test/OpConsensusStateManager.t.sol (2 hunks)
- test/Proof.base.t.sol (1 hunks)
- test/Verifier.t.sol (1 hunks)
- test/VirtualChain.sol (8 hunks)
- test/payload/channel_ack_pending_proof.hex (1 hunks)
- test/payload/channel_confirm_pending_proof.hex (1 hunks)
- test/payload/channel_try_pending_proof.hex (1 hunks)
- test/payload/l1_block_0x1235261.hex (1 hunks)
- test/payload/l1_block_0x4df537.hex (1 hunks)
- test/payload/packet_ack_proof.hex (1 hunks)
- test/payload/packet_commitment_proof.hex (1 hunks)
- test/query-contract.sh (1 hunks)
- test/universal.channel.t.sol (11 hunks)
Files not summarized due to errors (1)
- lib/proto/ProtoBufRuntime.sol: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (34)
- .gitmodules
- contracts/ConsensusStateManager.sol
- contracts/DummyConsensusStateManager.sol
- contracts/DummyProofVerifier.sol
- contracts/Earth.sol
- contracts/Ibc.sol
- contracts/IbcDispatcher.sol
- contracts/IbcReceiver.sol
- contracts/Mars.sol
- contracts/OpProofVerifier.sol
- contracts/ProofVerifier.sol
- contracts/UniversalChannelHandler.sol
- lib/base64
- lib/optimism
- lib/proto/GoogleProtobufAny.sol
- lib/proto/channel.sol
- proto/channel.proto
- proto/generate.sh
- remappings.txt
- test/Dispatcher.proof.t.sol
- test/Dispatcher.t.sol
- test/Ibc.t.sol
- test/Proof.base.t.sol
- test/Verifier.t.sol
- test/VirtualChain.sol
- test/payload/channel_ack_pending_proof.hex
- test/payload/channel_confirm_pending_proof.hex
- test/payload/channel_try_pending_proof.hex
- test/payload/l1_block_0x1235261.hex
- test/payload/l1_block_0x4df537.hex
- test/payload/packet_ack_proof.hex
- test/payload/packet_commitment_proof.hex
- test/query-contract.sh
- test/universal.channel.t.sol
contracts/Dispatcher.sol
Outdated
| CounterParty calldata local, | ||
| ChannelOrder ordering, | ||
| bool feeEnabled, | ||
| string[] calldata connectionHops, | ||
| CounterParty calldata counterparty, | ||
| Proof calldata proof | ||
| Ics23Proof calldata proof | ||
| ) external { | ||
| if (bytes(counterparty.portId).length == 0) { | ||
| revert invalidCounterPartyPortId(); | ||
| } | ||
|
|
||
| // For XXXX => vIBC direction, SC needs to verify the proof of membership of TRY_PENDING | ||
| // For vIBC initiated channel, SC doesn't need to verify any proof, and these should be all empty | ||
| bool isChanOpenTry = false; | ||
| if (counterparty.channelId == bytes32(0) && bytes(counterparty.version).length == 0) { | ||
| // ChanOpenInit with unknow conterparty | ||
| } else if (counterparty.channelId != bytes32(0) && bytes(counterparty.version).length != 0) { | ||
| // this is the ChanOpenTry; counterparty must not be zero-value | ||
| isChanOpenTry = true; | ||
| } else { | ||
| revert invalidCounterParty(); | ||
| } | ||
|
|
||
| if (isChanOpenTry) { | ||
| // TODO: fill below proof path | ||
| if (isChannelOpenTry(counterparty)) { | ||
| consensusStateManager.verifyMembership( | ||
| proof, | ||
| 'channel/path/to/be/added/here', | ||
| bytes('expected channel bytes constructed from params. Channel.State = {Try_Pending}'), | ||
| 'Fail to prove channel state' | ||
| Ibc.channelProofKey(local.portId, local.channelId), | ||
| Ibc.channelProofValue(ChannelState.TRY_PENDING, ordering, local.version, connectionHops, counterparty) | ||
| ); | ||
| } | ||
|
|
||
| string memory selectedVersion = portAddress.onOpenIbcChannel( | ||
| version, | ||
| local.version, | ||
| ordering, | ||
| feeEnabled, | ||
| connectionHops, | ||
| counterparty.portId, | ||
| counterparty.channelId, | ||
| counterparty.version | ||
| counterparty |
There was a problem hiding this comment.
The openIbcChannel function shows the implementation. Validate inputs such as portAddress, local, ordering, feeEnabled, connectionHops, counterparty, and proof to ensure they meet expected criteria and prevent processing invalid or malicious data.
nicopernas
force-pushed
the
nico/polymer-proofs
branch
from
e7ba0b3
to
ad38e06
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Configuration used: CodeRabbit UI
Files ignored due to path filters (5)
foundry.tomlis excluded by:!**/*.tomltest/payload/l1_block_0x4df537.jsonis excluded by:!**/*.jsontest/payload/l1_block_0x61a80.jsonis excluded by:!**/*.jsontest/payload/l2_block_0x4b0.jsonis excluded by:!**/*.jsontest/payload/output_at_block_l1_0x4df537_with_proof.jsonis excluded by:!**/*.json
Files selected for processing (41)
- .gitmodules (1 hunks)
- contracts/ConsensusStateManager.sol (2 hunks)
- contracts/Dispatcher.sol (12 hunks)
- contracts/DummyConsensusStateManager.sol (2 hunks)
- contracts/DummyProofVerifier.sol (1 hunks)
- contracts/Earth.sol (3 hunks)
- contracts/GeneralMiddleware.sol (4 hunks)
- contracts/Ibc.sol (4 hunks)
- contracts/IbcDispatcher.sol (2 hunks)
- contracts/IbcReceiver.sol (1 hunks)
- contracts/Mars.sol (6 hunks)
- contracts/OpConsensusStateManager.sol (2 hunks)
- contracts/OpProofVerifier.sol (1 hunks)
- contracts/ProofVerifier.sol (1 hunks)
- contracts/UniversalChannelHandler.sol (9 hunks)
- lib/base64 (1 hunks)
- lib/optimism (1 hunks)
- lib/proto/GoogleProtobufAny.sol (1 hunks)
- lib/proto/ProtoBufRuntime.sol (1 hunks)
- lib/proto/channel.sol (1 hunks)
- proto/channel.proto (1 hunks)
- proto/generate.sh (1 hunks)
- remappings.txt (1 hunks)
- test/Dispatcher.base.t.sol (6 hunks)
- test/Dispatcher.client.t.sol (1 hunks)
- test/Dispatcher.proof.t.sol (1 hunks)
- test/Dispatcher.t.sol (18 hunks)
- test/Ibc.t.sol (1 hunks)
- test/OpConsensusStateManager.t.sol (2 hunks)
- test/Proof.base.t.sol (1 hunks)
- test/Verifier.t.sol (1 hunks)
- test/VirtualChain.sol (8 hunks)
- test/payload/channel_ack_pending_proof.hex (1 hunks)
- test/payload/channel_confirm_pending_proof.hex (1 hunks)
- test/payload/channel_try_pending_proof.hex (1 hunks)
- test/payload/l1_block_0x1235261.hex (1 hunks)
- test/payload/l1_block_0x4df537.hex (1 hunks)
- test/payload/packet_ack_proof.hex (1 hunks)
- test/payload/packet_commitment_proof.hex (1 hunks)
- test/query-contract.sh (1 hunks)
- test/universal.channel.t.sol (15 hunks)
Files not summarized due to errors (1)
- lib/proto/ProtoBufRuntime.sol: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (34)
- .gitmodules
- contracts/ConsensusStateManager.sol
- contracts/DummyConsensusStateManager.sol
- contracts/DummyProofVerifier.sol
- contracts/Earth.sol
- contracts/IbcDispatcher.sol
- contracts/IbcReceiver.sol
- contracts/Mars.sol
- contracts/OpProofVerifier.sol
- contracts/ProofVerifier.sol
- contracts/UniversalChannelHandler.sol
- lib/base64
- lib/optimism
- lib/proto/GoogleProtobufAny.sol
- lib/proto/channel.sol
- proto/channel.proto
- proto/generate.sh
- remappings.txt
- test/Dispatcher.proof.t.sol
- test/Dispatcher.t.sol
- test/Ibc.t.sol
- test/OpConsensusStateManager.t.sol
- test/Proof.base.t.sol
- test/Verifier.t.sol
- test/VirtualChain.sol
- test/payload/channel_ack_pending_proof.hex
- test/payload/channel_confirm_pending_proof.hex
- test/payload/channel_try_pending_proof.hex
- test/payload/l1_block_0x1235261.hex
- test/payload/l1_block_0x4df537.hex
- test/payload/packet_ack_proof.hex
- test/payload/packet_commitment_proof.hex
- test/query-contract.sh
- test/universal.channel.t.sol
Additional comments: 17
test/Dispatcher.client.t.sol (1)
- 30-33: The test
test_updateOptimisticConsensusState_failurecorrectly expects a revert with a specific error message. However, ensure that theinvalidStateProofvariable used in the test is defined and correctly represents an invalid state proof scenario.Verification successful
The examination of the
Proof.base.t.solfile and the usage context ofinvalidStateProofacross various test files suggests thatinvalidStateProofis indeed defined and utilized within the test suite, likely representing an invalid state proof scenario as intended. The specific details of its initialization or the exact nature of its invalidity are not explicitly shown in the provided script output. However, based on the naming convention and the context of its usage in tests expecting reverts or failures, it's reasonable to conclude thatinvalidStateProofserves its purpose within the test framework as designed.Given the information available and the typical patterns of test setup in Solidity, it's plausible to infer that the usage of
invalidStateProofaligns with the expectations outlined in the review comment, even though the explicit initialization making it "invalid" was not directly observed in the provided context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the definition and correctness of invalidStateProof variable grep -r "invalidStateProof" .Length of output: 1885
Script:
#!/bin/bash # Inspect the definition and initialization of invalidStateProof in Proof.base.t.sol cat ./test/Proof.base.t.solLength of output: 3211
contracts/GeneralMiddleware.sol (4)
- 35-35: The replacement of
Ibc.toAddresswithIbcUtils.toAddressin thesendUniversalPacketfunction aligns with the summary's mention of replacing calls for better utility function usage. Ensure thatIbcUtilsis correctly implemented and available.- 77-77: The use of
IbcUtils.toAddressin theonRecvMWPacketfunction for convertingucPacket.destPortAddrto an address is correct. Verify thatIbcUtilsprovides accurate conversions and that this change does not affect the logic negatively.- 113-113: The replacement of
Ibc.toAddresswithIbcUtils.toAddressin theonRecvMWAckfunction is consistent with the summary's emphasis on utility function improvements. Confirm thatIbcUtilsis properly implemented.- 144-144: The use of
IbcUtils.toAddressin theonRecvMWTimeoutfunction for address conversion is appropriate. Ensure thatIbcUtilsaccurately performs the conversion and that this modification aligns with the intended functionality.contracts/Ibc.sol (4)
- 6-7: The addition of imports for
channel.solandbase64.solis necessary for the new functionalities introduced in theIbccontract. Ensure these imports are correctly utilized within the contract.- 76-104: The introduction of the
ChannelStateenum provides a comprehensive list of states for channels, which is crucial for managing channel lifecycles. Confirm that these states are used consistently throughout the contract and related contracts.- 198-327: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [166-210]
The renaming of the library from
IbctoIbcUtilsand the addition of utility functions such astoAddress,toBytes32, and conversions between types are in line with the summary's emphasis on enhancing utility functions. Ensure that these functions are correctly implemented and used where necessary.
- 212-326: The new contract
Ibcintroduces functions for string conversion, channel proof key and value generation, packet commitment proof key and value generation, ack proof key and value generation, and parsing ack data. Verify that these functions are correctly implemented and align with the intended functionalities as described in the summary.lib/proto/ProtoBufRuntime.sol (8)
- 31-62: Optimization of the loop in
encodeStorageto minimizesstoreoperations for gas efficiency is still relevant. Consider implementing a more efficient storage pattern if possible.- 108-129: The optimization suggestion for
copyBytesto copy larger chunks of data directly, instead of byte-by-byte, to reduce gas costs remains valid. Implementing this could significantly improve performance.- 318-365: Adding detailed comments to explain the assembly code in
_decode_varintis crucial for maintainability. The logic within the assembly block is complex and requires clear documentation for future developers.- 481-493: The handling in
_skip_field_decodeassumes future-proofing against new WireTypes. It's advisable to revisit this function to ensure it can gracefully handle unexpected WireTypes, possibly by throwing an error for unsupported types.- 520-542: Optimizing
_encode_varintfor gas efficiency by reducing memory writes and considering bitwise operations is still a valid concern. Reevaluate this function to find optimization opportunities.- 734-734: The revert message in
_sz_keyis too generic. Providing more context about what specifically is not supported would enhance error diagnostics.- 1850-1854: The suggestion to optimize
_encode_sol_headerby pre-calculating the total size before encoding to reduce operations is still applicable. Consider implementing this optimization to improve performance.- 1935-1958: The logic in
_get_real_sizefor signed integers is complex and could benefit from simplification or better documentation. Review this function to enhance readability and maintainability.
| function setUp() public virtual override { | ||
| super.setUp(); | ||
| dispatcher.createClient(initClientMsg); | ||
| } | ||
|
|
||
| function test_updateConsensusState_success() public { | ||
| dispatcher.updateClientWithConsensusState(trustedState, proof); | ||
| } | ||
|
|
||
| function test_updateConsensusState_invalid() public { | ||
| vm.expectRevert(abi.encodeWithSignature('consensusStateVerificationFailed()')); | ||
| dispatcher.updateClientWithConsensusState(untrustedState, proof); | ||
|
|
||
| vm.expectRevert(abi.encodeWithSignature('consensusStateVerificationFailed()')); | ||
| ConsensusState memory invalidConsensusState; | ||
| dispatcher.updateClientWithConsensusState(invalidConsensusState, proof); | ||
| dispatcher = new Dispatcher(portPrefix, opConsensusStateManager); |
There was a problem hiding this comment.
The setUp method in ClientTestBase initializes the dispatcher without reflecting the changes mentioned in the summary about creating Dispatcher with different parameters. Ensure the setup aligns with the new requirements for testing.
| constructor(uint32 fraudProofWindowSeconds_, ProofVerifier verifier_, L1Block _l1BlockProvider) { | ||
| fraudProofWindowSeconds = fraudProofWindowSeconds_; | ||
| verifier = verifier_; | ||
| l1BlockProvider = _l1BlockProvider; |
There was a problem hiding this comment.
The constructor of OptimisticConsensusStateManager initializes fraudProofWindowSeconds, verifier, and l1BlockProvider. Ensure that these fields are correctly used throughout the contract and that their values are validated if necessary.
| L1Header calldata l1header, | ||
| OpL2StateProof calldata proof, | ||
| uint256 height, | ||
| uint256 appHash | ||
| ) external override returns (uint256 fraudProofEndTime, bool ended) { | ||
| uint256 hash = consensusStates[height]; | ||
| if (hash == 0) { | ||
| // if this is a new apphash we need to verify the provided proof. This method will revert in case | ||
| // of invalid proof. | ||
| verifier.verifyStateUpdate( | ||
| l1header, | ||
| proof, | ||
| bytes32(appHash), | ||
| l1BlockProvider.hash(), | ||
| l1BlockProvider.number() | ||
| ); |
There was a problem hiding this comment.
The addOpConsensusState method includes verification logic for new appHash using the verifier. Ensure that the verification logic correctly handles all possible outcomes of the verifyStateUpdate call, including potential reverts or invalid proofs.
| Ics23Proof calldata proof, | ||
| bytes calldata key, | ||
| bytes calldata expectedValue | ||
| ) external view { | ||
| (uint256 appHash, , bool ended) = getInternalState(proof.proofHeight.revision_height); | ||
| // a proof generated at height H can only be verified against state root (app hash) from block H - 1. | ||
| // this means the relayer must have updated the contract with the app hash from the previous block and | ||
| // that is why we use proof.height - 1 here. | ||
| (uint256 appHash, , bool ended) = getInternalState(proof.height - 1); | ||
| require(ended, "appHash hasn't passed the fraud proof window"); | ||
| require(verifier.verifyMembership(appHash, proof, key, expectedValue), message); | ||
| verifier.verifyMembership(bytes32(appHash), key, expectedValue, proof); |
There was a problem hiding this comment.
The verifyMembership method uses Ics23Proof, key, and expectedValue parameters for membership verification. Verify that the method correctly handles the proof verification process and that it properly interacts with the verifier to ensure the integrity of the state being verified.
| @@ -50,7 +31,7 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable { | |||
| // only stores a bit to mark packet has not been ack'ed or timed out yet; actual IBC packet verification is done on | |||
| // Polymer chain. | |||
| // Keep track of sent packets | |||
| mapping(address => mapping(bytes32 => mapping(uint64 => bool))) sendPacketCommitment; | |||
| mapping(address => mapping(bytes32 => mapping(uint64 => bool))) public sendPacketCommitment; | |||
There was a problem hiding this comment.
The documentation for sendPacketCommitment mapping is still missing. Adding comments to explain its purpose and usage would enhance code readability and maintainability.
| L1Header calldata l1header, | ||
| OpL2StateProof calldata proof, | ||
| uint256 height, | ||
| uint256 appHash | ||
| ) external returns (uint256 fraudProofEndTime, bool ended) { | ||
| if (!isClientCreated) { | ||
| revert clientNotCreated(); | ||
| } | ||
|
|
||
| return consensusStateManager.addOpConsensusState(height, appHash); | ||
| return consensusStateManager.addOpConsensusState(l1header, proof, height, appHash); |
There was a problem hiding this comment.
The function updateClientWithOptimisticConsensusState implementation is present. Ensure input validation for l1header, proof, height, and appHash is considered to prevent processing invalid or malicious data.
| CounterParty calldata local, | ||
| string[] calldata connectionHops, | ||
| ChannelOrder ordering, | ||
| bool feeEnabled, | ||
| string calldata counterpartyPortId, | ||
| bytes32 counterpartyChannelId, | ||
| string calldata counterpartyVersion, | ||
| Proof calldata proof | ||
| bool isChanConfirm, | ||
| CounterParty calldata counterparty, | ||
| Ics23Proof calldata proof | ||
| ) external { | ||
| consensusStateManager.verifyMembership( | ||
| proof, | ||
| bytes('channel/path/to/be/added/here'), | ||
| bytes('expected channel bytes constructed from params. Channel.State = {Ack_Pending, Confirm_Pending}'), | ||
| 'Fail to prove channel state' | ||
| ); | ||
| verifyConnectIbcChannelProof(local, connectionHops, ordering, isChanConfirm, counterparty, proof); | ||
|
|
||
| portAddress.onConnectIbcChannel(channelId, counterpartyChannelId, counterpartyVersion); | ||
| portAddress.onConnectIbcChannel(local.channelId, counterparty.channelId, counterparty.version); | ||
|
|
||
| // Register port and channel mapping | ||
| // TODO: check duplicated channel registration? | ||
| // TODO: The call to `Channel` constructor MUST be move to `openIbcChannel` phase | ||
| // Then `connectIbcChannel` phase can use the `version` as part of `require` condition. | ||
| portChannelMap[address(portAddress)][channelId] = Channel( | ||
| counterpartyVersion, // TODO: this should be self version instead of counterparty version | ||
| portChannelMap[address(portAddress)][local.channelId] = Channel( | ||
| counterparty.version, // TODO: this should be self version instead of counterparty version | ||
| ordering, | ||
| feeEnabled, | ||
| connectionHops, | ||
| counterpartyPortId, | ||
| counterpartyChannelId | ||
| counterparty.portId, | ||
| counterparty.channelId |
There was a problem hiding this comment.
The connectIbcChannel function implementation is visible. Consider adding input validation and checks for potential edge cases or invalid states before proceeding with the channel connection logic.
| function onCloseIbcChannel(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external { | ||
| // verify VIBC/IBC hub chain has processed ChanCloseConfirm event | ||
| consensusStateManager.verifyMembership( | ||
| proof, | ||
| bytes('channel/path/to/be/added/here'), | ||
| bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}'), | ||
| 'Fail to prove channel state' | ||
| bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}') |
There was a problem hiding this comment.
The hardcoded path in verifyMembership call within onCloseIbcChannel function is addressed. Replace with dynamic values based on actual channel paths and states to ensure the verification logic is accurate and adaptable.
- bytes('channel/path/to/be/added/here'),
- bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}')
+ actualChannelPath,
+ actualChannelStateBytesCommittable 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.
| function onCloseIbcChannel(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external { | |
| // verify VIBC/IBC hub chain has processed ChanCloseConfirm event | |
| consensusStateManager.verifyMembership( | |
| proof, | |
| bytes('channel/path/to/be/added/here'), | |
| bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}'), | |
| 'Fail to prove channel state' | |
| bytes('expected channel bytes constructed from params. Channel.State = {Closed(_Pending?)}') | |
| function onCloseIbcChannel(address portAddress, bytes32 channelId, Ics23Proof calldata proof) external { | |
| // verify VIBC/IBC hub chain has processed ChanCloseConfirm event | |
| consensusStateManager.verifyMembership( | |
| proof, | |
| actualChannelPath, | |
| actualChannelStateBytes |
| function timeout(IbcPacketReceiver receiver, IbcPacket calldata packet, Ics23Proof calldata proof) external { | ||
| // verify `receiver` is the original packet sender | ||
| if (!portIdAddressMatch(address(receiver), packet.src.portId)) { | ||
| revert receiverNotIndtendedPacketDestination(); | ||
| } | ||
|
|
||
| // prove absence of packet receipt on Polymer chain | ||
| consensusStateManager.verifyNonMembership(proof, 'packet/receipt/path', 'Fail to prove timeout'); | ||
| // TODO: add non membership support | ||
| consensusStateManager.verifyNonMembership(proof, 'packet/receipt/path'); |
There was a problem hiding this comment.
The timeout function's TODO for adding non-membership support is noted. Implementing this feature is crucial for fully verifying packet timeouts and enhancing the contract's functionality.
- // TODO: add non membership support
+ // Implement non-membership verification logic hereCommittable 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.
| function timeout(IbcPacketReceiver receiver, IbcPacket calldata packet, Ics23Proof calldata proof) external { | |
| // verify `receiver` is the original packet sender | |
| if (!portIdAddressMatch(address(receiver), packet.src.portId)) { | |
| revert receiverNotIndtendedPacketDestination(); | |
| } | |
| // prove absence of packet receipt on Polymer chain | |
| consensusStateManager.verifyNonMembership(proof, 'packet/receipt/path', 'Fail to prove timeout'); | |
| // TODO: add non membership support | |
| consensusStateManager.verifyNonMembership(proof, 'packet/receipt/path'); | |
| function timeout(IbcPacketReceiver receiver, IbcPacket calldata packet, Ics23Proof calldata proof) external { | |
| // verify `receiver` is the original packet sender | |
| if (!portIdAddressMatch(address(receiver), packet.src.portId)) { | |
| revert receiverNotIndtendedPacketDestination(); | |
| } | |
| // prove absence of packet receipt on Polymer chain | |
| // Implement non-membership verification logic here | |
| consensusStateManager.verifyNonMembership(proof, 'packet/receipt/path'); |
This PR adds proof support to the vibc-core sc. Specifically to these operations
Things that are not covered yet
There's some changes to the channel handshake API since we were missing information required to verify the provided proofs. Although these changes are minimal to make the releyer dev a bit easier. We will have to do a proper clean up later
To verify that everything is running as expected, run
forge testSummary by CodeRabbit