revert api split changes #62
Conversation
This reverts commit 7777f17.
WalkthroughThe updates focus on refining the IBC (Inter-Blockchain Communication) channel handling in smart contracts, particularly around opening and connecting channels. This includes the consolidation of channel opening functions into more streamlined versions, enhanced version compatibility checks, and the introduction of new events and errors to better reflect the process. The changes aim to simplify the interaction model and improve the reliability of cross-chain communication. 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 (
|
nicopernas
requested review from
RnkSngh and
alfredo-stonk
and removed request for
RnkSngh
![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: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- contracts/core/Dispatcher.sol (5 hunks)
- contracts/core/UniversalChannelHandler.sol (2 hunks)
- contracts/examples/Mars.sol (2 hunks)
- contracts/interfaces/IbcDispatcher.sol (3 hunks)
- contracts/interfaces/IbcReceiver.sol (2 hunks)
- test/Dispatcher.base.t.sol (2 hunks)
- test/Dispatcher.proof.t.sol (1 hunks)
- test/Dispatcher.t.sol (6 hunks)
- test/VirtualChain.sol (5 hunks)
- test/universal.channel.t.sol (1 hunks)
Additional comments: 29
contracts/interfaces/IbcReceiver.sol (3)
- 15-21: The consolidation of channel opening functions into
onOpenIbcChannelsimplifies the interface and potentially reduces the complexity of implementing the IBC protocol. However, ensure that all implementing contracts are updated to reflect these changes, as this could break existing implementations that rely on the previous function signatures.- 23-24: The addition of
onConnectIbcChannelis a logical extension of the interface to handle the connection phase of the IBC channel lifecycle. This change aligns with the PR's objective to simplify the channel handshake process. Ensure that the documentation is updated to clearly explain the expected behavior and use cases of this function.- 56-56: The introduction of the
VersionMismatcherror is a good practice for handling version incompatibilities more explicitly. This can improve error handling and debugging experiences for developers. Ensure that all functions that could potentially throw this error are documented accordingly.contracts/interfaces/IbcDispatcher.sol (2)
- 26-33: Renaming
channelOpenInittoopenIbcChanneland the addition of theIcs23Proofparameter align with the PR's objective to simplify and enhance the channel opening process. Ensure that all dependent contracts and tests are updated to reflect this change. Additionally, consider updating the documentation to explain the role of theIcs23Proofparameter in the channel opening process.- 46-52: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [49-59]
The replacement of
ChannelOpenInitandChannelOpenTryevents withOpenIbcChannelandConnectIbcChannelis consistent with the interface function changes. This should simplify event handling and improve clarity. Ensure that all event listeners and handlers are updated accordingly to prevent any potential issues with event processing.contracts/examples/Mars.sol (2)
- 37-51: The implementation of
onConnectIbcChannelto verify supported versions before connecting channels is a good practice. It ensures that only compatible channel versions are connected, which can prevent potential issues down the line. Ensure that the list ofsupportedVersionsis maintained and updated as necessary to reflect the versions supported by the contract.- 84-117: Modifying
onOpenIbcChannelto handle version selection based on the channel initiation type (ChanOpenInit or ChanOpenTry) is a smart approach to streamline the channel opening process. This change makes the version negotiation more explicit and manageable. Ensure that the logic correctly handles all possible scenarios and that comprehensive tests are in place to verify its behavior.test/Dispatcher.base.t.sol (2)
- 75-96: The refactoring of the IBC channel opening steps to use the new
openChannelfunction is a positive change that aligns with the interface updates. Ensure that the tests cover all possible scenarios and edge cases to verify the correct behavior of the channel opening process. Additionally, verify that the event emissions are correctly captured and asserted in the tests.- 115-126: The implementation of
connectChannelto handle the connection phase of the IBC channel lifecycle in tests is consistent with the changes in the main contracts. Ensure that the tests adequately cover the new logic and that the expected behavior is clearly documented in the test cases.test/Dispatcher.proof.t.sol (2)
- 34-39: Updating the test to use the
openIbcChannelmethod with the new parameter structure is a necessary change following the interface updates. Ensure that the tests are comprehensive and cover various scenarios to validate the correct behavior of theopenIbcChannelmethod, especially with the inclusion of proofs.- 55-57: The adjustment to use
connectIbcChannelfor acknowledging channel connections in tests is in line with the main contract changes. Ensure that the tests validate the correct handling of proofs and the expected event emissions during the channel acknowledgment process.contracts/core/UniversalChannelHandler.sol (2)
- 36-45: The implementation of
onConnectIbcChannelin theUniversalChannelHandlercontract to enforce version compatibility is a good practice. It ensures that channels can only be connected if they are compatible, which is crucial for maintaining the integrity of the IBC protocol. Ensure that the list of supported versions (VERSION) is correctly managed and that this function is thoroughly tested.- 152-168: The modifications to
onOpenIbcChannelto handle bothChanOpenInitandChanOpenTryscenarios with version checks are well thought out. This approach simplifies the channel opening process by consolidating the logic into a single function. Ensure that the function correctly handles all scenarios and that comprehensive tests are in place to verify its behavior.test/VirtualChain.sol (2)
- 135-138: Renaming
channelOpenConfirmtochannelOpenAckOrConfirmand adding theisChanConfirmparameter to differentiate between channel acknowledgment and confirmation is a logical change that aligns with the PR's objectives. Ensure that the tests cover both scenarios (acknowledgment and confirmation) and that the behavior is consistent with the updated interface.- 217-248: The adjustments made to the
channelOpenAckOrConfirmfunction to handle the acknowledgment or confirmation of channel connections in tests are consistent with the changes in the main contracts. Ensure that the tests are comprehensive and validate the correct behavior of the channel connection process, including the handling of the newisChanConfirmparameter.test/universal.channel.t.sol (2)
- 80-80: The addition of a boolean parameter in
channelOpenAckOrConfirmcalls is noted. This change aligns with the PR's objective to simplify the channel handshake process by mergingchannelOpenAckandchannelOpenConfirminto a single function. However, it's crucial to ensure that the boolean parameter's purpose (likely indicating whether it's an ack or confirm operation) is clearly documented to avoid confusion.- 83-83: Similar to the previous comment, the addition of a boolean parameter in
channelOpenAckOrConfirmis observed here as well. It's essential to maintain consistency in how this parameter is used across different calls to this function to ensure the logic remains clear and understandable.test/Dispatcher.t.sol (9)
- 35-35: Renaming functions to
openChannelandconnectChannelsimplifies the channel handshake process, making it more intuitive. This change is consistent with the PR's objective to revert problematic modifications and improve the API's stability. Ensure that all references to the old function names are updated across the codebase to avoid any broken functionality.- 52-52: The consolidation of channel opening functions into
openChannelis noted here as well. It's important to verify that the new function adequately handles all scenarios previously covered by separate functions, ensuring no loss of functionality or error handling capabilities.- 56-56: The use of an empty string for version selection in
openChannelsuggests an auto-selection mechanism. This approach can be beneficial for flexibility but requires clear documentation and robust error handling to manage potential version mismatches effectively.- 72-73: The introduction of
connectChannelas part of the channel handshake process is a significant change. It's crucial to ensure that this function integrates seamlessly with the rest of the handshake process, particularly in terms of version compatibility checks and error handling.- 78-93: Adding tests for version mismatches (
test_openChannel_receiver_fail_versionMismatch) is a positive step towards ensuring robustness in the channel handshake process. It's important to cover various scenarios where version mismatches can occur to ensure comprehensive testing.- 69-98: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [95-105]
The addition of tests for unsupported versions (
test_openChannel_initiator_fail_unsupportedVersion) is crucial for validating the channel handshake process's resilience. Ensure that these tests adequately cover different scenarios where unsupported versions might be encountered.
- 117-159: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [111-121]
Testing for invalid proof in
test_openChannel_receiver_fail_invalidProofis essential for security. It's important to ensure that the contract correctly rejects invalid proofs to prevent unauthorized channel openings.
- 127-138: The tests for unsupported versions in
test_connectChannel_fail_unsupportedVersionare important for ensuring compatibility and error handling in the channel connection process. Ensure comprehensive coverage of scenarios where unsupported versions might be encountered.- 144-156: Testing for invalid proof in
test_connectChannel_fail_invalidProofis crucial for maintaining the integrity of the channel connection process. It's important to verify that the contract correctly identifies and rejects invalid proofs to prevent unauthorized actions.contracts/core/Dispatcher.sol (3)
- 87-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [77-101]
The
openIbcChannelfunction introduces a consolidated approach to initiating or trying to open an IBC channel. A few observations and suggestions:
Parameter Validation (Lines 77-96): The validation of
counterparty.portIdis crucial to ensure that the channel opening process is initiated with a valid counterparty. However, consider adding more detailed comments explaining the significance of each validation step for future maintainability.Proof Verification (Lines 90-96): The use of
consensusStateManager.verifyMembershipfor proof verification is appropriate. Ensure that theproof,channelProofKey, andchannelProofValueare correctly formed and that their integrity is maintained across updates to related components.Callback Invocation (Lines 98-99): The callback to
portAddress.onOpenIbcChannelis a critical step in the channel opening process. It's important to ensure that theIbcChannelReceiverinterface and its implementations are robust and handle all possible outcomes of this callback.Event Emission (Lines 101): The
OpenIbcChannelevent provides transparency and traceability for channel opening actions. Ensure that all parameters of the event are accurately populated and that the event is documented for consumers.Overall, the changes align with the PR's objectives to simplify the channel opening process. However, thorough testing and validation are recommended to ensure that these changes do not introduce regressions or new issues.
Ensure comprehensive testing, especially for the proof verification logic and callback invocation, to validate the correctness and robustness of the channel opening process.
- 456-473: The
_verifyConnectIbcChannelProofinternal function plays a key role in verifying the connection proof during the channel connection process. A few observations:
Proof Verification (Lines 464-473): The use of
consensusStateManager.verifyMembershipfor proof verification is appropriate. Ensure that the proof, keys, and values are correctly formed and that their integrity is maintained across updates to related components.Channel State Handling (Lines 468-469): The conditional logic to determine the channel state (
CONFIRM_PENDINGorACK_PENDING) based onisChanConfirmis clear and logical. Ensure that all possible states are correctly handled and that the state transitions are well-documented.This function is critical for the security and integrity of the channel connection process. Thorough testing and validation are recommended to ensure its correctness.
Consider adding more detailed comments explaining the significance of each step in the proof verification process for future maintainability and clarity.
- 521-530: The
_isChannelOpenTryinternal function effectively determines if the channel open attempt is aChanOpenTry. Key observations:
Logic Clarity (Lines 521-530): The function's logic is clear and effectively uses the counterparty information to determine the channel open attempt type. The use of
counterparty.channelIdandcounterparty.versionto differentiate betweenChanOpenInitandChanOpenTryis logical.Error Handling (Line 529): The use of
revert IBCErrors.invalidCounterParty();for invalid counterparty scenarios is appropriate. Ensure that all possible invalid scenarios are correctly identified and handled.This function is crucial for correctly handling different channel open attempts. Ensure that it is thoroughly tested, especially with edge cases and invalid counterparty scenarios.
The implementation and logic of
_isChannelOpenTryare sound and align with the contract's requirements for handling different channel open attempts.
| function connectIbcChannel( | ||
| IbcChannelReceiver portAddress, | ||
| CounterParty calldata local, | ||
| string[] calldata connectionHops, | ||
| ChannelOrder ordering, | ||
| bool feeEnabled, | ||
| bool isChanConfirm, | ||
| CounterParty calldata counterparty, | ||
| Ics23Proof calldata proof | ||
| ) external { | ||
| consensusStateManager.verifyMembership( | ||
| proof, | ||
| channelProofKey(local.portId, local.channelId), | ||
| channelProofValue(ChannelState.ACK_PENDING, ordering, local.version, connectionHops, counterparty) | ||
| ); | ||
|
|
||
| _connectChannel(portAddress, local, connectionHops, ordering, feeEnabled, counterparty); | ||
|
|
||
| portAddress.onChanOpenAck(local.channelId, counterparty.version); | ||
| _verifyConnectIbcChannelProof(local, connectionHops, ordering, isChanConfirm, counterparty, proof); | ||
|
|
||
| emit ChannelOpenAck(address(portAddress), local.channelId); | ||
| } | ||
| portAddress.onConnectIbcChannel(local.channelId, counterparty.channelId, counterparty.version); | ||
|
|
||
| /** | ||
| * This func is called by a 'relayer' after the IBC/VIBC hub chain has processed the ChannelOpenTry event. | ||
| * The dApp should implement the onChannelConnect method to handle the last channel handshake method: | ||
| * ChannelOpenConfirm | ||
| */ | ||
| function channelOpenConfirm( | ||
| IbcChannelReceiver portAddress, | ||
| CounterParty calldata local, | ||
| string[] calldata connectionHops, | ||
| ChannelOrder ordering, | ||
| bool feeEnabled, | ||
| CounterParty calldata counterparty, | ||
| Ics23Proof calldata proof | ||
| ) external { | ||
| consensusStateManager.verifyMembership( | ||
| proof, | ||
| channelProofKey(local.portId, local.channelId), | ||
| channelProofValue(ChannelState.CONFIRM_PENDING, ordering, local.version, connectionHops, counterparty) | ||
| // 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)][local.channelId] = Channel( | ||
| counterparty.version, // TODO: this should be self version instead of counterparty version | ||
| ordering, | ||
| feeEnabled, | ||
| connectionHops, | ||
| counterparty.portId, | ||
| counterparty.channelId | ||
| ); | ||
|
|
||
| _connectChannel(portAddress, local, connectionHops, ordering, feeEnabled, counterparty); | ||
|
|
||
| portAddress.onChanOpenConfirm(local.channelId, counterparty.version); | ||
| // initialize channel sequences | ||
| nextSequenceSend[address(portAddress)][local.channelId] = 1; | ||
| nextSequenceRecv[address(portAddress)][local.channelId] = 1; | ||
| nextSequenceAck[address(portAddress)][local.channelId] = 1; | ||
|
|
||
| emit ChannelOpenConfirm(address(portAddress), local.channelId); | ||
| emit ConnectIbcChannel(address(portAddress), local.channelId); |
There was a problem hiding this comment.
The connectIbcChannel function manages the connection phase of the IBC channel handshake. Key points include:
-
Proof Verification and Callback Invocation (Lines 127-129): The
_verifyConnectIbcChannelProofinternal function is used for proof verification, which is a good practice. The callback toportAddress.onConnectIbcChannelis crucial for the dApp to handle the connection phase. Ensure that the callback's implementation in dApps is robust and handles all edge cases. -
Channel Registration (Lines 131-141): The registration of the channel in the
portChannelMapis a critical step. The comment on line 136 indicates a potential issue with using the counterparty version instead of the self version. This needs to be addressed to ensure that the channel information is correctly registered. -
Event Emission (Line 149): Emitting the
ConnectIbcChannelevent provides transparency for the channel connection process. Ensure that the event parameters are accurately populated. -
TODO Comments (Lines 132-134): The TODO comments suggest areas for improvement, such as checking for duplicated channel registration and moving the call to the
Channelconstructor to theopenIbcChannelphase. These improvements should be prioritized to enhance the contract's robustness and clarity.
Line 136 uses the counterparty version instead of the self version for channel registration. This could lead to incorrect channel information being stored. Consider revising this to use the correct version.
Would you like assistance in addressing the TODO comments mentioned in lines 132-134? Implementing these improvements could enhance the contract's functionality and maintainability.
Revert channel handshake "split api" changes since those break the api and will make contract
deployment hard
Summary by CodeRabbit
OpenIbcChannelandConnectIbcChannelfor better visibility into channel operations.openIbcChannelandconnectIbcChannelacross multiple contracts for clarity and efficiency.UniversalChannelHandlerto handle channel openings more effectively.VersionMismatchto handle version incompatibilities during channel connections.