-
Notifications
You must be signed in to change notification settings - Fork 54
Migrate descriptor types to proc macros #742
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
b536602
to
3307731
Compare
6e6fde0
to
751134c
Compare
751134c is rebased |
751134c
to
2130c64
Compare
2130c64 rebased once more |
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'm trying to decide if these are part of the API we agreed not to break as part of the 1.0, because the renaming of the keychain
argument to keychainKind
is a breaking change.
The removal of the as_string()
methods in favour of the Display trait which would give us the better toString()
would also be, but in this case we can simply have both and not create a break.
bdk-ffi/src/keys.rs
Outdated
pub fn as_string(&self) -> String { | ||
self.0.to_string() | ||
} |
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.
Same as above: this is a vestige of the times where we could not get the Display trait on types.
I think we should add the Display in this case so the .toString()
method is implemented for DescriptorPublicKey
.
bdk-ffi/src/keys.rs
Outdated
pub fn as_string(&self) -> String { | ||
self.0.to_string() | ||
} |
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 a vestige of the times where we could not get the Display trait on types.
I think we should add the Display in this case so the .toString()
method is implemented for DescriptorSecretKey
.
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 am not sure I'm following fully, meaning just derive Display
? Or also remove this?
5e8c186
to
1c9574b
Compare
|
1c9574b
to
84a14bc
Compare
Description
Move
Descriptor
,DescriptorPublicKey
,DescriptorPrivateKey
,DerivationPath
, andMnemonic
to proc macros. Adds documentation when appropriate, including links. Usesuniffi::export(Display)
to deriveDisplay
, think that is the proper way to do it?Notes to the reviewers
There was a
TODO
questioning the use ofthread_rng
to create a mnemonic. Yes,thread_rng
implementsCryptoRng
: https://docs.rs/rand/latest/rand/rngs/struct.ThreadRng.html#impl-CryptoRng-for-ThreadRngChecklists
All Submissions:
cargo fmt
andcargo clippy
before committing