Skip to content

Conversation

@jochem-brouwer
Copy link
Member

@jochem-brouwer jochem-brouwer commented Feb 24, 2025

Part of #3712

This PR:

  • Adds a generic EthereumJSError class. This should not be used directly, instead more specific errors classes on top of this error should be created
  • Changes all new Error( to use the EthereumJSError with the default error code (this is deprecated and should be updated to the relevant error rather soon-ish)
  • Adds a lint rule which prevents new Error. It forces us to use the new EthereumJSError and does not allow to create new Error.

Note: I have excluded the rlp package from these rules, because it has no util dependency. I have also not included the wallet package.

@codecov
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 58.74214% with 328 lines in your changes missing coverage. Please review.

Project coverage is 75.88%. Comparing base (52c08f7) to head (988b6f3).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 76.80% <60.00%> (-0.07%) ⬇️
blockchain 85.43% <52.45%> (-0.27%) ⬇️
client 66.23% <37.81%> (-0.05%) ⬇️
common 90.76% <90.47%> (+0.02%) ⬆️
devp2p 76.33% <81.96%> (+0.05%) ⬆️
ethash 81.04% <33.33%> (ø)
evm 71.06% <52.94%> (+0.01%) ⬆️
genesis 99.84% <ø> (ø)
mpt 60.12% <32.65%> (+0.40%) ⬆️
rlp 69.70% <ø> (ø)
statemanager 70.48% <51.02%> (+0.01%) ⬆️
tx 81.20% <75.80%> (+0.23%) ⬆️
util 85.27% <71.17%> (-0.28%) ⬇️
vm 57.77% <43.18%> (-0.04%) ⬇️
wallet 83.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jochem-brouwer
Copy link
Member Author

Don't be to daunted by the diff, this only replaces new Error with EthereumJSErrorUnsetCode, not in the RLP and wallet package and also not in the externally imported packages in client/src/ext and devp2p/src/ext.

This now general error throwing paves the way to update the errors to more specific errors by for instance EVMRuntimeError extends EthereumJSError where we can, without being a breaking change add context to the error type 😄

Have marked the method as deprecated to indicate we want to replace this ASAP.

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

I haven't reviewed every line but this looks super clean. A few nits about terminology but otherwise great!

'no-restricted-syntax': [
'error',
{
selector: "ThrowStatement > NewExpression[callee.name='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 really like this. Can we do something similar for NoBuffer and get rid of our custom rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

ChatGPT gives this

{
  "rules": {
    "no-restricted-globals": [
      "error",
      {
        "name": "Buffer",
        "message": "Use Uint8Array or another safer alternative instead of Buffer."
      }
    ]
  }
}
{
  "rules": {
    "no-restricted-properties": [
      "error",
      {
        "object": "Buffer",
        "message": "Use Uint8Array or another safer alternative instead."
      }
    ]
  }
}

If we want this I'd like to address that in a "lint cleanup" PR or something like that (I also noticed that it seems that the specific eslint configs within the packages are sometimes ignored, for instance when calling lint:fix on monorepo it seems - could also be checked in that PR).

acolytec3
acolytec3 previously approved these changes Feb 24, 2025
Co-authored-by: acolytec3 <[email protected]>
acolytec3
acolytec3 previously approved these changes Feb 24, 2025
@jochem-brouwer jochem-brouwer merged commit bbc2068 into master Feb 28, 2025
39 of 41 checks passed
@acolytec3 acolytec3 deleted the ethjs-errors branch February 28, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants