Add error type for combine keys + test and doc #304
Conversation
|
This is a breaking change - let me check if we can push out a minor release before this. |
|
Yes. Will need to wait for #305 |
Is it because of deleting |
|
Yes, though also because we added the other variant that replaces it. |
| @@ -395,11 +395,15 @@ 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, or if the provided slice has no element in it. | |||
| pub fn combine_keys(keys: &[&PublicKey]) -> Result<PublicKey, Error> { | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Tibo-lg
dismissed stale reviews from sgeisler, apoelstra, and dr-orlovsky
via
674cc79
Tibo-lg
force-pushed
the
combine-keys-test-and-doc
branch
from
e2e1abf to
674cc79
Compare
|
utACK 674cc79 |
Trying to address the issues raised here:
Deleted the
TweakCheckFailedat the same time since it's not used but let me know if you'd prefer to have that in a separate commit/PR.@real-or-random I was not sure if you wanted to change something in the fuzz_dummy module and I'm not so familiar with what it does so let me know if it does need a change.