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

IBC port id creation is not compatible with Bech32 characters space #1707

Open
marcotradenet opened this issue Nov 9, 2023 · 8 comments · May be fixed by #1710
Open

IBC port id creation is not compatible with Bech32 characters space #1707

marcotradenet opened this issue Nov 9, 2023 · 8 comments · May be fixed by #1710

Comments

@marcotradenet
Copy link

Description

The Bech32 address format allows the use of 1 to 83 US-ASCII characters for the Human-Readable Part (hrp). An address could have an hrp like this:

xxx:yyy

Wasmd generates the port ID for IBC using the address of a contract, which is in Bech32 format. When the hrp is used as shown above, the resulting port ID takes the following form:

wasm.xxx:yyy1......

However, as specified in ICS-024 Host Requirements, IBC port IDs are only allowed to contain alphanumeric characters, as well as the following symbols:

'.', '_', '+', '-', '#', '[', ']', '<', '>'

This means that Bech32 addresses are incompatible with the requirements of ICS-024 Host Requirements.

The code responsible for creating the port ID in this way can be found in the following location:

ibc.go

The functions involved in this issue are:

  • PortIDForContract
  • ContractFromPortID

Possible Solutions

  1. Create parameters within the Wasmd module to allow for the translation of disallowed characters into the permitted set. For example, in the Wasmd module, you can add a parameter that specifies the translation of : to ., or to another compatible character.
  2. Change the method for creating the port ID by using, for example, using hexadecimal representation of the address and adding the chain ID as a prefix.
  3. Create a fork of the Wasmd repository, keep it aligned with the original, and add a custom translation. This is more of a question than a solution: Is it possible to do this without introducing compatibility issues?
@marcotradenet marcotradenet changed the title IBC port id creation is not compatible with Bech32 character space IBC port id creation is not compatible with Bech32 characters space Nov 9, 2023
@webmaster128
Copy link
Member

Related: #1600

@alpe alpe linked a pull request Nov 10, 2023 that will close this issue
@alpe
Copy link
Contributor

alpe commented Nov 10, 2023

Thanks for bringing attention to the topic! You have observed well that the Bech32 format can contain characters that prevent an address from being used as an IBC port.
I was worried that there is any vulnerability with the current implementation and have let my box generate addresses with the classic algo for more than an hour and there was no conflict. The instantiation code binds the IBC port immediately so that that we are also safe to not fail later.
Please correct me if I am wrong but I understand this issue now as a feature request to make the code more extendable to support your use case where you have a different address format.

Any string replacement or hex code address would be hard to communicate with existing chains, docs and maybe expectations. Having this said, it is not hard to have another constructor Option for the keeper to let you fully customize the port. I have started a draft PR with #1710. Please take a look and let me know if this would work for you.

Although this is not a solution to #1600, yet the extension point can be used for implementations that generate a shorter name as well. If anybody is working on this, please comment on the issue

@webmaster128
Copy link
Member

I like the approach in #1710 as the right direction for both tickets. But it would be good to not require a stateless reverse operation port -> contract address. If we keep that requirement, a chain can never change its port generation algorithm. Instead, if you only specify PortIDForContract(addr sdk.AccAddress) string (contract -> port) and then store a port->contract address map in state you get plenty of benefits:

  • it is possible to change PortIDForContract over time and only apply the new format to newly assigned ports
  • You can use non-reversible replacement solutions like replace : -> _ when _ is also a valid address character (bech prefix did:my_comp:1hfjdhdjf -> wasm.did_my_comp_1hfjdhdjf cannot easily be reversed)
  • You can use hash based solutions like wasm.<hex(sha256(contract_address)[0:32])>

The port ID of a contract then has to be looked up for off chain activities like creating channels. But this information is already stored in the contract info and can easily be an information shown by CosmWasm supporting explorers or other tools. Celatone shows port IDs already, see here.

@alpe
Copy link
Contributor

alpe commented Nov 13, 2023

Thanks for the feedback! Good thinking to support a path to change the port algorithm in the future.

An in memory map for port->address resolution would add some complexity for this task and occupy additional memory for the default scenario. It must also be loaded on startup from contract-infos.
With the context past to the methods though, somebody can easily check the store:
if !keeper.HasContractInfo(ctx, fromOldAlgo(port)){ return fromNewAlgo(port)}
This does not prevent any caching solution but optimizes for the default case with our vanilla address resolution algorithm.

@webmaster128
Copy link
Member

An in memory map for port->address resolution would add some complexity for this task and occupy additional memory for the default scenario. It must also be loaded on startup from contract-infos.

I'd not create an in-memory solution. My idea was more to add a state migration which creates a portID -> address map in state to be able to get the information always with a single lookup for all contracts that have a port. Newly created ports are then added to that map on in contract instantiation.

@marcotradenet
Copy link
Author

Thank you for your answers and possible solution @alpe

Any string replacement or hex code address would be hard to communicate with existing chains, docs and maybe expectations. Having this said, it is not hard to have another constructor Option for the keeper to let you fully customize the port. I have started a draft PR with #1710. Please take a look and let me know if this would work for you.

I think it is a good solution. I tried to create a fork and use it in a stack with two chains communicating with each other using simple replace and things work in IBC both input and output. The solution proposed in your branch @alpe can very well be used to contain what I did in my fork.

  • You can use non-reversible replacement solutions like replace : -> _ when _ is also a valid address character (bech prefix did:my_comp:1hfjdhdjf -> wasm.did_my_comp_1hfjdhdjf cannot easily be reversed)

@webmaster128 I'm not sure I understand this point correctly: are you saying that hardly then there is total reversibility if I use something like replace? If so, I agree, only I thought it didn't matter anyway since the replace function would be limited to one chain with a specific HRP.

  • You can use hash based solutions like wasm.<hex(sha256(contract_address)[0:32])>

I agree that this is probably a better solution

I don't think I understood exactly the following comments

Perhaps are you talking about how to handle the "past"?

@webmaster128
Copy link
Member

@webmaster128 I'm not sure I understand this point correctly: are you saying that hardly then there is total reversibility if I use something like replace? If so, I agree, only I thought it didn't matter anyway since the replace function would be limited to one chain with a specific HRP.

In your case we can certainly create a reversible conversion. But it would be nice if only one way is required by the interface as it makes your life easier and at the same time allows for tons of other cases. especially working towards shorter ports.

@alpe alpe added this to the v0.46.0 milestone Nov 15, 2023
@hashedone hashedone modified the milestones: v0.46.0, v0.51 Dec 11, 2023
@marcotradenet
Copy link
Author

I tried the proposed implementation on a repository fork, and used it within our testnet by connecting it to the osmosis testnet and everything seems to work correctly:

  • Created a channel between our testnet and the osmosis testnet.
  • Sent cw20 tokens through ibc to osmosis
  • Sent the tokens from osmosis tp our chain

@chipshort chipshort modified the milestones: v0.51, v0.52 Mar 12, 2024
@pinosu pinosu removed this from the v0.53.0 milestone Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants