From cda477d89ab459f8df19e809296a86d6b39607c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Pernas=20Maradei?= <1394289+nicopernas@users.noreply.github.com> Date: Thu, 21 Mar 2024 06:01:01 +0100 Subject: [PATCH] nico/split channel handshake api again (#66) * channel handshake: split open-init and open-try * channel handshake: split open-ack and open-confirm --- contracts/core/Dispatcher.sol | 193 +++++++++++++-------- contracts/core/UniversalChannelHandler.sol | 65 ++++--- contracts/examples/Mars.sol | 97 +++++------ contracts/interfaces/IbcDispatcher.sol | 32 ++-- contracts/interfaces/IbcReceiver.sol | 18 +- test/Dispatcher.base.t.sol | 73 ++++++-- test/Dispatcher.proof.t.sol | 19 +- test/Dispatcher.t.sol | 96 +++++----- test/VirtualChain.sol | 73 +++++--- test/universal.channel.t.sol | 4 +- 10 files changed, 383 insertions(+), 287 deletions(-) diff --git a/contracts/core/Dispatcher.sol b/contracts/core/Dispatcher.sol index 0198b803..4040a0c2 100644 --- a/contracts/core/Dispatcher.sol +++ b/contracts/core/Dispatcher.sol @@ -69,14 +69,42 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable { } /** - * This func is called by a 'relayer' on behalf of a dApp. The dApp should be implements IbcChannelHandler. - * The dApp should implement the onOpenIbcChannel method to handle one of the first two channel handshake methods, - * ie. ChanOpenInit or ChanOpenTry. - * If callback succeeds, the dApp should return the selected version, and an emitted event will be relayed to the - * IBC/VIBC hub chain. + * This function is called by a 'relayer' on behalf of a dApp. The dApp should implement IbcChannelHandler's + * onChanOpenInit. If the callback succeeds, the dApp should return the selected version and the emitted event + * will be relayed to the IBC/VIBC hub chain. */ - function openIbcChannel( - IbcChannelReceiver portAddress, + function channelOpenInit( + IbcChannelReceiver receiver, + string calldata version, + ChannelOrder ordering, + bool feeEnabled, + string[] calldata connectionHops, + string calldata counterpartyPortId + ) external { + if (bytes(counterpartyPortId).length == 0) { + revert IBCErrors.invalidCounterPartyPortId(); + } + + (bool success, bytes memory data) = _callIfContract( + address(receiver), abi.encodeWithSelector(IbcChannelReceiver.onChanOpenInit.selector, version) + ); + + if (success) { + emit ChannelOpenInit( + address(receiver), abi.decode(data, (string)), ordering, feeEnabled, connectionHops, counterpartyPortId + ); + } else { + emit ChannelOpenInitError(address(receiver), data); + } + } + + /** + * This function is called by a 'relayer' on behalf of a dApp. The dApp should implement IbcChannelHandler's + * onChanOpenTry. If the callback succeeds, the dApp should return the selected version and the emitted event + * will be relayed to the IBC/VIBC hub chain. + */ + function channelOpenTry( + IbcChannelReceiver receiver, CounterParty calldata local, ChannelOrder ordering, bool feeEnabled, @@ -88,28 +116,19 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable { revert IBCErrors.invalidCounterPartyPortId(); } - if (Ibc._isChannelOpenTry(counterparty)) { - consensusStateManager.verifyMembership( - proof, - Ibc.channelProofKey(local.portId, local.channelId), - Ibc.channelProofValue(ChannelState.TRY_PENDING, ordering, local.version, connectionHops, counterparty) - ); - } + consensusStateManager.verifyMembership( + proof, + Ibc.channelProofKey(local.portId, local.channelId), + Ibc.channelProofValue(ChannelState.TRY_PENDING, ordering, local.version, connectionHops, counterparty) + ); (bool success, bytes memory data) = _callIfContract( - address(portAddress), - abi.encodeWithSelector( - IbcChannelReceiver.onOpenIbcChannel.selector, - local.version, - ordering, - feeEnabled, - connectionHops, - counterparty - ) + address(receiver), abi.encodeWithSelector(IbcChannelReceiver.onChanOpenTry.selector, counterparty.version) ); + if (success) { - emit OpenIbcChannel( - address(portAddress), + emit ChannelOpenTry( + address(receiver), abi.decode(data, (string)), ordering, feeEnabled, @@ -118,58 +137,72 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable { counterparty.channelId ); } else { - emit OpenIbcChannelError(address(portAddress), data); + emit ChannelOpenTryError(address(receiver), data); } } /** - * This func is called by a 'relayer' after the IBC/VIBC hub chain has processed the onOpenIbcChannel event. - * The dApp should implement the onConnectIbcChannel method to handle the last two channel handshake methods, ie. - * ChanOpenAck or ChanOpenConfirm. + * 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 third channel handshake method: ChanOpenAck */ - function connectIbcChannel( - IbcChannelReceiver portAddress, + function channelOpenAck( + IbcChannelReceiver receiver, CounterParty calldata local, string[] calldata connectionHops, ChannelOrder ordering, bool feeEnabled, - bool isChanConfirm, CounterParty calldata counterparty, Ics23Proof calldata proof ) external { - _verifyConnectIbcChannelProof(local, connectionHops, ordering, isChanConfirm, counterparty, proof); + consensusStateManager.verifyMembership( + proof, + Ibc.channelProofKey(local.portId, local.channelId), + Ibc.channelProofValue(ChannelState.ACK_PENDING, ordering, local.version, connectionHops, counterparty) + ); (bool success, bytes memory data) = _callIfContract( - address(portAddress), - abi.encodeWithSelector( - IbcChannelReceiver.onConnectIbcChannel.selector, - local.channelId, - counterparty.channelId, - counterparty.version - ) + address(receiver), + abi.encodeWithSelector(IbcChannelReceiver.onChanOpenAck.selector, local.channelId, counterparty.version) ); + if (success) { - emit ConnectIbcChannel(address(portAddress), local.channelId); - - // 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(receiver, local, connectionHops, ordering, feeEnabled, counterparty); + emit ChannelOpenAck(address(receiver), local.channelId); + } else { + emit ChannelOpenAckError(address(receiver), data); + } + } - // initialize channel sequences - nextSequenceSend[address(portAddress)][local.channelId] = 1; - nextSequenceRecv[address(portAddress)][local.channelId] = 1; - nextSequenceAck[address(portAddress)][local.channelId] = 1; + /** + * 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 receiver, + CounterParty calldata local, + string[] calldata connectionHops, + ChannelOrder ordering, + bool feeEnabled, + CounterParty calldata counterparty, + Ics23Proof calldata proof + ) external { + consensusStateManager.verifyMembership( + proof, + Ibc.channelProofKey(local.portId, local.channelId), + Ibc.channelProofValue(ChannelState.CONFIRM_PENDING, ordering, local.version, connectionHops, counterparty) + ); + + (bool success, bytes memory data) = _callIfContract( + address(receiver), + abi.encodeWithSelector(IbcChannelReceiver.onChanOpenConfirm.selector, local.channelId, counterparty.version) + ); + + if (success) { + _connectChannel(receiver, local, connectionHops, ordering, feeEnabled, counterparty); + emit ChannelOpenConfirm(address(receiver), local.channelId); } else { - emit ConnectIbcChannelError(address(portAddress), data); + emit ChannelOpenConfirmError(address(receiver), data); } } @@ -504,39 +537,45 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable { emit SendPacket(sender, channelId, packet, sequence, timeoutTimestamp); } - function _verifyConnectIbcChannelProof( + function _connectChannel( + IbcChannelReceiver portAddress, CounterParty calldata local, string[] calldata connectionHops, ChannelOrder ordering, - bool isChanConfirm, - CounterParty calldata counterparty, - Ics23Proof calldata proof + bool feeEnabled, + CounterParty calldata counterparty ) internal { - consensusStateManager.verifyMembership( - proof, - Ibc.channelProofKey(local.portId, local.channelId), - Ibc.channelProofValue( - isChanConfirm ? ChannelState.CONFIRM_PENDING : ChannelState.ACK_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 ); + + // initialize channel sequences + nextSequenceSend[address(portAddress)][local.channelId] = 1; + nextSequenceRecv[address(portAddress)][local.channelId] = 1; + nextSequenceAck[address(portAddress)][local.channelId] = 1; } // Returns the result of the call if no revert, otherwise returns the error if thrown. - function _callIfContract(address portAddress, bytes memory args) + function _callIfContract(address receiver, bytes memory args) internal returns (bool success, bytes memory message) { - if (!Address.isContract(portAddress)) { + if (!Address.isContract(receiver)) { return (false, bytes("call to non-contract")); } - // Only call if we are sure portAddress is a contract + // Only call if we are sure receiver is a contract // Note: This tx won't revert if the low-level call fails, see // https://docs.soliditylang.org/en/latest/cheatsheet.html#members-of-address - (success, message) = portAddress.call(args); + (success, message) = receiver.call(args); } // _isPacketTimeout returns true if the given packet has timed out acoording to host chain's block height and diff --git a/contracts/core/UniversalChannelHandler.sol b/contracts/core/UniversalChannelHandler.sol index 058f6cb0..2ab9b6b5 100644 --- a/contracts/core/UniversalChannelHandler.sol +++ b/contracts/core/UniversalChannelHandler.sol @@ -33,17 +33,6 @@ contract UniversalChannelHandler is IbcReceiverBase, IbcUniversalChannelMW { dispatcher.closeIbcChannel(channelId); } - // IBC callback functions - function onConnectIbcChannel(bytes32 channelId, bytes32, string calldata counterpartyVersion) - external - onlyIbcDispatcher - { - if (keccak256(abi.encodePacked(counterpartyVersion)) != keccak256(abi.encodePacked(VERSION))) { - revert UnsupportedVersion(); - } - connectedChannels.push(channelId); - } - function onCloseIbcChannel(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher { // logic to determin if the channel should be closed bool channelFound = false; @@ -149,23 +138,43 @@ contract UniversalChannelHandler is IbcReceiverBase, IbcUniversalChannelMW { mwStackAddrs[mwBitmap] = mwAddrs; } - function onOpenIbcChannel( - string calldata version, - ChannelOrder, - bool, - string[] calldata, - CounterParty calldata counterparty - ) external view onlyIbcDispatcher returns (string memory selectedVersion) { - if (counterparty.channelId == bytes32(0)) { - // ChanOpenInit - if (keccak256(abi.encodePacked(version)) != keccak256(abi.encodePacked(VERSION))) { - revert UnsupportedVersion(); - } - } else { - // ChanOpenTry - if (keccak256(abi.encodePacked(counterparty.version)) != keccak256(abi.encodePacked(VERSION))) { - revert UnsupportedVersion(); - } + // IBC callback functions + function onChanOpenAck(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher { + _connectChannel(channelId, counterpartyVersion); + } + + function onChanOpenConfirm(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher { + _connectChannel(channelId, counterpartyVersion); + } + + function onChanOpenInit(string calldata version) + external + view + onlyIbcDispatcher + returns (string memory selectedVersion) + { + return _openChannel(version); + } + + function onChanOpenTry(string calldata counterpartyVersion) + external + view + onlyIbcDispatcher + returns (string memory selectedVersion) + { + return _openChannel(counterpartyVersion); + } + + function _connectChannel(bytes32 channelId, string calldata version) private { + if (keccak256(abi.encodePacked(version)) != keccak256(abi.encodePacked(VERSION))) { + revert UnsupportedVersion(); + } + connectedChannels.push(channelId); + } + + function _openChannel(string calldata version) private pure returns (string memory selectedVersion) { + if (keccak256(abi.encodePacked(version)) != keccak256(abi.encodePacked(VERSION))) { + revert UnsupportedVersion(); } return VERSION; } diff --git a/contracts/examples/Mars.sol b/contracts/examples/Mars.sol index 42b44272..2a0b50c1 100644 --- a/contracts/examples/Mars.sol +++ b/contracts/examples/Mars.sol @@ -39,23 +39,6 @@ contract Mars is IbcReceiverBase, IbcReceiver { timeoutPackets.push(packet); } - function onConnectIbcChannel(bytes32 channelId, bytes32, string calldata counterpartyVersion) - external - virtual - onlyIbcDispatcher - { - // ensure negotiated version is supported - bool foundVersion = false; - for (uint256 i = 0; i < supportedVersions.length; i++) { - if (keccak256(abi.encodePacked(counterpartyVersion)) == keccak256(abi.encodePacked(supportedVersions[i]))) { - foundVersion = true; - break; - } - } - if (!foundVersion) revert UnsupportedVersion(); - connectedChannels.push(channelId); - } - function onCloseIbcChannel(bytes32 channelId, string calldata, bytes32) external virtual onlyIbcDispatcher { // logic to determin if the channel should be closed bool channelFound = false; @@ -87,40 +70,52 @@ contract Mars is IbcReceiverBase, IbcReceiver { dispatcher.sendPacket(channelId, bytes(message), timeoutTimestamp); } - function onOpenIbcChannel( - string calldata version, - ChannelOrder, - bool, - string[] calldata, - CounterParty calldata counterparty - ) external view virtual onlyIbcDispatcher returns (string memory selectedVersion) { - if (bytes(counterparty.portId).length <= 8) { - revert IBCErrors.invalidCounterPartyPortId(); - } - /** - * Version selection is determined by if the callback is invoked on behalf of ChanOpenInit or ChanOpenTry. - * ChanOpenInit: self version should be provided whereas the counterparty version is empty. - * ChanOpenTry: counterparty version should be provided whereas the self version is empty. - * In both cases, the selected version should be in the supported versions list. - */ - bool foundVersion = false; - selectedVersion = - keccak256(abi.encodePacked(version)) == keccak256(abi.encodePacked("")) ? counterparty.version : version; + function onChanOpenAck(bytes32 channelId, string calldata counterpartyVersion) external virtual onlyIbcDispatcher { + _connectChannel(channelId, counterpartyVersion); + } + + function onChanOpenConfirm(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher { + _connectChannel(channelId, counterpartyVersion); + } + + function onChanOpenInit(string calldata version) + external + view + virtual + onlyIbcDispatcher + returns (string memory selectedVersion) + { + return _openChannel(version); + } + + function onChanOpenTry(string calldata counterpartyVersion) + external + view + virtual + onlyIbcDispatcher + returns (string memory selectedVersion) + { + return _openChannel(counterpartyVersion); + } + + function _connectChannel(bytes32 channelId, string calldata counterpartyVersion) private { + // ensure negotiated version is supported for (uint256 i = 0; i < supportedVersions.length; i++) { - if (keccak256(abi.encodePacked(selectedVersion)) == keccak256(abi.encodePacked(supportedVersions[i]))) { - foundVersion = true; - break; + if (keccak256(abi.encodePacked(counterpartyVersion)) == keccak256(abi.encodePacked(supportedVersions[i]))) { + connectedChannels.push(channelId); + return; } } - if (!foundVersion) revert UnsupportedVersion(); - // if counterpartyVersion is not empty, then it must be the same foundVersion - if (keccak256(abi.encodePacked(counterparty.version)) != keccak256(abi.encodePacked(""))) { - if (keccak256(abi.encodePacked(counterparty.version)) != keccak256(abi.encodePacked(selectedVersion))) { - revert VersionMismatch(); + revert UnsupportedVersion(); + } + + function _openChannel(string calldata version) private view returns (string memory selectedVersion) { + for (uint256 i = 0; i < supportedVersions.length; i++) { + if (keccak256(abi.encodePacked(version)) == keccak256(abi.encodePacked(supportedVersions[i]))) { + return version; } } - - return selectedVersion; + revert UnsupportedVersion(); } } @@ -132,13 +127,7 @@ contract RevertingStringMars is Mars { constructor(IbcDispatcher _dispatcher) Mars(_dispatcher) {} // solhint-disable-next-line - function onOpenIbcChannel(string calldata, ChannelOrder, bool, string[] calldata, CounterParty calldata) - external - view - override - onlyIbcDispatcher - returns (string memory) - { + function onChanOpenInit(string calldata) external view override onlyIbcDispatcher returns (string memory) { // solhint-disable-next-line require(false, "open ibc channel is reverting"); return ""; @@ -152,7 +141,7 @@ contract RevertingStringMars is Mars { } // solhint-disable-next-line - function onConnectIbcChannel(bytes32, bytes32, string calldata) external view override onlyIbcDispatcher { + function onChanOpenAck(bytes32, string calldata) external view override onlyIbcDispatcher { // solhint-disable-next-line require(false, "connect ibc channel is reverting"); } diff --git a/contracts/interfaces/IbcDispatcher.sol b/contracts/interfaces/IbcDispatcher.sol index 2292d9f4..122e301f 100644 --- a/contracts/interfaces/IbcDispatcher.sol +++ b/contracts/interfaces/IbcDispatcher.sol @@ -23,14 +23,13 @@ interface IbcPacketSender { * Other features are implemented as callback methods in the IbcReceiver interface. */ interface IbcDispatcher is IbcPacketSender { - function openIbcChannel( - IbcChannelReceiver portAddress, - CounterParty calldata self, + function channelOpenInit( + IbcChannelReceiver receiver, + string calldata version, ChannelOrder ordering, bool feeEnabled, string[] calldata connectionHops, - CounterParty calldata counterparty, - Ics23Proof calldata proof + string calldata counterpartyPortId ) external; function closeIbcChannel(bytes32 channelId) external; @@ -46,8 +45,18 @@ interface IbcEventsEmitter { // // channel events // - event OpenIbcChannel( - address indexed portAddress, + event ChannelOpenInit( + address indexed recevier, + string version, + ChannelOrder ordering, + bool feeEnabled, + string[] connectionHops, + string counterpartyPortId + ); + event ChannelOpenInitError(address indexed receiver, bytes error); + + event ChannelOpenTry( + address indexed receiver, string version, ChannelOrder ordering, bool feeEnabled, @@ -55,13 +64,16 @@ interface IbcEventsEmitter { string counterpartyPortId, bytes32 counterpartyChannelId ); + event ChannelOpenTryError(address indexed receiver, bytes error); + + event ChannelOpenAck(address indexed receiver, bytes32 channelId); + event ChannelOpenAckError(address indexed receiver, bytes error); - event ConnectIbcChannel(address indexed portAddress, bytes32 channelId); - event ConnectIbcChannelError(address indexed portAddress, bytes error); + event ChannelOpenConfirm(address indexed receiver, bytes32 channelId); + event ChannelOpenConfirmError(address indexed receiver, bytes error); event CloseIbcChannel(address indexed portAddress, bytes32 indexed channelId); - event OpenIbcChannelError(address indexed portAddress, bytes error); event CloseIbcChannelError(address indexed receiver, bytes error); event AcknowledgementError(address indexed receiver, bytes error); event TimeoutError(address indexed receiver, bytes error); diff --git a/contracts/interfaces/IbcReceiver.sol b/contracts/interfaces/IbcReceiver.sol index 5c655f93..1e458aef 100644 --- a/contracts/interfaces/IbcReceiver.sol +++ b/contracts/interfaces/IbcReceiver.sol @@ -12,16 +12,13 @@ import {ChannelOrder, CounterParty, IbcPacket, AckPacket} from "../libs/Ibc.sol" * handshake callbacks. */ interface IbcChannelReceiver { - function onOpenIbcChannel( - string calldata version, - ChannelOrder ordering, - bool feeEnabled, - string[] calldata connectionHops, - CounterParty calldata counterparty - ) external returns (string memory selectedVersion); - - function onConnectIbcChannel(bytes32 channelId, bytes32 counterpartyChannelId, string calldata counterpartyVersion) - external; + function onChanOpenInit(string calldata version) external returns (string memory selectedVersion); + + function onChanOpenTry(string calldata counterpartyVersion) external returns (string memory selectedVersion); + + function onChanOpenAck(bytes32 channelId, string calldata counterpartyVersion) external; + + function onChanOpenConfirm(bytes32 channelId, string calldata counterpartyVersion) external; function onCloseIbcChannel(bytes32 channelId, string calldata counterpartyPortId, bytes32 counterpartyChannelId) external; @@ -53,7 +50,6 @@ contract IbcReceiverBase is Ownable { error notIbcDispatcher(); error UnsupportedVersion(); - error VersionMismatch(); error ChannelNotFound(); /** diff --git a/test/Dispatcher.base.t.sol b/test/Dispatcher.base.t.sol index 9d143199..f2065885 100644 --- a/test/Dispatcher.base.t.sol +++ b/test/Dispatcher.base.t.sol @@ -65,35 +65,50 @@ contract Base is IbcEventsEmitter, ProofBase { // ⬇️ IBC functions for testing /** - * @dev Step-1/2 of the 4-step handshake to open an IBC channel. + * @dev Step-1 of the 4-step handshake to open an IBC channel. * @param le Local end settings for the channel. * @param re Remote end settings for the channel. * @param s Channel handshake settings. * @param expPass Expected pass status of the operation. * If expPass is false, `vm.expectRevert` should be called before this function. */ - function openChannel(LocalEnd memory le, CounterParty memory re, ChannelHandshakeSetting memory s, bool expPass) + function channelOpenInit(LocalEnd memory le, CounterParty memory re, ChannelHandshakeSetting memory s, bool expPass) public { - CounterParty memory cp; - cp.portId = re.portId; - if (!s.localInitiate) { - cp.channelId = re.channelId; - cp.version = re.version; + if (expPass) { + vm.expectEmit(true, true, true, true); + emit ChannelOpenInit( + address(le.receiver), le.versionExpected, s.ordering, s.feeEnabled, le.connectionHops, re.portId + ); } + dispatcher.channelOpenInit(le.receiver, le.versionCall, s.ordering, s.feeEnabled, le.connectionHops, re.portId); + } + + /** + * @dev Step-2 of the 4-step handshake to open an IBC channel. + * @param le Local end settings for the channel. + * @param re Remote end settings for the channel. + * @param s Channel handshake settings. + * @param expPass Expected pass status of the operation. + * If expPass is false, `vm.expectRevert` should be called before this function. + */ + function channelOpenTry(LocalEnd memory le, CounterParty memory re, ChannelHandshakeSetting memory s, bool expPass) + public + { if (expPass) { vm.expectEmit(true, true, true, true); - emit OpenIbcChannel( + emit ChannelOpenTry( address(le.receiver), le.versionExpected, s.ordering, s.feeEnabled, le.connectionHops, - cp.portId, - cp.channelId + re.portId, + re.channelId ); } - dispatcher.openIbcChannel( + CounterParty memory cp = CounterParty(re.portId, re.channelId, re.version); + dispatcher.channelOpenTry( le.receiver, CounterParty(le.portId, le.channelId, le.versionCall), s.ordering, @@ -105,31 +120,55 @@ contract Base is IbcEventsEmitter, ProofBase { } /** - * @dev Step-3/4 of the 4-step handshake to open an IBC channel. + * @dev Step-3 of the 4-step handshake to open an IBC channel. + * @param le Local end settings for the channel. + * @param re Remote end settings for the channel. + * @param s Channel handshake settings. + * @param expPass Expected pass status of the operation. + * If expPass is false, `vm.expectRevert` should be called before this function. + */ + function channelOpenAck(LocalEnd memory le, CounterParty memory re, ChannelHandshakeSetting memory s, bool expPass) + public + { + if (expPass) { + vm.expectEmit(true, true, true, true); + emit ChannelOpenAck(address(le.receiver), le.channelId); + } + dispatcher.channelOpenAck( + le.receiver, + CounterParty(le.portId, le.channelId, le.versionCall), + le.connectionHops, + s.ordering, + s.feeEnabled, + re, + s.proof + ); + } + + /** + * @dev Step-4 of the 4-step handshake to open an IBC channel. * @param le Local end settings for the channel. * @param re Remote end settings for the channel. * @param s Channel handshake settings. * @param expPass Expected pass status of the operation. * If expPass is false, `vm.expectRevert` should be called before this function. */ - function connectChannel( + function channelOpenConfirm( LocalEnd memory le, CounterParty memory re, ChannelHandshakeSetting memory s, - bool isChannConfirm, bool expPass ) public { if (expPass) { vm.expectEmit(true, true, true, true); - emit ConnectIbcChannel(address(le.receiver), le.channelId); + emit ChannelOpenConfirm(address(le.receiver), le.channelId); } - dispatcher.connectIbcChannel( + dispatcher.channelOpenConfirm( le.receiver, CounterParty(le.portId, le.channelId, le.versionCall), le.connectionHops, s.ordering, s.feeEnabled, - isChannConfirm, re, s.proof ); diff --git a/test/Dispatcher.proof.t.sol b/test/Dispatcher.proof.t.sol index d6cd1608..2f06baa3 100644 --- a/test/Dispatcher.proof.t.sol +++ b/test/Dispatcher.proof.t.sol @@ -31,39 +31,38 @@ contract DispatcherIbcWithRealProofs is IbcEventsEmitter, ProofBase { } function test_ibc_channel_open_init() public { - CounterParty memory counterparty = CounterParty(ch1.portId, bytes32(0), ""); - vm.expectEmit(true, true, true, true); - emit OpenIbcChannel(address(mars), "1.0", ChannelOrder.NONE, false, connectionHops1, ch1.portId, bytes32(0)); + emit ChannelOpenInit(address(mars), "1.0", ChannelOrder.NONE, false, connectionHops1, ch1.portId); + // since this is open chann init, the proof is not used. so use an invalid one - dispatcher.openIbcChannel(mars, ch1, ChannelOrder.NONE, false, connectionHops1, counterparty, invalidProof); + dispatcher.channelOpenInit(mars, ch1.version, ChannelOrder.NONE, false, connectionHops1, ch1.portId); } function test_ibc_channel_open_try() public { Ics23Proof memory proof = load_proof("/test/payload/channel_try_pending_proof.hex"); vm.expectEmit(true, true, true, true); - emit OpenIbcChannel(address(mars), "1.0", ChannelOrder.NONE, false, connectionHops1, ch0.portId, ch0.channelId); + emit ChannelOpenTry(address(mars), "1.0", ChannelOrder.NONE, false, connectionHops1, ch0.portId, ch0.channelId); - dispatcher.openIbcChannel(mars, ch1, ChannelOrder.NONE, false, connectionHops1, ch0, proof); + dispatcher.channelOpenTry(mars, ch1, ChannelOrder.NONE, false, connectionHops1, ch0, proof); } function test_ibc_channel_ack() public { Ics23Proof memory proof = load_proof("/test/payload/channel_ack_pending_proof.hex"); vm.expectEmit(true, true, true, true); - emit ConnectIbcChannel(address(mars), ch0.channelId); + emit ChannelOpenAck(address(mars), ch0.channelId); - dispatcher.connectIbcChannel(mars, ch0, connectionHops0, ChannelOrder.NONE, false, false, ch1, proof); + dispatcher.channelOpenAck(mars, ch0, connectionHops0, ChannelOrder.NONE, false, ch1, proof); } function test_ibc_channel_confirm() public { Ics23Proof memory proof = load_proof("/test/payload/channel_confirm_pending_proof.hex"); vm.expectEmit(true, true, true, true); - emit ConnectIbcChannel(address(mars), ch1.channelId); + emit ChannelOpenConfirm(address(mars), ch1.channelId); - dispatcher.connectIbcChannel(mars, ch1, connectionHops1, ChannelOrder.NONE, false, true, ch0, proof); + dispatcher.channelOpenConfirm(mars, ch1, connectionHops1, ChannelOrder.NONE, false, ch0, proof); } function test_ack_packet() public { diff --git a/test/Dispatcher.t.sol b/test/Dispatcher.t.sol index 4765dd2a..70c2c663 100644 --- a/test/Dispatcher.t.sol +++ b/test/Dispatcher.t.sol @@ -33,7 +33,7 @@ contract ChannelHandshakeTest is Base { le.versionCall = versions[j]; le.versionExpected = versions[j]; // remoteEnd has no channelId or version if localEnd is the initiator - openChannel(le, re, settings[i], true); + channelOpenInit(le, re, settings[i], true); } } } @@ -50,11 +50,11 @@ contract ChannelHandshakeTest is Base { le.versionCall = versions[j]; le.versionExpected = versions[j]; // remoteEnd version is used - openChannel(le, re, settings[i], true); + channelOpenInit(le, re, settings[i], true); // auto version selection le.versionCall = ""; - openChannel(le, re, settings[i], true); + channelOpenTry(le, re, settings[i], true); } } } @@ -70,28 +70,10 @@ contract ChannelHandshakeTest is Base { le.versionCall = versions[j]; le.versionExpected = versions[j]; re.version = versions[j]; - openChannel(le, re, settings[i], true); - connectChannel(le, re, settings[i], false, true); - } - } - } - - function test_openChannel_receiver_fail_versionMismatch() public { - ChannelHandshakeSetting[4] memory settings = createSettings(false, true); - string[2] memory versions = ["1.0", "2.0"]; - for (uint256 i = 0; i < settings.length; i++) { - for (uint256 j = 0; j < versions.length; j++) { - LocalEnd memory le = _local; - CounterParty memory re = _remote; - re.version = versions[j]; - // always select the wrong version - bool isVersionOne = keccak256(abi.encodePacked(versions[j])) == keccak256(abi.encodePacked("1.0")); - le.versionCall = isVersionOne ? "2.0" : "1.0"; - vm.expectEmit(true, true, true, true); - emit IbcEventsEmitter.OpenIbcChannelError( - address(le.receiver), abi.encodeWithSelector(IbcReceiverBase.VersionMismatch.selector) - ); - openChannel(le, re, settings[i], false); + channelOpenInit(le, re, settings[i], true); + channelOpenTry(le, re, settings[i], true); + channelOpenAck(le, re, settings[i], true); + channelOpenConfirm(le, re, settings[i], true); } } } @@ -106,16 +88,16 @@ contract ChannelHandshakeTest is Base { le.versionCall = versions[j]; le.versionExpected = versions[j]; vm.expectEmit(true, true, true, true); - emit IbcEventsEmitter.OpenIbcChannelError( + emit IbcEventsEmitter.ChannelOpenInitError( address(le.receiver), abi.encodeWithSelector(IbcReceiverBase.UnsupportedVersion.selector) ); - openChannel(le, re, settings[i], false); + channelOpenInit(le, re, settings[i], false); } } } function test_openChannel_receiver_fail_invalidProof() public { - // When localEnd initiates, no proof verification is done in openIbcChannel + // When localEnd initiates, no proof verification is done in channelOpenTry ChannelHandshakeSetting[4] memory settings = createSettings(false, false); string[1] memory versions = ["1.0"]; for (uint256 i = 0; i < settings.length; i++) { @@ -124,14 +106,15 @@ contract ChannelHandshakeTest is Base { CounterParty memory re = _remote; le.versionCall = versions[j]; le.versionExpected = versions[j]; + vm.expectRevert(DummyConsensusStateManager.InvalidDummyMembershipProof.selector); - openChannel(le, re, settings[i], false); + channelOpenTry(le, re, settings[i], false); } } } - function test_connectChannel_revert_unsupportedVersion() public { - // When localEnd initiates, counterparty version is only available in connectIbcChannel + function test_connectChannel_fail_unsupportedVersion() public { + // When localEnd initiates, counterparty version is only available in channelOpenAck ChannelHandshakeSetting[4] memory settings = createSettings(true, true); string[2] memory versions = ["", "xxxxxxx"]; for (uint256 i = 0; i < settings.length; i++) { @@ -139,19 +122,21 @@ contract ChannelHandshakeTest is Base { LocalEnd memory le = _local; CounterParty memory re = _remote; // no remote version applied in openChannel - openChannel(le, re, settings[i], true); + channelOpenInit(le, re, settings[i], true); + channelOpenTry(le, re, settings[i], true); re.version = versions[j]; vm.expectEmit(true, true, true, true); - emit IbcEventsEmitter.ConnectIbcChannelError( + emit IbcEventsEmitter.ChannelOpenAckError( address(le.receiver), abi.encodeWithSelector(IbcReceiverBase.UnsupportedVersion.selector) ); - connectChannel(le, re, settings[i], false, false); + + channelOpenAck(le, re, settings[i], false); } } } - function test_connectChannel_revert_invalidProof() public { - // When localEnd initiates, counterparty version is only available in connectIbcChannel + function test_connectChannel_fail_invalidProof() public { + // When localEnd initiates, counterparty version is only available in channelOpenAck ChannelHandshakeSetting[8] memory settings = createSettings2(true); string[1] memory versions = ["1.0"]; for (uint256 i = 0; i < settings.length; i++) { @@ -159,11 +144,13 @@ contract ChannelHandshakeTest is Base { LocalEnd memory le = _local; CounterParty memory re = _remote; // no remote version applied in openChannel - openChannel(le, re, settings[i], true); + channelOpenInit(le, re, settings[i], true); + channelOpenTry(le, re, settings[i], true); re.version = versions[j]; settings[i].proof = invalidProof; + vm.expectRevert(DummyConsensusStateManager.InvalidDummyMembershipProof.selector); - connectChannel(le, re, settings[i], false, false); + channelOpenAck(le, re, settings[i], false); } } } @@ -226,10 +213,11 @@ contract ChannelOpenTestBase is Base { _localRevertingMars = LocalEnd(revertingBytesMars, portId, channelId, connectionHops, "1.0", "1.0"); _remote = CounterParty("eth2.7E5F4552091A69125d5DfCb7b8C2659029395Bdf", "channel-2", "1.0"); - openChannel(_local, _remote, setting, true); - openChannel(_localRevertingMars, _remote, setting, true); - connectChannel(_local, _remote, setting, false, true); - connectChannel(_localRevertingMars, _remote, setting, false, true); + channelOpenInit(_local, _remote, setting, true); + channelOpenTry(_localRevertingMars, _remote, setting, true); + + channelOpenAck(_local, _remote, setting, true); + channelOpenConfirm(_localRevertingMars, _remote, setting, true); } } @@ -559,17 +547,17 @@ contract DappRevertTests is Base { function test_ibc_channel_open_non_dapp_call() public { address nonDappAddr = vm.addr(1); - emit OpenIbcChannelError(nonDappAddr, bytes("call to non-contract")); - dispatcher.openIbcChannel( - IbcChannelReceiver(nonDappAddr), ch1, ChannelOrder.NONE, false, connectionHops1, ch0, validProof + emit ChannelOpenInitError(nonDappAddr, bytes("call to non-contract")); + dispatcher.channelOpenInit( + IbcChannelReceiver(nonDappAddr), ch1.version, ChannelOrder.NONE, false, connectionHops1, ch0.portId ); } function test_ibc_channel_open_dapp_without_handler() public { Earth earth = new Earth(vm.addr(1)); - emit OpenIbcChannelError(address(earth), ""); - dispatcher.openIbcChannel( - IbcChannelReceiver(address(earth)), ch1, ChannelOrder.NONE, false, connectionHops1, ch0, validProof + emit ChannelOpenInitError(address(earth), ""); + dispatcher.channelOpenInit( + IbcChannelReceiver(address(earth)), ch1.version, ChannelOrder.NONE, false, connectionHops1, ch0.portId ); } @@ -654,19 +642,19 @@ contract DappRevertTests is Base { function test_ibc_channel_open_dapp_revert() public { vm.expectEmit(true, true, true, true); - emit OpenIbcChannelError( + emit ChannelOpenInitError( address(revertingStringMars), abi.encodeWithSignature("Error(string)", "open ibc channel is reverting") ); - dispatcher.openIbcChannel(revertingStringMars, ch1, ChannelOrder.NONE, false, connectionHops1, ch0, validProof); + dispatcher.channelOpenInit( + revertingStringMars, ch1.version, ChannelOrder.NONE, false, connectionHops1, ch0.portId + ); } function test_ibc_channel_ack_dapp_revert() public { vm.expectEmit(true, true, true, true); - emit ConnectIbcChannelError( + emit ChannelOpenAckError( address(revertingStringMars), abi.encodeWithSignature("Error(string)", "connect ibc channel is reverting") ); - dispatcher.connectIbcChannel( - revertingStringMars, ch0, connectionHops0, ChannelOrder.NONE, false, false, ch1, validProof - ); + dispatcher.channelOpenAck(revertingStringMars, ch0, connectionHops0, ChannelOrder.NONE, false, ch1, validProof); } } diff --git a/test/VirtualChain.sol b/test/VirtualChain.sol index 70ca30a9..d23e6f89 100644 --- a/test/VirtualChain.sol +++ b/test/VirtualChain.sol @@ -132,10 +132,10 @@ contract VirtualChain is Test, IbcEventsEmitter { remoteChain.channelOpenTry(remoteEnd, this, localEnd, setting, true); // step-2 vm.prank(address(this)); - this.channelOpenAckOrConfirm(localEnd, remoteChain, remoteEnd, setting, false, true); // step-3 + this.channelOpenConfirm(localEnd, remoteChain, remoteEnd, setting, true); // step-3 vm.prank(address(remoteChain)); - remoteChain.channelOpenAckOrConfirm(remoteEnd, this, localEnd, setting, true, true); // step-4 + remoteChain.channelOpenAck(remoteEnd, this, localEnd, setting, true); // step-4 } function channelOpenInit( @@ -153,25 +153,17 @@ contract VirtualChain is Test, IbcEventsEmitter { if (expPass) { vm.expectEmit(true, true, true, true); - emit OpenIbcChannel( + emit ChannelOpenInit( address(localEnd), setting.version, setting.ordering, setting.feeEnabled, connectionHops, - remoteChain.portIds(address(remoteEnd)), - bytes32(0) + remoteChain.portIds(address(remoteEnd)) ); } - dispatcher.openIbcChannel( - localEnd, - CounterParty(setting.portId, setting.channelId, setting.version), - setting.ordering, - setting.feeEnabled, - connectionHops, - // counterparty channelId and version are not known at this point - CounterParty(cpPortId, bytes32(0), ""), - setting.proof + dispatcher.channelOpenInit( + localEnd, setting.version, setting.ordering, setting.feeEnabled, connectionHops, cpPortId ); } @@ -193,7 +185,7 @@ contract VirtualChain is Test, IbcEventsEmitter { if (expPass) { vm.expectEmit(true, true, true, true); - emit OpenIbcChannel( + emit ChannelOpenTry( address(localEnd), setting.version, setting.ordering, @@ -203,7 +195,7 @@ contract VirtualChain is Test, IbcEventsEmitter { cpChanId ); } - dispatcher.openIbcChannel( + dispatcher.channelOpenTry( localEnd, CounterParty(setting.portId, setting.channelId, setting.version), setting.ordering, @@ -214,38 +206,71 @@ contract VirtualChain is Test, IbcEventsEmitter { ); } - function channelOpenAckOrConfirm( + function channelOpenAck( + IbcChannelReceiver localEnd, + VirtualChain remoteChain, + IbcChannelReceiver remoteEnd, + ChannelSetting memory setting, + bool expPass + ) external { + // local channel Id must be known + bytes32 chanId = channelIds[address(localEnd)][address(remoteEnd)]; + require(chanId != bytes32(0), "channelOpenAck: channel does not exist"); + + bytes32 cpChanId = remoteChain.channelIds(address(remoteEnd), address(localEnd)); + require(cpChanId != bytes32(0), "channelOpenAck: channel does not exist"); + + string memory cpPortId = remoteChain.portIds(address(remoteEnd)); + require(bytes(cpPortId).length > 0, "channelOpenAck: counterparty portId does not exist"); + + // set dispatcher's msg.sender to this function's msg.sender + vm.prank(msg.sender); + + if (expPass) { + vm.expectEmit(true, true, true, true); + emit ChannelOpenAck(address(localEnd), chanId); + } + dispatcher.channelOpenAck( + localEnd, + CounterParty(setting.portId, chanId, setting.version), + connectionHops, + setting.ordering, + setting.feeEnabled, + CounterParty(cpPortId, cpChanId, setting.version), + setting.proof + ); + } + + function channelOpenConfirm( IbcChannelReceiver localEnd, VirtualChain remoteChain, IbcChannelReceiver remoteEnd, ChannelSetting memory setting, - bool isChanConfirm, bool expPass ) external { // local channel Id must be known bytes32 chanId = channelIds[address(localEnd)][address(remoteEnd)]; - require(chanId != bytes32(0), "channelOpenAckOrConfirm: channel does not exist"); + require(chanId != bytes32(0), "channelOpenConfirm: channel does not exist"); bytes32 cpChanId = remoteChain.channelIds(address(remoteEnd), address(localEnd)); - require(cpChanId != bytes32(0), "channelOpenAckOrConfirm: channel does not exist"); + require(cpChanId != bytes32(0), "channelOpenConfirm: channel does not exist"); string memory cpPortId = remoteChain.portIds(address(remoteEnd)); - require(bytes(cpPortId).length > 0, "channelOpenAckOrConfirm: counterparty portId does not exist"); + require(bytes(cpPortId).length > 0, "channelOpenConfirm: counterparty portId does not exist"); // set dispatcher's msg.sender to this function's msg.sender vm.prank(msg.sender); if (expPass) { vm.expectEmit(true, true, true, true); - emit ConnectIbcChannel(address(localEnd), chanId); + emit ChannelOpenConfirm(address(localEnd), chanId); } - dispatcher.connectIbcChannel( + dispatcher.channelOpenConfirm( localEnd, CounterParty(setting.portId, chanId, setting.version), connectionHops, setting.ordering, setting.feeEnabled, - isChanConfirm, CounterParty(cpPortId, cpChanId, setting.version), setting.proof ); diff --git a/test/universal.channel.t.sol b/test/universal.channel.t.sol index 08fe64fb..61296b91 100644 --- a/test/universal.channel.t.sol +++ b/test/universal.channel.t.sol @@ -77,10 +77,10 @@ contract UniversalChannelTest is Base { eth2.channelOpenTry(ucHandler2, eth1, ucHandler1, setting, true); vm.prank(unauthorized); - eth1.channelOpenAckOrConfirm(ucHandler1, eth2, ucHandler2, setting, false, true); + eth1.channelOpenAck(ucHandler1, eth2, ucHandler2, setting, true); vm.prank(unauthorized); - eth2.channelOpenAckOrConfirm(ucHandler2, eth1, ucHandler1, setting, true, true); + eth2.channelOpenConfirm(ucHandler2, eth1, ucHandler1, setting, true); } }