Skip to content

Conversation

@ItoroD
Copy link
Collaborator

@ItoroD ItoroD commented Oct 28, 2025

Description

Notes to the reviewers

Added more useful methods for DerivationPath for now I added the simplest ones. More should be added later; like child, into_child.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing
  • I've added a changelog in the next release tracking issue (see example)

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@ItoroD ItoroD force-pushed the derivationpath-method branch 2 times, most recently from 37561a5 to 68670ac Compare October 28, 2025 18:01
@ItoroD ItoroD requested a review from thunderbiscuit October 28, 2025 18:18
@reez
Copy link
Collaborator

reez commented Oct 29, 2025

I dig this addition. Are you going to add #[uniffi::export(Display)] to DerivationPath?

@ItoroD ItoroD force-pushed the derivationpath-method branch 2 times, most recently from a9875c2 to 6544423 Compare October 29, 2025 23:46
@ItoroD
Copy link
Collaborator Author

ItoroD commented Oct 29, 2025

I dig this addition. Are you going to add #[uniffi::export(Display)] to DerivationPath?

Good call.

@thunderbiscuit
Copy link
Member

This is looking good! I would need 2 things before merging:

  1. Rebase on latest (new commit comming from Refactor DerivationPath to use tuple #895)
  2. Add a commit that refactors the KeySource type to use DerivationPath instead of String.

@ItoroD ItoroD force-pushed the derivationpath-method branch 2 times, most recently from 8f39944 to 1cd2df0 Compare October 30, 2025 23:00
@ItoroD ItoroD force-pushed the derivationpath-method branch from 1cd2df0 to e2809be Compare October 30, 2025 23:11
@ItoroD
Copy link
Collaborator Author

ItoroD commented Oct 30, 2025

This is looking good! I would need 2 things before merging:

  1. Rebase on latest (new commit comming from Refactor DerivationPath to use tuple #895)
  2. Add a commit that refactors the KeySource type to use DerivationPath instead of String.

Done! Just arranged the commits in second push

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.

Just a few cleanup ideas.

Comment on lines 727 to 736
fingerprint: v.1 .0.to_string(),
path: v.1 .1.to_string(),
path: Arc::new(v.1 .1.clone().into()),
Copy link
Member

Choose a reason for hiding this comment

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

The spaces here I thought would have been caught by the cargo formatter, but they weren't. Those lines could be cleaned up as:

fingerprint: v.1.0.to_string(),
path: Arc::new(v.1.1.clone().into()),

Copy link
Member

Choose a reason for hiding this comment

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

Man those deeply nested types are quite a handful 😆😆😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cargo fmt is actually the one adding those spaces


/// A BIP-32 derivation path.
#[derive(uniffi::Object)]
#[derive(Clone, Debug, Hash, Eq, PartialEq, uniffi::Object)]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think they hurt much, but in general I'm wary of adding trait implementations that are not needed. Note also that those are not exported to the bindings, so they'd need to be useful at the Rust layer, but I don't know that they are for now (the Clone is, but not the Eq, PartialEq, and Hash).

Copy link
Member

Choose a reason for hiding this comment

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

After playing with it locally, the Debug trait is also used (as well as the Clone). Others can be dropped on this one and others that have them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call

/// A BIP-32 derivation path.
#[derive(uniffi::Object)]
#[derive(Clone, Debug, Hash, Eq, PartialEq, uniffi::Object)]
pub struct DerivationPath(pub(crate) BdkDerivationPath);
Copy link
Member

Choose a reason for hiding this comment

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

I think you're missing the Display trait export here:

#[derive(Debug)]
#[derive(uniffi::Object)]
#[uniffi::export(Display)]
pub struct DerivationPath(pub(crate) BdkDerivationPath);

Copy link
Member

Choose a reason for hiding this comment

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

You have the Display implemented below, but bindings users won't get it unless we add the #[uniffi::export(Display)] line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again! I thought I added that. I really do need to "Shine my eye". Maybe lost on rebase and merge resolution. Thanks

@ItoroD ItoroD force-pushed the derivationpath-method branch from e2809be to 4a320b1 Compare October 31, 2025 14:26
@ItoroD ItoroD force-pushed the derivationpath-method branch from 4a320b1 to ffcae65 Compare October 31, 2025 14:43
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 ffcae65.

@thunderbiscuit thunderbiscuit merged commit ffcae65 into bitcoindevkit:master Oct 31, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants