Skip to content

Conversation

@chris-ricketts
Copy link
Contributor

This PR has a go at refactoring DescriptorKeyParseError into an enum, as suggested in the FIXME comment:

/// Descriptor Key parsing errors
// FIXME: replace with error enums
#[derive(Debug, PartialEq, Clone, Copy)]
pub struct DescriptorKeyParseError(&'static str);

I have separated the error variants by different inner parse error types and result types (e.g, public key, private key, etc) where I think it made sense to retain the inner error.

I left a variant that wraps a &'static str and its used for the various ad-hoc parsing errors instead of adding the boilerplate for variants only created once and without retained data.

The generic parse_xkey_deriv function now takes a parse function as its first parameter to allow specific key parsing errors:

https://github.com/chris-ricketts/rust-miniscript/blob/790b08fcf46f53ce6ee80355a7db8e5108ed860f/src/descriptor/key.rs#L935-L952

@apoelstra
Copy link
Member

instead of adding the boilerplate for variants only created once and without retained data.

Can you add the boilerplate? I know it's annoying to write but it makes things much more maintainable since it forces a visible API change whenever we add or change variants.

@chris-ricketts
Copy link
Contributor Author

chris-ricketts commented Mar 31, 2025

Can you add the boilerplate? I know it's annoying to write but it makes things much more maintainable since it forces a visible API change whenever we add or change variants.

@apoelstra no problem, please see 895da06

Before the PR I had them all as 'top-level' error variants and it was a bit unwieldily, I think the above is a better middle ground.

@apoelstra
Copy link
Member

In 895da06:

You need to re-export the new error type MalformedKeyDataKind from src/descriptor/mod.rs. Unfortunately I think the compiler will then immediately whine that you need a doccomment on every single variant. I'd be ok with just sticking #[allow(missing_docs)] on the enum since it's a C-like enum where every variant is named what it is.

@apoelstra
Copy link
Member

In 895da06 and 790b08f:

Can you run through all your error messages and change them to start with a lowercase letter?

@apoelstra
Copy link
Member

Other than those two comments 895da06 looks great! Thanks for grinding through this.

@chris-ricketts
Copy link
Contributor Author

Thanks for the review @apoelstra

  • Re-export MalformedKeyDataKind: bf058c9

  • Allow missing docs for MalformedKeyDataKind: a5c1785

  • Start all error messages will lower case letter: 0e98dd2

Let me know if you would prefer all the commits in this PR to be squashed into one.

@apoelstra
Copy link
Member

Yes please -- can you just squash everything?

refactor: add MalformedKeyDataKind enum to DescriptorKeyParseError definition

fix: re-export MalformedKeyDataKind from descriptor module root

fix: allow missing docs on C-style MalformedKeyDataKind enum

fix: start error strings with lowercase in descriptors::key module
@chris-ricketts
Copy link
Contributor Author

Sorted and noted for the future

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 497c5a3; successfully ran local tests; thanks!

@apoelstra apoelstra merged commit 0010f1c into rust-bitcoin:master Apr 2, 2025
31 checks passed
heap-coder added a commit to heap-coder/rust-miniscript that referenced this pull request Sep 27, 2025
…ey::DescriptorKeyParseError an enum

497c5a31974e9a19f8b3b5286edb323281957147 refactor: make descriptors::key::DescriptorKeyParseError an enum (Chris Ricketts)

Pull request description:

  This PR has a go at refactoring `DescriptorKeyParseError` into an enum, as suggested in the `FIXME` comment:

  https://github.com/rust-bitcoin/rust-miniscript/blob/d0da327417cb177884c5d77edffa0891ba9b6969/src/descriptor/key.rs#L327-L330

  I have separated the error variants by different inner parse error types and result types (e.g, public key, private key, etc) where I think it made sense to retain the inner error.

  I left a variant that wraps a `&'static str` and its used for the various ad-hoc parsing errors instead of adding the boilerplate for variants only created once and without retained data.

  The generic `parse_xkey_deriv` function now takes a parse function as its first parameter to allow specific key parsing errors:

  https://github.com/chris-ricketts/rust-miniscript/blob/790b08fcf46f53ce6ee80355a7db8e5108ed860f/src/descriptor/key.rs#L935-L952

ACKs for top commit:
  apoelstra:
    ACK 497c5a31974e9a19f8b3b5286edb323281957147; successfully ran local tests; thanks!

Tree-SHA512: c9fb26b6484c90124947912070e9cca4e93b4d3bdb674a07926fb1a1adc6fe2c272a1d38925b35fc4842a527651a45b00dc6381d057bb02f36006dbda653809b
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