Merge pull request #299 from hyperledger-labs/improve-code-docs

Improve code docs

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

Author: bluele

contracts/apps/20-transfer/ICS20Bank.sol

@@ -41,6 +41,7 @@ contract ICS20Bank is Context, AccessControl, IICS20Bank, IICS20Errors {
             revert ICS20InsufficientBalance(from, fromBalance, amount);
         }
         unchecked {
+            // SAFETY: balance is checked above
             _balances[denom][from] = fromBalance - amount;
         }
         _balances[denom][to] += amount;
@@ -94,6 +95,7 @@ contract ICS20Bank is Context, AccessControl, IICS20Bank, IICS20Errors {
             revert ICS20InsufficientBalance(account, accountBalance, amount);
         }
         unchecked {
+            // SAFETY: balance is checked above
             _balances[denom][account] = accountBalance - amount;
         }
     }

contracts/apps/20-transfer/ICS20Lib.sol

@@ -20,16 +20,16 @@ library ICS20Lib {
     bytes internal constant FAILED_ACKNOWLEDGEMENT_JSON = bytes('{"error":"failed"}');
     bytes32 internal constant KECCAK256_SUCCESSFUL_ACKNOWLEDGEMENT_JSON = keccak256(SUCCESSFUL_ACKNOWLEDGEMENT_JSON);
 
-    uint256 private constant CHAR_DOUBLE_QUOTE = 0x22;
-    uint256 private constant CHAR_SLASH = 0x2f;
+    uint256 private constant CHAR_DOUBLE_QUOTE = 0x22; // '"'
+    uint256 private constant CHAR_SLASH = 0x2f; // "/"
     uint256 private constant CHAR_BACKSLASH = 0x5c;
-    uint256 private constant CHAR_F = 0x66;
-    uint256 private constant CHAR_R = 0x72;
-    uint256 private constant CHAR_N = 0x6e;
-    uint256 private constant CHAR_B = 0x62;
-    uint256 private constant CHAR_T = 0x74;
-    uint256 private constant CHAR_CLOSING_BRACE = 0x7d;
-    uint256 private constant CHAR_M = 0x6d;
+    uint256 private constant CHAR_F = 0x66; // "f"
+    uint256 private constant CHAR_R = 0x72; // "r"
+    uint256 private constant CHAR_N = 0x6e; // "n"
+    uint256 private constant CHAR_B = 0x62; // "b"
+    uint256 private constant CHAR_T = 0x74; // "t"
+    uint256 private constant CHAR_CLOSING_BRACE = 0x7d; // "}"
+    uint256 private constant CHAR_M = 0x6d; // "m"
 
     bytes16 private constant HEX_DIGITS = "0123456789abcdef";
 
@@ -47,6 +47,11 @@ library ICS20Lib {
 
     /**
      * @dev marshalJSON marshals PacketData into JSON bytes with escaping.
+     * @param escapedDenom is the denom string escaped.
+     * @param amount is the amount field.
+     * @param escapedSender is the sender string escaped.
+     * @param escapedReceiver is the receiver string escaped.
+     * @param escapedMemo is the memo string escaped.
      */
     function marshalJSON(
         string memory escapedDenom,
@@ -72,6 +77,10 @@ library ICS20Lib {
 
     /**
      * @dev marshalJSON marshals PacketData into JSON bytes with escaping.
+     * @param escapedDenom is the denom string escaped.
+     * @param amount is the amount field.
+     * @param escapedSender is the sender string escaped.
+     * @param escapedReceiver is the receiver string escaped.
      */
     function marshalJSON(
         string memory escapedDenom,
@@ -94,12 +103,16 @@ library ICS20Lib {
 
     /**
      * @dev unmarshalJSON unmarshals JSON bytes into PacketData.
+     * @param bz the JSON bytes to unmarshal. It must be either of the following JSON formats. It is assumed that string fields are escaped.
+     * 1. {"amount":"<uint256>","denom":"<string>","memo":"<string>","receiver":"<string>","sender":"<string>"}
+     * 2. {"amount":"<uint256>","denom":"<string>","receiver":"<string>","sender":"<string>"}
      */
     function unmarshalJSON(bytes calldata bz) internal pure returns (PacketData memory) {
         PacketData memory pd;
         uint256 pos = 0;
 
         unchecked {
+            // SAFETY: `pos` never overflow because it is always less than `bz.length`.
             if (bytes32(bz[pos:pos + 11]) != bytes32('{"amount":"')) {
                 revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32('{"amount":"'), bytes32(bz[pos:pos + 11]));
             }
@@ -135,35 +148,51 @@ library ICS20Lib {
     }
 
     /**
-     * @dev parseUint256String parses `bz` from a position `pos` to produce a uint256.
+     * @dev parseUint256String parses `bz` from a position `pos` to produce a uint256 value.
+     * The parse will stop parsing when it encounters a non-digit character.
+     * @param bz the byte array to parse.
+     * @param pos the position to start parsing.
+     * @return ret the parsed uint256 value.
+     * @return pos the new position after parsing.
      */
     function parseUint256String(bytes calldata bz, uint256 pos) internal pure returns (uint256, uint256) {
         uint256 ret = 0;
-        unchecked {
-            for (; pos < bz.length; pos++) {
-                uint256 c = uint256(uint8(bz[pos]));
-                if (c < 48 || c > 57) {
-                    break;
-                }
-                ret = ret * 10 + (c - 48);
+        uint256 bzLen = bz.length;
+        for (; pos < bzLen; pos++) {
+            uint256 c = uint256(uint8(bz[pos]));
+            if (c < 48 || c > 57) {
+                break;
             }
-            if (pos >= bz.length || uint256(uint8(bz[pos])) != CHAR_DOUBLE_QUOTE) {
-                revert IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]);
+            unchecked {
+                // SAFETY: we assume that the amount is uint256, so `ret` never overflows.
+                ret = ret * 10 + (c - 48);
             }
+        }
+        if (uint256(uint8(bz[pos])) != CHAR_DOUBLE_QUOTE) {
+            revert IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]);
+        }
+        unchecked {
+            // SAFETY: `pos` is always less than `bz.length`.
             return (ret, pos + 1);
         }
     }
 
     /**
      * @dev parseString parses `bz` from a position `pos` to produce a string.
+     * @param bz the byte array to parse.
+     * @param pos the position to start parsing.
+     * @return parsedStr the parsed string.
+     * @return position the new position after parsing.
      */
     function parseString(bytes calldata bz, uint256 pos) internal pure returns (string memory, uint256) {
+        uint256 bzLen = bz.length;
         unchecked {
-            for (uint256 i = pos; i < bz.length; i++) {
+            // SAFETY: i + 1 <= bzLen <= type(uint256).max
+            for (uint256 i = pos; i < bzLen; i++) {
                 uint256 c = uint256(uint8(bz[i]));
                 if (c == CHAR_DOUBLE_QUOTE) {
                     return (string(bz[pos:i]), i + 1);
-                } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) {
+                } else if (c == CHAR_BACKSLASH && i + 1 < bzLen) {
                     i++;
                     c = uint256(uint8(bz[i]));
                     if (
@@ -178,14 +207,19 @@ library ICS20Lib {
         revert IICS20Errors.ICS20JSONStringUnclosed(bz, pos);
     }
 
+    /**
+     * @dev isEscapedJSONString checks if a string is escaped JSON.
+     */
     function isEscapedJSONString(string calldata s) internal pure returns (bool) {
         bytes memory bz = bytes(s);
-        unchecked {
-            for (uint256 i = 0; i < bz.length; i++) {
+        uint256 bzLen = bz.length;
+        for (uint256 i = 0; i < bzLen; i++) {
+            unchecked {
                 uint256 c = uint256(uint8(bz[i]));
                 if (c == CHAR_DOUBLE_QUOTE) {
                     return false;
-                } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) {
+                } else if (c == CHAR_BACKSLASH && i + 1 < bzLen) {
+                    // SAFETY: i + 1 <= bzLen <= type(uint256).max
                     i++;
                     c = uint256(uint8(bz[i]));
                     if (
@@ -200,29 +234,35 @@ library ICS20Lib {
         return true;
     }
 
+    /**
+     * @dev isEscapeNeededString checks if a given string needs to be escaped.
+     * @param bz the byte array to check.
+     */
     function isEscapeNeededString(bytes memory bz) internal pure returns (bool) {
-        unchecked {
-            for (uint256 i = 0; i < bz.length; i++) {
-                uint256 c = uint256(uint8(bz[i]));
-                if (c == CHAR_DOUBLE_QUOTE) {
-                    return true;
-                }
+        uint256 bzLen = bz.length;
+        for (uint256 i = 0; i < bzLen; i++) {
+            uint256 c = uint256(uint8(bz[i]));
+            if (c == CHAR_DOUBLE_QUOTE) {
+                return true;
             }
         }
         return false;
     }
 
     /**
      * @dev addressToHexString converts an address to a hex string.
+     * @param addr the address to convert.
+     * @return the hex string.
      */
     function addressToHexString(address addr) internal pure returns (string memory) {
         uint256 localValue = uint256(uint160(addr));
         bytes memory buffer = new bytes(42);
         buffer[0] = "0";
         buffer[1] = "x";
         unchecked {
-            for (int256 i = 41; i >= 2; --i) {
-                buffer[uint256(i)] = HEX_DIGITS[localValue & 0xf];
+            // SAFETY: `i` is always greater than or equal to 1.
+            for (uint256 i = 41; i >= 2; --i) {
+                buffer[i] = HEX_DIGITS[localValue & 0xf];
                 localValue >>= 4;
             }
         }
@@ -231,6 +271,8 @@ library ICS20Lib {
 
     /**
      * @dev hexStringToAddress converts a hex string to an address.
+     * @param addrHexString the hex string to convert. It must be 42 characters long and start with "0x".
+     * @return the address and a boolean indicating whether the conversion was successful.
      */
     function hexStringToAddress(string memory addrHexString) internal pure returns (address, bool) {
         bytes memory addrBytes = bytes(addrHexString);
@@ -240,9 +282,10 @@ library ICS20Lib {
             return (address(0), false);
         }
         uint256 addr = 0;
-        unchecked {
-            for (uint256 i = 2; i < 42; i++) {
-                uint256 c = uint256(uint8(addrBytes[i]));
+        for (uint256 i = 2; i < 42; i++) {
+            uint256 c = uint256(uint8(addrBytes[i]));
+            unchecked {
+                // SAFETY: we assume that the address is a valid ethereum addrress, so `addr` never overflows.
                 if (c >= 48 && c <= 57) {
                     addr = addr * 16 + (c - 48);
                 } else if (c >= 97 && c <= 102) {

contracts/core/02-client/IBCClientLib.sol

@@ -8,27 +8,29 @@ library IBCClientLib {
      *   - clientType must be in the form of `^[a-z][a-z0-9-]*[a-z0-9]$`
      */
     function validateClientType(bytes memory clientTypeBytes) internal pure returns (bool) {
-        if (clientTypeBytes.length == 0) {
+        uint256 byteLength = clientTypeBytes.length;
+        if (byteLength == 0) {
             return false;
         }
-        unchecked {
-            for (uint256 i = 0; i < clientTypeBytes.length; i++) {
-                uint256 c = uint256(uint8(clientTypeBytes[i]));
-                if (0x61 <= c && c <= 0x7a) {
-                    // a-z
-                    continue;
-                } else if (c == 0x2d) {
-                    // hyphen cannot be the first or last character
-                    if (i == 0 || i == clientTypeBytes.length - 1) {
+        for (uint256 i = 0; i < byteLength; i++) {
+            uint256 c = uint256(uint8(clientTypeBytes[i]));
+            if (0x61 <= c && c <= 0x7a) {
+                // a-z
+                continue;
+            } else if (c == 0x2d) {
+                // hyphen cannot be the first or last character
+                unchecked {
+                    // SAFETY: `byteLength` is greater than 0
+                    if (i == 0 || i == byteLength - 1) {
                         return false;
                     }
-                    continue;
-                } else if (0x30 <= c && c <= 0x39) {
-                    // 0-9
-                    continue;
-                } else {
-                    return false;
                 }
+                continue;
+            } else if (0x30 <= c && c <= 0x39) {
+                // 0-9
+                continue;
+            } else {
+                return false;
             }
         }
         return true;

contracts/core/04-channel/IBCChannelUpgrade.sol

@@ -340,6 +340,7 @@ abstract contract IBCChannelUpgradeBase is
     function toString(UpgradeHandshakeError err) internal pure returns (string memory) {
         bytes memory result = new bytes(1);
         unchecked {
+            // SAFETY: `err` is always less than or equal to 6, so overflow never occurs
             result[0] = bytes1(uint8(err) + 48);
         }
         return string(result);
@@ -434,6 +435,7 @@ contract IBCChannelUpgradeInitTryAck is IBCChannelUpgradeBase, IIBCChannelUpgrad
             // and abort their out-of-sync upgrade without aborting our own since
             // the error receipt sequence is lower than ours and higher than the counterparty.
             unchecked {
+                // SAFETY: `msg_.counterpartyUpgradeSequence` is always greater than 0, so underflow never occurs
                 writeErrorReceipt(
                     msg_.portId, msg_.channelId, expectedUpgradeSequence - 1, UpgradeHandshakeError.OutOfSync
                 );

contracts/core/24-host/IBCHostLib.sol

@@ -17,22 +17,20 @@ library IBCHostLib {
         if (portIdLength < 2 || portIdLength > 128) {
             return false;
         }
-        unchecked {
-            for (uint256 i = 0; i < portIdLength; i++) {
-                uint256 c = uint256(uint8(portId[i]));
-                // return false if the character is not in one of the following categories:
-                // a-z
-                // 0-9
-                // A-Z
-                // ".", "_", "+", "-"
-                // "#", "[", "]", "<", ">"
-                if (
-                    !(c >= 0x61 && c <= 0x7A) && !(c >= 0x30 && c <= 0x39) && !(c >= 0x41 && c <= 0x5A)
-                        && !(c == 0x2E || c == 0x5F || c == 0x2B || c == 0x2D)
-                        && !(c == 0x23 || c == 0x5B || c == 0x5D || c == 0x3C || c == 0x3E)
-                ) {
-                    return false;
-                }
+        for (uint256 i = 0; i < portIdLength; i++) {
+            uint256 c = uint256(uint8(portId[i]));
+            // return false if the character is not in one of the following categories:
+            // a-z
+            // 0-9
+            // A-Z
+            // ".", "_", "+", "-"
+            // "#", "[", "]", "<", ">"
+            if (
+                !(c >= 0x61 && c <= 0x7A) && !(c >= 0x30 && c <= 0x39) && !(c >= 0x41 && c <= 0x5A)
+                    && !(c == 0x2E || c == 0x5F || c == 0x2B || c == 0x2D)
+                    && !(c == 0x23 || c == 0x5B || c == 0x5D || c == 0x3C || c == 0x3E)
+            ) {
+                return false;
             }
         }
         return true;

tests/foundry/src/ICS20.t.sol

@@ -106,6 +106,18 @@ contract TestICS20 is Test {
         // This should not revert if the input is not a valid hex string.
         ICS20LibTestHelper.hexStringToAddress(any);
     }
+
+    function testParseUint256String() public {
+        bytes memory maxAmountStr = bytes("115792089237316195423570985008687907853269984665640564039457584007913129639935\"");
+        (uint256 amount, uint256 pos) = ICS20LibTestHelper.parseUint256String(maxAmountStr, 0);
+        assertEq(amount, 115792089237316195423570985008687907853269984665640564039457584007913129639935);
+        assertEq(pos, maxAmountStr.length);
+
+        bytes memory minAmountStr = bytes("1\"");
+        (amount, pos) = ICS20LibTestHelper.parseUint256String(minAmountStr, 0);
+        assertEq(amount, 1);
+        assertEq(pos, minAmountStr.length);
+    }
 }
 
 library ICS20LibTestHelper {
@@ -128,4 +140,8 @@ library ICS20LibTestHelper {
     function isEscapedJSONString(string calldata s) public pure returns (bool) {
         return ICS20Lib.isEscapedJSONString(s);
     }
+
+    function parseUint256String(bytes calldata bz, uint256 pos) public pure returns (uint256, uint256) {
+        return ICS20Lib.parseUint256String(bz, pos);
+    }
 }