This looks great to me. A minor point on naming is that I notice we have from_*_idx methods, which are weird because (a) idx is an abbreviation that we don't use in other method names (according to our API docs it does appear in the bip152 module which we should also fix), but (b) our types are called Numbers not indexes.

I think we should change the names of the new types to HardenedChildNumber etc rather than just HardenedChild. This is a mouthful but I think usually users won't be using these, since in almost all cases they'd want the more-generic-and-shorter ChildNumber enum. And we should rename from_hardened_idx to from_hardened_child_number and same for the non-hardened one.

This looks great to me. A minor point on naming is that I notice we have from_*_idx methods, which are weird because (a) idx is an abbreviation that we don't use in other method names (according to our API docs it does appear in the bip152 module which we should also fix), but (b) our types are called Numbers not indexes.

Ok will do in this PR.

I think we should change the names of the new types to HardenedChildNumber etc rather than just HardenedChild. This is a mouthful but I think usually users won't be using these, since in almost all cases they'd want the more-generic-and-shorter ChildNumber enum.

Should we remove the pub from these new structs?

Pull Request Test Coverage Report for Build 9650838641

Details

• 93 of 122 (76.23%) changed or added relevant lines in 2 files are covered.
• 12 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.09%) to 83.052%

Totals
Change from base Build 9639464882:	-0.09%
Covered Lines:	19553
Relevant Lines:	23543

💛 - Coveralls

Pull Request Test Coverage Report for Build 9650886863

Details

• 93 of 122 (76.23%) changed or added relevant lines in 2 files are covered.
• 12 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.09%) to 83.052%

Totals
Change from base Build 9639464882:	-0.09%
Covered Lines:	19553
Relevant Lines:	23543

💛 - Coveralls

Pull Request Test Coverage Report for Build 9659761194

Details

• 94 of 127 (74.02%) changed or added relevant lines in 2 files are covered.
• 12 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.1%) to 83.039%

Totals
Change from base Build 9639464882:	-0.1%
Covered Lines:	19554
Relevant Lines:	23548

💛 - Coveralls

Note to myself (TODO)

Ok, we can now accept in Xpub::derive_pub a AsRef<[NormalChildNumber]>> but we need to implement the AsRef<[NormalChildNumber]>> for DerivationPath

Pull Request Test Coverage Report for Build 9664378557

Details

• 84 of 114 (73.68%) changed or added relevant lines in 2 files are covered.
• 12 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.1%) to 83.055%

Totals
Change from base Build 9663915609:	-0.1%
Covered Lines:	19576
Relevant Lines:	23570

💛 - Coveralls

we need to implement the AsRef<[NormalChildNumber]>> for DerivationPath

That is impossible. At best we could completely revamp the layout to allow unsafe casts but that loses conversions to enum. Now I realize the situation is pretty annoying and maybe the extension traits for Vec<*ChildNumber> & co are the right approach, so people could enforce normal numbers at parsing time but that still may suck. I think that a fallible version would still be useful.

(BTW time locks kinda have the same problem but people don't store them in arrays, so it's fine.)

I did a PR right now to @storopoli s branch...
where we did a tryfrom function that, from a DerivationPath, we could extract a vec<NormalChildNumber> that made the changes to work properly...
Still not resolving everything but we got progress.

@jaoleal that can be useful in a bunch of cases but it allocates so it'd be nice to see a non-allocating version too.

I`m not certain of what kind of "allocation" do you mean.

I wonder if is because of the
let mut ret = Vec::new()

Its possible to exclude that and retrieve directly from the DerivationPath struct. the only restriction is that we need to return a vec<KindOfKey> instead of a direct slicing...
But not possible for this one to be infallible because of the unreachable possibility of containing a HardenedChildNumber

Pedantically, it's not Vec::new but it's close enough. Returning a new vec requires an allocation (and the fact you mentioned new suggests your conversion isn't even optimal).

Its possible to exclude that and retrieve directly from the DerivationPath struct. the only restriction is that we need to return a vec<KindOfKey> instead of a direct slicing...

I don't understand this but it's getting late here, so maybe I'm just tired.

But not possible for this one to be infallible

Exactly that's why a fallible alternative method is useful.

That is impossible. At best we could completely revamp the layout to allow unsafe casts but that loses conversions to enum. Now I realize the situation is pretty annoying and maybe the extension traits for Vec<*ChildNumber> & co are the right approach, so people could enforce normal numbers at parsing time but that still may suck. I think that a fallible version would still be useful.

Yes, exactly. If we've Xpub::derive_pub only accepting AsRef<[NormalChildNumber]>> we then break the Xpub::derive_pub API and we need to fix all examples and other code to use NormalChildNumber.
This is a significant departure from the current API and also it would make Xpub::derive_pub very different from siblings that take a DerivationPath.

We can't infallible convert DerivationPath into a [NormalChildNumber] because nothing guarantees that the derivation path only contains NormalChildNumber (can still have HardenedChildNumber).

Is this a good tradeoff? Make Xpub::derive_pub but breaking the API?
If yes, then I will finish the work in this PR.
Otherwise we can close.

Pull Request Test Coverage Report for Build 9678710862

Details

• 94 of 126 (74.6%) changed or added relevant lines in 2 files are covered.
• 11 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.1%) to 83.058%

Totals
Change from base Build 9667039141:	-0.1%
Covered Lines:	19581
Relevant Lines:	23575

💛 - Coveralls

@storopoli API breaks are expected these days and we prefer better API over unbroken API.

I personally believe we should have both methods because some applications will prefer to enforce normal path at parsing and the conversion is complicated. Something like this should work:

// note the lack of pub
fn internal_derive_pub<T: TryInto<NormalChildNumber>(&self, path: impl Iterator<Item=T>) -> Result<Xpub, T::Error> {
// current code here, call .try_into()? on each item in the for loop
}

pub fn derive_pub<I: IntoIterator<Item=NormalChildNumber>>(&self, path: I) -> Xpub {
    self.internal_derive_pub(path.into_iter()).unwrap_or_else(|never| match never {})
}

pub fn try_derive_pub<I: IntoIterator<Item=ChildNumber>>(&self, path: I) -> Result<Xpub, NormalChildNumberError> {
    self.internal_derive_pub(path.into_iter())
}

(We could also just expose the internal method and let people use unwrap_or_else but I think we should make the better approach less annoying.)

pub fn derive_pub<I: IntoIterator<Item=NormalChildNumber>>(&self, path: I) -> Xpub {
    self.internal_derive_pub(path.into_iter()).unwrap_or_else(|never| match never {})
}

What is the point of using a non-null safe language to do this ?

Including the

impl TryFrom<DerivationPath> for Vec<NormalChildNumber> {
    type Error = Error;
    fn try_from(value: DerivationPath) -> Result<Self, Self::Error> {
        let mut ret = Vec::new();
        for cnum in value.0.iter() {
            match *cnum {
                ChildNumber::Normal(normal) => {
                    ret.push(normal);
                },
                _ => return Err(Error::InvalidChildNumberFormat),
            }
        }
        Ok(ret)
    }
}

we could improve your proposal of a function for each possibility. Like:

// Certain of normalized path
fn derive_pub_pure( v: Vec<NormalChildNumber>) -> Xpub {}

// Uncertain about whats inside
fn try_derive_pub( v: DerivationPath) -> Result<Xpub, Error>{}

And use it like

let path: Vec<NormalChildNumber> = DerivationPath::from(*).try_into()

derive_pub_pure(path)


///Would work here too
try_derive_pub(path)

///Since a vector of Normals are easily convertible for DerivationPath

Pedantically, it's not Vec::new but it's close enough. Returning a new vec requires an allocation (and the fact you mentioned new suggests your conversion isn't even optimal).

Improving is not hard, we can iterate DerivationPath and return a owned vector slice...

Is this a good tradeoff? Make Xpub::derive_pub but breaking the API?

Yes. Don't worry about DerivationPath. Maybe we need to make a second NonHardenedDerivationPath or something to handle the case where users want an entire path of non-hardened child numbers, but we can do that later in a non-breaking way.

Right now we have a derive_pub method which unambiguously returns errors that can be caught at compile-time because it is doing an internal type coversion that users could be doing themselves (and which may not even be necessary).

What is the point of using a non-null safe language to do this ?

I don't understand what relation this has to non-null. Care to explain?

we could improve your proposal of a function for each possibility. Like:

Sorry, now I see I messed up the types. Check the fixed version.

And use it like

In your example you're doing an unneeded heap allocation. Let's not do those.

Improving is not hard, we can iterate DerivationPath and return a owned vector slice...

I don't understand, returning an owned vec requires heap allocation.

In your example you're doing an unneeded heap allocation. Let's not do those.

The only way that i could think of have certainty of ChildNumbers types and not using a heap allocation(I think, not expert on memory allocation) is:

//Here we just get rid of some abstraction since `NormalChildNumber` and `HardenedChildNumber`
//implement the same traits.
pub enum ChildNumber {
    /// Non-hardened key
    NormalChildNumber(u32),
    /// Hardened key
    HardenedChildNumber(u32),
}

struct DerivationPath<ChildNumber> (Vec<ChildNumber>);

impl DerivationPath<ChildNumber>{
    fn to_normal( path: Self) -> Result<DerivationPath<NormalChildNumber>, Error> {
        path.iter().for_each(|cnum| {
            if cnum < 2147483647 {
                cnum = ChildNumber::NormalChildNumber(cnum);
            } else {
                return Error
        }
        });
        Self
    }
    fn to_hard( path: Self) -> Result<DerivationPath<HardenedChildNumber>, Error> {
        path.iter().for_each(|cnum| {
            if cnum > 2147483647 {
                cnum = ChildNumber::HardenedChildNumber(cnum);
            } else {
                return Error
        }
        });
        Self
    }
}
//No more uncertainty
fn derive_pub(p: DerivationPath<NormalChildNumber>) -> Xpub {}

Maybe the 2147483647 is wrong, but is just a poc.

@jaoleal that doesn't work and there's no need to do that. The code that's written right now already avoids allocation. We only need to refactor it a bit to make fallibility generic.

I've spoken offline with @jaoleal and he will be taking over this PR.
We need more developers to rust-bitcoin.
I'll gladly review the PR and, if necessary, take over future PRs.

I`ll reopen the PR tagging the issue when i think better of the @Kixunil Iterator's strategy