-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add Bech32 Command Tree #9
Conversation
One thing I'm trying to think about is to avoid short flags if they could collide with global ones. I didn't know it's possible to flag a PR as "draft", TIL! |
We should be able to drop the bitcoin-bech32 dep as well once rust-bitcoin v0.19 lands. But I have some more branches ready related to addresses for when v0.19 lands. |
Great point. We might as well just output both, if we're in structured data mode. On another note, would it be possible to output a trailing newline after the JSON or YAML, so the command prompt is clean for the next input? |
On second thought, I'd like to move the "payload" output to non-default, since it displays each byte of a separate line, which is ultra-verbose. |
Sure.
Yeah personally I don't really see much value in an interger-list-representation of bytes over hex. |
For decoding, this echoes back the original input string, along with decoded values. For encoding, the inputs are echoed back alongside the bech32 field output. Cargo fmt
Complete bech32 encode command with payload field output Remove integer vector from output
Ok, ready for review here. I've tweaked the interface and options again. Please let me know if it makes any sense or if we can word the help text better for what's going on with 5-bit and 8-bit values. |
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.
Nice! The first real contributor! I left some minor remarks!
src/bin/hal/cmd/bech32/encode.rs
Outdated
bech32: result.unwrap(), | ||
hrp: hrp.to_string(), | ||
payload: payload_bytes.into(), | ||
payload_base256: match Vec::<u8>::from_base32(&payload_base32) { |
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.
Just curious: what would this field be useful for?
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.
The payload
field is the 5-bit values, and payload_base256
is the result of convert_bits
from 5- to 8-bit, so we get back the original bytes. This does not work for BIP-173 addresses, since they abuse the encoding format with the version number taking the first 5-bit slot.
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.
Aaaah, lol, I was reading "base256" as 256-bit. The base thing confuses me, though. Do you think "payload_base256" is more clear than "payload_bytes"? By definition a "byte" is base256, right?
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.
Let's go with payload_bytes
and see how that feels.
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.
Ok, renamed.
Use .expect instead of explicit error checking. Lowercase expect error messages. Put payload_base256 assignment inline with struct definition.
Ok, first round of feedback addressed. |
Wanted to add a quick interface to the Bech32 library. This will allow encoding and decoding of Bech32 strings and payloads.
Decode
Sample output:
Encode
Sample output: