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

Support compat addresses for ibc-in #1079

Merged
merged 2 commits into from
May 10, 2024
Merged

Support compat addresses for ibc-in #1079

merged 2 commits into from
May 10, 2024

Conversation

grod220
Copy link
Collaborator

@grod220 grod220 commented May 9, 2024

Closes #1053

For noble, we need to encode our addresses to bech32 (not bech32m). In core, we have a special prefix for this specific case: https://github.com/penumbra-zone/penumbra/blob/main/crates/proto/src/serializers/bech32str.rs#L146.

Amended our bech32m package to support this new encoding method.

@grod220 grod220 force-pushed the penumbra-compact-addr branch 2 times, most recently from f122d24 to 5ed0240 Compare May 9, 2024 15:09
@Valentine1898
Copy link
Contributor

How long will the ibc transfer from noble take?
I sent a transaction, but my balance in noble has changed only by the value of the fee 🤔

@grod220
Copy link
Collaborator Author

grod220 commented May 9, 2024

Currently the client state has expired. We'll need to work with the protocol team in getting these back up before we get a successful transfer. However, after this PR, we are no longer getting the bech32 error like we had before. Thanks for testing though!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to just penumbracompat.ts to match other files in this dir

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But aren't these files named according to the prefix? For some reason, this prefix uniquely has the 1 in it: https://github.com/penumbra-zone/penumbra/blob/d13d731fadd665d7f2ec38c3bde57f53b2e86891/crates/proto/src/serializers/bech32str.rs#L146

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wonder if it has this number in it because we may need to have penumbracompat2 for some other chain in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting. 1 is a valid prefix character, but it is also the separator character, and this may be a bug in core or has some unexplained meaning like version as you suggest.

i can't find any examples or documentation on this.

cc @avahowell you committed the penumbracompat1 in core?

@turbocrime
Copy link
Contributor

suggestions in #1083

@grod220 grod220 force-pushed the penumbra-compact-addr branch from 5ed0240 to cd828cc Compare May 10, 2024 09:48
@grod220 grod220 requested a review from turbocrime May 10, 2024 09:53
@grod220 grod220 force-pushed the penumbra-compact-addr branch from cd828cc to 23bc671 Compare May 10, 2024 17:15
@grod220 grod220 merged commit 640feb1 into main May 10, 2024
6 checks passed
@grod220 grod220 deleted the penumbra-compact-addr branch May 10, 2024 17:26
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.

nobletestnet IBC-in error
3 participants