Monorepo: Lodestar-inspired Error Management PoC

[holgerd77]

Continues/replaces #1469

This is a first PoC of a Lodestar-inspired monorepo error management based on the discussion here.

The basic Lodestar error class is defined here, I used the Eth1Error from here and applied e.g. here as some orientation on how things could be done.

I adopted and simplified various things and I am relatively pleased with the result since I have the impression that this should sufficiently satisfy our needs. 🙂

I adopted the following things:

• code is not part of the - on the Lodestar side - called metadata any more but a separate constructor argument, this simplified some typings and the UI (from my PoV) gets a bit nicer
• message is also a dedicated separate constructor argument being mandatory. I didn't want the code be used for the original error object message property but rather have this separated
• I've renamed metadata to errorContext which I find a bit less "technocratic" 😋 and fitting

This is how an error is thrown:

const e = new HeaderValidationError(
  'transactionsTrie must be 32 bytes',
  HeaderValidationErrorCode.WRONG_TX_TRIE_LENGTH,
  {
    block: this.errorStr(),
    received: `${transactionsTrie.toString('hex')} (${transactionsTrie.length} bytes)`,
  }
)
throw e

(can of course also be thrown directly)

An error thrown with this now looks like this:

Uncaught Error: transactionsTrie must be 32 bytes
    at BlockHeader._validateHeaderFields (/ethereumjs-monorepo-develop/packages/block/src/header.ts:359:13)
    at new BlockHeader (/ethereumjs-monorepo-develop/packages/block/src/header.ts:303:10)
    at/ethereumjs-monorepo-develop/packages/block/<repl>.ts:2:9
    at Script.runInThisContext (node:vm:129:12)
    at runInContext (/node_modules/ts-node/src/repl.ts:603:19)
    at Object.execCommand (/node_modules/ts-node/src/repl.ts:569:28)
    at /node_modules/ts-node/src/repl.ts:591:47
    at Array.reduce (<anonymous>)
    at appendCompileAndEvalInput (/node_modules/ts-node/src/repl.ts:591:23)
    at evalCodeInternal (/node_modules/ts-node/src/repl.ts:212:12) {
  code: 'WRONG_TX_TRIE_LENGTH',
  errorContext: {
    block: 'block header number=0 hash=0x7939e4f199f7ec79ce8cff286a0b4b0af01bbd782ff0e10305915d0d2e099de0 hf=london baseFeePerGas=7',
    lengthReceived: 32
  }

All this leads to the following properties (which we didn't have before):

• The more generic Error (here HeaderValidationError) can be checked with e instanceof HeaderValidationError (tested)
• The specific error can be checked with e.code === HeaderValidationErrorCode.WRONG_TX_TRIE_LENGTH (tested) independently from an eventually changing error message
• An arbitrary and typed error context can be provided via the errorContext property

At the same time I have the impression that this remains sufficiently simple.

Let me know what you think!

@gabrocheleau: it would be good if you can have some deepened look here if this respects and covers all eventual particularities which you might have already covered in some adopted form in #1469.

[codecov]

Codecov Report

Merging #1910 (73a487a) into develop (66f8779) will increase coverage by 1.02%.
The diff coverage is 63.63%.

Flag	Coverage Δ
block	86.42% <58.33%> (+1.13%)	⬆️
blockchain	83.13% <ø> (?)
client	77.75% <ø> (+<0.01%)	⬆️
common	95.52% <ø> (+0.59%)	⬆️
devp2p	82.96% <ø> (+0.13%)	⬆️
ethash	90.81% <ø> (?)
statemanager	84.24% <ø> (ø)
trie	80.14% <ø> (?)
tx	92.09% <ø> (?)
util	86.62% <70.00%> (?)
vm	81.08% <ø> (ø)

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

[g11tech]

LGTM! 🚀

[holgerd77]

I now had a closer look in the Block library on the feasibility/practicability of a broader application of this and realized that it would be good if we have a layer of generic error type classes in between since the goal should be, that things are sufficiently differentiated but that we do have as minimal additional work as possible along defining new types, error classes and the like.

I do have a version that looks roughly as what I would like to have, it is not completely working though since my TypeScript knowledge is too limited to get this up and running. 😜

@gabrocheleau: could you please pick this up (respectively do the next round)? I guess you will know a lot better what you are doing than me.

So, the thing is: I have now defined two first generic error type classes in Util, ValidationError (if the format validation goes wrong, e.g. 'transactionsTrie must be 32 bytes') and UsageError (if the API is used in a wrong way, e.g. 'The prevRandao parameter can only be accessed when EIP-4399 is activated').

1. What I would like to have is that the type of the specific error class (in this case HeaderValidationError respectively HeaderValidationErrorType) is inheriting and extending the generic error type from Util (ValidationErrorType) so that in many cases it won't be necessary to add additional type definitions but one can use re-use the more generic ones. In a first step it would be great if you can realize that and "make this working", lol (tried so much but gave up). So the test here would be that you could delete block: this.errorStr(), from the first test error usage in BlockHeader and things would still work since the type is taken from ValidationErrorType.

This is just to make this work in general.

2. In a second step I think that the HeaderValidationErrorType is generally still so generic that we likely don't want to go down that level (so to clarify: particularly in this (and likely a lot of other) specific case(s), not in general) - and - as often as possible - use the types from the more generic error types in Util. It would still be good if we keep the code we realized in 1. just as an example that this would still be working (or alternatively we can directly apply on a more fitting context, mainly for having this as reference code for this subclass error case).

It would then be good if you extend the more generic ValidationErrorType in a way that it can take either { received: string } or { objectContext: string, received: string } (there might be a better name here, so this is the generic version of the { block: string, received: string } type).

3. I guess then we can have another look at the errors in BlockHeader, maybe also in other files. I think for these kind of errors in BlockHeader it might actually generally be sufficient (and if it is sufficient this would be highly prefered to keep things simple) to use the generic ValidationError and UsageError classes, together with the specific error codes (like WRONG_TX_TRIE_LENGTH) it very much seems to me that this differentiates enough.

I would very much assume (for now) that we likely still want to keep this two-level-structure and keep the possibility with also defining the application/domain specific error classes and types like HeaderValidationError and HeaderValidationErrorType.

4. Then we can expand the concept from above along the whole Block package and see if things "hold" or if we still need to adjust. I think we'll see this along the way. You can also continue or at least start with this, I can also do or we share, we'll see, but generall would be cool if we can work on this together. 🙂

Whew, long post. All the thoughts I had this morning. 😋

Side quest: ah, and what I would also like to have is that we inject the package name and version number (e.g. library version: @ethereumjs/block v7.1.4, or in two entries, just to get the idea) in a as generic as possible way (so optimally: only once in the code in Util without the need of an extra argument to be passed in). This would just safe us from a lot of "what version are you on?" check backs or confusion about version numbers along community support. 😋

Ok, so far. 😁

[holgerd77]

(have done a lot of post-submission edits, please read on site)

[gabrocheleau]

I now had a closer look in the Block library on the feasibility/practicability of a broader application of this and realized that it would be good if we have a layer of generic error type classes in between since the goal should be, that things are sufficiently differentiated but that we do have as minimal additional work as possible along defining new types, error classes and the like.

I do have a version that looks roughly as what I would like to have, it is not completely working though since my TypeScript knowledge is too limited to get this up and running. stuck_out_tongue_winking_eye

@gabrocheleau: could you please pick this up (respectively do the next round)? I guess you will know a lot better what you are doing than me.

So, the thing is: I have now defined two first generic error type classes in Util, ValidationError (if the format validation goes wrong, e.g. 'transactionsTrie must be 32 bytes') and UsageError (if the API is used in a wrong way, e.g. 'The prevRandao parameter can only be accessed when EIP-4399 is activated').

1. What I would like to have is that the type of the specific error class (in this case HeaderValidationError respectively HeaderValidationErrorType) is inheriting and extending the generic error type from Util (ValidationErrorType) so that in many cases it won't be necessary to add additional type definitions but one can use re-use the more generic ones. In a first step it would be great if you can realize that and "make this working", lol (tried so much but gave up). So the test here would be that you could delete block: this.errorStr(), from the first test error usage in BlockHeader and things would still work since the type is taken from ValidationErrorType.

This is just to make this work in general.

2. In a second step I think that the HeaderValidationErrorType is generally still so generic that we likely don't want to go down that level (so to clarify: particularly in this (and likely a lot of other) specific case(s), not in general) - and - as often as possible - use the types from the more generic error types in Util. It would still be good if we keep the code we realized in 1. just as an example that this would still be working (or alternatively we can directly apply on a more fitting context, mainly for having this as reference code for this subclass error case).

It would then be good if you extend the more generic ValidationErrorType in a way that it can take either { received: string } or { objectContext: string, received: string } (there might be a better name here, so this is the generic version of the { block: string, received: string } type).

3. I guess then we can have another look at the errors in BlockHeader, maybe also in other files. I think for these kind of errors in BlockHeader it might actually generally be sufficient (and if it is sufficient this would be highly prefered to keep things simple) to use the generic ValidationError and UsageError classes, together with the specific error codes (like WRONG_TX_TRIE_LENGTH) it very much seems to me that this differentiates enough.

I would very much assume (for now) that we likely still want to keep this two-level-structure and keep the possibility with also defining the application/domain specific error classes and types like HeaderValidationError and HeaderValidationErrorType.

4. Then we can expand the concept from above along the whole Block package and see if things "hold" or if we still need to adjust. I think we'll see this along the way. You can also continue or at least start with this, I can also do or we share, we'll see, but generall would be cool if we can work on this together. slightly_smiling_face

Whew, long post. All the thoughts I had this morning. yum

Side quest: ah, and what I would also like to have is that we inject the package name and version number (e.g. library version: @ethereumjs/block v7.1.4, or in two entries, just to get the idea) in a as generic as possible way (so optimally: only once in the code in Util without the need of an extra argument to be passed in). This would just safe us from a lot of "what version are you on?" check backs or confusion about version numbers along community support. yum

Ok, so far. grin

Hey holger!

I think I've addressed point 1, but am not sure I understand what you mean by this:

So the test here would be that you could delete block: this.errorStr(), from the first test error usage in BlockHeader

The first part of the text led me to think that the goal was for the block package error types to inherit and extend the shape of their parent type (e.g. so that we don't have to add the received prop on every instance). I don't see how this could lead us to delete block: this.errorStr(), on line 363 of header.ts, since we've actually specifically defined that HeaderValidationError needs a block prop? Perhaps I am misunderstanding?

Also, general thought on the current approach. Do you think there would be room for simplification (mostly from the point of view of usage)? One thing I liked about the previous approach is that the type (and therefore shape) of the error was automatically inferred by the error code, so I'm wondering if we could perhaps do without the more specific HeaderValidationError class, just use the more general ValidationError class and let the ValidationErrorCode.WRONG_TX_TRIE_LENGTH code automatically adjust the shape of the errorContext prop. What do you think?

[holgerd77]

Hi there, thanks for having a look here so quickly! 😄

The first part of the text led me to think that the goal was for the block package error types to inherit and extend the shape of their parent type (e.g. so that we don't have to add the received prop on every instance). I don't see how this could lead us to delete block: this.errorStr(), on line 363 of header.ts, since we've actually specifically defined that HeaderValidationError needs a block prop? Perhaps I am misunderstanding?

Ah no, that was explicitly not the goal ("e.g. so that we don't have to add the received prop on every instance"). If you look at the Lodestar errors like here you can see that there are various type "flavours" unionized so that one can choose one fitting from this list. I think I brought you on the wrong track with pointing to "exending" - sorry - my misunderstanding. I just wanted the additional option to use either the type flavors from ValidationErrorType or from e.g. HeaderValidationErrorType (so then this "delete block: this.errorStr()" suggestion/test also makes sense again). I found it now and used the union operator to combine the types, that was actually the point where I was stuck. 😋

Also, general thought on the current approach. Do you think there would be room for simplification (mostly from the point of view of usage)? One thing I liked about the previous approach is that the type (and therefore shape) of the error was automatically inferred by the error code, so I'm wondering if we could perhaps do without the more specific HeaderValidationError class, just use the more general ValidationError class and let the ValidationErrorCode.WRONG_TX_TRIE_LENGTH code automatically adjust the shape of the errorContext prop. What do you think?

Ah no, I wouldn't want to simplify in this direction. I wouldn't want to have this error thing as some "exact science" ("one specific type shape per error", everything automatically inferred) but rather as some toolkit - where things are sufficiently typed - but which remains enough flexibility for various loose use cases. So this would basically be the Lodestar approach structurally. I also would like to keep the suberrors like HeaderValidationErrror (or at least the possibility to define such suberrors) since I would assume that this gives a useful framing to check for a certain error class (if HeaderValidationError doSomething;).

I will continue on this this morning and roll this out to the broader Block library. 🙂 Again, thanks for jumping in so quickly!

[holgerd77]

That said, I will radically simplify.

One already gets tired after transforming 2 errors. I will throw away the sub error level and also stick with one generic ErrorCode enum per class, substructured by simple code comments.

Will see how far this will get me. 🙂

[holgerd77]

Ok, I've radically simplified since I noticed that this is not practicable to roll this out throughout all our libraries and maybe not wanted in general.

So I removed the two-level error structure, errors are now solely in Util. I also removed the intended local package errors.ts file and also moved the error codes to Util. These are now more generic than I originally anticipated. I think this should in many/most cases still be sufficient to "target" an error happening, at the same time this is giving additional information about what type of error actually happened.

I rolled this now out to a broader part of the BlockHeader file and this seems to hold also in a more broadly applied context. I would nevertheless stop for now, just stumbled about the thought that Emmett is still heavily working on these files, so it might actually be better to start and/or continue with another library (might generally be a good idea to get an even broader idea if this is a scalable and widely appliable approach).

[holgerd77]

It has been decided to not add to the initial breaking release round, will add a "Blocked" label for now.

General concept/code structure can be picked up to be released along some follow-up minor release versions.