Expose typed Python exceptions#114
Conversation
93aef5b to
15f8310
Compare
Codecov Report
@@ Coverage Diff @@
## main #114 +/- ##
==========================================
- Coverage 79.67% 77.68% -1.99%
==========================================
Files 23 24 +1
Lines 4796 4924 +128
==========================================
+ Hits 3821 3825 +4
- Misses 975 1099 +124
|
|
@KPrasch Let me know how that works for you |
ferveo-python/ferveo/__init__.pyi
Outdated
| class BincodeError(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class ArkSerializeError(Exception): |
There was a problem hiding this comment.
Seems like we are leaking implementation details for these...perhaps something more generic? SerializationError or ... ?
There was a problem hiding this comment.
This is a good point and on a similar note: How many typed errors do we need? Specifically, what are the cases where nucypher has to distinguish between Python exceptions in order to act on that information? Perhaps we only need a few o them, like InvalidAggregate and ThresholdEncryptionError, and everything we could put into exception.ValueError or something.
There was a problem hiding this comment.
Good question. Unsure at the moment but something we can look into. Happy to have a more in-depth discussion if you think it's worth exploring - defintely could use @KPrasch here.
That being said, perhaps we could also think about the best inheritance for these classes - eg. is it possible for some exceptions to subclass ValueError instead of Exception where appropriate eg. InvalidShareNumberParameter or InvalidPvssTranscript? Perhaps other logical sub-class groupings make sense.
Then it can potentially be as general or granular as we like in Python - we either check for the superclass or the more specific exception type as needed. The granular work is already done in ferveo in this PR.
| create_exception!(exceptions, ThresholdEncryptionError, PyException); | ||
| create_exception!(exceptions, InvalidShareNumberParameter, PyValueError); | ||
| create_exception!(exceptions, InvalidDkgStateToDeal, PyRuntimeError); | ||
| create_exception!(exceptions, InvalidDkgStateToAggregate, PyRuntimeError); | ||
| create_exception!(exceptions, InvalidDkgStateToVerify, PyRuntimeError); | ||
| create_exception!(exceptions, InvalidDkgStateToIngest, PyRuntimeError); | ||
| create_exception!(exceptions, DealerNotInValidatorSet, PyValueError); | ||
| create_exception!(exceptions, UnknownDealer, PyValueError); | ||
| create_exception!(exceptions, DuplicateDealer, PyValueError); | ||
| create_exception!(exceptions, InvalidPvssTranscript, PyValueError); | ||
| create_exception!(exceptions, InsufficientTranscriptsForAggregate, PyException); | ||
| create_exception!(exceptions, InvalidDkgPublicKey, PyValueError); | ||
| create_exception!(exceptions, InsufficientValidators, PyValueError); | ||
| create_exception!(exceptions, InvalidTranscriptAggregate, PyValueError); | ||
| create_exception!(exceptions, ValidatorsNotSorted, PyValueError); | ||
| create_exception!(exceptions, ValidatorPublicKeyMismatch, PyValueError); | ||
| create_exception!(exceptions, SerializationError, PyValueError); |
There was a problem hiding this comment.
I updated these exceptions to inherit from certain Python exceptions
b871ce1 to
86ef49e
Compare
| class InvalidDkgStateToDeal(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class InvalidDkgStateToAggregate(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class InvalidDkgStateToVerify(Exception): | ||
| pass | ||
|
|
||
|
|
||
| class InvalidDkgStateToIngest(Exception): | ||
| pass |
There was a problem hiding this comment.
Aside from the use of Python base exceptions eg. ValueError (thanks for adding that 👍 ), my other thought there was around other logical groupings. For example, this group of exceptions can all potentially subclass a common parent.
So for these highlighted exceptions something like the following for example:
| class InvalidDkgStateToDeal(Exception): | |
| pass | |
| class InvalidDkgStateToAggregate(Exception): | |
| pass | |
| class InvalidDkgStateToVerify(Exception): | |
| pass | |
| class InvalidDkgStateToIngest(Exception): | |
| pass | |
| class InvalidDkgStateException(Exception): | |
| pass | |
| class InvalidDkgStateToDeal(InvalidDkgStateException): | |
| pass | |
| class InvalidDkgStateToAggregate(InvalidDkgStateException): | |
| pass | |
| class InvalidDkgStateToVerify(InvalidDkgStateException): | |
| pass | |
| class InvalidDkgStateToIngest(InvalidDkgStateException): | |
| pass |
Perhaps something similar can be done for the Dealer exceptions below where the base class is
class DealerException(Exception):
pass
and the other Dealer Exceptions subclass it.
Perhaps there are other similar groupings that can be done. Would these additional Iogical groupings also make sense for what we are trying to achieve? wdyt?
There was a problem hiding this comment.
I think it makes sense, but I don't know if it makes those exceptions more actionable.
For example, InvalidDkgStateToDeal and InvalidDkgStateToAggregate would materialize in completely different situations, so catching InvalidDkgStateException and then trying to distinguish them based on that wouldn't make a lot of sense from my perspective. I would probably just try to catch InvalidDkgStateToAggregate directly.
But maybe there exists a grouping that makes perfect sense, I just don't see one yet. Maybe we could try using those exceptions as they are and see if we learn something from integrating them in nucypher. I can always add these later in another PR.
ferveo-py@0.1.13