Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Fix Sending Tokens Back #19

Merged

Conversation

AdityaSripal
Copy link
Contributor

Quick PR with my suspicion of what went wrong in router logic to prevent tokens from being correctly sent back.

I haven't tested this or written any test cases.

The routing logic needs to basically reconstruct the denom that would be constructed in RecvPacket, which wasn't happening in the case where router chain was source of funds in first hop.

See here:

https://github.com/cosmos/ibc-go/blob/main/modules/apps/transfer/keeper/relay.go#L229

I've basically copied over that code, please test manually and write test cases for this logic before merging.

Thanks!

@jtieri
Copy link
Contributor

jtieri commented Mar 16, 2022

Hey Aditya, thanks for taking some time to address the issue!

This was part of the originally reported issue which we have fixed in our dev branch. Without this fix packets trying to be sent from C->B->A would always timeout going C->B

99ba746

I did exactly what you have in this PR which I also took from ibc-go. With these changes transfer packets will succeed in going C->B, with the appropriate acknowledgement, but going B->A fails every time with this error being logged on the Gaia fork we are using for Chain B.

[90m2:19PM�[0m �[32mINF�[0m acknowledgement written �[36mdst_channel=�[0mchannel-1 �[36mdst_port=�[0mtransfer �[36mmodule=�[0mx/ibc/channel �[36msequence=�[0m"marshaling error: json: unsupported type: func() uint64" �[36msrc_channel=�[0mchannel-0 �[36msrc_port=�[0mtransfer

Here is the full log from Gaia (B) going C->B:

�[90m2:19PM�[0m �[32mINF�[0m client state updated �[36mclient-id=�[0m07-tendermint-1 �[36mheight=�[0m2-88 �[36mmodule=�[0mx/ibc/client
�[90m2:19PM�[0m �[32mINF�[0m packet received �[36mdst_channel=�[0mchannel-1 �[36mdst_port=�[0mtransfer �[36mmodule=�[0mx/ibc/channel �[36msequence=�[0m1 �[36msrc_channel=�[0mchannel-0 �[36msrc_port=�[0mtransfer
�[90m2:19PM�[0m �[32mINF�[0m packet sent �[36mdst_channel=�[0mchannel-0 �[36mdst_port=�[0mtransfer �[36mmodule=�[0mx/ibc/channel �[36msequence=�[0m2 �[36msrc_channel=�[0mchannel-1 �[36msrc_port=�[0mtransfer
�[90m2:19PM�[0m �[32mINF�[0m acknowledgement written �[36mdst_channel=�[0mchannel-1 �[36mdst_port=�[0mtransfer �[36mmodule=�[0mx/ibc/channel �[36msequence=�[0m"marshaling error: json: unsupported type: func() uint64" �[36msrc_channel=�[0mchannel-0 �[36msrc_port=�[0mtransfer

My method for testing all of this is using a branch on the go-relayer that pulls down a forked gaia with the dev branch on the packet-forward-middleware

https://github.com/cosmos/relayer/blob/justin/ibc-router-demo/dev-env-demo

@AdityaSripal
Copy link
Contributor Author

AdityaSripal commented Mar 17, 2022

marshaling error: json: unsupported type: func() uint64

This is just a logging error. It's not a real bug. We've fixed it now in cosmos/ibc-go#1130 so I would ignore this. Double check that there is a real bug. Are you sure that the tokens are not actually sent back? Did you query the original address to see if it received the tokens.


Hmm so when I tried the dev-env-demo on main, I got the forward direction to work correctly but not the reverse direction.

When I tried it on your branch, I wasn't able to get the forward direction to work either.

I'm assuming I have something wrong with my setup and you have your forward direction correctly depositing the IBC tokens to final destination and that you are not able to receive the packet on the second hop in the reverse direction. So i will just mention what I would do with your setup.

A couple directions to start debugging this:

  1. Are you sure that the packet is actually being sent on the router chain? Double check this by querying for a packet commitment on the channel from ibc-1 to ibc-0. If it isn't sent, that's the problem; investigate why,
  2. If the packet is sent, is the relayer sending the RecvPacket to ibc-0? If yes, then if the RecvPacket isn't writing any acknowledgment that means that the RecvPacket is failing at the core IBC handler. If you can figure out where then I can help you with figuring out why. If the relayer isn't sending a RecvPacket message at all but we're sure that ibc-1 is sending the packet, then its possible that the events aren't getting emitted or the relayer is for some reason not catching them.

Let me know if this helps you narrow the issue down at all, I can help more if you can narrow down the behavior further with the steps above. If you can figure out why I'm getting different behavior then you, that would be useful to

@jtieri
Copy link
Contributor

jtieri commented Mar 17, 2022

marshaling error: json: unsupported type: func() uint64

This is just a logging error. It's not a real bug. We've fixed it now in cosmos/ibc-go#1130 so I would ignore this. Double check that there is a real bug. Are you sure that the tokens are not actually sent back? Did you query the original address to see if it received the tokens.

Duly noted. This was just the last bit of logging when trying to go C->B->A so thought there could be some correlation between that and the packet failing to be sent B->A.

re: using that dev branch. I updated the test to use the v7.0.0-rc0 tag on Gaia but it looks like the middleware is no longer being invoked, presumably because of an issue with how it is wired up in app.go. I took a look myself but lack the technical insight to really know what the issue could be there.

I'll take a look at your suggestions and verify that the packet is being sent correctly (if at all) but just glancing at the logs from the relayer in that dev-env-demo it looks like C->B is successful and delivers the appropriate acknowledgement but B->A fails to send the packet at all.

Thanks again for taking the time to lend a hand here!

@jtieri jtieri merged commit 8834cbe into strangelove-ventures:main Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants