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

Use in-house Web3 exceptions for all of the codebase #3300

Merged

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Mar 25, 2024

What was wrong?

How was it fixed?

  • Instead of ValueError, raise Web3ValueError and extend from ValueError and Web3Exception
  • Instead of TypeError, raise Web3TypeError and extend from TypeError and Web3Exception
  • Instead of AssertionError, raise Web3AssertionError and extend from AssertionError and Web3Exception
  • Instead of AttributeError, raise Web3AttributeError and extend from AttributeError and Web3Exception
  • Add ENSValueError and ENSTypeError as well. Seems like that could only help with better control over exception handling.

Todo:

Cute Animal Picture

Screenshot 2024-03-26 at 18 44 18

@fselmo fselmo force-pushed the use-web3exception-for-all-library-exceptions branch 3 times, most recently from 2a20a19 to d3e45ea Compare March 26, 2024 02:57
@fselmo fselmo marked this pull request as ready for review March 26, 2024 14:52
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

No major concerns, lgtm!

Noticed two empty files, tests/core/pm-module/test_ens_integration.py and web3/tools/pytest_ethereum/deployer.py

tests/library_wide_test.py Outdated Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request Mar 27, 2024
@fselmo fselmo force-pushed the use-web3exception-for-all-library-exceptions branch from d3e45ea to 5011b41 Compare March 27, 2024 00:24
fselmo added a commit to fselmo/web3.py that referenced this pull request Mar 27, 2024
@fselmo fselmo force-pushed the use-web3exception-for-all-library-exceptions branch from 5011b41 to 6200970 Compare March 27, 2024 00:25
@fselmo
Copy link
Collaborator Author

fselmo commented Mar 27, 2024

My ultimate question here is whether or not it answers the call to have a more specific error thrown? I suppose we can take care of that part after as well. This helps with user exception handling, ideally catching all (or close to all) web3.py errors by catching Web3Exception or any combination of exceptions that inherit from it for fine-tuning. But if we want to throw an error that isn't so generic as any value error is bound to be I think we should probably end up tracking that in a separate ticket maybe?

@kclowes I'll wait for your review / input on this since you seem to be in this conversation quite a while longer than I.

cc: @LefterisJP for input via this comment.

fselmo added a commit to fselmo/web3.py that referenced this pull request Mar 27, 2024
@fselmo fselmo force-pushed the use-web3exception-for-all-library-exceptions branch from 6200970 to 686d430 Compare March 27, 2024 01:20
@LefterisJP
Copy link

This helps with user exception handling, ideally catching all (or close to all) web3.py errors by catching Web3Exception or any combination of exceptions that inherit from it for fine-tuning

Exactly this is what prompted my original comment. To be able to catch all the library errors easily and not have to add extra except clauses whenever a user does something that raises a ValueError, or TypeError which we did not expect.

But if we want to throw an error that isn't so generic as any value error is bound to be I think we should probably end up tracking that in a separate ticket maybe?

I am not sure I understand what you mean here. What kind of error would not be generic? Can you maybe give an example?

cc @yabirgb from the rotki team here just an fyi for the upcoming changes and for potential input.

@fselmo
Copy link
Collaborator Author

fselmo commented Apr 1, 2024

I am not sure I understand what you mean here. What kind of error would not be generic? Can you maybe give an example?

I think your answer covered it. I just mean that in some cases a more specific class name may make more sense. Something like Web3RPCError but I get the sense that the important part for you was fine tuning what is coming from the library and not how generic the exception scope was.

- `ValueError` -> `Web3ValueError(ValueError)`
- `TypeError` -> `Web3TypeError(TypeError)`
- `AssertionError` -> `Web3AssertionError(AssertionError)`
- `AttributeError` -> `Web3AttributeError(AttributeError)`
@fselmo fselmo force-pushed the use-web3exception-for-all-library-exceptions branch from 686d430 to a81227a Compare April 1, 2024 21:01
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

This looks great! I left one nit on a file name, but that was all I saw. It'll be great to get this in!

tests/core/library_wide_test.py Outdated Show resolved Hide resolved
@fselmo fselmo merged commit 9bf1500 into ethereum:main Apr 8, 2024
81 checks passed
fselmo added a commit that referenced this pull request Apr 8, 2024
@fselmo fselmo deleted the use-web3exception-for-all-library-exceptions branch April 8, 2024 21:57
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

Successfully merging this pull request may close these issues.

Use Web3Exception for all exceptions within the library
4 participants