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

Increase coin denom maxsize in order to support transactions with IBC tokens #56

Merged

Conversation

nddeluca
Copy link
Contributor

@nddeluca nddeluca commented Apr 15, 2022

Description

Currently, signing any transaction with an ibc token results in unrecognized error code due to the current coin denom maxsize validation of 50 characters including the null byte.

IBC denoms, such as ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2, are 68 characters in length, and the cosmos-sdk supports denominations up to 128 characters in length.

⚠️ This is currently blocking ledger users that have updated to the latest app version from managing their IBC funds on all cosmos chains that use app.

Changes

This PR adds a failing test with an example transaction containing an ibc denom, and then increases the denom max length to 129 (128 characters + null byte) to pass the test.

Testing

make cpp_test and make zemu_test both passed locally.

I also used make load and tested I was able to successfully sign transactions with ibc denoms on the command line and from the browser with this PR.

the denom maxsize also matches the cosmos-sdk coin validations
@nddeluca
Copy link
Contributor Author

hi @jleni any chance this can be looked at? We are seeing more users updating and then being unable to manage their IBC token positions.

I'm also happy to add a test case to the zemu test suite -- do you generally write a separate script to create the initial snapshots? Or is there a flag that can be set to update them during the the test run?

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 this pull request may close these issues.

2 participants