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.

Sign up for GitHub

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

Add error type for combine keys + test and doc #304

Merged
merged 1 commit into from
Sep 24, 2021
Merged

Add error type for combine keys + test and doc #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
use core::{fmt, str};

use super::{from_hex, Secp256k1};
use super::Error::{self, InvalidPublicKey, InvalidSecretKey};
use super::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey};
use Signing;
use Verification;
use constants;
Expand Down Expand Up @@ -395,12 +395,16 @@ impl PublicKey {

/// Adds the keys in the provided slice together, returning the sum. Returns
/// an error if the result would be the point at infinity, i.e. we are adding
/// a point to its own negation
/// a point to its own negation, if the provided slice has no element in it,
/// or if the number of element it contains is greater than i32::MAX.
pub fn combine_keys(keys: &[&PublicKey]) -> Result<PublicKey, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure whether it is a good practice to return general error type when in fact only a single error subtype may be caused by the function. I think replacing Result<_, Error> on Result<_, InvalidPublicKeySum> will do a better job (the caller, if needed, may convert the error into general Error crate type).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't speak for what is good practice or not, but looking at the rest of the code I feel like doing what you propose would be inconsistent with it, so if that's the way we want to go it might be better to make the change everywhere at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I also have seen that all functions in the lib used to return the single error type, which is a thing is not a good API, but this will certainly require distinct PR

Copy link
Member

Choose a reason for hiding this comment

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

If all of our functions returned distinct error types it would be an extremely unergonomic API, requring downstream users to wrap all of our individual types and implementing From on them to convert to their own error types.

use core::mem::transmute;
use core::i32::MAX;

debug_assert!(keys.len() < MAX as usize);
if keys.is_empty() || keys.len() > MAX as usize {
return Err(InvalidPublicKeySum);
}

unsafe {
let mut ret = ffi::PublicKey::new();
let ptrs : &[*const ffi::PublicKey] =
Expand All @@ -414,7 +418,7 @@ impl PublicKey {
{
Ok(PublicKey(ret))
} else {
Err(InvalidPublicKey)
Err(InvalidPublicKeySum)
}
}
}
Expand Down Expand Up @@ -893,6 +897,11 @@ mod test {
assert_eq!(sum1.unwrap(), exp_sum);
}

#[cfg_attr(not(fuzzing), test)]
fn pubkey_combine_keys_empty_slice() {
assert!(PublicKey::combine_keys(&[]).is_err());
}

#[test]
fn create_pubkey_combine() {
let s = Secp256k1::new();
Expand Down
6 changes: 3 additions & 3 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,10 +527,10 @@ pub enum Error {
InvalidRecoveryId,
/// Invalid tweak for add_*_assign or mul_*_assign
InvalidTweak,
/// `tweak_add_check` failed on an xonly public key
TweakCheckFailed,
/// Didn't pass enough memory to context creation with preallocated memory
NotEnoughMemory,
/// Bad set of public keys
InvalidPublicKeySum,
}

impl Error {
Expand All @@ -543,8 +543,8 @@ impl Error {
Error::InvalidSecretKey => "secp: malformed or out-of-range secret key",
Error::InvalidRecoveryId => "secp: bad recovery id",
Error::InvalidTweak => "secp: bad tweak",
Error::TweakCheckFailed => "secp: xonly_pubkey_tewak_add_check failed",
Error::NotEnoughMemory => "secp: not enough memory allocated",
Error::InvalidPublicKeySum => "secp: the sum of public keys was invalid or the input vector lengths was less than 1",
}
}
}
Expand Down