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

Add support for extended BIP32 #13

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

randombit
Copy link

No description provided.

@BenjaminLoison BenjaminLoison self-requested a review July 20, 2022 22:46
@BenjaminLoison
Copy link
Collaborator

BenjaminLoison commented Jul 20, 2022

The extended BIP 32 derivation tests are missing and failing if they are added AFAIU as:

extended public key public key chain code
xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8 0339a36013301597daef41fbe593a02cc513d0b55527ec2df1050e2e8ff49c85c2 873dff81c02f525623fd1fe5167eac3a55a049de3d314bb42ee227ffed37d508
xpub661MyMwAqRbcGsSdYunw6oK1384d9MkdQWK3472cWknZ2v6QyuXM8B5YRXSHBdSCbQoG25HfdhzpsBDsE4Mz9fgxiH27e1VEPMFtAcktt97 026df3c4ae791c077ca502731dee24f6256bc1c3de7993bb11ae9f978aa6a8ca28 e9cd21099080b51aa259ad8959310f60390e3f3ac0cdd1575dee040c2e7929e7
xpub661MyMwAqRbcFzWy5kpQCd4RXzF1b5MmeeErvCkoSgWwmnW32wmayo9G2tzPo1C6zzENmforcwhtcF1omLFgcfNmLoQAzpuwFeUjH6nWzqr 02dca3e632f5b42cac3c79c622f931dae45263ffb96d772ab5fd041ca3f2bde666 919c85259ed83eb8f92ca35442173a27905ea6444b525ecd70c638f64d4531da
xpub661MyMwAqRbcFUpN6pbTnL6ygjLitCXnjat7UVwyhcU4F2NMRu3KQ8J8bh6hrxHDiG3KdjWTWw9ozxWdFP98W1rCvPjRqWob5QX7RjPJMTj 02027909cb580dfc0b25de2d23c31d9a1bd34e02617f28464847674273afaa4a6d 5e2da3317cb2196083fc5d14dc6522647857a091f9771ce4b90356399022320f

@randombit
Copy link
Author

I think this issue is that the xpub serialization format assumes the index is exactly 4 bytes, no more or less. Probably attempting to serialize an extended key without that size index should fail.

@BenjaminLoison
Copy link
Collaborator

BenjaminLoison commented Jul 26, 2022

I think this issue is that the xpub serialization format assumes the index is exactly 4 bytes, no more or less. Probably attempting to serialize an extended key without that size index should fail.

I am fine with this argument as the PR looks good to me and this test problem would requite to redesign them a bit as they are currently based on xpub.

UPDATE: Having the same tests for both Rust and Motoko may makes sense if it's worth it, as discussed with @THLO, it's up to @tgalal to adapt its tests to make sure that the extended BIP-32 you proposed is correct.

Copy link
Owner

@tgalal tgalal left a comment

Choose a reason for hiding this comment

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

Do we need to support standard Nat32 indices at all? In the changes they are just converted to [Nat8] which I am not sure if it's on purpose or was just a quick fix. To me the options are:

  • Keep the implicit conversion as is done at the moment
  • Support extended and standard indices by treating them independently by never converting one to the other, then we need to bring back the isHardenedIndex check and its associated couple of tests.
  • Completely omit Nat32 indices (but then we also either omit the function parsing paths from strings or still convert the parsed indices into [Nat8], and callers must always supply extended paths.

I understand the extended derivation is a generalization of the standard one, so I am just talking from an API perspective here.

@THLO
Copy link
Contributor

THLO commented Aug 15, 2022

I think we should support standard (Nat32) indices for everybody who wants to use BIP-32 compliant derivation paths. So, as far as the API is concerned, we need a function that accepts a Nat32 vector as input. Additionally, we want to support arbitrary bytes, which should be a second function that accepts a Nat8 vector.

As far as the implementation is concerned, I'm not sure which option is better: Having completely separate implementations or checking that each level is non-hardened and then converting the 32-bit vector to an 8-bit vector to be fed into the other function.
I would assume the second approach to produce less code and so it should be easier to maintain. @tgalal, what do you think?

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.

4 participants