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

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

Closed
3 tasks
channa-figure opened this issue Aug 16, 2021 · 6 comments · Fixed by #344
Closed
3 tasks
Assignees
Milestone

Comments

@channa-figure
Copy link

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:

if len(id) < min || len(id) > max {

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 :

func (k *Keeper) BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability {

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


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@ethanfrey
Copy link
Contributor

Thanks for flagging this.

It was already agreed to increase the port limit to 128 characters in the ibc spec cosmos/ibc#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 colin-axner self-assigned this Aug 23, 2021
@colin-axner colin-axner added this to the 1.1.0 milestone Aug 23, 2021
@colin-axner
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

The fix has been included in v1.2.0!

@ethanfrey
Copy link
Contributor

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
Copy link
Contributor

colin-axner commented Sep 13, 2021

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

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.

4 participants