uint64 for ICS20 may be insufficient for chains with large decimal places

[colin-axner]

see original issue

Signaling for discussion. Is there interest in defining a custom amount type which allow for more precise amounts? This would change the packet data and would have to be introduced in a version 2 of ICS20

[ethanfrey]

I am ambivalent on this request, but I do think big.Int is overkill.

Ethereum uses u256. And CosmWasm uses Uint128, which can easily hold the entire supply of Ethereum as wei. (and about 1.000.000.000 times more).

If it is chosen to extend this type, we could just focus on u128 support, rather than arbitrary big.Int, which is much harder to get write in multiple languages, as ics20 is supposed to be blockchain agnostic.

e.g. one could add a second uint64 field to the current definition and then combine those two u64 to represent a u128. Which would be easier to specify in a blockchain agnostic way than arbitrary large numbers.

[alexanderbez]

Why not just represent the amount as a string and let the chain decide on how it wants to handle and decode it? Or are we limited by VMs?

[ethanfrey]

Why not just represent the amount as a string and let the chain decide on how it wants to handle and decode it? Or are we limited by VMs?

Fair enough. I just didn't want to tie it to any go library

[alexanderbez]

Agree on that point @ethanfrey 😆 . Would this suffice @colin-axner?

[colin-axner]

A string sounds like a great solution to me. This still requires a v2 of ICS 20 that handles backwards compatibility with v1 implementations. Does anyone have the capacity to propose the changes?

Any concerns @cwgoes and @milosevic?

[alexanderbez]

@colin-axner in order to remain backwards compatible, can we just introduce a new field into the packet structure?

[zmanian]

If we go with strings, the UI layer is going to have to be aware that MAX_INT on the reviving size might vary from chain to chain.

Would it be possible to have sending chain get the MAX_INT metadata and store it in the channel?

[colin-axner]

@colin-axner in order to remain backwards compatible, can we just introduce a new field into the packet structure?

Unfortunately not. We need a new ICS20 version. Lets say chainA uses the new packet and chainB uses the old packet. ChainA will commit to the new packet via a hash(timeout + packet data). ChainB will try to prove this commitment, but I'm fairly certain the proto compiler will disregard any unknown fields causing the packet data to be different and thus the commitment hash to be different. ChainA and ChainB either both need to use the old packet or both need to use the new packet. Mixing packets will be problematic

Using a new version to decode the packet data is not a technically challenging specification change, the biggest downside is that we would need new channels with the new version (and more unique IBC tokens originating from the same chain)

@AdityaSripal or I can likely make the specification change in a few weeks if it has not been done by then

[colin-axner]

Would it be possible to have sending chain get the MAX_INT metadata and store it in the channel?

Yes it is possible. It can be apart of the version and negotiated in the channel handshake

[colin-axner]

We could batch this change with the implementation of #584, which I'm fairly certain requires a version bump as well

[timlind]

@colin-axner looks like the memo addition in that proposal you mentioned already has the amount as uint256?

[colin-axner]

@colin-axner looks like the memo addition in that proposal you mentioned already has the amount as uint256?

That's correct and it is the same in the specification, but to the best of my knowledge protobuf doesn't support uint256. The ICS specs should be referencing protobuf definitions not defining interfaces for packet data, otherwise communication will break down between different implementations of the same application. I think the specs are outdated

[ethanfrey]

Just a comment.

I remembered ics20 uses json to encode the packet data not protobuf. So we should look at upgrade paths there. u128 may be supported by encoding/json, but javascript only guarantees 53 bits in parsing json integers.

I think string is better for big numbers that will end up as json

[colin-axner]

That is an excellent point @ethanfrey. It looks like the proto3 JSON encoding of uint64 is a string. This appears to be the case in our implementation at a quick glance:

{"amount":"100","denom":"transfer/channel-0/atom","receiver":"cosmos1w3jhxarpv3j8yvs7f9y7g","sender":"cosmos1tj54kjf9z4pva39kee7s9wxuzd0ykfhguwg80u"}

In that case it sounds possible for implementations such as ibc-go to replace the amount uint64 field with a string without changing the packet encoding. Of course, chains on older version will still be unable to receive packets with amounts > uint64

[colin-axner]

I did some basic testing and swapping the amount for a string appears to work just fine without requiring any application version change. I can start on a fix for ibc-go and do further testing against existing binaries when that work is completed

I have two unresolved concerns on this topic:

• the encoding format should be specified in each application spec
• should ICS20 specify a max_int (such as u256)?

[zmanian]

if ICS20 specified max_int u256, I don't think cosmwasm could conform because it's max_int is u128.

[ethanfrey]

I would prefer u128 as that is what we use in cosmwasm. We could add u256 support if needed, but it is quite expensive, and I have never seen a case where more than 128 bits was needed. even with 100 trillion tokens at 18 decimal places.

[zmanian]

I don't think we should implement a standard int max and leave it up to implementers to choose the largest values to support.

[tomtau]

the current spec: https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer appears to specify all amounts as uint256, not uint64, unless I overlooked something?

[colin-axner]

The spec already specifies the packet amount encoding as string

Note that both the FungibleTokenPacketData as well as FungibleTokenPacketAcknowledgement must be JSON-encoded (not Protobuf encoded) when they serialized into packet data. Also note that uint256 is string encoded when converted to JSON, but must be a valid decimal number of the form [0-9]+.

Going to close this issue as there doesn't seem to be anything to do/discuss. Feel free to reopen if there is something I am missing