refactor(contracts): refactor contracts and consistent use of NatSpec (…

…#27)

* docs: natspec

* test: simplified tests

* refactor

contracts/script/SP1ICS07Tendermint.s.sol

@@ -6,7 +6,7 @@ import {Script} from "forge-std/Script.sol";
 import {stdJson} from "forge-std/StdJson.sol";
 import {SP1ICS07Tendermint} from "../src/SP1ICS07Tendermint.sol";
 import {SP1Verifier} from "@sp1-contracts/SP1Verifier.sol";
-import {ICS07Tendermint} from "ibc-lite-shared/ics07-tendermint/ICS07Tendermint.sol";
+import {ICS07Tendermint} from "../src/ics07-tendermint/ICS07Tendermint.sol";
 
 struct SP1ICS07TendermintGenesisJson {
     bytes trustedClientState;

contracts/src/SP1ICS07Tendermint.sol

@@ -1,30 +1,33 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.13;
 
-import {ICS07Tendermint} from "ibc-lite-shared/ics07-tendermint/ICS07Tendermint.sol";
+import {ICS07Tendermint} from "./ics07-tendermint/ICS07Tendermint.sol";
 import {ISP1Verifier} from "@sp1-contracts/ISP1Verifier.sol";
 import "forge-std/console.sol";
 
 /// @title SP1ICS07Tendermint
 /// @author srdtrk
 /// @notice This contract implements an ICS07 IBC tendermint light client.
+/// @custom:poc This is a proof of concept implementation.
 contract SP1ICS07Tendermint {
     /// @notice The verification key for the program.
     bytes32 public ics07UpdateClientProgramVkey;
-    // @notice The SP1 verifier contract.
+    /// @notice The SP1 verifier contract.
     ISP1Verifier public verifier;
 
-    // @notice The ICS07Tendermint client state
+    /// @notice The ICS07Tendermint client state
     ICS07Tendermint.ClientState public clientState;
-    // @notice The mapping from height to consensus state
+    /// @notice The mapping from height to consensus state
     mapping(uint64 => ICS07Tendermint.ConsensusState) public consensusStates;
 
     /// Allowed clock drift in nanoseconds
     uint64 public constant ALLOWED_SP1_CLOCK_DRIFT = 30_000_000_000_000; // 30000 seconds
 
-    // @notice The constructor sets the program verification key.
-    // @param _ics07ProgramVkey The verification key for the program.
-    // @param _verifier The address of the SP1 verifier contract.
+    /// @notice The constructor sets the program verification key and the initial client and consensus states.
+    /// @param _ics07ProgramVkey The verification key for the program.
+    /// @param _verifier The address of the SP1 verifier contract.
+    /// @param _clientState The encoded initial client state.
+    /// @param _consensusState The encoded initial consensus state.
     constructor(
         bytes32 _ics07ProgramVkey,
         address _verifier,
@@ -44,6 +47,8 @@ contract SP1ICS07Tendermint {
         ] = consensusState;
     }
 
+    /// @notice Returns the client state.
+    /// @return The client state.
     function getClientState()
         public
         view
@@ -52,13 +57,17 @@ contract SP1ICS07Tendermint {
         return clientState;
     }
 
+    /// @notice Returns the consensus state at the given revision height.
+    /// @param revisionHeight The revision height.
+    /// @return The consensus state at the given revision height.
     function getConsensusState(
         uint64 revisionHeight
     ) public view returns (ICS07Tendermint.ConsensusState memory) {
         return consensusStates[revisionHeight];
     }
 
     /// @notice The entrypoint for verifying the proof.
+    /// @dev This function verifies the public values and forwards the proof to the SP1 verifier.
     /// @param proof The encoded proof.
     /// @param publicValues The encoded public values.
     function verifyIcs07UpdateClientProof(
@@ -119,7 +128,7 @@ contract SP1ICS07Tendermint {
             .new_consensus_state;
     }
 
-    /// The public value output for the sp1 program.
+    /// @notice The public value output for the sp1 program.
     struct SP1ICS07TendermintOutput {
         /// The trusted consensus state.
         ICS07Tendermint.ConsensusState trusted_consensus_state;
@@ -133,7 +142,7 @@ contract SP1ICS07Tendermint {
         ICS07Tendermint.Height new_height;
     }
 
-    /// The environment output for the sp1 program.
+    /// @notice The environment output for the sp1 program.
     struct Env {
         /// The chain ID of the chain that the client is tracking.
         string chain_id;

contracts/src/ics07-tendermint/ICS07Tendermint.sol

@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.13;
+
+/// @title ICS07Tendermint
+/// @author srdtrk
+/// @notice Defines shared types for ICS07Tendermint implementations.
+contract ICS07Tendermint {
+    /// @notice Height of the counterparty chain
+    struct Height {
+        /// Previously known as "epoch"
+        uint64 revision_number;
+        /// The height of a block
+        uint64 revision_height;
+    }
+
+    /// Fraction of validator overlap needed to update header
+    struct TrustThreshold {
+        /// Numerator of the fraction
+        uint64 numerator;
+        /// Denominator of the fraction
+        uint64 denominator;
+    }
+
+    /// @notice Defines the ICS07Tendermint ClientState for ibc-lite
+    struct ClientState {
+        /// Chain ID
+        string chain_id;
+        /// Fraction of validator overlap needed to update header
+        TrustThreshold trust_level;
+        /// Latest height the client was updated to
+        Height latest_height;
+        /// duration of the period since the LatestTimestamp during which the
+        /// submitted headers are valid for upgrade
+        uint64 trusting_period;
+        /// duration of the staking unbonding period
+        uint64 unbonding_period;
+        /// whether or not client is frozen (due to misbehavior)
+        bool is_frozen;
+    }
+
+    /// Defines the Tendermint light client's consensus state at some height.
+    struct ConsensusState {
+        /// timestamp that corresponds to the block height in which the ConsensusState
+        /// was stored.
+        uint64 timestamp;
+        /// commitment root (i.e app hash)
+        bytes root;
+        /// next validators hash
+        bytes next_validators_hash;
+    }
+}

contracts/test/SP1ICS07Tendermint.t.sol

@@ -5,7 +5,7 @@ import "forge-std/console.sol";
 import {Test} from "forge-std/Test.sol";
 import {stdJson} from "forge-std/StdJson.sol";
 import {stdError} from "forge-std/StdError.sol";
-import {ICS07Tendermint} from "ibc-lite-shared/ics07-tendermint/ICS07Tendermint.sol";
+import {ICS07Tendermint} from "../src/ics07-tendermint/ICS07Tendermint.sol";
 import {SP1ICS07Tendermint} from "../src/SP1ICS07Tendermint.sol";
 import {SP1Verifier} from "@sp1-contracts/SP1Verifier.sol";
 import {SP1MockVerifier} from "@sp1-contracts/SP1MockVerifier.sol";
@@ -49,33 +49,29 @@ contract SP1ICS07TendermintTest is Test {
             mockFixture.trustedConsensusState
         );
 
-        (
-            string memory chain_id,
-            ICS07Tendermint.TrustThreshold memory trust_level,
-            ICS07Tendermint.Height memory latest_height,
-            uint64 trusting_period,
-            uint64 unbonding_period,
-            bool is_frozen
-        ) = mockIcs07Tendermint.clientState();
-
-        assert(keccak256(bytes(chain_id)) == keccak256(bytes("mocha-4")));
-        assert(trust_level.numerator == 1);
-        assert(trust_level.denominator == 3);
-        assert(latest_height.revision_number == 4);
-        assert(latest_height.revision_height == 2110658);
-        assert(trusting_period == 1_209_600_000_000_000);
-        assert(unbonding_period == 1_209_600_000_000_000);
-        assert(is_frozen == false);
-
-        (
-            uint64 timestamp,
-            bytes memory root,
-            bytes memory next_validators_hash
-        ) = mockIcs07Tendermint.consensusStates(2110658);
-
-        assert(timestamp > 0);
-        assert(root.length > 0);
-        assert(next_validators_hash.length > 0);
+        ICS07Tendermint.ClientState memory clientState = mockIcs07Tendermint
+            .getClientState();
+
+        assert(
+            keccak256(bytes(clientState.chain_id)) ==
+                keccak256(bytes("mocha-4"))
+        );
+        assert(clientState.trust_level.numerator == 1);
+        assert(clientState.trust_level.denominator == 3);
+        assert(clientState.latest_height.revision_number == 4);
+        assert(clientState.latest_height.revision_height == 2110658);
+        assert(clientState.trusting_period == 1_209_600_000_000_000);
+        assert(clientState.unbonding_period == 1_209_600_000_000_000);
+        assert(clientState.is_frozen == false);
+
+        ICS07Tendermint.ConsensusState
+            memory consensusState = mockIcs07Tendermint.getConsensusState(
+                2110658
+            );
+
+        assert(consensusState.timestamp > 0);
+        assert(consensusState.root.length > 0);
+        assert(consensusState.next_validators_hash.length > 0);
     }
 
     function loadFixture(
@@ -121,23 +117,20 @@ contract SP1ICS07TendermintTest is Test {
             fixture.publicValues
         );
 
-        (
-            string memory chain_id,
-            ICS07Tendermint.TrustThreshold memory trust_level,
-            ICS07Tendermint.Height memory latest_height,
-            uint64 trusting_period,
-            uint64 unbonding_period,
-            bool is_frozen
-        ) = ics07Tendermint.clientState();
-
-        assert(keccak256(bytes(chain_id)) == keccak256(bytes("mocha-4")));
-        assert(trust_level.numerator == 1);
-        assert(trust_level.denominator == 3);
-        assert(latest_height.revision_number == 4);
-        assert(latest_height.revision_height == 2110668);
-        assert(trusting_period == 1_209_600_000_000_000);
-        assert(unbonding_period == 1_209_600_000_000_000);
-        assert(is_frozen == false);
+        ICS07Tendermint.ClientState memory clientState = ics07Tendermint
+            .getClientState();
+
+        assert(
+            keccak256(bytes(clientState.chain_id)) ==
+                keccak256(bytes("mocha-4"))
+        );
+        assert(clientState.trust_level.numerator == 1);
+        assert(clientState.trust_level.denominator == 3);
+        assert(clientState.latest_height.revision_number == 4);
+        assert(clientState.latest_height.revision_height == 2110668);
+        assert(clientState.trusting_period == 1_209_600_000_000_000);
+        assert(clientState.unbonding_period == 1_209_600_000_000_000);
+        assert(clientState.is_frozen == false);
 
         ICS07Tendermint.ConsensusState memory consensusState = ics07Tendermint
             .getConsensusState(2110668);
@@ -158,23 +151,20 @@ contract SP1ICS07TendermintTest is Test {
             fixture.publicValues
         );
 
-        (
-            string memory chain_id,
-            ICS07Tendermint.TrustThreshold memory trust_level,
-            ICS07Tendermint.Height memory latest_height,
-            uint64 trusting_period,
-            uint64 unbonding_period,
-            bool is_frozen
-        ) = mockIcs07Tendermint.clientState();
-
-        assert(keccak256(bytes(chain_id)) == keccak256(bytes("mocha-4")));
-        assert(trust_level.numerator == 1);
-        assert(trust_level.denominator == 3);
-        assert(latest_height.revision_number == 4);
-        assert(latest_height.revision_height == 2110668);
-        assert(trusting_period == 1_209_600_000_000_000);
-        assert(unbonding_period == 1_209_600_000_000_000);
-        assert(is_frozen == false);
+        ICS07Tendermint.ClientState memory clientState = mockIcs07Tendermint
+            .getClientState();
+
+        assert(
+            keccak256(bytes(clientState.chain_id)) ==
+                keccak256(bytes("mocha-4"))
+        );
+        assert(clientState.trust_level.numerator == 1);
+        assert(clientState.trust_level.denominator == 3);
+        assert(clientState.latest_height.revision_number == 4);
+        assert(clientState.latest_height.revision_height == 2110668);
+        assert(clientState.trusting_period == 1_209_600_000_000_000);
+        assert(clientState.unbonding_period == 1_209_600_000_000_000);
+        assert(clientState.is_frozen == false);
 
         ICS07Tendermint.ConsensusState
             memory consensusState = mockIcs07Tendermint.getConsensusState(