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

Improvements to error handling #633

Open
sanket1729 opened this issue Jul 23, 2023 · 5 comments
Open

Improvements to error handling #633

sanket1729 opened this issue Jul 23, 2023 · 5 comments

Comments

@sanket1729
Copy link
Member

sanket1729 commented Jul 23, 2023

Here is one strategy that I think we should do to improve the ergonomics of errors. Broadly speaking, there are two ways users can interact with secp API.

  1. Users who just bubble up the error.
  2. Users who match on errors on variants and deal with them in some way.

Right now, our API is not the most convenient for users (2). For example, Signature::from_der returns only one error variant but the user is forced to match against all variants.

Proposal

Have each API return a separate error type that does not have any variants that are unreachable. Provide a top level common error type to unify all error types for users who just want to bubble up errors.

pub Error{
	InvalidSig(InvalidSigError),
	PubkeyParseError(PubKeyParseError),
}
impl Signature{
	pub fn from_der() -> Result<.., InvalidSigError>;
}
impl FromStr for PublicKey {
	pub fn from_str() -> Result<..., PubkeyParseError>;
}

impl From<InvalidSigError> for Error {}
impl From<PubkeyParseError> for Error {}
@apoelstra
Copy link
Member

concept ACK. We will probably wind up with multiple layers of nested errors and we should make sure that we have From implemented from each error type to all of its containing error types. Otherwise users need to type .map_err(Error1::from).map_err(Error2::from) etc.

@tcharding
Copy link
Member

I like the idea, any reason you posted this in rust-secp256k1 and not in rust-bitcoin? Perhaps this should be an org wide idea - or was the idea to test run it here in secp first to see how it goes? We have an error epic on rust-bitcoin crate also: rust-bitcoin/rust-bitcoin#820

@sanket1729
Copy link
Member Author

sanket1729 commented Jul 23, 2023

Otherwise users need to type .map_err(Error1::from).map_err(Error2::from) etc.

Yes, we will add impls from all individual types to top level error. Note that users can call .into() which does transitively do the correct thing.

I like the idea, any reason you posted this in rust-secp256k1 and not in rust-bitcoin? Perhaps this should be an org wide idea - or was the idea to test run it here in secp first to see how it goes?

No special reason. I wanted to try a PR in this repo first because there is small code surface and APIs can be easily separated.

@tcharding
Copy link
Member

Just read this, I already fixed up rust-bitcoin/rust-bitcoin#1950 to try out your idea.

@Kixunil
Copy link
Collaborator

Kixunil commented Oct 31, 2023

FYI this is exactly what I wanted for a while, so I'm happy with this.

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

No branches or pull requests

4 participants