search

cosmos / ibc-go

Inter-Blockchain Communication Protocol (IBC) implementation in Golang.
https://ibc.cosmos.network/
MIT License
548 stars 586 forks source link

Port DefaultMaxCharacterLength value of 64 is incompatible with ADR-028 addresses #339

Closed channa-figure closed 3 years ago

channa-figure commented 3 years ago

Summary of Bug

The port DefaultMaxCharacterLength value of 64 is incompatible with ADR-028 addresses. Module based addresses or an address derived from a key type other than secp256k1 will error on validation: https://github.com/cosmos/ibc-go/blob/2c5ea11e4a2c5f44ee8a81ece3b5d031496c79c1/modules/core/24-host/validate.go#L44

I would suggest the max length be set to 84 to account for the address length increase of ADR-028.

Version

v1.0.0

Steps to Reproduce

1.) Created a fork of CosmWasm wasmd (https://github.com/CosmWasm/wasmd) application and updated cosmos-sdk to v0.43.0 and added ibc-go v1.0.0 in the go.mod file.
2.) Removed the truncation of module based address of 20 bytes to use the whole 32 byte address : https://github.com/CosmWasm/wasmd/blob/93e2e669402dd96b99f223bc6176abef6e794455/x/wasm/keeper/keeper.go#L875 3.) Attempted to create a new ibc channel for a wasm contract with port wasm.cosmos14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s4hmalr, where cosmos14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s4hmalr was derived from the 32 byte module address. The method to create the ibc port was : https://github.com/cosmos/ibc-go/blob/2c5ea11e4a2c5f44ee8a81ece3b5d031496c79c1/modules/core/05-port/keeper/keeper.go#L42

Produced error: "identifier wasm.cosmos14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s4hmalr has invalid length: 70, must be between 2-64 characters"

For Admin Use

ethanfrey commented 3 years ago

Thanks for flagging this.

It was already agreed to increase the port limit to 128 characters in the ibc spec https://github.com/cosmos/ibc/issues/594

@colin-axner It would be awesome if that could be implemented in ibc-go and in some new release. 1.0.1? 1.1? No idea what naming you want, but this is a blocker for cosmwasm + cosmos-sdk 0.43

colin-axner commented 3 years ago

Thanks for opening this issue. I have opened a pr to update the IBC specs and will subsequently fix this in ibc-go. It can be included in v1.1.0 (we don't include state breaking changes in patches)

In the short term for testing purposes, DefaultMaxCharacterLength is public and thus can be changed upon app init

colin-axner commented 3 years ago

Sorry for the delay on doing a release with this fix. We decided to make v1.1.0 isolated to just the SDK bump since it contained a security fix and we didn't want to introduce any extra complexity. We are also still working on drafting a document for our release procedure and the exact meaning of our semantic versioning. Hopefully in the future it'll be more clear what release this goes in and how long it might take for the change to be included in a release

I will see if we can get a v1.2.0 out this week (state breaking changes at minimum are included in minor releases for us, or at least that is what we are currently going with)

colin-axner commented 3 years ago

The fix has been included in v1.2.0!

ethanfrey commented 3 years ago

Great news!

Is v1.2.0 compatible with Cosmos SDK v0.44.0? eg. Could I import them both in my app? (Updating go.mod only in my app)

colin-axner commented 3 years ago

Is v1.2.0 compatible with Cosmos SDK v0.44.0? eg. Could I import them both in my app? (Updating go.mod only in my app)

Yes it is! v1.2.0 uses v0.44.0, so no changes should be required