-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Update bech32 human readable spec #2103
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2103 +/- ##
========================================
Coverage 64.86% 64.86%
========================================
Files 115 115
Lines 6863 6863
========================================
Hits 4452 4452
Misses 2127 2127
Partials 284 284 |
docs/spec/other/bech32.md
Outdated
| `cosmosaccpub` | Cosmos Account Public Key | | ||
| `cosmosvaladdr` | Cosmos Consensus Address | | ||
| `cosmosvalpub` | Cosmos Consensus Public Key| | ||
| `cosa` | Cosmos Account Address | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does indeed shorten them, but I actually find this harder to reason about & read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cosa
means "thing" in spanish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with @alexanderbez here. I don't mind how verbose they are because the descriptions are actually human readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; human-readable prefixes are more helpful for newer users, for whom distinguishability is most critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that quite a bit. Much improved @jaekwon!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+++++
fat fingered -- sorry! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be consensus on this. Should we merge and create a PR for the actual Bech32 changes?
@alexanderbez I don't think the Markdown file was updated to reflect the comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed @cwgoes -- you are correct
| `cosmos` | Cosmos Account Address | | ||
| `cosmospub` | Cosmos Account Public Key | | ||
| `cosmosval` | Cosmos Validator Consensus Address | | ||
| `cosmosval` | Cosmos Validator Consensus Public Key| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, merged the wrong thing, will fix this to be "cosmosvalpub" on develop.
The existing bech32 human readable prefix is too long esp for user accounts. This shortens them.
For employee accounts, we need to ensure that we convert correctly by first decoding using the old prefix, then encoding again with the new prefix, as it changes the last few base32 checksum characters.