channel handshake: split open-ack and open-confirm

open-ibc · Mar 4, 2024 · 1775735 · 1775735
1 parent d20ac23
commit 1775735
Show file tree

Hide file tree

Showing 10 changed files with 145 additions and 46 deletions.
diff --git a/contracts/core/Dispatcher.sol b/contracts/core/Dispatcher.sol
@@ -133,19 +133,15 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
      * The dApp should implement the onConnectIbcChannel method to handle the last two channel handshake methods, ie.
      * ChanOpenAck or ChanOpenConfirm.
      */
-    function connectIbcChannel(
+    function connectChannel(
         IbcChannelReceiver portAddress,
         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);
-
-        portAddress.onConnectIbcChannel(local.channelId, counterparty.channelId, counterparty.version);
+        CounterParty calldata counterparty
+    ) internal {
+        portAddress.onChannelConnect(local.channelId, counterparty.version);
 
         // Register port and channel mapping
         // TODO: check duplicated channel registration?
@@ -164,8 +160,55 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
         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 third channel handshake method: ChanOpenAck
+     */
+    function channelOpenAck(
+        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.ACK_PENDING, ordering, local.version, connectionHops, counterparty)
+        );
+
+        connectChannel(portAddress, local, connectionHops, ordering, feeEnabled, counterparty);
+
+        emit ChannelOpenAck(address(portAddress), local.channelId);
+    }
+
+    /**
+     * 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)
+        );
+
+        connectChannel(portAddress, local, connectionHops, ordering, feeEnabled, counterparty);
 
-        emit ConnectIbcChannel(address(portAddress), local.channelId);
+        emit ChannelOpenConfirm(address(portAddress), local.channelId);
     }
 
     /**

diff --git a/contracts/core/UniversalChannelHandler.sol b/contracts/core/UniversalChannelHandler.sol
@@ -58,10 +58,7 @@ contract UniversalChannelHandler is IbcReceiverBase, IbcUniversalChannelMW {
         return VERSION;
     }
 
-    function onConnectIbcChannel(bytes32 channelId, bytes32, string calldata counterpartyVersion)
-        external
-        onlyIbcDispatcher
-    {
+    function onChannelConnect(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher {
         if (keccak256(abi.encodePacked(counterpartyVersion)) != keccak256(abi.encodePacked(VERSION))) {
             revert UnsupportedVersion();
         }

diff --git a/contracts/examples/Mars.sol b/contracts/examples/Mars.sol
@@ -62,10 +62,7 @@ contract Mars is IbcReceiverBase, IbcReceiver {
         revert UnsupportedVersion();
     }
 
-    function onConnectIbcChannel(bytes32 channelId, bytes32, string calldata counterpartyVersion)
-        external
-        onlyIbcDispatcher
-    {
+    function onChannelConnect(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher {
         // ensure negotiated version is supported
         bool foundVersion = false;
         for (uint256 i = 0; i < supportedVersions.length; i++) {

diff --git a/contracts/interfaces/IbcDispatcher.sol b/contracts/interfaces/IbcDispatcher.sol
@@ -64,7 +64,9 @@ interface IbcEventsEmitter {
         bytes32 counterpartyChannelId
     );
 
-    event ConnectIbcChannel(address indexed portAddress, bytes32 channelId);
+    event ChannelOpenAck(address indexed portAddress, bytes32 channelId);
+
+    event ChannelOpenConfirm(address indexed portAddress, bytes32 channelId);
 
     event CloseIbcChannel(address indexed portAddress, bytes32 indexed channelId);
 

diff --git a/contracts/interfaces/IbcReceiver.sol b/contracts/interfaces/IbcReceiver.sol
@@ -16,8 +16,7 @@ interface IbcChannelReceiver {
 
     function onChannelOpenTry(string calldata counterpartyVersion) external returns (string memory selectedVersion);
 
-    function onConnectIbcChannel(bytes32 channelId, bytes32 counterpartyChannelId, string calldata counterpartyVersion)
-        external;
+    function onChannelConnect(bytes32 channelId, string calldata counterpartyVersion) external;
 
     function onCloseIbcChannel(bytes32 channelId, string calldata counterpartyPortId, bytes32 counterpartyChannelId)
         external;

diff --git a/test/Dispatcher.base.t.sol b/test/Dispatcher.base.t.sol
@@ -120,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 connectChannel(
+    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 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
@@ -51,18 +51,18 @@ contract DispatcherIbcWithRealProofs is IbcEventsEmitter, ProofBase {
         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
@@ -71,7 +71,8 @@ contract ChannelHandshakeTest is Base {
                 re.version = versions[j];
                 channelOpenInit(le, re, settings[i], true);
                 channelOpenTry(le, re, settings[i], true);
-                connectChannel(le, re, settings[i], false, true);
+                channelOpenAck(le, re, settings[i], true);
+                channelOpenConfirm(le, re, settings[i], true);
             }
         }
     }
@@ -120,8 +121,9 @@ contract ChannelHandshakeTest is Base {
                 channelOpenInit(le, re, settings[i], true);
                 channelOpenTry(le, re, settings[i], true);
                 re.version = versions[j];
+
                 vm.expectRevert(IbcReceiverBase.UnsupportedVersion.selector);
-                connectChannel(le, re, settings[i], false, false);
+                channelOpenAck(le, re, settings[i], false);
             }
         }
     }
@@ -139,8 +141,9 @@ contract ChannelHandshakeTest is Base {
                 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);
             }
         }
     }
@@ -200,7 +203,8 @@ contract ChannelOpenTestBase is Base {
 
         channelOpenInit(_local, _remote, setting, true);
         channelOpenTry(_local, _remote, setting, true);
-        connectChannel(_local, _remote, setting, false, true);
+        channelOpenAck(_local, _remote, setting, true);
+        channelOpenConfirm(_local, _remote, setting, true);
     }
 }
 

diff --git 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(
@@ -206,38 +206,71 @@ contract VirtualChain is Test, IbcEventsEmitter {
         );
     }
 
-    function channelOpenAckOrConfirm(
+    function channelOpenAck(
         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), "channelOpenAck: 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), "channelOpenAck: 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, "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 ConnectIbcChannel(address(localEnd), chanId);
+            emit ChannelOpenAck(address(localEnd), chanId);
         }
-        dispatcher.connectIbcChannel(
+        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 expPass
+    ) external {
+        // local channel Id must be known
+        bytes32 chanId = channelIds[address(localEnd)][address(remoteEnd)];
+        require(chanId != bytes32(0), "channelOpenConfirm: channel does not exist");
+
+        bytes32 cpChanId = remoteChain.channelIds(address(remoteEnd), address(localEnd));
+        require(cpChanId != bytes32(0), "channelOpenConfirm: channel does not exist");
+
+        string memory cpPortId = remoteChain.portIds(address(remoteEnd));
+        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 ChannelOpenConfirm(address(localEnd), chanId);
+        }
+        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
@@ -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);
     }
 }