Support Big Int IBC Transfer Amounts

[timlind]

Summary

Support a larger amount field on FungibleTokenPacketData instead of uint64. This would make it's support in line with what MsgTransfer.Amount supports.

The SDK currently panics when trying to convert a large MsgTransfer.Amount to the underlying FungibleTokenPacketData.Amount.

Problem Definition

Transferring tokens with 18 decimal places, like many Ethereum tokens for instance, are limited to insignificant amounts (< $1).

Transferring these tokens by splitting the transfer into multiple smaller transfers would require several hundred or thousands messages.

The fees increase significantly to an uneconomical state when needing to split a transfer to this degree.

Keprl also does not handle this well in the UI, as this many messages shouldn't really be a use case.

Some relayers also don't handle these large transactions well.

Proposal

Introduce a larger amount field (i.e string) on FungibleTokenPacketData proto message, to replace the current amount field.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[colin-axner]

I've opened an issue on cosmos/ibc. We won't change the packet structure until a change is approved on the specifications

The packet structure will not adopt any SDK structures. This is intentional as packets need to be sent/received on non-SDK chains. They may decide to introduce their own amount type which may or may not be the same as sdk.Int.

In this repo, we can at least add a check that the MsgTransfer amount can be converted to uint64 and return an error instead of panici'ing

[albertchon]

Why not represent the amount as a string?

[fedekunze]

This is relevant for Ethermint as well. Happy to provide resources or reviews to get this fixed ASAP @colin-axner

[jtremback]

The packet structure will not adopt any SDK structures. This is intentional as packets need to be sent/received on non-SDK chains. They may decide to introduce their own amount type which may or may not be the same as sdk.Int.

This is kind of funny since the only blockchain I've ever seen that uses u64 for numbers is the Cosmos hub. So u64 amounts are already an SDK structure

[mccallofthewild]

@colin-axner The IBC spec defines that FungibleTokenPacketData.amount should be uint256. https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer#data-structures

[colin-axner]

@colin-axner The IBC spec defines that FungibleTokenPacketData.amount should be uint256. https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer#data-structures

yup, see comment. For context, the protobuf definitions of packet data cannot simply be changed. This would cause all existing ICS20 transfers to break unless there was a synchronous upgrade between every chain using IBC (infeasible). Instead there would need to be a new version of ICS20 with a new packet data format and the packet data would need to be handled based on the channel version

[colin-axner]

Since ICS 20 uses protojson, the uint64 is actually encoded as a string. Thus it appears to be possible to modify the packet data amount to be a string instead of a uint64 without changing the packet encoding and thus breaking packet receipt proofs

I will start on a fix when I get back to work on the 23rd. This breaks API and thus will be included in a 2.0 release (assuming no unforeseen difficulties). While the fix is straightforward, it is a significant change to a sensitive area of the codebase and thus will require extra care and testing. If anyone could assist with testing the fix once it is ready, that would be a tremendous help

Is this an immediate blocker for anyone? Since there exist short term workarounds, I'm inclined not to do a release for only this fix. I am open to doing an alpha pre-release once this fix is merged if it would be greatly appreciated. Otherwise I anticipate to be released with the implementation of ICS 29 (relayer incentivization)

[zmanian]

I'd prefer that we take this change out of the 2.0 milestone and do a new milestone that just fixes this issue.

This issues is going to be pretty relevant to all the ERC20 assets that start coming into Cosmos via the Injective, the Secret Bridge, Terra Columbus-5 etc.

We should prioritize a fast upgrade path here especially because we aren't changing the wire format to resolve the issue.

[colin-axner]

We will be including this in a v2.0 release which is scoped just for this fix (ics27 and ics29 will be pushed into a later release). This is a top priority for me to get done and a release will be cut when the fix is implemented and well tested. The exact testing that needs to be done is still to be determined, but no migrations are required, so the focus will be on ensuring backwards compatibility functions as expected

[timlind]

@colin-axner a couple questions:

Can you provide an estimate for when this will be released?

Will this be back ported to the 0.42 series, or can ibc-go be used with 0.42?

Does backwards compatibility here mean that small amounts still work with older chains, but larger amounts will still panic?

If there's anything I can do to help, back port, test etc, let me know.

[colin-axner]

Can you provide an estimate for when this will be released?

As soon as sufficient QA has been done. I know that's a little vague, but it is because we haven't taken the time to define the criteria for when new changes are okay to be released. This is something we are working on. For now, at minimum I would like to verify that:

• the fix works on Hermes
• the fix works on ts-relayer
• any chain using the fix can communicate with existing chains using amounts <= max(int64)
• new chains both using the fix can communicate using amounts <= max(uint256)
• new chains sending amounts > max(int64) interact with expected behaviour (ideally sending back a failed ack or at the very least allowing for a timeout)

As a side note, I realized due to a bug in telemetry, existing chains can only send up to max(int64) and not uint64. This will be fixed in v2

Will this be back ported to the 0.42 series, or can ibc-go be used with 0.42?

No, state breaking changes are never backported. Furthermore, there is a known security vulnerability in v0.42, so I would highly recommend upgrading to v0.44 as soon as possible

Does backwards compatibility here mean that small amounts still work with older chains, but larger amounts will still panic?

Correct. Any amount <= max(int64) will work for old and new chains sending transfers to one another. Larger amounts will still panic when sending on an old chain. When sending larger amounts from a new chain to an old chain, the receive will also panic due to the telemetry bug, thus requiring the packet to be timed out

If there's anything I can do to help, back port, test etc, let me know.

Yes! Any testing of the fix #350 would be greatly appreciated. I assume you ran into this issue working on something which required larger amounts? Let me know if you need me to provide a binary with the fix

[timlind]

@colin-axner If you can send over a binary I can do some testing with ts-relayer.

[colin-axner]

@colin-axner If you can send over a binary I can do some testing with ts-relayer.

Try out this gaia branch. Let me know if you run into any issues

[colin-axner]

Keeping this issue open until it is included in a release (testing still needs to be finalized)

[crodriguezvega]

@timlind how is it going with the testing with the ts-relayer? Is there anything we can help you with?

We appreciate very much your help!

[Jedi2002]

@crodriguezvega , @colin-axner , @timlind
I was able to test the fix on the ts-relayer and it works fine.

I used the tag to test the negative test case by triggering the panic ,and the branch to test the fix .

[timlind]

Thanks @Jedi2002! Does this mean we have a date for a release @colin-axner?

[colin-axner]

Does this mean we have a date for a release @colin-axner?

Currently there is a beta1 release. An rc0 will be released shortly. After some time if no issues are reported we will finalize the release

[akc2267]

@colin-axner do you have an estimate on how long until the rc0 is released and then how long after that will a finalized release happen, assuming no issues reported?

Sifchain had to do a 10decimal workaround involving a new token (xrowan instead of rowan). this complicates the integration of sifchain into Emeris

[crodriguezvega]

@akc2267 Maybe I can comment, since @colin-axner I think is traveling today.

The v2.0.0-rc0 was already created. The golang relayer and the hermes relayer teams are doing some testing with it. We will also do a final round of testing in the last week of October. If everything is ok, then we will probably cut the final release in the first week of November. In the meantime I guess you could use the rc0.

[sifmoon]

@crodriguezvega just wanted to check in to see if this is still on track to be released next week?

[crodriguezvega]

@caseyturnerarrington Unfortunately some health issues have hit the team and I am afraid that we will need to postpone the release a bit. Our target is now to cut the final release in the week of November 15th.

Sorry for the inconvenience that this may cause.

[crodriguezvega]

Happy to share that v2.0.0 is finally out!