Merge pull request #115 from ComposableFi/nadeem/o2-client-refactor

02-client-refactor added support for multiple consensus states inside a header

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

contracts/core/IBCClient.sol

@@ -6,7 +6,6 @@ import "./IBCHost.sol";
 import "./IBCMsgs.sol";
 
 library IBCClient {
-
     /**
      * @dev createClient creates a new client state and populates it with a given consensus state
      */
@@ -29,20 +28,19 @@ library IBCClient {
     function updateClient(IBCHost host, IBCMsgs.MsgUpdateClient calldata msg_) external {
         host.onlyIBCModule();
         bytes memory clientStateBytes;
-        bytes memory consensusStateBytes;
-        Height.Data memory height;
         bool found;
-    
+
         (clientStateBytes, found) = host.getClientState(msg_.clientId);
         require(found, "clientState not found");
 
-        (clientStateBytes, consensusStateBytes, height) = checkHeaderAndUpdateState(host, msg_.clientId, clientStateBytes, msg_.header);
-
-        //// persist states ////
-        host.setClientState(msg_.clientId, clientStateBytes);
-        host.setConsensusState(msg_.clientId, height, consensusStateBytes);
-        host.setProcessedTime(msg_.clientId, height, block.timestamp);
-        host.setProcessedHeight(msg_.clientId, height, block.number);
+        // this function call is intended to perform:
+        // 1. client message verification
+        // 2. check for duplicate height misbehaviour
+        // 3. if misbehaviour is found, update state accordingly and return
+        // 4. update state in a way that verified headers carrying one or more consensus states can be updated
+        // 5. persist the state internally
+        // 6. return an array of consensus heights
+        verifyClientMessageAndUpdateState(host, msg_.clientId, clientStateBytes, msg_.clientMessage);
     }
 
     // TODO implements
@@ -70,18 +68,18 @@ library IBCClient {
         return (IClient(addr), true);
     }
 
-    function checkHeaderAndUpdateState(
+    function verifyClientMessageAndUpdateState(
         IBCHost host,
         string memory clientId,
         bytes memory clientStateBytes,
-        bytes memory headerBytes
-    ) public returns (bytes memory newClientStateBytes, bytes memory newConsensusStateBytes, Height.Data memory height) {
+        bytes memory clientMessageBytes
+    ) public returns (bool) {
         (bool success, bytes memory res) = address(getClient(host, clientId)).delegatecall(
             abi.encodeWithSelector(
-                IClient.checkHeaderAndUpdateState.selector,
-                host, clientId, clientStateBytes, headerBytes));
+                IClient.verifyClientMessageAndUpdateState.selector,
+                host, clientId, clientStateBytes, clientMessageBytes));
         assert(success);
-        return abi.decode(res, (bytes, bytes, Height.Data));
+        return abi.decode(res, (bool));
     }
 
     function verifyClientState(

contracts/core/IBCMsgs.sol

@@ -21,7 +21,7 @@ library IBCMsgs {
 
     struct MsgUpdateClient {
         string clientId;
-        bytes header;
+        bytes clientMessage;
     }
 
     /// Connection handshake ///

contracts/core/IBFT2Client.sol

@@ -71,15 +71,12 @@ contract IBFT2Client is IClient {
         return (clientState.latest_height, true);
     }
 
-    /**
-     * @dev checkHeaderAndUpdateState checks if the provided header is valid
-     */
-    function checkHeaderAndUpdateState(
+    function verifyClientMessageAndUpdateState(
         IBCHost host,
-        string memory clientId, 
+        string memory clientId,
         bytes memory clientStateBytes,
-        bytes memory headerBytes
-    ) public override view returns (bytes memory newClientStateBytes, bytes memory newConsensusStateBytes, Height.Data memory height) {
+        bytes memory clientMessageBytes
+    ) public override returns (bool) {
         Header.Data memory header;
         ClientState.Data memory clientState;
         ConsensusState.Data memory consensusState;
@@ -89,19 +86,25 @@ contract IBFT2Client is IClient {
         (clientState, ok) = unmarshalClientState(clientStateBytes);
         require(ok, "client state is invalid");
 
-        (header, ok) = unmarshalHeader(headerBytes);
+        (header, ok) = unmarshalHeader(clientMessageBytes);
         require(ok, "header is invalid");
 
         (consensusState, ok) = getConsensusState(host, clientId, header.trusted_height);
         require(ok, "consensusState not found");
 
-        //// check validity ////
+        // check if the provided client message is valid
         ParsedBesuHeader memory parsedHeader = parseBesuHeader(header);
         require(parsedHeader.height.gt(header.trusted_height), "header height <= consensus state height");
         (validators, ok) = verify(consensusState, parsedHeader);
         require(ok, "failed to verify the header");
 
-        //// update ////
+        // check for duplicate height misbehaviour
+
+        // updates state upon misbehaviour, freezing the ClientState.
+        // This method should only be called when misbehaviour is detected
+        // as it does not perform any misbehaviour checks.
+
+        // if client message is verified and there is no misbehaviour, update state
         consensusState.timestamp = parsedHeader.time;
         consensusState.root = abi.encodePacked(
             verifyStorageProof(Bytes.toAddress(clientState.ibc_store_address), parsedHeader.stateRoot, header.account_state_proof)
@@ -111,7 +114,16 @@ contract IBFT2Client is IClient {
         if (parsedHeader.height.gt(clientState.latest_height)) {
             clientState.latest_height = parsedHeader.height;
         }
-        return (marshalClientState(clientState), marshalConsensusState(consensusState), parsedHeader.height);
+
+        host.setClientState(clientId, marshalClientState(clientState));
+        host.setConsensusState(
+            clientId,
+            parsedHeader.height,
+            marshalConsensusState(consensusState)
+        );
+        host.setProcessedTime(clientId, parsedHeader.height, block.timestamp);
+        host.setProcessedHeight(clientId, parsedHeader.height, block.number);
+        return true;
     }
 
     function marshalClientState(ClientState.Data memory clientState) internal pure returns (bytes memory) {

contracts/core/IClient.sol

@@ -20,12 +20,12 @@ interface IClient {
         string calldata clientId
     ) external view returns (Height.Data memory, bool);
 
-    function checkHeaderAndUpdateState(
+    function verifyClientMessageAndUpdateState(
         IBCHost host,
-        string calldata clientId, 
+        string calldata clientId,
         bytes calldata clientStateBytes,
-        bytes calldata headerBytes
-    ) external returns (bytes memory newClientStateBytes, bytes memory newConsensusStateBytes, Height.Data memory height);
+        bytes calldata clientMessageBytes
+    ) external returns (bool);
 
     function verifyClientState(
         IBCHost host,

contracts/core/MockClient.sol

@@ -51,30 +51,51 @@ contract MockClient is IClient {
     }
 
     /**
-     * @dev checkHeaderAndUpdateState checks if the provided header is valid
+     * @dev verifyClientMessageAndUpdateState is intended to perform:
+     * 1. client message verification
+     * 2. check for duplicate height misbehaviour
+     * 3. if misbehaviour is found, update state accordingly and return
+     * 4. update state in a way that verified headers carrying one or more consensus states can be updated
+     * 5. persist the state internally
+     * 6. return an array of consensus heights
      */
-    function checkHeaderAndUpdateState(
-        IBCHost,
-        string memory,
+    function verifyClientMessageAndUpdateState(
+        IBCHost host,
+        string memory clientId,
         bytes memory clientStateBytes,
-        bytes memory headerBytes
-    ) public override pure returns (bytes memory newClientStateBytes, bytes memory newConsensusStateBytes, Height.Data memory height) {
+        bytes memory clientMessageBytes
+    ) public override returns (bool) {
+        // verify clientMessageBytes
+
+        // check for misbehaviour
+
+        // updates state upon misbehaviour, freezing the ClientState.
+        // This method should only be called when misbehaviour is detected
+        // as it does not perform any misbehaviour checks.
+
+        // if client message is verified and there is no misbehaviour, update state
+        Height.Data memory height;
         uint64 timestamp;
         Any.Data memory anyClientState;
         Any.Data memory anyConsensusState;
 
         anyClientState = Any.decode(clientStateBytes);
         require(keccak256(abi.encodePacked(anyClientState.type_url)) == clientStateTypeUrlHash, "invalid client type");
         ClientState.Data memory clientState = ClientState.decode(anyClientState.value);
-        (height, timestamp) = parseHeader(headerBytes);
+        (height, timestamp) = parseHeader(clientMessageBytes);
         if (height.gt(clientState.latest_height)) {
             clientState.latest_height = height;
         }
 
         anyClientState.value = ClientState.encode(clientState);
         anyConsensusState.type_url = "/ibc.lightclients.mock.v1.ConsensusState";
         anyConsensusState.value = ConsensusState.encode(ConsensusState.Data({timestamp: timestamp}));
-        return (Any.encode(anyClientState), Any.encode(anyConsensusState), height);
+
+        host.setClientState(clientId, Any.encode(anyClientState));
+        host.setConsensusState(clientId, height, Any.encode(anyConsensusState));
+        host.setProcessedTime(clientId, height, block.timestamp);
+        host.setProcessedHeight(clientId, height, block.number);
+        return true;
     }
 
     function verifyClientState(