Skip to content
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

Allow easy concatenation of bip32 derivation paths #459

Merged

Conversation

sgeisler
Copy link
Contributor

Currently one has to convert the path into a Vec<ChildNumber>, extend it and finally convert it back again.

@sgeisler sgeisler force-pushed the 2020-08-extend-derivation-path branch from 6965a26 to 195fad1 Compare August 19, 2020 14:10
@apoelstra
Copy link
Member

warning: unused variable: `path`
   --> /home/apoelstra/dload/code/rust-bitcoin/rust-bitcoin/src/util/bip32.rs:330:26
    |
330 |     pub fn extend(&self, path: impl AsRef<[ChildNumber]>) -> DerivationPath {
    |                          ^^^^ help: if this is intentional, prefix it with an underscore: `_path`
    |
    = note: `#[warn(unused_variables)]` on by default

error[E0502]: cannot borrow `path.0` as mutable because it is also borrowed as immutable
   --> /home/apoelstra/dload/code/rust-bitcoin/rust-bitcoin/src/util/bip32.rs:332:9
    |
332 |         path.0.extend_from_slice(path.as_ref());
    |         ^^^^^^^-----------------^----^^^^^^^^^^
    |         |      |                 |
    |         |      |                 immutable borrow occurs here
    |         |      immutable borrow later used by call
    |         mutable borrow occurs here

error: aborting due to previous error; 1 warning emitted

For more information about this error, try `rustc --explain E0502`.
error: could not compile `bitcoin`.

@@ -326,6 +326,12 @@ impl DerivationPath {
pub fn hardened_children(&self) -> DerivationPathIterator {
DerivationPathIterator::start_from(&self, ChildNumber::Hardened{ index: 0 })
}

pub fn extend(&self, path: impl AsRef<[ChildNumber]>) -> DerivationPath {
let mut path = self.clone();
Copy link
Member

@elichai elichai Sep 9, 2020

Choose a reason for hiding this comment

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

  1. you override the path input variable here

  2. why does this create a new DerivationPath instead of extending the current? (I see that child() does the same)

  3. why is impl AsRef<[ChildNumber]> better than a simple &[ChildNumber]?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Number 3 makes this non-compiling for the current MSRV btw

Currently one has to convert the path into a Vec<ChildNumber>, extend it and finally convert it back again.
@sgeisler sgeisler force-pushed the 2020-08-extend-derivation-path branch from 195fad1 to 202a946 Compare September 9, 2020 19:43
@sgeisler
Copy link
Contributor Author

sgeisler commented Sep 9, 2020

Sorry, fixed.

@elichai:

  1. I still wonder how that happened
  2. Mainly because I didn't want to mutate the object the method is called on and it fit my use case (would require a lot of clones anyway otherwise) and as you noted other methods like child does it in the same way
  3. It allows you to use slices too. I'm inclined to make DerivationPath a trait to extend &[ChildNumber] eventually, it's weird to only have an owned version of it.

Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

I think more idiomatic way of doing this may be

    pub fn extend<T: IntoIterator<Item=ChildNumber>>(&self, path: T) -> DerivationPath {
        self.into_iter(0).chain(path).collect()
    }

This will require impl IntoIter for DerivationPath, but this is trivial and will be useful anyway

Also, if you will choose that route, I propose to impl ExactSizeIterator for DerivationPath as well

@sgeisler
Copy link
Contributor Author

sgeisler commented Sep 9, 2020

IntoIterator::into_iter would consume self though and I don't know how well vec -> clone -> into_iter -> vec will be optimized to avoid one unnecessary allocation. Would it be that much more idiomatic?

PS: the current travis failure seems related to #468

@elichai
Copy link
Member

elichai commented Sep 10, 2020

  1. It allows you to use slices too.

Could you give a concrete example of what it will allow that &[ChildNumber] won't?

@apoelstra
Copy link
Member

concept ACK. I'm a little ambivalent about using extend as the name when it disagrees with the signature of the standard extend function but otherwise this approach seems fine.

@apoelstra
Copy link
Member

Also tested ACK

@apoelstra apoelstra merged commit 3748e8f into rust-bitcoin:master Oct 7, 2020
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.

5 participants