Skip to content

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rustaceanrob
Copy link
Collaborator

Description

Move Descriptor, DescriptorPublicKey, DescriptorPrivateKey, DerivationPath, and Mnemonic to proc macros. Adds documentation when appropriate, including links. Uses uniffi::export(Display) to derive Display, think that is the proper way to do it?

Notes to the reviewers

There was a TODO questioning the use of thread_rng to create a mnemonic. Yes, thread_rng implements CryptoRng: https://docs.rs/rand/latest/rand/rngs/struct.ThreadRng.html#impl-CryptoRng-for-ThreadRng

Checklists

All Submissions:

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

@rustaceanrob rustaceanrob force-pushed the descriptor-4-25 branch 2 times, most recently from b536602 to 3307731 Compare April 25, 2025 13:17
@thunderbiscuit thunderbiscuit self-requested a review April 25, 2025 15:16
@rustaceanrob rustaceanrob force-pushed the descriptor-4-25 branch 2 times, most recently from 6e6fde0 to 751134c Compare April 25, 2025 16:07
@rustaceanrob
Copy link
Collaborator Author

751134c is rebased

@rustaceanrob
Copy link
Collaborator Author

2130c64 rebased once more

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.

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.

Comment on lines 257 to 259
pub fn as_string(&self) -> String {
self.0.to_string()
}
Copy link
Member

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.

Comment on lines 188 to 190
pub fn as_string(&self) -> String {
self.0.to_string()
}
Copy link
Member

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.

Copy link
Collaborator Author

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?

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.

2 participants