Return error from at_derivation_index when descriptor has no wildcard

[febyeji]

Closes #829.

• Previously, calling at_derivation_index() on a descriptor without wildcards would silently ignore the index and return the descriptor unchanged. This was error-prone because callers would expect different indices to produce different addresses.
• Callers that need to convert a non-wildcard descriptor to a definite one should use DefiniteDescriptorKey::new() on individual keys, or check has_wildcard() before calling at_derivation_index().
• Add edge case tests for at_derivation_index wildcard check

[trevarj]

But what should the workflow be if you just want to convert a DescriptorPublicKey descriptor to a DefiniteDescriptorKey one, not touching wildcards if they don't exist?

I'm ok with this as long as we address what Andrew said in the linked issue. I also would like a way to convert from Descriptor<DescriptorPublicKey> into Descriptor<DefiniteDescriptorKey> since I am currently using at_derivation_index as an agnostic translation.

Maybe it can be solved with just adding a helper function into_definite() or something.

[febyeji]

Thanks for the review! Extracted a public into_definite() method which resolves the duplication and also addresses Andrew's question in the issue. find_derivation_index_for_spk now just calls self.into_definite()?.derived_descriptor(secp).

[apoelstra]

• Previously, calling at_derivation_index() on a descriptor without wildcards would silently ignore the index and return the descriptor unchanged. This was error-prone because callers would expect different indices to produce different addresses.

I agree this is a bad and footgunny API.

• Callers that need to convert a non-wildcard descriptor to a definite one should use DefiniteDescriptorKey::new() on individual keys, or check has_wildcard() before calling at_derivation_index().

But nither of these are reasonable API choices. Converting every key requires the user to run translate_pk over the whole descriptor (and maybe implement their own Translator type), which is not very discoverable. And checking has_wildcard on a DescriptorPublicKey doesn't help a user who needs a `DefiniteDescriptorKey'.

[apoelstra]

How about we also add a TryFrom<Descriptor<DescriptorPublicKey>> for Descriptor<DefiniteDescriptorKey>, which attempts the "dumb conversion" and only works on wildcard-less descriptors?

And in the docs for at_derivation_index we should point users at this method.

Secondly, because existing users are calling at_derivation_index(0) or something to do this conversion (I know I am), and this will break their code, I think we need a better transition strategy. I would suggest deprecating the existing method without changing its behavior, and adding a new at_index method with the new behavior. Then we could even backport this.

After a couple versions we can rename the method back because I do prefer the old name :/ but I think that the "silent breakage" of returning an error for previously-legitimate use is too much of a problem.

[trevarj]

How about we also add a TryFrom<Descriptor<DescriptorPublicKey>> for Descriptor<DefiniteDescriptorKey>, which attempts the "dumb conversion" and only works on wildcard-less descriptors?

I like this. Then the into_definite() can call that (or just get rid of it).

I would suggest deprecating the existing method without changing its behavior, and adding a new at_index method with the new behavior. Then we could even backport this.

After a couple versions we can rename the method back because I do prefer the old name :/

Possibly even derive_at_index()

[apoelstra]

Possibly even derive_at_index()

Oh, nice, I like this name even better than at_derivation_index.

[febyeji]

Possibly even derive_at_index()

Nice, I like this name, too.

[apoelstra]

In 74075e9:

This enum Definitor is cute but it should really be split into two separate Translator instances because the two variants of the enum do completely different things. One should be local to the at_derivation_index and one should be local to the into_definite method.

Then derive_at_index can call at_derivation_index rather than directly doing the translation. (And at_derivation_index will not change its code at all.)

[trevarj]

This enum Definitor is cute but it should really be split into two separate Translator instances

That was my fault, sorry.

[apoelstra]

Oh, lol, I missed that. Sorry to jerk you around @febyeji. But I do think it's better design to have them split up.

[febyeji]

No worries, both suggestions made sense in context! The split looks cleaner so I just applied it.

[apoelstra]

Nice, looks good! A couple thoughts:

• Might be nice if the NoWildcard error variant had the original descriptor converted to a definite descriptior, so the user could unwrap the error rather than if-gating and calling a different method. But I'd need to think about how exactly we want the error structs to look. Probably best to leave this for a followup.
• Would be nice to have an as_definite alongside into_definite. Would need to use #[repr(transparent)] and some unsafe code. Probably I should do this in a followup.

[apoelstra]

ACK 1062144; successfully ran local tests

[trevarj]

• Might be nice if the NoWildcard error variant had the original descriptor converted to a definite descriptior, so the user could unwrap the error rather than if-gating and calling a different method.

This reminds me of what I was originally expecting, in terms of ergonomics.

It would be nice to be able to do something like:

(don't implement this)

enum DerivationResult<'a, T, E> {
    Ok(T),
    Err { error: E, original: &'a T },
}

desc.derive_at_index(0).into_result()?; // returns `DerivationResult<T, E>` -> `Result<T, E>` then errors if no wildcards
desc.derive_at_index(0)?; // same as above but nicer
// returns `DerivationResult<T, E>`, then ignores no wildcard and returns original, not sure about `fallback()` naming.
desc.derive_at_index(0).fallback();

[apoelstra]

Promoted to #918. We'll definitely want to iterate a bit if we try to do this.