search

CosmWasm / wasmd

Basic cosmos-sdk app with web assembly smart contracts
Other
369 stars 406 forks source link

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

Open marcotradenet opened 1 year ago

marcotradenet commented 1 year ago

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:

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?
webmaster128 commented 1 year ago

Related: https://github.com/CosmWasm/wasmd/issues/1600

alpe commented 1 year ago

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 https://github.com/CosmWasm/wasmd/pull/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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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.

marcotradenet commented 10 months ago

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: