feat(ics20): receive foreign tokens #25

gjermundgaraba · 2024-08-08T14:22:12Z

No description provided.

gjermundgaraba · 2024-08-08T14:25:51Z

src/ICS20Transfer.sol

+        returns (ICS20Lib.SendPacketData memory)
+    {
+        ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(packet.data);
+        ICS20Lib.SendPacketData memory receivePacketData = ICS20Lib.SendPacketData({


I just wanted to mention why this is being used instead of local variables further down. I implemented this first with a lot of local variables to make things as readable and nice as I could, but encountered some "stack too deep" issue because there were too many local variables. So I ended up using the return value as a way to avoid that issue.

There are a couple of alternatives to this that can be considered:

Splitting up the function more

Using brackets to scope temporary variables along the way so they don't affect the stack too much (can be a bit ugly, but might be more readable than the current way).

Might be some other way to optimise this, but those were the options I could see, at least.

I'm ok with introducing a new function for these purposes. But not ok with introducing new structs ICS20Lib.SendPacketData and ICS20Lib.ReceivePacketData unless we absolutely have to. This is a large addition to complexity that we should remove imo.

Yeah, I understand what you mean. The main reason I ended up with doing that is because of the difference in addresses/string between sender and receiver and figuring out source/not source being kind of opposite. The other option is to only use the ICS20Lib.PacketDataJSON, or add a single slightly expanded version that keeps all fields the same (but would have to extract sender/receiver addresses when they are needed.

Why can't these addresses be just returned as additional return arguments instead of whole structs?

Consider guys that I if we have the stack too deep problem, if we really need the variables the only real solution we have is to split the logic over multiple contract/libraries.

Overall I agree we should always try to keep things as minimal as we can IF we can.

We should use nat spec (just give the function to gpt-4 or whatever AI you use and ask for it, don't spend time doing it manually)

We should create a specific library to handle this unwrapping things

That is also an option. Will look into it and see what makes most sense

srdtrk

Thanks a lot, this is a good start.

srdtrk · 2024-08-09T00:40:52Z

src/ICS20Transfer.sol


        emit ICS20Transfer(packetData);
    }

    function onRecvPacket(OnRecvPacketCallback calldata msg_) external onlyOwner nonReentrant returns (bytes memory) {
-        // TODO Emit error event
+        // Since this function mostly returns acks, also when it fails, the ics26router (the caller) will log the ack
        if (keccak256(abi.encodePacked(msg_.packet.version)) != keccak256(abi.encodePacked(ICS20Lib.ICS20_VERSION))) {
            return ICS20Lib.errorAck(abi.encodePacked("unexpected version: ", msg_.packet.version));


I actually think maybe we should just revert here, since processing an unknown version could be dangerous. Perhaps ask Aditya

Since this is the first check and it returns I was thinking it was OK to just return the ack. I have tried to follow the logic of the ibc-go implementation for acks, where they return errors and in the top level handler an error is just turned into an error ack:

if ackErr = im.keeper.OnRecvPacket(ctx, packet, data); ackErr != nil { ack = channeltypes.NewErrorAcknowledgement(ackErr) im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) return ack }

But happy to get some more input from Aditya as well :)

So a revert would revert the entire tx correct?

In this case, that would leave a second relayer open to submitting the exact same packet again and reaching the same revert.

This is why we return error acks on application error. That way we have a record of processing the packet and the error ack can be processed by the other side. It prevents infinite replays of the same error and also we want to give the sender chain an error ack saying your version is invalid rather than just having it timeout correct?

Are there any scenarios where we would want to revert the entire tx? If not, would it be better revert in ICS20Transfer (instead of returning the ack) and catch any errors in the router? It is a bit ugly, but some reverts might be hard to avoid without adding custom error returns and handling everywhere else.

Inside the ICS20 recv callback? No i don't think so.

Because at that point the packet was a valid receive from the IBC protocol standpoint. So then its just up to the application to tell us whether this packet was processed correctly or not, which it should communicate in the form of an error ack.

Basically:

Error in IBC core recv packet handler -> revert Tx
Error in ICS20 app onRecvPacket callback -> error ACK and commit tx

srdtrk · 2024-08-09T00:43:00Z

src/utils/IBCERC20.sol

+import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
+
+contract IBCERC20 is ERC20, Ownable {
+    // TODO: Figure out naming and symbol for IBC denoms


The denom should be IBC denom that we use in ibc-go. In IBC-go, we also have a name for the tokens (in metadata). So we should just copy that logic from there.

You mean the ibc/{hash(trace+base_denom)} think we use for the denom? We can do that here too, for sure. I just wonder if it is necessary or provide a lot of value, compared to actually having the trace available directly in the name (less hashing to do, less lookups for both us and others).

We must do it here since spec says MUST.

The spec only says It must encode the trace and base denomination to ensure that tokens coming over different paths are not treated as fungible., which the full denom does, but it takes more space than a hash.

I see, but still, these minted tokens only have the name and symbol "IBC". We need to encode the info in the name and symbol right?

I'd actually propose we use ibc-go's implementation regardless as it has been battle tested for so long. I don't want us to do our own implementation and mess something up with some type of collision or a denom that contains slashes or something like that.

Yes, we do need to add the info there. We could add the full denom as the name, as it might be harder to look up the full trace (given that the erc20 itself doesn't contain any link back to the ICS20Transfer contract where that information would most likely be). We could also use a hash as the name and have a separate denom or even trace with hops field like in the new Denom struct.

The symbol should be fine, will do what the ibc-go implementation does

"mess something up with some type of collision or a denom that contains slashes or something like that."
Note that in the EVM world we have to consider that we may event different hash collision problems to consider. Another note for future thoughts and audits.

src/utils/ICS20Lib.sol

srdtrk · 2024-08-09T00:48:37Z

src/ICS20Transfer.sol

+        returns (ICS20Lib.SendPacketData memory)
+    {
+        ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(packet.data);
+        ICS20Lib.SendPacketData memory receivePacketData = ICS20Lib.SendPacketData({


I'm ok with introducing a new function for these purposes. But not ok with introducing new structs ICS20Lib.SendPacketData and ICS20Lib.ReceivePacketData unless we absolutely have to. This is a large addition to complexity that we should remove imo.

srdtrk · 2024-08-09T00:49:00Z

src/ICS20Transfer.sol

@@ -168,4 +153,124 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
            revert ICS20UnexpectedERC20Balance(expectedEndingBalance, actualEndingBalance);
        }
    }
+
+    function _unwrapSendPacketData(IICS26RouterMsgs.Packet calldata packet)


Function should probably be moved.

Currently it cannot (at least not to ICS20Lib), as it depends on looking up _foreignDenomContracts. I suppose we could try to move most of the logic over and do contract dependent stuff here.

srdtrk · 2024-08-09T00:50:35Z

src/ICS20Transfer.sol

@@ -168,4 +153,124 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
            revert ICS20UnexpectedERC20Balance(expectedEndingBalance, actualEndingBalance);
        }
    }
+
+    function _unwrapSendPacketData(IICS26RouterMsgs.Packet calldata packet)


Please add natspec and link to the function it's based on from ibc-go or yui-ibc-solidity

srdtrk · 2024-08-09T00:51:28Z

src/ICS20Transfer.sol

+        return receivePacketData;
+    }
+
+    function _unwrapReceivePacketData(IICS26RouterMsgs.Packet calldata packet)


Same as above:

Should be moved

Should have natspec

Shouldn't introduce new structs

Same comment as above.
Natspec part I will do on both :) pending decision on the other

srdtrk · 2024-08-09T00:56:47Z

src/ICS20Transfer.sol

+
+    function _unwrapReceivePacketData(IICS26RouterMsgs.Packet calldata packet)
+        private
+        returns (ICS20Lib.ReceivePacketData memory, bytes memory)


I feel a bit uneasy using bytes as ack/error handling. What if we allow this function to revert if it fails, and it can be try catch'ed above.

Yeah, I am not happy about it either.
Try/catch would be nice, but it is not possible to try/catch an internal function in Solidity. We could change the router to try/catch instead, but it might be a little weird also.

The try-catch shouldn't be in the router imo. We could keep this for now unless there isn't a better error handling pattern

srdtrk · 2024-08-09T00:59:36Z

src/ICS20Transfer.sol

+            // we are not the source of this token, so we add a denom trace and find or create a token contract
+            receivePacketData.senderChainIsSource = true;
+            bytes memory newDenomPrefix = ICS20Lib.getDenomPrefix(packet.destPort, packet.destChannel);
+            receivePacketData.denom = string(abi.encodePacked(newDenomPrefix, packetData.denom));


Hmm, shouldn't we be following ICS20 specs to determine the IBCDenom?

Hmm, what do you mean? Are we not?

Do you mean the ibc/{hash(trace+base_denom)} stuff? It is not really spec, but an implementation detail of ibc-go. But we could certainly do the same here if we want to.

It is a MUST part of the spec

Not from how I read it. From what I can tell the spec only dictates that the denom is 1. unique and 2. possible to reconstruct into the full denom. Having the full denom does both.

I think we should try to keep consistency between implementation unless we have a reason for not doing that e.g. gas consumption convenience .

I think you're right. I'd still propose we use ibc-go's implementation regardless as it has been battle tested for so long. I don't want us to do our own implementation and mess something up with some type of collision or a denom that contains slashes or something like that.

srdtrk · 2024-08-09T01:01:08Z

src/utils/ICS20Lib.sol

+    /// @dev SendPacketData is a type used to represent the data needed to send a packet.
+    struct SendPacketData {
+        string denom;
+        bool receiverChainIsSource;
+        address erc20Contract;
        address sender;
        string receiver;
+        uint256 amount;
+        string memo;
+    }
+
+    /// @dev SendPacketData is a type used to represent the data needed to send a packet.
+    struct ReceivePacketData {
+        string denom;
+        bool senderChainIsSource;
+        address erc20Contract;
+        string sender;
+        address receiver;
+        uint256 amount;
        string memo;
    }


We shouldn't introduce new structs. I am not convinced this is needed. We can add the extra bool as a return arguments to the functions that produce these structs.

Yeah, the only real difference right now (except the bool, which you mentioned) is the address difference for sender and receiver. I can look into what it would look like to have a single struct and do the respective sender receiver unpacking in each method.

gjermundgaraba · 2024-08-09T08:01:03Z

Thanks a lot, this is a good start.

Thanks for the detailed review @srdtrk, really good stuff! I left some comments and we can hash (no put intended) out the details 💡

sangier

Thanks @gjermundgaraba for all this work! Really great even the use of testFuzz! I left some comments, but if we need this to implement to complete workflow, we can evaluate not to address them immediately.

sangier · 2024-08-09T08:17:40Z

src/ICS20Transfer.sol

+        if (packetData.receiverChainIsSource) {
+            // receiver chain is source: burn the vouchers
+            // TODO: Implement escrow balance tracking (#6)
+            IBCERC20 ibcERC20Contract = IBCERC20(packetData.erc20Contract);


Wondering if we should check the balance before and after the transfer before the burn. Let's say if we have a to deal with a malicious implementation of _transferFrom this may protect us. But it's more a thought future audits.

The _transferFrom that happens before the burn has that check already, actually, and the burn itself is safe as it only happens in our own ERC20 contract.

function _transferFrom(address sender, address receiver, address tokenContract, uint256 amount) private { // we snapshot our current balance of this token uint256 ourStartingBalance = IERC20(tokenContract).balanceOf(receiver); IERC20(tokenContract).safeTransferFrom(sender, receiver, amount); // check what this particular ERC20 implementation actually gave us, since it doesn't // have to be at all related to the _amount uint256 actualEndingBalance = IERC20(tokenContract).balanceOf(receiver); 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); } }

sangier · 2024-08-09T08:24:55Z

src/ICS20Transfer.sol

+        returns (ICS20Lib.SendPacketData memory)
+    {
+        ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(packet.data);
+        ICS20Lib.SendPacketData memory receivePacketData = ICS20Lib.SendPacketData({


Consider guys that I if we have the stack too deep problem, if we really need the variables the only real solution we have is to split the logic over multiple contract/libraries.

Overall I agree we should always try to keep things as minimal as we can IF we can.

We should use nat spec (just give the function to gpt-4 or whatever AI you use and ask for it, don't spend time doing it manually)

We should create a specific library to handle this unwrapping things

sangier · 2024-08-09T08:40:19Z

src/ICS20Transfer.sol

+            // we are not the source of this token, so we add a denom trace and find or create a token contract
+            receivePacketData.senderChainIsSource = true;
+            bytes memory newDenomPrefix = ICS20Lib.getDenomPrefix(packet.destPort, packet.destChannel);
+            receivePacketData.denom = string(abi.encodePacked(newDenomPrefix, packetData.denom));


I think we should try to keep consistency between implementation unless we have a reason for not doing that e.g. gas consumption convenience .

sangier · 2024-08-09T08:42:23Z

src/interfaces/IICS20Transfer.sol

@@ -7,22 +7,21 @@ import { IICS20TransferMsgs } from "../msgs/IICS20TransferMsgs.sol";
 interface IICS20Transfer is IICS20TransferMsgs {
    /// @notice Called when a packet is handled in onSendPacket and a transfer has been initiated
    /// @param packetData The transfer packet data
-    event ICS20Transfer(ICS20Lib.UnwrappedFungibleTokenPacketData packetData);
+    event ICS20Transfer(ICS20Lib.SendPacketData packetData);


That's a general comment. As we have specific files for errors, I have seen many projects having the specific files for events too. We should consider it.

srdtrk · 2024-08-12T04:44:21Z

e2e/interchaintestv8/ibc_eureka_test.go

@@ -602,6 +603,305 @@ func (s *IbcEurekaTestSuite) TestICS20Transfer() {
 	}))
 }

+func (s *IbcEurekaTestSuite) TestICS20TransferWithForeignCoin() {


Add this test to CI please

Ah, forgot that needed to be done manually, will do!

srdtrk · 2024-08-12T04:45:42Z

e2e/interchaintestv8/ibc_eureka_test.go

@@ -60,6 +60,7 @@ type IbcEurekaTestSuite struct {

 	contractAddresses e2esuite.DeployedContracts

+	ethClient        *ethclient.Client


If there is no good reason to move this up, I'd rather not do it. Keeping the test suite simple is a good idea in general. We can define a new client with client, err := ethclient.Dial(eth.GetHostRPCAddress()) whenever need be.

This change also doesn't have anything to do with the reason for this PR. You should only do this if this very much helps you write your new e2e test, which doesn't seem to be the case

OK. It was used in the new test, but I agree that it is better to keep the suite simpler 👍

srdtrk · 2024-08-12T04:48:35Z

e2e/interchaintestv8/ibc_eureka_test.go

@@ -602,6 +603,305 @@ func (s *IbcEurekaTestSuite) TestICS20Transfer() {
 	}))
 }

+func (s *IbcEurekaTestSuite) TestICS20TransferWithForeignCoin() {


It is native to cosmos in a way. So maybe we can come up with another name?

It doesn't strictly have to be, but it is in this case - for sure!
What about TestICS20TransferWithCosmosNativeCoin?

Maybe TestICS20TransferNativeSdkCoin?

That would be consistent and nice :) Will rename to that!

srdtrk · 2024-08-12T04:52:33Z

e2e/interchaintestv8/ibc_eureka_test.go

+				TimeoutTimestamp:   returnPacket.TimeoutTimestamp * 1_000_000_000,
+			},
+			ProofCommitment: []byte("doesn't matter"),
+			ProofHeight:     clientState.LatestHeight,


The mock client checks latest height?

Nope :) fixing this

srdtrk · 2024-08-12T05:05:45Z

src/ICS20Transfer.sol

+
+        if (packetData.amount == 0) {
+            revert ICS20InvalidAmount(packetData.amount);
+        }


Can ypu create a issue to create validation functions for the structs that we define? So that we don't need to write these by hand each time

srdtrk · 2024-08-12T05:09:01Z

src/ICS20Transfer.sol

+        // transfer the tokens to the receiver
+        IERC20(packetData.erc20Contract).safeTransfer(receiver, packetData.amount);


Should we return an error ack if this call fails?

Yes, that would probably make the most sense. The reason it is not done right now is because with safeTransfer, which is a Library, we can't do try/catch (because it is technically not an external call right here). There is a little "hack" we can do instead, by wrapping the transfer in an external function and calling it with this.wrappedSafeTransfer. If you prefer that I can go ahead and implement that :)

How is this not an external call? It seems we are calling packetData.erc20Contract?

Note that the mint function is being invoked on a contract at packetData.erc20Contract, which is external to the current contract executing the onRecvPacket function.
A typical way to handle external calls is to check if they succeed or not.
Kind like: bool success: IERC20(packetData.erc20Contract).safeTransfer(receiver, packetData.amount); and then check the return value of success.

I know, but since the safeTransfer is an actually a library wrapper, we are not the doing the external call here. The safeTransfer library is doing it :P So we don't get an external call to try/catch, nor a return value.

Error (2536): Try can only be used with external function calls and contract creation calls. --> src/ICS20Transfer.sol:121:13: | 121 | try IERC20(packetData.erc20Contract).safeTransfer(receiver, packetData.amount) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

src/utils/ICS20Lib.sol

srdtrk · 2024-08-12T05:17:24Z

src/ICS20Transfer.sol

+
+        receivePacketData.originatorChainIsSource = !ICS20Lib.hasPrefix(denomBz, denomPrefix);
+
+        if (receivePacketData.originatorChainIsSource) {


I wonder if we can somehow utilize an enum, since it seems that there is a lot of nested if logic. But I'm happy with what we have at the moment if you don't see a natural way of using enums.

Yeah, I don't love it either. I will give it a shot and see if there is a smoother way to do this.

Co-authored-by: srdtrk <[email protected]>

srdtrk

ci passing

gjermundgaraba added 3 commits August 8, 2024 15:57

feat(ics20): add support for receiving and sending foreign tokens

dc5c54b

Merge branch 'main' into gjermund/feat-ics20-receive-foreign-tokens

8a8bbd7

genesis

d34f5fb

gjermundgaraba requested review from sangier and srdtrk August 8, 2024 14:22

gjermundgaraba commented Aug 8, 2024

View reviewed changes

srdtrk reviewed Aug 9, 2024

View reviewed changes

sangier reviewed Aug 9, 2024

View reviewed changes

gjermundgaraba added 3 commits August 9, 2024 14:24

test(e2e): ICS20 transfer cosmos native coin to evm

3e898ff

Merge branch 'main' into gjermund/feat-ics20-receive-foreign-tokens

9e7b56b

refactor as per code review

f10629e

srdtrk reviewed Aug 12, 2024

View reviewed changes

Merge branch 'main' into gjermund/feat-ics20-receive-foreign-tokens

0a0904b

gjermundgaraba changed the title ~~feat(ics20): receive foreign tokens (solidity only)~~ feat(ics20): receive foreign tokens Aug 12, 2024

gjermundgaraba and others added 7 commits August 12, 2024 09:47

Update src/utils/ICS20Lib.sol

0c751d0

Co-authored-by: srdtrk <[email protected]>

Add TestICS20TransferWithForeignCoin to CI

7c4f112

pr fixes

f04b476

Merge branch 'main' into gjermund/feat-ics20-receive-foreign-tokens

4b2566c

Fix tests

602932d

lint

26ae55f

ci: fix test suite failing

970e4c7

srdtrk approved these changes Aug 13, 2024

View reviewed changes

srdtrk merged commit f1dffcc into main Aug 13, 2024
8 checks passed

srdtrk deleted the gjermund/feat-ics20-receive-foreign-tokens branch August 13, 2024 08:27

		@@ -60,6 +60,7 @@ type IbcEurekaTestSuite struct {

		contractAddresses e2esuite.DeployedContracts

		ethClient *ethclient.Client

		// transfer the tokens to the receiver
		IERC20(packetData.erc20Contract).safeTransfer(receiver, packetData.amount);


		receivePacketData.originatorChainIsSource = !ICS20Lib.hasPrefix(denomBz, denomPrefix);

		if (receivePacketData.originatorChainIsSource) {

feat(ics20): receive foreign tokens #25

feat(ics20): receive foreign tokens #25

Conversation

gjermundgaraba commented Aug 8, 2024 • edited Loading

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

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

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

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 Aug 9, 2024

sangier 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

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

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

gjermundgaraba commented Aug 8, 2024 •

edited

Loading