Add Web3Exception to all exception classes #1478

jpic · 2019-10-22T13:49:31Z

Exception mixin inherited by all exceptions of web3.py

This allows:

try:
    some_call()
except Web3Exception as e:
    # deal with web3 exception
except:
    # deal with other exceptions

What was wrong?

I currently have this code:

        try:    
            result = something_that_calls_web3()
        except (    
            web3.exceptions.BadFunctionCallOutput    
            web3.exceptions.BlockNotFound    
            web3.exceptions.BlockNumberOutofRange    
            web3.exceptions.CannotHandleRequest    
            web3.exceptions.FallbackNotFound    
            web3.exceptions.InfuraKeyNotFound    
            web3.exceptions.InsufficientData    
            web3.exceptions.InvalidAddress    
            web3.exceptions.InvalidEventABI    
            web3.exceptions.LogTopicError    
            web3.exceptions.ManifestValidationError    
            web3.exceptions.MismatchedABI    
            web3.exceptions.NameNotFound    
            web3.exceptions.NoABIEventsFound    
            web3.exceptions.NoABIFound    
            web3.exceptions.NoABIFunctionsFound    
            web3.exceptions.PMError    
            web3.exceptions.StaleBlockchain    
            web3.exceptions.TimeExhausted    
            web3.exceptions.TransactionNotFound    
            web3.exceptions.ValidationError    
        ) as e:    
            deal_with_web3_exception()

This has readability problem, and also not forward-compatible with new exceptions, and also will break if an exception is removed from web3 package.

How was it fixed?

With this patch, I could replace the above with:

        try:    
            result = something_that_calls_web3()
        except web3.exceptions.Web3Exception as e:    
            deal_with_web3_exception()

This fixes readability and forward-compatibility.

Cute Animal Picture

pipermerriam · 2019-10-22T15:06:08Z

web3/exceptions.py

+    """
+
+
+class BadFunctionCallOutput(Exception, Web3Exception):


I'd advocate for Web3Exception to inherit from Exception and to keep the inheritance chain for all of these child exception classes to only have a single parent of Web3Exception.

Thanks for review and permission to break BC, notes added in new v6 migration document.

pipermerriam · 2019-10-22T15:08:06Z

web3/exceptions.py

    """
    Raised when an ABI is present, but doesn't contain any functions.
    """
    pass


-class NoABIFound(AttributeError):
+class NoABIFound(AttributeError, Web3Exception):


@kclowes can you make a note in your breaking v6 changes tracking issue to remove AttributeError from these exception classes bases.

pipermerriam · 2019-10-22T15:09:05Z

web3/exceptions.py

    """
    Raised when there is no Infura Project Id set.
    """
    pass


-class LogTopicError(ValueError):
+class LogTopicError(ValueError, Web3Exception):


@kclowes same here.

jpic · 2019-10-25T11:21:09Z

Added v6 migration notes

kclowes · 2022-10-27T21:29:35Z

Thank you for this @jpic! We're finally about ready to pull it in. I just fixed a bunch of merge conflicts that have accumulated, and need to make some updates in the v6 migration guide, but then it looks good to go!

web3/exceptions.py

Exception mixin inherited by all exceptions of each module. This allows: try: some_call() except Web3Exception as e: # deal with web3 exception except: # deal with other exceptions

….PyEthPMException

pacrob

lgtm!

fselmo

I'm happy to see this making it in. I just need a bit of clarification on some things.

eth_utils has a ValidationError that we use in a few places within web3.py while in others we use web3.exceptions.ValidationError. It looks like at some point there was an idea to use the eth_utils.exceptions.ValidationError across all of the python libraries. Indeed this isn't used anywhere within the actual eth-utils library. Do we want to keep this design? Are there good arguments for or against it? This seems like a good time to revisit this for web3.py and it wouldn't be so hard of a change to make. If we think a better design is for each project to have their own ValidationError class, do we want to go toward namespace validation errors, as we did with EthPMValidationError?
nit: I slightly prefer the less lengthy EthPMException over PyEthPMException. The Py seems redundant and the module name is ethpm. But I can take or leave it. It does make EthPMValidationError seem like it needs a Py if we keep it... but I prefer both without it.

edit: link to py-evm PR for this change. Also, note, this can (and probably should?) be a separate PR... just noticing that it is related and not on the list for v6 breaking change considerations.

kclowes · 2023-01-12T22:14:36Z

eth_utils has a ValidationError that we use in a few places within web3.py while in others we use web3.exceptions.ValidationError. It looks like at some point there was an idea to use the eth_utils.exceptions.ValidationError Add ValidationError eth-utils#114. Indeed this isn't used anywhere within the actual eth-utils library. Do we want to keep this design? Are there good arguments for or against it? This seems like a good time to revisit this for web3.py and it wouldn't be so hard of a change to make. If we think a better design is for each project to have their own ValidationError class, do we want to go toward namespace validation errors, as we did with EthPMValidationError?

I lean toward each project having their own ValidationError, because the whole point of this PR is to be able to catch the top-level Web3Exception. If the ValidationError is from eth-utils, then you'd have to catch Web3Exception and eth_utils.exceptions.ValidationError. I guess another route we could go is to have the ValidationError from web3 inherit from the eth-utils exception, but I'm not sure what would be gained from that. I guess I should rename to Web3ValidationError then? Also curious if you have a different opinion @fselmo. It looks like in the py-evm PR they hit an issue where they were catching the wrong ValidationError but that would be remedied if the ValidationError class was renamed to Web3ValidationError. And if we're going to prefix the ValidationError with Web3 I could see an argument for prefixing all Exceptions with Web3 but that seems redundant.

nit: I slightly prefer the less lengthy EthPMException over PyEthPMException. The Py seems redundant and the module name is ethpm. But I can take or leave it. It does make EthPMValidationError seem like it needs a Py if we keep it... but I prefer both without it.

Yeah, agree the Py seems redundant. Will update!

fselmo · 2023-01-13T17:59:49Z

I personally don't see a scenario where I'd want to catch only web3 validation errors instead of catching them across all web3 libraries that web3.py interacts with. web3.py hooks up its codec from eth-abi, makes calls through eth-tester to py-evm, uses eth-account for the account logic... etc... I think that catching a single ValidationError between all these still makes sense.

However... if we want the most fine tuning, I don't see why we can't make Web3ValidationError, similarly to EthPMValidationError (which you mentioned as well). And if we go this route, I don't think we need to prefix all web3.py exception classes with "Web3"... only the ones that are shared across all the libraries (so far only the ValidationError).

Without a general ValidationError, catching all validation errors across stack calls would look something like:

from web3.exceptions import Web3ValidationError
from eth_tester.exceptions import EthTesterValidationError
from eth.exceptions import PyEVMValidationError
...

ALL_WEB3_VALIDATION_ERRORS = [
    Web3ValidationError, EthTesterValidationError, PyEvmValidationError, ...,
]

try:
    ...
except Exception as e:
    if isinstance(e, ALL_WEB3_VALIDATION_ERRORS):
        ...

which looks a bit similar to the original issue filed for web3 exceptions in general.

With a general ValidationError within eth-utils:

from eth_utils.exceptions import ValidationError

try:
    ...
except ValidationError:
        ...

For even more fine tuning, if we still opt for name-spaced validation errors which inherit from the general eth-utils class:

from eth_utils.exceptions import ValidationError

class Web3ValidationError(Web3Exception, ValidationError):
    pass
    
 
try:
    ...
    
# catch all web3 exceptions, including Web3ValidationError
except Web3Exception:
    ...
    
# OR catch only web3 validation exceptions
except Web3ValidationError:
    ...
    
# OR catch any validation errors that may happen on calls to other web3 libraries
except ValidationError:
    ...

My vote is either using ValidationError across all libraries (the old intended way discussed in 2018) or inheriting each library's validation error from that eth-utils class. Thoughts?

kclowes · 2023-01-13T18:17:09Z

I personally don't see a scenario where I'd want to catch only web3 validation errors instead of catching them across all web3 libraries that web3.py interacts with

Yeah, that's a good point. It feels a little like clunky to me to expect a user to import from eth-utils exceptions through the web3 library. Let's go with the inheritance, since that offers the most customization and I'll update ValidationError -> Web3ValidationError here.

fselmo · 2023-01-13T18:33:49Z

Makes sense. We would only need to import ValidationError into the web3.exceptions file in order to pass it through but that's a pretty roundabout way of doing it and not immediately obvious what's happening to someone reading the code. I dig the inheritance approach 👍🏼

fselmo

lgtm 👍🏼

We can definitely benefit from some brief documentation on this hierarchy, perhaps in a separate PR?

…tils.exceptions.ValidationError

kclowes · 2023-01-13T20:05:32Z

Good thought! I'll add it to the migration guide PR that was created off of this branch. As soon as this gets in, I'll PR that.

jpic force-pushed the web3exception branch from a3666cf to 1f585cb Compare October 22, 2019 13:58

jpic changed the title ~~Add Web3Exception (OOP/Exception handling best practice)~~ Add Web3Exception to all exception classes Oct 22, 2019

pipermerriam reviewed Oct 22, 2019

View reviewed changes

jpic force-pushed the web3exception branch 2 times, most recently from 6fa55e9 to 8a396f7 Compare October 22, 2019 16:27

kclowes mentioned this pull request Oct 24, 2019

v6 Breaking Changes #1416

Closed

22 tasks

kclowes mentioned this pull request Feb 17, 2021

Unhandled ValueError when calling contract functions #1864

Closed

fselmo force-pushed the master branch from 5ef554e to 1417eb8 Compare July 13, 2021 17:04

kclowes force-pushed the web3exception branch from 8a396f7 to 0614a33 Compare October 27, 2022 21:29

kclowes force-pushed the web3exception branch from 0614a33 to 41a5919 Compare November 2, 2022 20:04

kclowes force-pushed the web3exception branch 4 times, most recently from 4dd52c7 to ff1158a Compare January 11, 2023 18:04

kclowes requested review from fselmo and pacrob January 11, 2023 18:06

pacrob reviewed Jan 11, 2023

View reviewed changes

web3/exceptions.py Show resolved Hide resolved

jpic and others added 3 commits January 11, 2023 12:04

Add base Web3Exception, ENSError, and PyEthPMError

80b7701

Exception mixin inherited by all exceptions of each module. This allows: try: some_call() except Web3Exception as e: # deal with web3 exception except: # deal with other exceptions

Move PyEthPMError -> PyEthPMException, and ENSError -> ENSException

f2ecb00

Remove redundant web3.exceptions.PMError in favor of ethpm.exceptions…

b7ef938

….PyEthPMException

kclowes force-pushed the web3exception branch from ff1158a to b7ef938 Compare January 11, 2023 19:05

pacrob approved these changes Jan 11, 2023

View reviewed changes

fselmo reviewed Jan 12, 2023

View reviewed changes

Move PyEthPMException -> EthPMException

3e77455

kclowes force-pushed the web3exception branch from 2709a33 to 3e77455 Compare January 13, 2023 17:54

kclowes force-pushed the web3exception branch 3 times, most recently from ae7f5fe to 6ed320b Compare January 13, 2023 19:48

fselmo approved these changes Jan 13, 2023

View reviewed changes

Add specific ValidationError for each module that inherits from eth_u…

e348d68

…tils.exceptions.ValidationError

kclowes force-pushed the web3exception branch from 6ed320b to e348d68 Compare January 13, 2023 20:04

kclowes merged commit b365dc3 into ethereum:master Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add Web3Exception to all exception classes #1478

Add Web3Exception to all exception classes #1478

Uh oh!

jpic commented Oct 22, 2019 •

edited

Loading

Uh oh!

pipermerriam Oct 22, 2019

Uh oh!

jpic Oct 22, 2019 •

edited

Loading

Uh oh!

pipermerriam Oct 22, 2019

Uh oh!

pipermerriam Oct 22, 2019

Uh oh!

jpic commented Oct 25, 2019

Uh oh!

kclowes commented Oct 27, 2022

Uh oh!

Uh oh!

pacrob left a comment

Uh oh!

fselmo left a comment •

edited

Loading

Uh oh!

kclowes commented Jan 12, 2023

Uh oh!

fselmo commented Jan 13, 2023

Uh oh!

kclowes commented Jan 13, 2023

Uh oh!

fselmo commented Jan 13, 2023

Uh oh!

fselmo left a comment

Uh oh!

kclowes commented Jan 13, 2023

Uh oh!

Uh oh!

Add Web3Exception to all exception classes #1478

Add Web3Exception to all exception classes #1478

Uh oh!

Conversation

jpic commented Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was wrong?

How was it fixed?

Cute Animal Picture

Uh oh!

pipermerriam Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

jpic Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipermerriam Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

pipermerriam Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

jpic commented Oct 25, 2019

Uh oh!

kclowes commented Oct 27, 2022

Uh oh!

Uh oh!

pacrob left a comment

Choose a reason for hiding this comment

Uh oh!

fselmo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kclowes commented Jan 12, 2023

Uh oh!

fselmo commented Jan 13, 2023

Uh oh!

kclowes commented Jan 13, 2023

Uh oh!

fselmo commented Jan 13, 2023

Uh oh!

fselmo left a comment

Choose a reason for hiding this comment

Uh oh!

kclowes commented Jan 13, 2023

Uh oh!

Uh oh!

jpic commented Oct 22, 2019 •

edited

Loading

jpic Oct 22, 2019 •

edited

Loading

fselmo left a comment •

edited

Loading