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

keytree: add serialization for xpub, xprv #228

Merged
merged 10 commits into from Mar 19, 2019
Merged

keytree: add serialization for xpub, xprv #228

merged 10 commits into from Mar 19, 2019

Conversation

tessr
Copy link
Contributor

@tessr tessr commented Mar 18, 2019

Addresses #179

keytree/src/lib.rs Outdated Show resolved Hide resolved
keytree/src/lib.rs Outdated Show resolved Hide resolved
keytree/src/lib.rs Outdated Show resolved Hide resolved
keytree/src/lib.rs Outdated Show resolved Hide resolved
keytree/src/lib.rs Outdated Show resolved Hide resolved
let xprv_bytes = xprv.to_bytes();

// hardcoded, but happens to be expected_scalar concatenated with expected_dk
let expected_bytes = [
Copy link
Contributor

@oleganza oleganza Mar 18, 2019

Choose a reason for hiding this comment

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

there's hex crate with hex::encode method so we can write a more readable test (and then copy this over to the spec as Test Vectors):

assert_eq!(hex::encode(&xprv.to_bytes()[..]), "df0da2b88d88de1ad7678d8f012...");

Copy link
Contributor

Choose a reason for hiding this comment

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

hex is only needed in dev-dependencies in Cargo.toml. So you'd include it only under the mod test { }, not in the top of lib.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried this out, and it's neat, but I'm not sure it actually produces a more readable test, especially since all the other tests in this package (for creating xprvs and xpubs) use non-hex encoded byte arrays. When I switched over to a hex string, I could no longer eyeball that these were the same.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think all have to be hex, across the board. But if it's a PITA to update, we can do that in a separate PR later.

Copy link
Contributor

Choose a reason for hiding this comment

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

we'd definitely want that to post the test vectors in the spec in hex. But before that we need to implement derivation for both xpub and xprv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do it all in another PR, then. (It's fine to do, I should just change them all together.)

@oleganza
Copy link
Contributor

looks good, just left a bunch of stylistic rusty nitpicks

oleganza and others added 4 commits March 18, 2019 15:27
Co-Authored-By: tessr <tess.rinearson@gmail.com>
Co-Authored-By: tessr <tess.rinearson@gmail.com>
Co-Authored-By: tessr <tess.rinearson@gmail.com>
Co-Authored-By: tessr <tess.rinearson@gmail.com>
@tessr
Copy link
Contributor Author

tessr commented Mar 18, 2019

Awesome. I'm going to commit your suggestions, fix any breakages, and then make the bigger changes. Thanks very much!

keytree/src/lib.rs Outdated Show resolved Hide resolved
keytree/src/lib.rs Outdated Show resolved Hide resolved
oleganza and others added 2 commits March 18, 2019 17:05
Co-Authored-By: tessr <tess.rinearson@gmail.com>
@tessr
Copy link
Contributor Author

tessr commented Mar 19, 2019

Ready for an (I think) final pass!

keytree/src/lib.rs Outdated Show resolved Hide resolved
keytree/src/lib.rs Outdated Show resolved Hide resolved
@tessr
Copy link
Contributor Author

tessr commented Mar 19, 2019

Ready for another pass! (I will not jinx it this time by suggesting it's final.)

@tessr tessr merged commit 55f3340 into main Mar 19, 2019
@tessr tessr deleted the tessr/serialize branch March 19, 2019 17:58
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.

None yet

2 participants