feat: replace ics20-1 with ics20-2 #244

gjermundgaraba · 2025-01-28T18:00:09Z

Description

closes: #114
closes: #219
^ let's discuss if this is true, but I think so.

There are a few important things to note about these changes:

IBCDenom has been replaced with an internal "Denom ID" used only to map denoms to an IBCERC20 address. See more details below on reasoning.
The ICS20Transfer flow has been adjusted and tested for middle chain scenarios and should find correct contracts to interact with for both sending and receiving. I would argue Validate and refactor ICS20 sendPacket flow #219 is not an issue in this repo anymore (if it is, we just need to remove the closes: for 219 above.
Additional validation to keep this implementation in line with ibc-go has been added and unit tested.
Multi-denom transfers are supported, but not tested properly here. Add tests for multi-denom transfer #249 will handle that
Some other changes that are worth to point out are commented directly in the code

I have replaced the IBCDenom format from ibc-go (ibc/{HASH_OF_PATH}) to a more gas effecient alternative. This is not a required format for the spec and while it would be nice to keep the same format as ibc-go, the cost is very different, as can be seen in the table below:

Test case	Gas IBC Denom	Gas "Denom Identifier"
base only	42566	1440
2 hops	44776	4276

A lot of this is due to the hex string manipulation, but even reducing that would still be at least 17-20k gas.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
Wrote unit and integration tests.
Added relevant natspec and godoc comments.
Provide a conventional commit message to follow the repository standards.
Re-reviewed Files changed in the GitHub PR explorer.
Review SonarCloud Report in the comment section below once CI passes.

codecov · 2025-01-29T20:13:14Z

Codecov Report

Attention: Patch coverage is 99.29078% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.67%. Comparing base (ec5227e) to head (22c85bf).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
contracts/utils/ICS20Lib.sol	98.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
+ Coverage   99.47%   99.67%   +0.19%     
==========================================
  Files          12       12              
  Lines         572      612      +40     
==========================================
+ Hits          569      610      +41     
+ Misses          3        2       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gjermundgaraba · 2025-02-01T15:43:28Z

contracts/msgs/IICS20TransferMsgs.sol

+        Hop[] hops;
+    }
+
+    // ICS20Transfer payload data structures:


I opted to move all these structures here to avoid circular dependencies. It would be possible to keep some in ICS20Lib, but if some need to be here, I thought it was cleaner to just have all the structures here, which is how we have done it other places too.

gjermundgaraba · 2025-02-01T15:58:37Z

e2e/interchaintestv8/chainconfig/chain_config.go

@@ -19,7 +19,7 @@ func IbcGoChainSpec(name, chainId string) *interchaintest.ChainSpec {
 			Images: []ibc.DockerImage{
 				{
 					Repository: "ghcr.io/cosmos/ibc-go-wasm-simd", // FOR LOCAL IMAGE USE: Docker Image Name
-					Version:    "feat-ibc-eureka",                 // FOR LOCAL IMAGE USE: Docker Image Tag
+					Version:    "gjermund-update-abi-for-ics20v2", // FOR LOCAL IMAGE USE: Docker Image Tag


Can be replaced with the proper one as soon as this is merged and the abigen dependency updated in feat/ibc-eureka.

gjermundgaraba · 2025-02-01T16:08:21Z

test/solidity-ibc/ICS20TransferTest.t.sol

        });

-        // just to prove that it works with the unaltered transfer message


I deleted a bunch of tests that were already covered in the integration tests, to keep the maintenance burden down.

gjermundgaraba · 2025-02-01T16:10:41Z

test/solidity-ibc/IntegrationTest.t.sol

-    string public senderStr;
-    address public receiver;
-    string public receiverStr = "someReceiver";
+    address public defaultSender;


I refactored the integration tests to reduce duplication and to make it easier to reason about which values where used in different scenarios.

srdtrk

Will close as not planned for now

srdtrk · 2025-02-02T21:17:31Z

contracts/ICS20Transfer.sol

@@ -18,7 +18,7 @@ import { MulticallUpgradeable } from "@openzeppelin-upgradeable/utils/MulticallU
 import { ICS20Lib } from "./utils/ICS20Lib.sol";
 import { IBCERC20 } from "./utils/IBCERC20.sol";
 import { Escrow } from "./utils/Escrow.sol";
-import { Bytes } from "@openzeppelin-contracts/utils/Bytes.sol";
+import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";


Suggested change

import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";

import { Strings } from "@openzeppelin-contracts/utils/Strings.sol";

This shouldn't have worked? At least maybe we should make sure it doesn't?

srdtrk · 2025-02-02T21:19:38Z

README.md

-| `multicall/recvPacket` | Receiving _back_ an `ERC20` token. | ~185,876 | ~179,615 |
-| `multicall/ackPacket` | Acknowledging an ICS20 packet. | ~96,436 | ~90,776 |
+| `multicall/recvPacket` | Receiving _back_ an `ERC20` token. | ~185,811 | ~181,461 |
+| `multicall/ackPacket` | Acknowledging an ICS20 packet. | ~82,914 | ~78,926 |


I don't understand why recv packet hasn't changed much but single packet benchmarks improved a lot

srdtrk · 2025-02-02T21:22:24Z

contracts/interfaces/IIBCERC20.sol

-    /// @notice Get the full denom path of the token
-    /// @return the full path of the token's denom
-    function fullDenomPath() external view returns (string memory);
+    /// @notice Get the full denom of the token


Suggested change

/// @notice Get the full denom of the token

/// @notice Get the full IBC denom of the token

This concept shouldn't be confused with ERC20's denom concept, so perhaps the function name should be changed as well

srdtrk · 2025-02-02T21:24:02Z

contracts/interfaces/IICS20Transfer.sol

+    /// @return The IBCERC20 contract address
+    function ibcERC20Contract(IICS20TransferMsgs.Denom calldata denom) external view returns (address);


We should probably make a version of this query which simply takes in a string. (For easier interaction/testing with frontends and ibc-go.)

srdtrk · 2025-02-02T21:29:32Z

contracts/ICS20Transfer.sol

-        return _getICS26Router().sendPacket(ICS20Lib.newMsgSendPacketV1(_msgSender(), msg_));
+        return _getICS26Router().sendPacket(ICS20Lib.newMsgSendPacketV2(_msgSender(), msg_));


I don't think this PR closes #219. We decided that we will attempt a full refactor of the send packet flow. So I think that issue should still stay open. (I gave more reasoning later in this review)

srdtrk · 2025-02-02T21:53:50Z

contracts/utils/ICS20Lib.sol

+    /// @param packetData The packet data to validate
+    /// @return isValid Whether the packet data is valid
+    /// @return reason The reason for invalidity, empty if valid
+    // solhint-disable-next-line code-complexity


bad idea imo

srdtrk · 2025-02-02T21:54:13Z

contracts/utils/ICS20Lib.sol

+    {
+        if (packetData.forwarding.hops.length > 0) {
+            if (packetData.forwarding.hops.length > MAX_HOPS) {
+                return (false, "too many hops");


Why not just revert with a custom error instead of returning an error string and returning a bool? I'd much rather prefer that

srdtrk · 2025-02-02T21:57:45Z

contracts/utils/ICS20Lib.sol

+        IICS20TransferMsgs.Denom memory newDenom =
+            IICS20TransferMsgs.Denom({ base: denom.base, trace: new IICS20TransferMsgs.Hop[](denom.trace.length - 1) });


This is so that we don't mutate the initial denom?

srdtrk · 2025-02-02T21:58:27Z

e2e/interchaintestv8/cosmos_relayer_test.go

+			transferPayload := ics20lib.IICS20TransferMsgsFungibleTokenPacketDataV2{
+				Tokens: []ics20lib.IICS20TransferMsgsToken{
+					{
+						Denom: ics20lib.IICS20TransferMsgsDenom{
+							Base: transferCoin.Denom,
+						},
+						Amount: transferCoin.Amount.BigInt(),
+					},
+				},


This is why #219 is not resolved

srdtrk · 2025-02-03T16:25:52Z

contracts/ICS20Transfer.sol

+            if (returningToSource) {
+                // token is returning to source, it is an IBCERC20 and we must burn the token (not keep it in escrow)
+                IBCERC20(erc20Address).burn(token.amount);
+            }


Maybe an issue for this?

gjermundgaraba added 15 commits January 25, 2025 16:50

initial setup

510c8ac

Merge branch 'main' into gjermund/114-include-support-for-ics20-v2

dc9deb7

ICS20Transfer unit tests running

102bba0

updated abi and abigen with v2 format

e28f715

Addded solidity encoded hex unit test to abigen

73d35ab

Fix wrong destination memo

fac0270

SendTransferMsg with address + fix trace order

bff3b83

integration tests running

cbdcf64

temporary ICS24 tests

995defa

Merge branch 'main' into gjermund/114-include-support-for-ics20-v2

693ab7c

disable some tests for now

0eeb656

Merge branch 'main' into gjermund/114-include-support-for-ics20-v2

6cf4203

Add packet commitment test with V2 format

13eb51c

update e2e tests with ICS20v2

3f449b2

Merge branch 'main' into gjermund/114-include-support-for-ics20-v2

2103122

gjermundgaraba linked an issue Jan 28, 2025 that may be closed by this pull request

Include support for ICS20-v2 #114

Open

4 tasks

gjermundgaraba added 7 commits January 29, 2025 12:44

fixed some failing tests

fb758fb

generated fixtures and fixed benchmark tests

81aff02

Merge branch 'main' into gjermund/114-include-support-for-ics20-v2

59666f5

fix compiler warning

5546b5f

lint

b68558f

fix coverage

4dc5561

abigen

ebb37e4

gjermundgaraba added 6 commits January 30, 2025 07:27

update abigen

888484e

rename IICS20TransferMsgs Token to ERC20Token

4e1bee3

Add denom identifier tests

85d4b19

unit testing

4925560

Merge branch 'main' into gjermund/114-include-support-for-ics20-v2

ed62351

remove circular dependency

1cbe989

gjermundgaraba added 13 commits January 30, 2025 19:43

lint

2f0aa2d

fix test

a0390e7

move types to IICS20TransferMsgs

9a75968

added more validation and testing

c03fefc

add forwarding hop validation

1a560b4

lint

e476eac

refactored integration tests

7ae8912

lint

872be33

cover last cases

333f1fb

remove old tests

41d9fd9

test middle chain scenarios

d0138f4

lint

08d7c34

update readme

dd474c8

gjermundgaraba commented Feb 1, 2025

View reviewed changes

gjermundgaraba added 2 commits February 1, 2025 17:26

update abigen

0c9d54b

update ibc-go version in e2e

7833490

gjermundgaraba marked this pull request as ready for review February 1, 2025 16:41

gjermundgaraba requested a review from srdtrk as a code owner February 1, 2025 16:41

gjermundgaraba added 2 commits February 1, 2025 21:04

update fixtures

25d2cf4

remove unecessary validation

22c85bf

srdtrk reviewed Feb 3, 2025

View reviewed changes

srdtrk closed this Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace ics20-1 with ics20-2 #244

feat: replace ics20-1 with ics20-2 #244

gjermundgaraba commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

gjermundgaraba Feb 1, 2025

gjermundgaraba Feb 1, 2025

gjermundgaraba Feb 1, 2025

gjermundgaraba Feb 1, 2025

srdtrk left a comment

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 2, 2025

srdtrk Feb 3, 2025

		});

		// just to prove that it works with the unaltered transfer message

	import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
	import { Strings } from "@openzeppelin-contracts/utils/Strings.sol";

	/// @notice Get the full denom of the token
	/// @notice Get the full IBC denom of the token

		/// @return The IBCERC20 contract address
		function ibcERC20Contract(IICS20TransferMsgs.Denom calldata denom) external view returns (address);

		return _getICS26Router().sendPacket(ICS20Lib.newMsgSendPacketV1(_msgSender(), msg_));
		return _getICS26Router().sendPacket(ICS20Lib.newMsgSendPacketV2(_msgSender(), msg_));

		IICS20TransferMsgs.Denom memory newDenom =
		IICS20TransferMsgs.Denom({ base: denom.base, trace: new IICS20TransferMsgs.Hop[](denom.trace.length - 1) });

feat: replace ics20-1 with ics20-2 #244

feat: replace ics20-1 with ics20-2 #244

Conversation

gjermundgaraba commented Jan 28, 2025 • edited Loading

Description

codecov bot commented Jan 29, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjermundgaraba commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading