Add child key pair generation api #154

dhruv-1001 · 2022-05-15T22:02:47Z

Completes #87

dhruv-1001 · 2022-05-15T22:05:51Z

@notmandatory @artfuldev please review this draft.

src/lib.rs

artfuldev · 2022-05-16T11:30:30Z

src/bdk.udl

  [Throws=BdkError]
  ExtendedKeyInfo restore_extended_key(Network network, string mnemonic, string? password);
+  [Throws=BdkError]
+  ChildKeyPair derive_child_key_pair(string str_xprv, string str_path);


Do we not need to expose ExtendedPrivKey and DerivationPath? @notmandatory

@notmandatory if this is the only way to create a child key pair, can it be a constructor?

I modified the bdk-ff commit that the bdk-kotlin uses, and made same changes. After that, I added test for the deriveChildKeyPair() function

@Test fun validChildKeys() { val str_xprv = "tprv8ZgxMBicQKsPdMTRB73kkTfC665bK3JL33eAFZ3MUiGZj8jniKQroG2yuamoQcRb24GG8dKRmVgaisjcZrDc9L3fyMf2NW19oajw8iWSwMV" val str_path = "m/84'/1'/0'/0" val childKeyPair = deriveChildKeyPair(str_xprv, str_path) assertEquals( "[59a3697d/84'/1'/0'/0]tprv8hZWYaWfKprzVnSVaaxwYun2yVp9TgpvcosjrGYwgL9hKMU1Jsrp3xKsNCpxSNBSEgUPp7c8A1MYLQcMcq7Ab9eafzNZUHQaBtGBvfCQiMg/*", childKeyPair.xprv ) assertEquals( "[59a3697d/84'/1'/0'/0]tpubDEFYgzYuUCYfPFUHUEdXxKS9YXL5d21qC7UX8nbF6bx69qimwGgQESwjYN52awKwDd5PWGKXWAw27EBUk6QzSK3eEwsK8odY8FxVn4poRDp/*", childKeyPair.xpub ) }

And It worked

(I used the CLI to derive this result first, and used the same key for testing)

Not discounting that it works, just checking if the types are okay or if we need something more elaborate. and if child key pair could have a constructor instead of this function deriveChildKeyPair. also wondering if ChildKeyPair is what we want to expose - it could be any key pair, not necessarily only a "child" key pair. just thinking out loud.

I think it's fine to keep the types and design for this simple to start and not add additional types for ExtendedPrivKey and DerivationPath. They can always be added later if we find a need, I think most apps will 1. generate_extended_key and get the user to write down the mnemonic, 2) from the ExtendedKeyInfo.xprv or possibly from importing a xprv from somewhere else, derive a descriptor key, and 3) create a descriptor for the wallet, currently needs to be done putting strings together but in future we can make it simpler with some standard/common templates. That all said, I have two naming change suggestions:

rename ChildKeyPair to DescriptorKeyInfo which is more in line with what bdk calls it

rename arguments str_xprv and str_path to simply xprv and path since type is already enforced thus redundant in the variable name.

As for creating the ChildKeyPair or DescriptorKeyInfo with a constructor, if we do that we need to change it from dictionary (pass by value) to an interface (pass by reference), so I'd prefer to keep it a dictionary and not use a constructor.

One other suggestion, in the case someone has a watch-only wallet then they won't have the xprv for the DescriptorKeyInfo and will only have the xpub. So I think you should make xprv optional, ie. string? xprv and xprv: Option<String>.

Noted. Will force push the changes soon.

Got it, thanks @notmandatory

artfuldev

Just have some questions, mostly for @notmandatory

artfuldev · 2022-05-16T11:33:41Z

src/lib.rs

+    if let Secret(desc_seckey, _, _) = derived_xprv_desc_key {
+        let desc_pubkey = desc_seckey
+            .as_public(&secp)
+            .map_err(|e| Error::Generic(e.to_string()))?;


The bdk-cli code here uses .unwrap() - @notmandatory can you add some context?

The bdk-cli code here uses .unwrap() - @notmandatory can you add some context?

I think maybe you're looking at an older version of bdk-cli 🙂 this is how the cli code works also without an .unwrap().

artfuldev · 2022-05-16T11:35:47Z

src/bdk.udl

  [Throws=BdkError]
  ExtendedKeyInfo restore_extended_key(Network network, string mnemonic, string? password);
+  [Throws=BdkError]
+  ChildKeyPair derive_child_key_pair(string str_xprv, string str_path);


@notmandatory if this is the only way to create a child key pair, can it be a constructor?

dhruv-1001 · 2022-05-25T02:28:34Z

@artfuldev @notmandatory any suggestions here?

artfuldev · 2022-05-25T12:15:13Z

src/lib.rs

+        network: Network,
+        word_count: WordCount,
+        password: Option<String>,
+    ) -> Self {


Can we retain Result<T, E> semantics here? Did we remove it intentionally? I'd prefer not having .unwrap() and throwing exceptions in Kotlin etc.

Got it, will add back the Result<T, E> and force push the changes.

src/lib.rs

kirillzh · 2022-06-28T01:37:29Z

src/bdk.udl

+};
+
+interface ExtendedPubKey {
+  string to_string();


Looks like this won't compile in Kotlin since toString() is an open, member function of Any that needs to be overriden and uniffi doesn't seem to have a way to declare an overriding API, as far as I can tell. A simple workaround here would be to rename to_string() to something else (maybe ExtendedPubKey#value or #raw read-only property?

)

Filed an issue with uniffi to see if it's feasible to allow adding override modifier in Kotlin: mozilla/uniffi-rs#1286.

src/lib.rs

notmandatory · 2022-06-29T02:37:09Z

Overall looks good, I commented on a few places you can remove the .unwrap() and use map_error and map to generate your Result. There are a few others I didn't comment on, but in general try to avoid .unwrap() if you can, except in tests where it's OK.

notmandatory · 2022-07-15T18:13:15Z

I had a little chat with @thunderbiscuit and @dhruv-1001 and it looks like the logic in this PR probably does belong in bdk. I'm going to take a stab at adding DescriptorKey::derive() and DescriptorKey::extend() functions in bdk. Then we can review and discuss there and expose what ever API we come up with to bdk-ffi. How does that sound @quad ?

notmandatory · 2022-07-15T20:05:35Z

@quad I've created a draftbdk PR to explore what I had in mind for an API. The main diff I think with what you have is that I'm doing everything at the DescriptorKey level. Let me know what you and your team think and/or we can jump on a call to discuss.

bitcoindevkit/bdk#670

notmandatory

Overall looks good! I'd just like the match statements cleaned up to avoid returning None and then unwrap(), which I don't think is as clear.

src/lib.rs

notmandatory

ACK 5bc3811

Good work! this one was a challenging API to simplify for FFI and I think this way strikes the right balance.

notmandatory · 2022-08-18T17:32:04Z

Note to anyone following this issue, as discussed in bitcoindevkit/bdk#670 we decided not to put this logic in the bdk lib, but once this PR is in I'll add some code example in the bdk repo, it should be only a couple lines to generate, derive or extend a DescriptorPublicKey or DescriptorSecretKey.

9866649 Added Mnemonic Interface (dhruvbaliyan) Pull request description: ### Description This PR adds `interface Mnemonic` which will make the API to generate new DescriptorSecretKey have type-safe arguments.  ### Notes to the reviewers This PR doesn't have any issue linked to it, as it was discusses on a call during the implementation of `DescriptorSecretKey` (PR #154). It was discussed to make `Mnemonic` an interface and use that instead of string `Mnemonic` so that the API to generate `DescriptorSecretKey` doesn't have any potential failure (like in case it's provided with incorrect Mnemonic words). APIs added ``` // generates and returns Mnemonic with random entropy Mnemonic::new(word_count: WordCount) -> Self { ... } // converts string Mnemonic to Mnemonic type with error (in case of incorrect string Mnemonic) Mnemonic::from_str(mnemonic: String) -> Result<Self, BdkError> { ... } // generates and returns Mnemonic with given entropy Mnemonic::from_entropy(entropy: Vec<u8>) -> Result<Self, BdkError> {...} // view mnemonic as string Mnemonic::as_string(&self) -> String { ... } ``` Along with some changes to `DescriptorSecretKey::new()` to fit these new APIs ### Changelog notice ``` - Added Struct Mnemonic with following methods [#219] - new(word_count: WordCount) generates and returns Mnemonic with random entropy - from_str(mnemonic: String) converts string Mnemonic to Mnemonic type with error - from_entropy(entropy: Vec<u8>) generates and returns Mnemonic with given entropy - as_string() view Mnemonic as string - API removed [#219] - generate_mnemonic(word_count: WordCount) [#219](#219) ``` ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added docs for the new feature #### Bugfixes: * [x] This pull request breaks the existing API * Top level function `generate_mnemonic(...)` was removed ACKs for top commit: thunderbiscuit: ACK 9866649. notmandatory: ACK 9866649 Tree-SHA512: 45f9158beb6fe7bfe2a901c3f17126db855fe0b4b479ecb2a16381e06a415eed295fe6be3c840bd1d1fc8cffaf58bd97dc27bdc1e82699367a827d700e8fd09b

dhruv-1001 marked this pull request as ready for review May 15, 2022 22:03

dhruv-1001 marked this pull request as draft May 15, 2022 22:04

artfuldev reviewed May 16, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

artfuldev reviewed May 16, 2022

View reviewed changes

notmandatory assigned dhruv-1001 May 16, 2022

thunderbiscuit mentioned this pull request May 17, 2022

W20 BDK Language Bindings Dev Call bitcoindevkit/.github#4

Closed

16 tasks

dhruv-1001 force-pushed the add-child-key-generation-api branch from 75d759e to 3291839 Compare May 25, 2022 02:23

artfuldev reviewed May 25, 2022

View reviewed changes

artfuldev reviewed May 26, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

This was referenced Jun 6, 2022

Expose xpub in ExtendeKeyInfo #156

Closed

Add ExtendedPrivKey and ExtendPubKey classes #138

Closed

thunderbiscuit mentioned this pull request Jun 14, 2022

W24 BDK Language Bindings Dev Call bitcoindevkit/.github#10

Closed

8 tasks

thunderbiscuit added this to the Release 0.8.0 milestone Jun 17, 2022

dhruv-1001 force-pushed the add-child-key-generation-api branch from 3291839 to 2929f15 Compare June 22, 2022 13:37

dhruv-1001 requested review from artfuldev and notmandatory June 22, 2022 13:39

notmandatory reviewed Jun 23, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

dhruv-1001 force-pushed the add-child-key-generation-api branch 2 times, most recently from 8069624 to b2b3a8c Compare June 23, 2022 17:27

kirillzh reviewed Jun 28, 2022

View reviewed changes

thunderbiscuit mentioned this pull request Jun 28, 2022

W26 BDK Language Bindings Dev Call bitcoindevkit/.github#11

Closed

notmandatory reviewed Jun 29, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

notmandatory reviewed Jun 29, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

dhruv-1001 force-pushed the add-child-key-generation-api branch from b2b3a8c to e538b72 Compare June 30, 2022 17:43

dhruv-1001 marked this pull request as ready for review June 30, 2022 17:43

dhruv-1001 force-pushed the add-child-key-generation-api branch from e538b72 to 2a2ffde Compare June 30, 2022 17:49

notmandatory mentioned this pull request Jul 15, 2022

Add DescriptorKey derive, extend, to_public and to_string functions bitcoindevkit/bdk#670

Closed

6 tasks

This was referenced Jul 18, 2022

W28 BDK Language Bindings Dev Call bitcoindevkit/.github#13

Closed

W30 BDK Language Bindings Dev Call bitcoindevkit/.github#15

Closed

notmandatory removed this from the Release 0.8.0 milestone Jul 29, 2022

dhruv-1001 marked this pull request as draft August 2, 2022 21:12

dhruv-1001 force-pushed the add-child-key-generation-api branch from 590ab37 to 2ab9e09 Compare August 9, 2022 09:13

notmandatory requested changes Aug 11, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

dhruv-1001 force-pushed the add-child-key-generation-api branch from 2ab9e09 to e77e48c Compare August 11, 2022 09:02

dhruv-1001 marked this pull request as ready for review August 16, 2022 12:03

dhruv-1001 requested a review from notmandatory August 16, 2022 12:04

dhruv-1001 force-pushed the add-child-key-generation-api branch from e77e48c to 5bc3811 Compare August 17, 2022 13:23

notmandatory approved these changes Aug 17, 2022

View reviewed changes

notmandatory added this to the Release 0.9.0 milestone Aug 17, 2022

notmandatory added enhancement New feature or request summer-of-bitcoin labels Aug 17, 2022

dhruv-1001 added 5 commits August 18, 2022 04:24

Removed ExtendedKeyInfo & related methods

973013c

Added generate_mnemonic method

930a1f1

Added interface DerivationPath

4977cb6

Added interfaces DescriptorSecretKey and DescriptorPublicKey

58fea6b

Added tests for DescriptorSecretKey and DescriptorPublicKey

5944756

dhruv-1001 force-pushed the add-child-key-generation-api branch from 5bc3811 to 5944756 Compare August 17, 2022 22:54

notmandatory merged commit 7da2865 into bitcoindevkit:master Aug 18, 2022

thunderbiscuit mentioned this pull request Aug 23, 2022

W34 BDK Language Bindings Dev Call bitcoindevkit/.github#19

Closed

ConorOkus added the ldk-compatibility label Aug 31, 2022

dhruv-1001 mentioned this pull request Oct 21, 2022

Implement basic spending policy APIs #80

Closed

dhruv-1001 mentioned this pull request Oct 28, 2022

Added Mnemonic Interface #219

Merged

5 tasks

Add child key pair generation api #154

Add child key pair generation api #154

Uh oh!

Conversation

dhruv-1001 commented May 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhruv-1001 commented May 15, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruv-1001 May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmandatory May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artfuldev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmandatory May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruv-1001 commented May 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kirillzh Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

notmandatory commented Jun 29, 2022

Uh oh!

notmandatory commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory commented Jul 15, 2022

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Aug 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

dhruv-1001 commented May 15, 2022 •

edited

Loading

dhruv-1001 May 16, 2022 •

edited

Loading

notmandatory May 17, 2022 •

edited

Loading

notmandatory May 17, 2022 •

edited

Loading

kirillzh Jun 28, 2022 •

edited

Loading

notmandatory commented Jul 15, 2022 •

edited

Loading