Skip to content

Added Mnemonic Interface #219

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

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

dhruv-1001
Copy link
Contributor

@dhruv-1001 dhruv-1001 commented Oct 28, 2022

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](https://github.com/bitcoindevkit/bdk-ffi/pull/219)

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
    • Top level function generate_mnemonic(...) was removed

@notmandatory
Copy link
Member

This looks good, can you also add a Mnemonic.from_entropy() constructor? See also #218 (comment).

@dhruv-1001
Copy link
Contributor Author

Added the Mnenomic.from_entropy() constructor

@notmandatory
Copy link
Member

I think this one is ready to go. I tested it by running also a quick test in test.kts below and all works as expected. We don't need to make any permanent change to test.kts but this was helpful for me to confirm that everything is OK without the Mutex.

import org.bitcoindevkit.*

val network = Network.TESTNET

val mnemonic = Mnemonic(WordCount.WORDS12)

println("mnemonic = ${mnemonic.asString()}")

val descriptorSecKey = DescriptorSecretKey(network, mnemonic, null)

println("descriptorSecKey = ${descriptorSecKey.asString()}")

println("mnemonic = ${mnemonic.asString()}")

@notmandatory notmandatory marked this pull request as ready for review November 1, 2022 23:03
@notmandatory
Copy link
Member

Also since this is calling bdk code that's already tested I don't think you need to add any addition testing for these new bindings APIs.

@notmandatory notmandatory added the enhancement New feature or request label Nov 1, 2022
@thunderbiscuit
Copy link
Member

thunderbiscuit commented Nov 2, 2022

Just to be clear the "changes" I'm requesting are regarding the documentation. The mnemonic generation is a bit weird to me but I don't want to block this PR on it. Just hoping to start a discussion.

Also: hey glad to see you back @dhruv-1001! It's awesome to see a Summer of Bitcoin student contributing again after the summer is over! 🚀 🚀

@dhruv-1001 dhruv-1001 requested review from thunderbiscuit and notmandatory and removed request for thunderbiscuit and notmandatory November 3, 2022 18:57
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 9866649.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 9866649

@notmandatory notmandatory merged commit 73ba73f into bitcoindevkit:master Nov 7, 2022
@thunderbiscuit thunderbiscuit mentioned this pull request Nov 8, 2022
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants