-
Notifications
You must be signed in to change notification settings - Fork 65
feat: Add derivation path child and extend methods #935
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
base: master
Are you sure you want to change the base?
Conversation
thunderbiscuit
left a comment
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 is looking good!
Because it's a simple type and methods I'd love to see a few tests that would play around with the DerivationPath type. Would you mind adding a few, at least on the Android tests?
Sure! I could add those. I did add some tests to the rust side before now. But its good to see it on bindings side too! |
92e4955 to
4602ed5
Compare
bdk-ffi/src/keys.rs
Outdated
| } | ||
|
|
||
| /// Create a new DerivationPath that is a child of this one. | ||
| pub fn child(&self, child_number: ChildNumber) -> Arc<Self> { |
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.
Should this validate ChildNumber before converting? ChildNumber exposes raw u32, so callers can pass index >= 2^31 and we’ll build a non‑BIP32 path. Upstream has from_normal_idx/from_hardened_idx maybe child should return Result and use those checked constructors
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.
Shouldn't a caller be able to enter an index >= 2^31 ? Those are hardened path. The child method will treat it as such.
EDIT - SMH. I see now
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.
When I was looking at https://github.com/rust-bitcoin/rust-bitcoin/blob/bitcoin-0.32.8/bitcoin/src/bip32.rs#L124-L162 I didnt think so, I was reading it as both Hardened and Normal required under that
bdk-ffi/src/types.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl From<ChildNumber> for BdkChildNumber { |
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.
Should this be TryFrom using the checked constructors to enforce the BIP32 index range? Right now any u32 passes through
| expected = expectedNormalChildPath, | ||
| actual = childDerivationPath.toString() | ||
| ) | ||
|
|
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 think if you set childIndex to 2147483648u and then add assertFailsWith<Exception> { DerivationPath(childDerivationPath.toString()) } it could fail because the path can be constructed but not parsed back because I think we’re bypassing the BIP32 index checks?
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.
Ahh! that is a dangerous catch! Thanks! I will address this tomorrow
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.
sounds good, overall this pr is looking good though 👍
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.
Made some updates. Added new test in relation to these changes too checkInvalidNormalChildNumberFails and checkInvalidHardenedChildNumberFails
4602ed5 to
6ccbb38
Compare
Description
Added more methods for the derivation path type. Below are 4 convenient methods I have chosen to leave out and why.
into_child: This does the same thingchilddoes except that one consumes self and the other clones. This will not make a difference on bindings level.children_from,normal_childrenandhardened_childrenThe above 3 can still be performed by the using using the
childmethod. Which is more idiomatic. (as long as the state of param is maintained properly)Notes to the reviewers
Documentation
bdk_walletbitcoinuniffiChecklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: