Improving bip32 ChildNumber display implementation #567
Conversation
src/util/bip32.rs
Outdated
| ChildNumber::Normal { index } => write!(f, "{}", index), | ||
| ChildNumber::Hardened { index } => { | ||
| fmt::Display::fmt(&index, f)?; | ||
| f.write_str(if f.alternate() { "'" } else { "h" }) |
There was a problem hiding this comment.
This is reversed, in normal mode it should still print ' and not h.
Also: just out of curiosity, what's the performance characteristic of write_str vs write_char? Even if it doesn't get optimized out it's probably nothing major though.
There was a problem hiding this comment.
Hmm, I think we should default to h rather than '.
There was a problem hiding this comment.
We can certainly do that, but it would break backwards compatibility in some sense. What would be the reason to do so?
There was a problem hiding this comment.
' sucks, it breaks shell escaping and is harder to read. I'm okay with the compatibility breakage.
Alternately we could make {#} output the h form like was suggested in the Decimal PR
There was a problem hiding this comment.
Alternately we could make {#} output the h form like was suggested in the Decimal PR
That's what I wanted. But I'm ok with {} outputting h and {#} outputting ' too if you consider our current behavior of always outputting ' bad.
There was a problem hiding this comment.
Alternately we could make {#} output the h form like was suggested in the Decimal PR
This is what happens now with the present version of the code: Sorry, this code does what you originaly suggested: displays h as a default for hardened derivation.
f.write_str(if f.alternate() { "'" } else { "h" })So do I need to change this?
There was a problem hiding this comment.
The current version (f.write_str(if f.alternate() { "'" } else { "h" })) is what @apoelstra wants afaik, but breaks compatibility. I'm fine with that as long as it happens consciously and not accidentally.
|
@sgeisler thank you for fixing! But how I can cherry-pick a detached head on GitHub? |
|
You can fetch it with |
|
@apoelstra updated with @sgeisler 1.29 fix and displays |
|
I'm comfortable merging this in a minor release, but maybe some others disagree? |
I'm ok with it. |
|
Seems fine, I really hope no one is depending on format output being exact.
… On Mar 12, 2021, at 11:26, Sebastian ***@***.***> wrote:
I'm comfortable merging this in a minor release, but maybe some others disagree?
I'm ok with it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Fixes rust-bitcoin#608. In rust-bitcoin#567 the Display impl for ChildNumber was consciously changed, assuming the semver break would not affect any correctly implemented downstream projects. We were wrong.
Fixes rust-bitcoin#608. In rust-bitcoin#567 the Display impl for ChildNumber was consciously changed, assuming the semver break would not affect any correctly implemented downstream projects. We were wrong.
Fixes rust-bitcoin#608. In rust-bitcoin#567 the Display impl for ChildNumber was consciously changed, assuming the semver break would not affect any correctly implemented downstream projects. We were wrong.
Fixes rust-bitcoin#608. In rust-bitcoin#567 the Display impl for ChildNumber was consciously changed, assuming the semver break would not affect any correctly implemented downstream projects. We were wrong.
Fixes rust-bitcoin#608. In rust-bitcoin#567 the Display impl for ChildNumber was consciously changed, assuming the semver break would not affect any correctly implemented downstream projects. We were wrong.
Trivial changes:
_hformat in hardened indexes using alternate format string (format!("{:#}", ChildNumber::from_hardened_idx(5))will print5hinstead of5')format!("{:#06}", ChildNumber::from_hardened_idx(5))will print000005h)