fix to correctly parse denoms with slashes in the base denom

[crodriguezvega]

Description

closes: #1446

---

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.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov-commenter]

Codecov Report

Merging #1451 (58d1133) into main (da232b8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1451      +/-   ##
==========================================
+ Coverage   80.27%   80.29%   +0.02%     
==========================================
  Files         166      166              
  Lines       12252    12269      +17     
==========================================
+ Hits         9835     9852      +17     
  Misses       1954     1954              
  Partials      463      463              

Impacted Files	Coverage Δ
modules/apps/transfer/types/trace.go	90.08% <100.00%> (+1.62%)	⬆️

[danburck]

Nice, LGTM!

[AdityaSripal]

Let's manually test the following:

• Sending slashed coins works
• Receiving slashed coins works, verify trace and base_denom is as expected
• (ideally but maybe not super necessary) receive to a third chain verify trace and base_denom is as expected
• Unwind token path completely and check that we get back the original token

We should also test a packet refund, I think the second chain will be most relevant here

[fedekunze]

Can we add the 2 denom examples (gamm/pool/1 and erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA) to TestValidatePrefixedDenom and TestDenomTrace_Validate just to make sure it won't fail elsewhere?

[crodriguezvega]

I've been doing some manual testing, so I just want to report back the results so far... I have tested 2 scenarios so far:

Scenario 1

Setup of 2 chains:

• chain1 and chain2 are both using the simd from the branch of this PR.

I setup the transfer channel between both chains and I try to send 100erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA from an account on chain1 to an account on chain2. The transfer succeeds and denom-trace and denom-hash CLI commands return the expected output on chain2:

> simd q ibc-transfer denom-trace C1D5D3AE1B5C9C9778DF7E8972352937B52C8E97C66505FF6639ED12BD9E8084
denom_trace:
  base_denom: erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA
  path: transfer/channel-0

> simd q ibc-transfer denom-hash transfer/channel-0/erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA
hash: C1D5D3AE1B5C9C9778DF7E8972352937B52C8E97C66505FF6639ED12BD9E8084

Scenario 2

Setup of 2 chains:

• chain1 is using the simd from the branch of this PR.
• chain2 is using the simd from main.

I setup the transfer channel between both chains and I try to send 100erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA from an account on chain1 to an account on chain2. The transfer fails:

{
  "type":"fungible_token_packet",
  "attributes":[
     {
        "key":"module",
        "value":"transfer"
     },
     {
        "key":"sender",
        "value":"cosmos1sc3r6elp0vh8qftt55kr9qawrwe6xq6qsl74xx"
     },
     {
        "key":"receiver",
        "value":"cosmos1qufuxgutlt0skpccyxlzmwtmvge0ccfrl96l80"
     },
     {
        "key":"denom",
        "value":"erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA"
     },
     {
        "key":"amount",
        "value":"100"
     },
     {
        "key":"acknowledgement",
        "value":"error:\"ABCI code: 1: error handling packet on destination chain: see events for details\" "
     },
     {
        "key":"error",
        "value":"ABCI code: 1: error handling packet on destination chain: see events for details"
     }
  ]
}

The error message is actually not very useful and I think there is nothing else in the events to give more information about what went wrong. But I suspect that the problem is the following (since chain2 is running the code from main without this fix):

In the OnRecvPacket callback there is a validation of the data in the FungibleTokenPacketData. The last step of this validation calls ValidatePrefixedDenom with the denom erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA. Inside ValidatePrefixedDenom the last step is to validate the trace identifiers. The input to validateTraceIdentifiers is going to be a string slice with only the element erc20. And then inside validateTraceIdentifiers I think an error is returned because len(identifiers)%2 != 0.

So I am afraid that for IBC transfers of coins with slashed denoms to succeed we are going to need to have this fix deployed as well in all the counterparty chains.

[fedekunze]

LGTM, pending changelog entry

[damiannolan]

Looks good to go, I just left some nits and a quick question! Thanks 🙏

[AdityaSripal]

In the OnRecvPacket callback there is a validation of the data in the FungibleTokenPacketData. The last step of this validation calls ValidatePrefixedDenom with the denom erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA. Inside ValidatePrefixedDenom the last step is to validate the trace identifiers. The input to validateTraceIdentifiers is going to be a string slice with only the element erc20. And then inside validateTraceIdentifiers I think an error is returned because len(identifiers)%2 != 0.

Yes in order for this fix to work correctly both sides must have the updated code.

Otherwise we enter into different scenarios depending on the denomination passed in:

1. If we have an even split like erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA

Then we will fail as described by @crodriguezvega at packet data validation. The receiving chain will write an error ack, and then the sending chain will simply refund to the sender. This is not a bad outcome.

2. If we have an odd split like gamm/pool/1

Then we will pass packet validation but the token will be issued on the receiving chain with BaseDenom: 1 and Trace: gamm/pool which is incorrect. The internal representation of the token would then be ibc/hash{gamm/pool/1}

This is not ideal for the receiving chain since it may mess with client UX but it isn't catastrophic.

When the chains get moved back to the original sender, since it is updated it will recognize that gamm/pool/1 is the base denom and appropriately unescrow funds.

When the funds go to other receiving chains, if they have the updated code then they will generate the trace and base denom correctly. If the other receiving chains don't have the update, they will also include gamm/pool to the end of the trace and have 1 in the base denom.

We should decide how to deal with the second scenario.

One option is to simply allow it and have some chains temporarily have the wrong trace information on their chains. This will only affect internal representations and users of that chain by giving them poor UX.

We can try to prevent slashed denoms from being received on chains that do not have this update. There is no great way to do this apart from creating middleware filters on updated sending chain that explicitly permission approved receiver chains that are known to have the update. This middleware can be added to until it is no longer necessary and can be removed. This would be an annoying mechanism to deal with, but is the cleanest one I can think of if we want to be very strict and avoid the problem all together.

[fedekunze]

We should decide how to deal with the second scenario.

@AdityaSripal IMO there should be an upgrade migration logic so that the denom hash is updated everywhere

[AdityaSripal]

IMO there should be an upgrade migration logic so that the denom hash is updated everywhere

Not sure I understand the proposal. Is the proposal that we allow slashed denoms to enter into all chains as is, and then once the upgrade happens on each chain we run migration logic to fix the traces?

The denom hashes shouldn't be an issue, since they are going to hash the full denom path regardless.

The issue is the hash -> trace mapping on the receiving chains that don't upgrade. We'll have to iterate over all of the traces and fix them. That seems plausible to me, but will be a decent size migration

[crodriguezvega]

Running chain1 with the fix and chain2 without it, I tested as well with denom gamm/pool/1 but the transfer fails on the receiving chain here because pool has fewer than 8 characters...

So I tried with denom gamm/poolpoolpool/1 to able to pass that validation and the transfer succeeded and on the receiving chain I can query for the denom trace and get the following:

denom_traces:
- base_denom: "1"
  path: transfer/channel-0/gamm/poolpoolpool

I can transfer the tokens back to the sending chain and the denom is correctly unwinded.

[AdityaSripal]

Great, thanks @crodriguezvega! So we can confirm the different behavior.

Since it gets correctly unwinded at the source and the trace information is purely local this isn't a critical issue.

I would say that we should release, and as Fede says, we should have a migration script that will update the trace information on receiver chains once they upgrade. Perhaps we can have that at some point after with one of the upcoming releases since it is more of a UX fix.

Note that in the interim we will have to live with the wrong trace information living on unupgraded receiver chains when they receive slashed denoms.

It looks to me like @fedekunze usecase of: erc20/{coin} will not be receivable anyway until the counterparty also upgrades so it is not as relevant an issue in evmos case

[colin-axner]

Nice work @crodriguezvega! It should be noted this introduces undocumented changes. Specifically the usage of PortID. ICS20 does not specify the PortID that should be used for the transfer channel. See specification callbacks. This is also true in our code. This code now assumes the PortID to be transfer. This should be enforced (in channel handshakes) if this the intended behaviour

I'm unaware of any chains which have changed the portID, but last I checked, Agoric used a modified portID in testing

[colin-axner]

Looks like it might be a problem with chains running wasm:

In order to enable IBC communication, a contract must expose the following 6 entry points. Upon detecting such an "IBC-Enabled" contract, the [x/wasm runtime](https://github.com/CosmWasm/wasmd) will automatically bind a port for this contract (wasm.<contract-address>), which allows a relayer to create channels between this contract and another chain. Once channels are created, the contract will process all packets and receipts.

[AdityaSripal]

Nice catch @colin-axner ! Unfortunately without a standardized port, it's impossible to correctly differentiate between what is trace info and what is the base denom. Since the trace is largely internal information for a chain and will not affect correctness, I think we can live with the fact that non-standard ports and their subsequent traces will be part of the base denom.

Reiterating for future viewers that this will not affect correctness of the protocol. But when querying trace information, any nonstandard trace will be in the base denom