From 00000006aafdc186fe00c595027c687a2a5bf4c7 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 14:31:54 +0800 Subject: [PATCH 01/17] imp: using require in ics26 --- src/ICS26Router.sol | 88 ++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 49 deletions(-) diff --git a/src/ICS26Router.sol b/src/ICS26Router.sol index 908ab4b..0fd66d9 100644 --- a/src/ICS26Router.sol +++ b/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(apps[newPortId] == IIBCApp(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({ From 000000051e76a947685acfeb47a941ad6961efcc Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 14:41:28 +0800 Subject: [PATCH 02/17] imp: using require in ICS20 --- src/ICS20Transfer.sol | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/ICS20Transfer.sol b/src/ICS20Transfer.sol index 03be1ee..d91a1e9 100644 --- a/src/ICS20Transfer.sol +++ b/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); } From 000000018f00e6ce35cc0a2782da9d49d3d438fd Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 14:45:20 +0800 Subject: [PATCH 03/17] imp: using require in ics02 --- src/ICS02Client.sol | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/ICS02Client.sol b/src/ICS02Client.sol index ab561d1..c040aea 100644 --- a/src/ICS02Client.sol +++ b/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,9 +33,7 @@ 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; } @@ -45,9 +41,7 @@ contract ICS02Client is IICS02Client, IICS02ClientErrors, Ownable { /// @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; } From 00000000f1ae2c9a852bbeba764b8057ae962561 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 14:47:18 +0800 Subject: [PATCH 04/17] imp: using require in ics24 --- src/utils/ICS24Host.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/utils/ICS24Host.sol b/src/utils/ICS24Host.sol index 2adede6..62ee2ce 100644 --- a/src/utils/ICS24Host.sol +++ b/src/utils/ICS24Host.sol @@ -167,9 +167,7 @@ library ICS24Host { /// @param path The path to append /// @return The prefixed path function prefixedPath(bytes[] memory merklePrefix, bytes memory path) internal pure returns (bytes[] memory) { - if (merklePrefix.length == 0) { - revert IICS24HostErrors.InvalidMerklePrefix(merklePrefix); - } + require(merklePrefix.length > 0, IICS24HostErrors.InvalidMerklePrefix(merklePrefix)); merklePrefix[merklePrefix.length - 1] = abi.encodePacked(merklePrefix[merklePrefix.length - 1], path); return merklePrefix; From 00000008e707268da07002356f667a043cdfaed5 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 14:58:49 +0800 Subject: [PATCH 05/17] imp: used require in ics20lib --- src/utils/ICS20Lib.sol | 67 +++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/src/utils/ICS20Lib.sol b/src/utils/ICS20Lib.sol index abd4208..244bc4f 100644 --- a/src/utils/ICS20Lib.sol +++ b/src/utils/ICS20Lib.sol @@ -106,40 +106,43 @@ library ICS20Lib { uint256 pos = 0; unchecked { - if (bytes32(bz[pos:pos + 10]) != bytes32("{\"denom\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"denom\":\""), bytes32(bz[pos:pos + 10])); - } + require( + bytes32(bz[pos:pos + 10]) == bytes32("{\"denom\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"denom\":\""), bytes32(bz[pos:pos + 10])) + ); (pd.denom, pos) = parseString(bz, pos + 10); - if (bytes32(bz[pos:pos + 11]) != bytes32(",\"amount\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"amount\":\""), bytes32(bz[pos:pos + 11])); - } + require( + bytes32(bz[pos:pos + 11]) == bytes32(",\"amount\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32("{\"amount\":\""), bytes32(bz[pos:pos + 11])) + ); (pd.amount, pos) = parseUint256String(bz, pos + 11); - if (bytes32(bz[pos:pos + 11]) != bytes32(",\"sender\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"sender\":\""), bytes32(bz[pos:pos + 11])); - } + require( + bytes32(bz[pos:pos + 11]) == bytes32(",\"sender\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"sender\":\""), bytes32(bz[pos:pos + 11])) + ); (pd.sender, pos) = parseString(bz, pos + 11); - if (bytes32(bz[pos:pos + 13]) != bytes32(",\"receiver\":\"")) { - revert IICS20Errors.ICS20JSONUnexpectedBytes( - pos, bytes32(",\"receiver\":\""), bytes32(bz[pos:pos + 13]) - ); - } + require( + bytes32(bz[pos:pos + 13]) == bytes32(",\"receiver\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"receiver\":\""), bytes32(bz[pos:pos + 13])) + ); (pd.receiver, pos) = parseString(bz, pos + 13); // check if the memo field is present, if not, we leave it empty if (pos != bz.length - 1 && uint256(uint8(bz[pos + 2])) == CHAR_M) { - if (bytes32(bz[pos:pos + 9]) != bytes32(",\"memo\":\"")) { - // solhint-disable-next-line max-line-length - revert IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"memo\":\""), bytes32(bz[pos:pos + 9])); - } + require( + bytes32(bz[pos:pos + 9]) == bytes32(",\"memo\":\""), + IICS20Errors.ICS20JSONUnexpectedBytes(pos, bytes32(",\"memo\":\""), bytes32(bz[pos:pos + 9])) + ); (pd.memo, pos) = parseString(bz, pos + 9); } - if (pos != bz.length - 1 || uint256(uint8(bz[pos])) != CHAR_CLOSING_BRACE) { - revert IICS20Errors.ICS20JSONClosingBraceNotFound(pos, bz[pos]); - } + require( + pos == bz.length - 1 && uint256(uint8(bz[pos])) == CHAR_CLOSING_BRACE, + IICS20Errors.ICS20JSONClosingBraceNotFound(pos, bz[pos]) + ); } return pd; @@ -160,9 +163,10 @@ library ICS20Lib { } ret = ret * 10 + (c - 48); } - if (pos >= bz.length || uint256(uint8(bz[pos])) != CHAR_DOUBLE_QUOTE) { - revert IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]); - } + require( + pos < bz.length && uint256(uint8(bz[pos])) == CHAR_DOUBLE_QUOTE, + IICS20Errors.ICS20JSONStringClosingDoubleQuoteNotFound(pos, bz[pos]) + ); return (ret, pos + 1); } } @@ -181,12 +185,11 @@ library ICS20Lib { } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) { i++; c = uint256(uint8(bz[i])); - if ( - c != CHAR_DOUBLE_QUOTE && c != CHAR_SLASH && c != CHAR_BACKSLASH && c != CHAR_F && c != CHAR_R - && c != CHAR_N && c != CHAR_B && c != CHAR_T - ) { - revert IICS20Errors.ICS20JSONInvalidEscape(i, bz[i]); - } + require( + c == CHAR_DOUBLE_QUOTE || c == CHAR_SLASH || c == CHAR_BACKSLASH || c == CHAR_F || c == CHAR_R + || c == CHAR_N || c == CHAR_B || c == CHAR_T, + IICS20Errors.ICS20JSONInvalidEscape(i, bz[i]) + ); } } } @@ -267,9 +270,7 @@ library ICS20Lib { /// @return address the converted address function mustHexStringToAddress(string memory addrHexString) internal pure returns (address) { (address addr, bool success) = hexStringToAddress(addrHexString); - if (!success) { - revert IICS20Errors.ICS20InvalidAddress(addrHexString); - } + require(success, IICS20Errors.ICS20InvalidAddress(addrHexString)); return addr; } From 0000000fb5f898740091087c92b41f1f7807d82b Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 15:02:01 +0800 Subject: [PATCH 06/17] imp: using require on ibcstore --- src/utils/IBCStore.sol | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/utils/IBCStore.sol b/src/utils/IBCStore.sol index 839503c..25908d9 100644 --- a/src/utils/IBCStore.sol +++ b/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,11 +63,12 @@ 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; } @@ -75,11 +76,12 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable { /// @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); From 0000000f0429b2e0969f662c509429b85cc49f29 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 15:08:50 +0800 Subject: [PATCH 07/17] style: fix linter complaint --- .solhint.json | 3 ++- scripts/E2ETestDeploy.s.sol | 2 +- test/IbcIdentifiersTest.t.sol | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.solhint.json b/.solhint.json index d11bf3f..2d5ddc4 100644 --- a/.solhint.json +++ b/.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" } } diff --git a/scripts/E2ETestDeploy.s.sol b/scripts/E2ETestDeploy.s.sol index 91ad363..5a7e63e 100644 --- a/scripts/E2ETestDeploy.s.sol +++ b/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"; diff --git a/test/IbcIdentifiersTest.t.sol b/test/IbcIdentifiersTest.t.sol index be60be0..075acbe 100644 --- a/test/IbcIdentifiersTest.t.sol +++ b/test/IbcIdentifiersTest.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.28; -// solhint-disable gas-custom-errors,max-line-length +// solhint-disable max-line-length import { Test } from "forge-std/Test.sol"; import { IBCIdentifiers } from "../src/utils/IBCIdentifiers.sol"; From e71e8f6f1ca48cfa09982859d9a4776588dded8e Mon Sep 17 00:00:00 2001 From: srdtrk <59252793+srdtrk@users.noreply.github.com> Date: Mon, 18 Nov 2024 15:20:56 +0800 Subject: [PATCH 08/17] imp: review item --- src/ICS26Router.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ICS26Router.sol b/src/ICS26Router.sol index 0fd66d9..a765598 100644 --- a/src/ICS26Router.sol +++ b/src/ICS26Router.sol @@ -57,7 +57,7 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua newPortId = Strings.toHexString(app); } - require(apps[newPortId] == IIBCApp(address(0)), IBCPortAlreadyExists(newPortId)); + require(address(apps[newPortId]) == address(0), IBCPortAlreadyExists(newPortId)); require(IBCIdentifiers.validatePortIdentifier(bytes(newPortId)), IBCInvalidPortIdentifier(newPortId)); apps[newPortId] = IIBCApp(app); From 0000000874a86bc0d8112a78943cec6b3b43b667 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 15:36:38 +0800 Subject: [PATCH 09/17] test: added test for untested line --- test/ICS20LibTest.t.sol | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/ICS20LibTest.t.sol b/test/ICS20LibTest.t.sol index 8980aca..ee6107d 100644 --- a/test/ICS20LibTest.t.sol +++ b/test/ICS20LibTest.t.sol @@ -5,6 +5,8 @@ pragma solidity ^0.8.28; import { Test } from "forge-std/Test.sol"; import { ICS20Lib } from "../src/utils/ICS20Lib.sol"; +import { Vm } from "forge-std/Vm.sol"; +import { IICS20Errors } from "../src/errors/IICS20Errors.sol"; contract ICS20LibTest is Test { struct IBCDenomTestCase { @@ -34,7 +36,7 @@ contract ICS20LibTest is Test { } } - function test_unmarshalJSON() public view { + function test_unmarshalJSON() public { // ICS20Lib marshalled json with memo bytes memory jsonBz = ICS20Lib.marshalJSON("denom", 42, "sender", "receiver", "memo"); ICS20Lib.PacketDataJSON memory packetData = this.unmarshalJSON(jsonBz); @@ -71,6 +73,11 @@ contract ICS20LibTest is Test { assertEq(packetData.sender, "sender3"); assertEq(packetData.receiver, "receiver3"); assertEq(packetData.memo, ""); + + // Test with a broken JSON string without memo + jsonBz = "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\\,\"receiver\":\"receiver3\"}"; + vm.expectRevert(abi.encodeWithSelector(IICS20Errors.ICS20JSONInvalidEscape.selector, 50, bytes1(0x2c))); + packetData = this.unmarshalJSON(jsonBz); } function unmarshalJSON(bytes calldata bz) external pure returns (ICS20Lib.PacketDataJSON memory) { From 00000006afd98fba11e5ecaf127287c5bdf280a8 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 15:40:43 +0800 Subject: [PATCH 10/17] imp: removed unused import --- test/ICS20LibTest.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/ICS20LibTest.t.sol b/test/ICS20LibTest.t.sol index ee6107d..ef208c8 100644 --- a/test/ICS20LibTest.t.sol +++ b/test/ICS20LibTest.t.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.28; import { Test } from "forge-std/Test.sol"; import { ICS20Lib } from "../src/utils/ICS20Lib.sol"; -import { Vm } from "forge-std/Vm.sol"; import { IICS20Errors } from "../src/errors/IICS20Errors.sol"; contract ICS20LibTest is Test { From 00000009ad9dcc751b0b4e9a8a13e2f79d1f2d2f Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 15:58:32 +0800 Subject: [PATCH 11/17] imp: removed unused functions from ics20lib --- src/utils/ICS20Lib.sol | 40 ---------------------------------------- 1 file changed, 40 deletions(-) diff --git a/src/utils/ICS20Lib.sol b/src/utils/ICS20Lib.sol index 244bc4f..aa8da93 100644 --- a/src/utils/ICS20Lib.sol +++ b/src/utils/ICS20Lib.sol @@ -196,46 +196,6 @@ library ICS20Lib { revert IICS20Errors.ICS20JSONStringUnclosed(bz, pos); } - /// @notice isEscapedJSONString checks if a string is escaped JSON. - /// @param s string - /// @return true if the string is escaped JSON - function isEscapedJSONString(string calldata s) private pure returns (bool) { - bytes memory bz = bytes(s); - unchecked { - for (uint256 i = 0; i < bz.length; i++) { - uint256 c = uint256(uint8(bz[i])); - if (c == CHAR_DOUBLE_QUOTE) { - return false; - } else if (c == CHAR_BACKSLASH && i + 1 < bz.length) { - i++; - c = uint256(uint8(bz[i])); - if ( - c != CHAR_DOUBLE_QUOTE && c != CHAR_SLASH && c != CHAR_BACKSLASH && c != CHAR_F && c != CHAR_R - && c != CHAR_N && c != CHAR_B && c != CHAR_T - ) { - return false; - } - } - } - } - return true; - } - - /// @notice isEscapeNeededString checks if a string needs to be escaped. - /// @param bz bytes - /// @return true if the string needs to be escaped - function isEscapeNeededString(bytes memory bz) private 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; - } - } - } - return false; - } - /// @notice hexStringToAddress converts a hex string to an address. /// @param addrHexString hex address string /// @return address value From 000000020b1193b8e3cebade2f17a9a7eeee50f4 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 16:30:29 +0800 Subject: [PATCH 12/17] ci: removed bash script --- .github/workflows/foundry.yml | 7 +++++-- scripts/checks/coverage.sh | 10 ---------- 2 files changed, 5 insertions(+), 12 deletions(-) delete mode 100644 scripts/checks/coverage.sh diff --git a/.github/workflows/foundry.yml b/.github/workflows/foundry.yml index c45fffa..ccd7123 100644 --- a/.github/workflows/foundry.yml +++ b/.github/workflows/foundry.yml @@ -80,8 +80,11 @@ jobs: uses: ./.github/setup - name: "Run coverage" - run: "bash scripts/checks/coverage.sh" + run: | + forge coverage --report lcov + # Remove zero hits + sed -i '/,0/d' lcov.info - uses: codecov/codecov-action@v4 env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} \ No newline at end of file + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} diff --git a/scripts/checks/coverage.sh b/scripts/checks/coverage.sh deleted file mode 100644 index 4bd9569..000000000 --- a/scripts/checks/coverage.sh +++ /dev/null @@ -1,10 +0,0 @@ -#!/usr/bin/env bash - -set -euo pipefail - -# Foundry coverage -forge coverage --report lcov -# Remove zero hits -sed -i '/,0/d' lcov.info - -# Reports are then uploaded to Codecov automatically by workflow, and merged. From 0000000282e924aa6c5adcc25845c2c06bba5c97 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 16:48:52 +0800 Subject: [PATCH 13/17] ci: removed version pin and removed redundant steps --- .github/setup/action.yml | 2 -- .github/workflows/foundry.yml | 4 ---- foundry.toml | 2 +- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/.github/setup/action.yml b/.github/setup/action.yml index 0061068..cdc7c51 100644 --- a/.github/setup/action.yml +++ b/.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" diff --git a/.github/workflows/foundry.yml b/.github/workflows/foundry.yml index ccd7123..c9d8675 100644 --- a/.github/workflows/foundry.yml +++ b/.github/workflows/foundry.yml @@ -16,7 +16,6 @@ jobs: name: "lint" runs-on: "ubuntu-latest" steps: - - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup @@ -32,7 +31,6 @@ jobs: name: "build" runs-on: "ubuntu-latest" steps: - - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup @@ -49,7 +47,6 @@ jobs: needs: ["lint", "build"] runs-on: "ubuntu-latest" steps: - - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup @@ -75,7 +72,6 @@ jobs: needs: ["test"] runs-on: "ubuntu-latest" steps: - - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup diff --git a/foundry.toml b/foundry.toml index b9d2939..8fdfb10 100644 --- a/foundry.toml +++ b/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 From 00000001e93389b52a47be811d402188bbbab717 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 16:50:37 +0800 Subject: [PATCH 14/17] ci: fix --- .github/workflows/foundry.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/foundry.yml b/.github/workflows/foundry.yml index c9d8675..ccd7123 100644 --- a/.github/workflows/foundry.yml +++ b/.github/workflows/foundry.yml @@ -16,6 +16,7 @@ jobs: name: "lint" runs-on: "ubuntu-latest" steps: + - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup @@ -31,6 +32,7 @@ jobs: name: "build" runs-on: "ubuntu-latest" steps: + - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup @@ -47,6 +49,7 @@ jobs: needs: ["lint", "build"] runs-on: "ubuntu-latest" steps: + - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup @@ -72,6 +75,7 @@ jobs: needs: ["test"] runs-on: "ubuntu-latest" steps: + - uses: actions/checkout@v4 - name: "Set up environment" uses: ./.github/setup From 00000001afd104a331ac3e40e9a04550eac820fc Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 17:33:18 +0800 Subject: [PATCH 15/17] ci: testing other theories --- .github/workflows/foundry.yml | 2 +- test/ICS20LibTest.t.sol | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/foundry.yml b/.github/workflows/foundry.yml index ccd7123..747e0b5 100644 --- a/.github/workflows/foundry.yml +++ b/.github/workflows/foundry.yml @@ -83,7 +83,7 @@ jobs: run: | forge coverage --report lcov # Remove zero hits - sed -i '/,0/d' lcov.info + # sed -i '/,0/d' lcov.info - uses: codecov/codecov-action@v4 env: diff --git a/test/ICS20LibTest.t.sol b/test/ICS20LibTest.t.sol index ef208c8..6114647 100644 --- a/test/ICS20LibTest.t.sol +++ b/test/ICS20LibTest.t.sol @@ -56,7 +56,7 @@ contract ICS20LibTest is Test { // Test with a manual JSON string with memo jsonBz = - "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\",\"memo\":\"memo3\"}"; + bytes("{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\",\"memo\":\"memo3\"}"); packetData = this.unmarshalJSON(jsonBz); assertEq(packetData.denom, "denom3"); assertEq(packetData.amount, 43); @@ -65,7 +65,7 @@ contract ICS20LibTest is Test { assertEq(packetData.memo, "memo3"); // Test with a manual JSON string without memo - jsonBz = "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\"}"; + jsonBz = bytes("{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\"}"); packetData = this.unmarshalJSON(jsonBz); assertEq(packetData.denom, "denom3"); assertEq(packetData.amount, 43); @@ -74,7 +74,7 @@ contract ICS20LibTest is Test { assertEq(packetData.memo, ""); // Test with a broken JSON string without memo - jsonBz = "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\\,\"receiver\":\"receiver3\"}"; + jsonBz = bytes("{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\\,\"receiver\":\"receiver3\"}"); vm.expectRevert(abi.encodeWithSelector(IICS20Errors.ICS20JSONInvalidEscape.selector, 50, bytes1(0x2c))); packetData = this.unmarshalJSON(jsonBz); } From 0000000a1a8a9df782e4f6f6fd1c12583b9f3f63 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 17:36:08 +0800 Subject: [PATCH 16/17] style: ran forge fmt --- test/ICS20LibTest.t.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/ICS20LibTest.t.sol b/test/ICS20LibTest.t.sol index 6114647..7d33e64 100644 --- a/test/ICS20LibTest.t.sol +++ b/test/ICS20LibTest.t.sol @@ -55,8 +55,9 @@ contract ICS20LibTest is Test { assertEq(packetData.memo, ""); // Test with a manual JSON string with memo - jsonBz = - bytes("{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\",\"memo\":\"memo3\"}"); + jsonBz = bytes( + "{\"denom\":\"denom3\",\"amount\":\"43\",\"sender\":\"sender3\",\"receiver\":\"receiver3\",\"memo\":\"memo3\"}" + ); packetData = this.unmarshalJSON(jsonBz); assertEq(packetData.denom, "denom3"); assertEq(packetData.amount, 43); From 00000003568c910299738612497859d19217b581 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Mon, 18 Nov 2024 17:48:52 +0800 Subject: [PATCH 17/17] ci: fixed codecov --- .github/workflows/foundry.yml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/foundry.yml b/.github/workflows/foundry.yml index 747e0b5..d9a3e57 100644 --- a/.github/workflows/foundry.yml +++ b/.github/workflows/foundry.yml @@ -80,11 +80,10 @@ jobs: uses: ./.github/setup - name: "Run coverage" - run: | - forge coverage --report lcov - # Remove zero hits - # sed -i '/,0/d' lcov.info + run: forge coverage --report lcov - uses: codecov/codecov-action@v4 + with: + exclude: test env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}