Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICS20: support base denoms with slashes #737

Closed
plafer opened this issue May 13, 2022 · 3 comments · Fixed by #935
Closed

ICS20: support base denoms with slashes #737

plafer opened this issue May 13, 2022 · 3 comments · Fixed by #935
Assignees
Labels
app Application layer.

Comments

@plafer
Copy link
Contributor

plafer commented May 13, 2022

ibc-go now supports base denoms with slashes. We should probably update the standard so that other implementations can also support chains that require it.

ibc-rs is planning on supporting base denoms with slashes as well (PR).

@crodriguezvega
Copy link
Contributor

crodriguezvega commented May 14, 2022

Thanks for opening this issue, @plafer.

When I opened the PR to add the support for base denoms with slashes, I thought that it was a bug on ibc-go that base denoms were not supported, since they are valid denoms for the Cosmos SDK. But back then I didn't realize (re-reading the spec now) that the spec said:

If {denom} contains /, then it must also be in the ics20 form which indicates that this token has a multi-hop record. Note that this requires that the / (slash character) is prohibited in non-IBC token denomination names.

So does the above means that ibc-go should not have added support for slashes in base denoms? Looks like we should have agreed on a spec change first before implementing the support in the implementation...

@ethanfrey
Copy link
Contributor

@sunnya96
@ValarDragon
this may be relevant to the token factory osmosis inplemented

@colin-axner
Copy link
Contributor

colin-axner commented May 18, 2022

I think this is only an issue if the chain implementing ics20 stores the received denominations using the full path (ie it doesn't differetiate between a minted token with the denom {ics20Port}/{ics20Channel}/{denom} and a IBC token received with the denom {ics20Port}/{ics20Channel}/{denom}). Since ibc-go uses the ibc/{hash} representation for internal tracking, a newly minted token in the form {ics20Port}/{ics20Channel}/{denom} will be treated differently than the actual token received via IBC with that same path

When a chain receives a token, it should be prefixing its port/channel on the denom path, so sending a denom with slashes should not make a difference

If my assumption is correct, I'd be in favor of changing the spec to indicate this. I think it also might be useful to recommend the ibc/{hash} approach as best practice, but maybe there are good reasons not to do this?

cc @AdityaSripal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Application layer.
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants