Merge pull request #229 from hyperledger-labs/improve-ics-04-handshake

Improve ICS-04 handshake and tests

Signed-off-by: Jun Kimura <[email protected]>

.gas-snapshot

@@ -1,32 +1,40 @@
 IBCTest:testBenchmarkCreateMockClient() (gas: 214228)
 IBCTest:testBenchmarkRecvPacket() (gas: 156012)
-IBCTest:testBenchmarkSendPacket() (gas: 86005)
+IBCTest:testBenchmarkSendPacket() (gas: 85983)
 IBCTest:testBenchmarkUpdateMockClient() (gas: 129873)
-IBCTest:testSendPacketInvalidTimestamp() (gas: 39254)
+IBCTest:testSendPacketInvalidTimestamp() (gas: 39232)
 IBCTest:testToUint128((uint64,uint64)) (runs: 256, μ: 1047, ~: 1047)
-TestICS02:testCreateClient() (gas: 19962279)
-TestICS02:testInvalidCreateClient() (gas: 19815956)
-TestICS02:testInvalidUpdateClient() (gas: 19818347)
-TestICS02:testRegisterClient() (gas: 19489199)
-TestICS02:testRegisterClientDuplicatedClientType() (gas: 19471179)
-TestICS02:testRegisterClientInvalidClientType() (gas: 19459978)
-TestICS02:testUpdateClient() (gas: 19960378)
-TestICS03Handshake:testConnOpenAck() (gas: 1635508)
-TestICS03Handshake:testConnOpenConfirm() (gas: 1774267)
-TestICS03Handshake:testConnOpenInit() (gas: 1312564)
-TestICS03Handshake:testConnOpenTry() (gas: 2141504)
-TestICS03Handshake:testInvalidConnOpenAck() (gas: 2024269)
-TestICS03Handshake:testInvalidConnOpenConfirm() (gas: 2030726)
-TestICS03Handshake:testInvalidConnOpenInit() (gas: 686391)
-TestICS03Handshake:testInvalidConnOpenTry() (gas: 2148632)
+TestICS02:testCreateClient() (gas: 20289885)
+TestICS02:testInvalidCreateClient() (gas: 20143126)
+TestICS02:testInvalidUpdateClient() (gas: 20145597)
+TestICS02:testRegisterClient() (gas: 19816449)
+TestICS02:testRegisterClientDuplicatedClientType() (gas: 19798429)
+TestICS02:testRegisterClientInvalidClientType() (gas: 19787228)
+TestICS02:testUpdateClient() (gas: 20287895)
+TestICS03Handshake:testConnOpenAck() (gas: 1635533)
+TestICS03Handshake:testConnOpenConfirm() (gas: 1774197)
+TestICS03Handshake:testConnOpenInit() (gas: 1312580)
+TestICS03Handshake:testConnOpenTry() (gas: 2141582)
+TestICS03Handshake:testInvalidConnOpenAck() (gas: 2024260)
+TestICS03Handshake:testInvalidConnOpenConfirm() (gas: 2030379)
+TestICS03Handshake:testInvalidConnOpenInit() (gas: 686407)
+TestICS03Handshake:testInvalidConnOpenTry() (gas: 2148717)
 TestICS03Version:testCopyVersions() (gas: 560316)
 TestICS03Version:testFindSupportedVersion() (gas: 19986)
 TestICS03Version:testIsSupportedVersion() (gas: 8317)
 TestICS03Version:testPickVersion() (gas: 27093)
 TestICS03Version:testVerifyProposedVersion() (gas: 12368)
 TestICS03Version:testVerifySupportedFeature() (gas: 4441)
-TestICS20:testAddressToHex(address) (runs: 256, μ: 23720, ~: 23889)
+TestICS04Handshake:testChanOpenAck() (gas: 3078342)
+TestICS04Handshake:testChanOpenConfirm() (gas: 3270361)
+TestICS04Handshake:testChanOpenInit() (gas: 2332442)
+TestICS04Handshake:testChanOpenTry() (gas: 2837918)
+TestICS04Handshake:testInvalidChanOpenAck() (gas: 2172745)
+TestICS04Handshake:testInvalidChanOpenConfirm() (gas: 2231572)
+TestICS04Handshake:testInvalidChanOpenInit() (gas: 1326388)
+TestICS04Handshake:testInvalidChanOpenTry() (gas: 1404332)
+TestICS20:testAddressToHex(address) (runs: 256, μ: 23716, ~: 23858)
 TestICS20:testHexToAddress(string) (runs: 256, μ: 4817, ~: 4829)
 TestICS20:testIsEscapedString() (gas: 49914)
 TestICS20:testMarshaling() (gas: 150062)
-TestICS20:testParseAmount(uint256) (runs: 256, μ: 27089, ~: 21814)
+TestICS20:testParseAmount(uint256) (runs: 256, μ: 27191, ~: 21814)

contracts/apps/mock/IBCMockApp.sol

@@ -7,6 +7,8 @@ import "../../core/25-handler/IBCHandler.sol";
 import "./IBCMockLib.sol";
 
 contract IBCMockApp is IBCAppBase {
+    string public constant MOCKAPP_VERSION = "mockapp-1";
+
     IBCHandler ibcHandler;
 
     constructor(IBCHandler ibcHandler_) {
@@ -56,4 +58,29 @@ contract IBCMockApp is IBCAppBase {
             require(keccak256(acknowledgement) == keccak256(IBCMockLib.FAILED_ACKNOWLEDGEMENT_JSON));
         }
     }
+
+    function onChanOpenInit(IIBCModule.MsgOnChanOpenInit calldata msg_)
+        external
+        virtual
+        override
+        onlyIBC
+        returns (string memory)
+    {
+        require(
+            bytes(msg_.version).length == 0 || keccak256(bytes(msg_.version)) == keccak256(bytes(MOCKAPP_VERSION)),
+            "version mismatch"
+        );
+        return MOCKAPP_VERSION;
+    }
+
+    function onChanOpenTry(IIBCModule.MsgOnChanOpenTry calldata msg_)
+        external
+        virtual
+        override
+        onlyIBC
+        returns (string memory)
+    {
+        require(keccak256(bytes(msg_.counterpartyVersion)) == keccak256(bytes(MOCKAPP_VERSION)), "version mismatch");
+        return MOCKAPP_VERSION;
+    }
 }

contracts/core/04-channel/IBCChannelHandshake.sol

@@ -3,20 +3,19 @@ pragma solidity ^0.8.9;
 
 import "@openzeppelin/contracts/utils/Strings.sol";
 import "../../proto/Channel.sol";
-import "../25-handler/IBCMsgs.sol";
 import "../02-client/IBCHeight.sol";
+import "../03-connection/IBCConnection.sol";
+import "../04-channel/IIBCChannel.sol";
 import "../24-host/IBCStore.sol";
 import "../24-host/IBCCommitment.sol";
-import "../04-channel/IIBCChannel.sol";
+import "../25-handler/IBCMsgs.sol";
 
 /**
  * @dev IBCChannelHandshake is a contract that implements [ICS-4](https://github.com/cosmos/ibc/tree/main/spec/core/ics-004-channel-and-packet-semantics).
  */
 contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
     using IBCHeight for Height.Data;
 
-    /* Handshake functions */
-
     /**
      * @dev channelOpenInit is called by a module to initiate a channel opening handshake with a module on another chain.
      */
@@ -26,18 +25,17 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
         require(
             connection.versions.length == 1, "single version must be negotiated on connection before opening channel"
         );
+        require(
+            IBCConnectionLib.verifySupportedFeature(
+                connection.versions[0], IBCChannelLib.toString(msg_.channel.ordering)
+            ),
+            "feature not supported"
+        );
         require(msg_.channel.state == Channel.State.STATE_INIT, "channel state must STATE_INIT");
-
-        // TODO verifySupportedFeature
+        require(bytes(msg_.channel.counterparty.channel_id).length == 0, "counterparty channel_id must be empty");
 
         string memory channelId = generateChannelIdentifier();
-        channels[msg_.portId][channelId] = msg_.channel;
-        nextSequenceSends[msg_.portId][channelId] = 1;
-        nextSequenceRecvs[msg_.portId][channelId] = 1;
-        nextSequenceAcks[msg_.portId][channelId] = 1;
-        updateChannelCommitment(msg_.portId, channelId);
-        commitments[IBCCommitment.nextSequenceRecvCommitmentKey(msg_.portId, channelId)] =
-            keccak256(abi.encodePacked((bytes8(uint64(1)))));
+        initializeSequences(msg_.portId, channelId);
         return channelId;
     }
 
@@ -47,20 +45,22 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
     function channelOpenTry(IBCMsgs.MsgChannelOpenTry calldata msg_) external returns (string memory) {
         require(msg_.channel.connection_hops.length == 1, "connection_hops length must be 1");
         ConnectionEnd.Data storage connection = connections[msg_.channel.connection_hops[0]];
+        require(connection.state == ConnectionEnd.State.STATE_OPEN, "connection state must be STATE_OPEN");
         require(
             connection.versions.length == 1, "single version must be negotiated on connection before opening channel"
         );
+        require(
+            IBCConnectionLib.verifySupportedFeature(
+                connection.versions[0], IBCChannelLib.toString(msg_.channel.ordering)
+            ),
+            "feature not supported"
+        );
         require(msg_.channel.state == Channel.State.STATE_TRYOPEN, "channel state must be STATE_TRYOPEN");
-        require(msg_.channel.connection_hops.length == 1);
 
-        // TODO verifySupportedFeature
-
-        ChannelCounterparty.Data memory expectedCounterparty =
-            ChannelCounterparty.Data({port_id: msg_.portId, channel_id: ""});
         Channel.Data memory expectedChannel = Channel.Data({
             state: Channel.State.STATE_INIT,
             ordering: msg_.channel.ordering,
-            counterparty: expectedCounterparty,
+            counterparty: ChannelCounterparty.Data({port_id: msg_.portId, channel_id: ""}),
             connection_hops: getCounterpartyHops(msg_.channel.connection_hops[0]),
             version: msg_.counterpartyVersion
         });
@@ -77,13 +77,7 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
         );
 
         string memory channelId = generateChannelIdentifier();
-        channels[msg_.portId][channelId] = msg_.channel;
-        nextSequenceSends[msg_.portId][channelId] = 1;
-        nextSequenceRecvs[msg_.portId][channelId] = 1;
-        nextSequenceAcks[msg_.portId][channelId] = 1;
-        updateChannelCommitment(msg_.portId, channelId);
-        commitments[IBCCommitment.nextSequenceRecvCommitmentKey(msg_.portId, channelId)] =
-            keccak256(abi.encodePacked((bytes8(uint64(1)))));
+        initializeSequences(msg_.portId, channelId);
         return channelId;
     }
 
@@ -98,12 +92,10 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
         require(connection.state == ConnectionEnd.State.STATE_OPEN, "connection state is not OPEN");
         require(channel.connection_hops.length == 1);
 
-        ChannelCounterparty.Data memory expectedCounterparty =
-            ChannelCounterparty.Data({port_id: msg_.portId, channel_id: msg_.channelId});
         Channel.Data memory expectedChannel = Channel.Data({
             state: Channel.State.STATE_TRYOPEN,
             ordering: channel.ordering,
-            counterparty: expectedCounterparty,
+            counterparty: ChannelCounterparty.Data({port_id: msg_.portId, channel_id: msg_.channelId}),
             connection_hops: getCounterpartyHops(channel.connection_hops[0]),
             version: msg_.counterpartyVersion
         });
@@ -135,12 +127,10 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
         require(connection.state == ConnectionEnd.State.STATE_OPEN, "connection state is not OPEN");
         require(channel.connection_hops.length == 1);
 
-        ChannelCounterparty.Data memory expectedCounterparty =
-            ChannelCounterparty.Data({port_id: msg_.portId, channel_id: msg_.channelId});
         Channel.Data memory expectedChannel = Channel.Data({
             state: Channel.State.STATE_OPEN,
             ordering: channel.ordering,
-            counterparty: expectedCounterparty,
+            counterparty: ChannelCounterparty.Data({port_id: msg_.portId, channel_id: msg_.channelId}),
             connection_hops: getCounterpartyHops(channel.connection_hops[0]),
             version: channel.version
         });
@@ -185,12 +175,10 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
         ConnectionEnd.Data storage connection = connections[channel.connection_hops[0]];
         require(connection.state == ConnectionEnd.State.STATE_OPEN, "connection state is not OPEN");
 
-        ChannelCounterparty.Data memory expectedCounterparty =
-            ChannelCounterparty.Data({port_id: msg_.portId, channel_id: msg_.channelId});
         Channel.Data memory expectedChannel = Channel.Data({
             state: Channel.State.STATE_CLOSED,
             ordering: channel.ordering,
-            counterparty: expectedCounterparty,
+            counterparty: ChannelCounterparty.Data({port_id: msg_.portId, channel_id: msg_.channelId}),
             connection_hops: getCounterpartyHops(channel.connection_hops[0]),
             version: channel.version
         });
@@ -209,6 +197,29 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
         updateChannelCommitment(msg_.portId, msg_.channelId);
     }
 
+    /**
+     * @dev writeChannel writes a channel which has successfully passed the OpenInit or OpenTry handshake step.
+     */
+    function writeChannel(
+        string calldata portId,
+        string calldata channelId,
+        Channel.State state,
+        Channel.Order order,
+        ChannelCounterparty.Data calldata counterparty,
+        string[] calldata connectionHops,
+        string calldata version
+    ) external {
+        Channel.Data storage channel = channels[portId][channelId];
+        channel.state = state;
+        channel.ordering = order;
+        channel.counterparty = counterparty;
+        for (uint256 i = 0; i < connectionHops.length; i++) {
+            channel.connection_hops.push(connectionHops[i]);
+        }
+        channel.version = version;
+        updateChannelCommitment(portId, channelId);
+    }
+
     function updateChannelCommitment(string memory portId, string memory channelId) private {
         commitments[IBCCommitment.channelCommitmentKey(portId, channelId)] =
             keccak256(Channel.encode(channels[portId][channelId]));
@@ -238,6 +249,14 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
 
     /* Internal functions */
 
+    function initializeSequences(string memory portId, string memory channelId) internal {
+        nextSequenceSends[portId][channelId] = 1;
+        nextSequenceRecvs[portId][channelId] = 1;
+        nextSequenceAcks[portId][channelId] = 1;
+        commitments[IBCCommitment.nextSequenceRecvCommitmentKey(portId, channelId)] =
+            keccak256(abi.encodePacked((bytes8(uint64(1)))));
+    }
+
     function getCounterpartyHops(string memory connectionId) internal view returns (string[] memory hops) {
         hops = new string[](1);
         hops[0] = connections[connectionId].counterparty.connection_id;
@@ -250,3 +269,15 @@ contract IBCChannelHandshake is IBCStore, IIBCChannelHandshake {
         return identifier;
     }
 }
+
+library IBCChannelLib {
+    function toString(Channel.Order order) internal pure returns (string memory) {
+        if (order == Channel.Order.ORDER_UNORDERED) {
+            return "ORDER_UNORDERED";
+        } else if (order == Channel.Order.ORDER_ORDERED) {
+            return "ORDER_ORDERED";
+        } else {
+            revert("unknown order");
+        }
+    }
+}

contracts/core/04-channel/IIBCChannel.sol

@@ -34,6 +34,19 @@ interface IIBCChannelHandshake {
      * channel, since the other end has been closed.
      */
     function channelCloseConfirm(IBCMsgs.MsgChannelCloseConfirm calldata msg_) external;
+
+    /**
+     * @dev writeChannel writes a channel which has successfully passed the OpenInit or OpenTry handshake step.
+     */
+    function writeChannel(
+        string calldata portId,
+        string calldata channelId,
+        Channel.State state,
+        Channel.Order order,
+        ChannelCounterparty.Data calldata counterparty,
+        string[] calldata connectionHops,
+        string calldata version
+    ) external;
 }
 
 interface IIBCPacket {

contracts/core/25-handler/IBCChannelHandler.sol

@@ -28,10 +28,10 @@ abstract contract IBCChannelHandler is ModuleManager {
         external
         returns (string memory channelId, string memory version)
     {
-        bytes memory res =
-            ibcChannel.functionDelegateCall(abi.encodeWithSelector(IIBCChannelHandshake.channelOpenInit.selector, msg_));
-        channelId = abi.decode(res, (string));
-
+        channelId = abi.decode(
+            ibcChannel.functionDelegateCall(abi.encodeWithSelector(IIBCChannelHandshake.channelOpenInit.selector, msg_)),
+            (string)
+        );
         IIBCModule module = lookupModuleByPort(msg_.portId);
         version = module.onChanOpenInit(
             IIBCModule.MsgOnChanOpenInit({
@@ -44,6 +44,18 @@ abstract contract IBCChannelHandler is ModuleManager {
             })
         );
         claimCapability(channelCapabilityPath(msg_.portId, channelId), address(module));
+        ibcChannel.functionDelegateCall(
+            abi.encodeWithSelector(
+                IIBCChannelHandshake.writeChannel.selector,
+                msg_.portId,
+                channelId,
+                msg_.channel.state,
+                msg_.channel.ordering,
+                msg_.channel.counterparty,
+                msg_.channel.connection_hops,
+                version
+            )
+        );
         emit GeneratedChannelIdentifier(channelId);
         return (channelId, version);
     }
@@ -52,9 +64,10 @@ abstract contract IBCChannelHandler is ModuleManager {
         external
         returns (string memory channelId, string memory version)
     {
-        bytes memory res =
-            ibcChannel.functionDelegateCall(abi.encodeWithSelector(IIBCChannelHandshake.channelOpenTry.selector, msg_));
-        channelId = abi.decode(res, (string));
+        channelId = abi.decode(
+            ibcChannel.functionDelegateCall(abi.encodeWithSelector(IIBCChannelHandshake.channelOpenTry.selector, msg_)),
+            (string)
+        );
         IIBCModule module = lookupModuleByPort(msg_.portId);
         version = module.onChanOpenTry(
             IIBCModule.MsgOnChanOpenTry({
@@ -67,6 +80,18 @@ abstract contract IBCChannelHandler is ModuleManager {
             })
         );
         claimCapability(channelCapabilityPath(msg_.portId, channelId), address(module));
+        ibcChannel.functionDelegateCall(
+            abi.encodeWithSelector(
+                IIBCChannelHandshake.writeChannel.selector,
+                msg_.portId,
+                channelId,
+                msg_.channel.state,
+                msg_.channel.ordering,
+                msg_.channel.counterparty,
+                msg_.channel.connection_hops,
+                version
+            )
+        );
         emit GeneratedChannelIdentifier(channelId);
         return (channelId, version);
     }