refactor: using require instead of revert where possible (#107)

.github/setup/action.yml

@@ -7,8 +7,6 @@ runs:
       uses: "actions/checkout@v4"
     - name: "Install Foundry"
       uses: "foundry-rs/foundry-toolchain@v1"
-      with:
-        version: nightly-f8aa4afec04cc0b7d364a5d78f0cde9e64fd14bf
     - name: "Install Bun"
       uses: "oven-sh/setup-bun@v2"
     - name: "Install the Node.js dependencies"

.github/workflows/foundry.yml

@@ -80,8 +80,10 @@ jobs:
         uses: ./.github/setup
 
       - name: "Run coverage"
-        run: "bash scripts/checks/coverage.sh"
+        run: forge coverage --report lcov
 
       - uses: codecov/codecov-action@v4
+        with:
+          exclude: test
         env:
-          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
+          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

.solhint.json

@@ -9,6 +9,7 @@
     "named-parameters-mapping": "warn",
     "no-console": "off",
     "not-rely-on-time": "off",
-    "one-contract-per-file": "off"
+    "one-contract-per-file": "off",
+    "gas-custom-errors": "off"
   }
 }

foundry.toml

@@ -5,7 +5,7 @@
   block_timestamp = 1_680_220_800 # March 31, 2023 at 00:00 GMT
   bytecode_hash = "none"
   evm_version = "cancun"
-  fuzz = { runs = 1_000_00 }
+  fuzz = { runs = 100_000 }
   gas_reports = ["*"]
   optimizer = true
   optimizer_runs = 10_000

scripts/E2ETestDeploy.s.sol

@@ -5,7 +5,7 @@ pragma solidity ^0.8.28;
     This script is used for end-to-end testing with SP1_PROVER=network.
 */
 
-// solhint-disable gas-custom-errors,custom-errors
+// solhint-disable custom-errors
 
 import { stdJson } from "forge-std/StdJson.sol";
 import { Script } from "forge-std/Script.sol";

src/ICS02Client.sol

@@ -23,9 +23,7 @@ contract ICS02Client is IICS02Client, IICS02ClientErrors, Ownable {
     /// @param clientType The client type
     /// @return The next client identifier
     function getNextClientId(string calldata clientType) private returns (string memory) {
-        if (!IBCIdentifiers.validateClientType(clientType)) {
-            revert IBCInvalidClientType(clientType);
-        }
+        require(IBCIdentifiers.validateClientType(clientType), IBCInvalidClientType(clientType));
 
         uint32 seq = nextClientSeq[clientType];
         nextClientSeq[clientType] = seq + 1;
@@ -35,19 +33,15 @@ contract ICS02Client is IICS02Client, IICS02ClientErrors, Ownable {
     /// @inheritdoc IICS02Client
     function getCounterparty(string calldata clientId) public view returns (CounterpartyInfo memory) {
         CounterpartyInfo memory counterpartyInfo = counterpartyInfos[clientId];
-        if (bytes(counterpartyInfo.clientId).length == 0) {
-            revert IBCCounterpartyClientNotFound(clientId);
-        }
+        require(bytes(counterpartyInfo.clientId).length != 0, IBCCounterpartyClientNotFound(clientId));
 
         return counterpartyInfo;
     }
 
     /// @inheritdoc IICS02Client
     function getClient(string calldata clientId) public view returns (ILightClient) {
         ILightClient client = clients[clientId];
-        if (client == ILightClient(address(0))) {
-            revert IBCClientNotFound(clientId);
-        }
+        require(address(client) != address(0), IBCClientNotFound(clientId));
 
         return client;
     }

src/ICS20Transfer.sol

@@ -42,9 +42,7 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
 
     /// @inheritdoc IICS20Transfer
     function sendTransfer(SendTransferMsg calldata msg_) external override returns (uint32) {
-        if (msg_.amount == 0) {
-            revert ICS20InvalidAmount(msg_.amount);
-        }
+        require(msg_.amount > 0, ICS20InvalidAmount(msg_.amount));
 
         // we expect the denom to be an erc20 address
         address contractAddress = ICS20Lib.mustHexStringToAddress(msg_.denom);
@@ -83,19 +81,16 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
         // The packet sender has to be the contract itself.
         // Because of the packetData massaging we do in sendTransfer to convert the amount to sdkCoin, we don't allow
         // this function to be called by anyone else. They could end up transferring a larger amount than intended.
-        if (msg_.sender != address(this)) {
-            revert ICS20UnauthorizedPacketSender(msg_.sender);
-        }
+        require(msg_.sender == address(this), ICS20UnauthorizedPacketSender(msg_.sender));
 
-        if (keccak256(bytes(msg_.payload.version)) != keccak256(bytes(ICS20Lib.ICS20_VERSION))) {
-            revert ICS20UnexpectedVersion(ICS20Lib.ICS20_VERSION, msg_.payload.version);
-        }
+        require(
+            keccak256(bytes(msg_.payload.version)) == keccak256(bytes(ICS20Lib.ICS20_VERSION)),
+            ICS20UnexpectedVersion(ICS20Lib.ICS20_VERSION, msg_.payload.version)
+        );
 
         ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(msg_.payload.value);
 
-        if (packetData.amount == 0) {
-            revert ICS20InvalidAmount(packetData.amount);
-        }
+        require(packetData.amount > 0, ICS20InvalidAmount(packetData.amount));
 
         address sender = ICS20Lib.mustHexStringToAddress(packetData.sender);
 
@@ -137,7 +132,6 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
             return ICS20Lib.errorAck(abi.encodePacked("invalid receiver: ", packetData.receiver));
         }
 
-        // TODO: Implement escrow balance tracking (#6)
         if (originatorChainIsSource) {
             // sender is source, so we mint vouchers
             // NOTE: getReceiveTokenContractAndSource has already created a contract if it didn't exist
@@ -202,9 +196,10 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
         uint256 expectedEndingBalance = ourStartingBalance + amount;
         // a very strange ERC20 may trigger this condition, if we didn't have this we would
         // underflow, so it's mostly just an error message printer
-        if (actualEndingBalance <= ourStartingBalance || actualEndingBalance != expectedEndingBalance) {
-            revert ICS20UnexpectedERC20Balance(expectedEndingBalance, actualEndingBalance);
-        }
+        require(
+            actualEndingBalance > ourStartingBalance && actualEndingBalance == expectedEndingBalance,
+            ICS20UnexpectedERC20Balance(expectedEndingBalance, actualEndingBalance)
+        );
     }
 
     /// @notice For a send packet, get the ERC20 address for the token and whether the originator chain is the source
@@ -234,9 +229,7 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
             // receiving chain is source of the token, so we've received and mapped this token before
             string memory ibcDenom = ICS20Lib.toIBCDenom(packetData.denom);
             erc20Address = address(_ibcDenomContracts[ibcDenom]);
-            if (erc20Address == address(0)) {
-                revert ICS20DenomNotFound(packetData.denom);
-            }
+            require(erc20Address != address(0), ICS20DenomNotFound(packetData.denom));
         }
         return (erc20Address, originatorChainIsSource);
     }

src/ICS26Router.sol

@@ -39,9 +39,7 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
     /// @inheritdoc IICS26Router
     function getIBCApp(string calldata portId) public view returns (IIBCApp) {
         IIBCApp app = apps[portId];
-        if (app == IIBCApp(address(0))) {
-            revert IBCAppNotFound(portId);
-        }
+        require(address(app) != address(0), IBCAppNotFound(portId));
         return app;
     }
 
@@ -59,12 +57,8 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
             newPortId = Strings.toHexString(app);
         }
 
-        if (apps[newPortId] != IIBCApp(address(0))) {
-            revert IBCPortAlreadyExists(newPortId);
-        }
-        if (!IBCIdentifiers.validatePortIdentifier(bytes(newPortId))) {
-            revert IBCInvalidPortIdentifier(newPortId);
-        }
+        require(address(apps[newPortId]) == address(0), IBCPortAlreadyExists(newPortId));
+        require(IBCIdentifiers.validatePortIdentifier(bytes(newPortId)), IBCInvalidPortIdentifier(newPortId));
 
         apps[newPortId] = IIBCApp(app);
 
@@ -77,17 +71,15 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
     /// @inheritdoc IICS26Router
     function sendPacket(MsgSendPacket calldata msg_) external nonReentrant returns (uint32) {
         // TODO: Support multi-payload packets #93
-        if (msg_.payloads.length != 1) {
-            revert IBCMultiPayloadPacketNotSupported();
-        }
+        require(msg_.payloads.length == 1, IBCMultiPayloadPacketNotSupported());
         Payload calldata payload = msg_.payloads[0];
 
         string memory counterpartyId = ICS02_CLIENT.getCounterparty(msg_.sourceChannel).clientId;
 
         // TODO: validate all identifiers
-        if (msg_.timeoutTimestamp <= block.timestamp) {
-            revert IBCInvalidTimeoutTimestamp(msg_.timeoutTimestamp, block.timestamp);
-        }
+        require(
+            msg_.timeoutTimestamp > block.timestamp, IBCInvalidTimeoutTimestamp(msg_.timeoutTimestamp, block.timestamp)
+        );
 
         uint32 sequence = IBC_STORE.nextSequenceSend(msg_.sourceChannel);
 
@@ -120,19 +112,18 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
     /// @inheritdoc IICS26Router
     function recvPacket(MsgRecvPacket calldata msg_) external nonReentrant {
         // TODO: Support multi-payload packets #93
-        if (msg_.packet.payloads.length != 1) {
-            revert IBCMultiPayloadPacketNotSupported();
-        }
+        require(msg_.packet.payloads.length == 1, IBCMultiPayloadPacketNotSupported());
         Payload calldata payload = msg_.packet.payloads[0];
 
         IICS02ClientMsgs.CounterpartyInfo memory cInfo = ICS02_CLIENT.getCounterparty(msg_.packet.destChannel);
-        if (keccak256(bytes(cInfo.clientId)) != keccak256(bytes(msg_.packet.sourceChannel))) {
-            revert IBCInvalidCounterparty(cInfo.clientId, msg_.packet.sourceChannel);
-        }
-
-        if (msg_.packet.timeoutTimestamp <= block.timestamp) {
-            revert IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, block.timestamp);
-        }
+        require(
+            keccak256(bytes(cInfo.clientId)) == keccak256(bytes(msg_.packet.sourceChannel)),
+            IBCInvalidCounterparty(cInfo.clientId, msg_.packet.sourceChannel)
+        );
+        require(
+            msg_.packet.timeoutTimestamp > block.timestamp,
+            IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, block.timestamp)
+        );
 
         bytes memory commitmentPath =
             ICS24Host.packetCommitmentPathCalldata(msg_.packet.sourceChannel, msg_.packet.sequence);
@@ -157,9 +148,7 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
                 relayer: _msgSender()
             })
         );
-        if (acks[0].length == 0) {
-            revert IBCAsyncAcknowledgementNotSupported();
-        }
+        require(acks[0].length != 0, IBCAsyncAcknowledgementNotSupported());
 
         writeAcknowledgement(msg_.packet, acks);
 
@@ -173,21 +162,21 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
     /// @inheritdoc IICS26Router
     function ackPacket(MsgAckPacket calldata msg_) external nonReentrant {
         // TODO: Support multi-payload packets #93
-        if (msg_.packet.payloads.length != 1) {
-            revert IBCMultiPayloadPacketNotSupported();
-        }
+        require(msg_.packet.payloads.length == 1, IBCMultiPayloadPacketNotSupported());
         Payload calldata payload = msg_.packet.payloads[0];
 
         IICS02ClientMsgs.CounterpartyInfo memory cInfo = ICS02_CLIENT.getCounterparty(msg_.packet.sourceChannel);
-        if (keccak256(bytes(cInfo.clientId)) != keccak256(bytes(msg_.packet.destChannel))) {
-            revert IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel);
-        }
+        require(
+            keccak256(bytes(cInfo.clientId)) == keccak256(bytes(msg_.packet.destChannel)),
+            IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel)
+        );
 
         // this will revert if the packet commitment does not exist
         bytes32 storedCommitment = IBC_STORE.deletePacketCommitment(msg_.packet);
-        if (storedCommitment != ICS24Host.packetCommitmentBytes32(msg_.packet)) {
-            revert IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet));
-        }
+        require(
+            storedCommitment == ICS24Host.packetCommitmentBytes32(msg_.packet),
+            IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet))
+        );
 
         bytes memory commitmentPath =
             ICS24Host.packetAcknowledgementCommitmentPathCalldata(msg_.packet.destChannel, msg_.packet.sequence);
@@ -224,21 +213,21 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
     /// @inheritdoc IICS26Router
     function timeoutPacket(MsgTimeoutPacket calldata msg_) external nonReentrant {
         // TODO: Support multi-payload packets #93
-        if (msg_.packet.payloads.length != 1) {
-            revert IBCMultiPayloadPacketNotSupported();
-        }
+        require(msg_.packet.payloads.length == 1, IBCMultiPayloadPacketNotSupported());
         Payload calldata payload = msg_.packet.payloads[0];
 
         IICS02ClientMsgs.CounterpartyInfo memory cInfo = ICS02_CLIENT.getCounterparty(msg_.packet.sourceChannel);
-        if (keccak256(bytes(cInfo.clientId)) != keccak256(bytes(msg_.packet.destChannel))) {
-            revert IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel);
-        }
+        require(
+            keccak256(bytes(cInfo.clientId)) == keccak256(bytes(msg_.packet.destChannel)),
+            IBCInvalidCounterparty(cInfo.clientId, msg_.packet.destChannel)
+        );
 
         // this will revert if the packet commitment does not exist
         bytes32 storedCommitment = IBC_STORE.deletePacketCommitment(msg_.packet);
-        if (storedCommitment != ICS24Host.packetCommitmentBytes32(msg_.packet)) {
-            revert IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet));
-        }
+        require(
+            storedCommitment == ICS24Host.packetCommitmentBytes32(msg_.packet),
+            IBCPacketCommitmentMismatch(storedCommitment, ICS24Host.packetCommitmentBytes32(msg_.packet))
+        );
 
         bytes memory receiptPath =
             ICS24Host.packetReceiptCommitmentPathCalldata(msg_.packet.destChannel, msg_.packet.sequence);
@@ -250,9 +239,10 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
         });
 
         uint256 counterpartyTimestamp = ICS02_CLIENT.getClient(msg_.packet.sourceChannel).membership(nonMembershipMsg);
-        if (counterpartyTimestamp < msg_.packet.timeoutTimestamp) {
-            revert IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, counterpartyTimestamp);
-        }
+        require(
+            counterpartyTimestamp >= msg_.packet.timeoutTimestamp,
+            IBCInvalidTimeoutTimestamp(msg_.packet.timeoutTimestamp, counterpartyTimestamp)
+        );
 
         getIBCApp(payload.sourcePort).onTimeoutPacket(
             IIBCAppCallbacks.OnTimeoutPacketCallback({

src/utils/IBCStore.sol

@@ -35,26 +35,26 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable {
     /// @inheritdoc IIBCStore
     function commitPacket(IICS26RouterMsgs.Packet memory packet) public onlyOwner {
         bytes32 path = ICS24Host.packetCommitmentKeyCalldata(packet.sourceChannel, packet.sequence);
-        if (commitments[path] != 0) {
-            revert IBCPacketCommitmentAlreadyExists(
+        require(
+            commitments[path] == 0,
+            IBCPacketCommitmentAlreadyExists(
                 ICS24Host.packetCommitmentPathCalldata(packet.sourceChannel, packet.sequence)
-            );
-        }
+            )
+        );
 
         bytes32 commitment = ICS24Host.packetCommitmentBytes32(packet);
-        emit PacketCommitted(path, commitment);
         commitments[path] = commitment;
+        emit PacketCommitted(path, commitment);
     }
 
     /// @inheritdoc IIBCStore
     function deletePacketCommitment(IICS26RouterMsgs.Packet memory packet) public onlyOwner returns (bytes32) {
         bytes32 path = ICS24Host.packetCommitmentKeyCalldata(packet.sourceChannel, packet.sequence);
         bytes32 commitment = commitments[path];
-        if (commitment == 0) {
-            revert IBCPacketCommitmentNotFound(
-                ICS24Host.packetCommitmentPathCalldata(packet.sourceChannel, packet.sequence)
-            );
-        }
+        require(
+            commitment != 0,
+            IBCPacketCommitmentNotFound(ICS24Host.packetCommitmentPathCalldata(packet.sourceChannel, packet.sequence))
+        );
 
         delete commitments[path];
         return commitment;
@@ -63,23 +63,25 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable {
     /// @inheritdoc IIBCStore
     function setPacketReceipt(IICS26RouterMsgs.Packet memory packet) public onlyOwner {
         bytes32 path = ICS24Host.packetReceiptCommitmentKeyCalldata(packet.destChannel, packet.sequence);
-        if (commitments[path] != 0) {
-            revert IBCPacketReceiptAlreadyExists(
+        require(
+            commitments[path] == 0,
+            IBCPacketReceiptAlreadyExists(
                 ICS24Host.packetReceiptCommitmentPathCalldata(packet.destChannel, packet.sequence)
-            );
-        }
+            )
+        );
 
         commitments[path] = ICS24Host.PACKET_RECEIPT_SUCCESSFUL_KECCAK256;
     }
 
     /// @inheritdoc IIBCStore
     function commitPacketAcknowledgement(IICS26RouterMsgs.Packet memory packet, bytes[] memory acks) public onlyOwner {
         bytes32 path = ICS24Host.packetAcknowledgementCommitmentKeyCalldata(packet.destChannel, packet.sequence);
-        if (commitments[path] != 0) {
-            revert IBCPacketAcknowledgementAlreadyExists(
+        require(
+            commitments[path] == 0,
+            IBCPacketAcknowledgementAlreadyExists(
                 ICS24Host.packetAcknowledgementCommitmentPathCalldata(packet.destChannel, packet.sequence)
-            );
-        }
+            )
+        );
 
         bytes32 commitment = ICS24Host.packetAcknowledgementCommitmentBytes32(acks);
         emit AckCommitted(path, commitment);