Skip to content

Fix unreachable code from calling derive_public_key #475

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

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

sanket1729
Copy link
Member

DescriptorPublicKey::derive_public_key API hit an unreachable line when
executing.

public_key.derive_public_key(&secp).unwrap_err();

This commit removes the API altogether and only exposes the method via
DefiniteDescriptorKey. Unfortunately, non-wildcard users would need
to pass a dummy value to .at_derivation_index to get this struct, but I
think this is okay.

@sanket1729
Copy link
Member Author

sanket1729 commented Oct 7, 2022

Perhaps, @LLFourn may have some comments here.

@apoelstra
Copy link
Member

I think we could add a derive_no_wildcard method or something which lets you do the conversion (and fails if there are actually any wildcards).

It could be as simple as just checking whether wildcards exist, and if not, calling at_derivation_index(0) internally, though maybe we want to do something more clever.

The point of #448 was that you wouldn't need to use dummy indices so I'd be sad to see that return..

DescriptorPublicKey::derive_public_key API hit an unreachable line when
executing.

public_key.derive_public_key(&secp).unwrap_err();

This commit removes the API altogether and only exposes the method via
`DefiniteDescriptorKey`. Unfortunately, non-wildcard users would need
to pass a dummy value to .at_derivation_index to get this struct, but I
think this is okay.
@sanket1729
Copy link
Member Author

Removed the last commit. Now, this forces the users to convert to DefiniteDescriptorKey.

@sanket1729 sanket1729 mentioned this pull request Oct 19, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK c14d1ce

@apoelstra apoelstra merged commit c9e6b53 into rust-bitcoin:master Oct 19, 2022
sanket1729 added a commit to sanket1729/elements-miniscript that referenced this pull request Oct 21, 2022
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