util: error management poc #1469
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Ok, this is now ready to review :) |
|
Is this already in a "Updated as a new suggestion" state or do you want to give this some more round? (completely haven't looked into the code yet, so just a plain question) |
The general error handling pattern is where I want it to be, so I think we could brainstorm on the taxonomy from that, if that's what you had in mind. I need to improve and fix the |
|
Ah, then maybe let's wait until you've fixed these things, I guess this should be settled first that things like stack traces are actually working with the intended concept (if I get your message correctly), and we do not need to rethink/rework some basic things. |
All right, the stack trace preservation issue has been fixed and I've simplified the makeError method. Should be good to serve as a basis for discussion and further development with the rest of the team. |
|
Ok, great 🙂 , one small (?) nit: can you see that the tests are passing (so for Util and VM)? |
|
@gabrocheleau thanks for fixing (small nit: please drop a note when you do, this is otherwise hard to spot)! 🙂 👍 Ok, great, then we can start discussing this as some base line. Do you want to give a short 6-line summary what you did and how things work? |
|
Hi there, again, can you give a short description here on the concept going beyond the pure code? |
Yes, apologies, missed the initial comment! Added a description to this PR's original post. Let me know if that works :) |
Cool, thanks a lot! 🙂 👍 |
| } | ||
| } | ||
|
|
||
| export const errorLog = new ErrorLogger() |
There was a problem hiding this comment.
I am generally not satisfied with the naming scheme we have here.
Can you do the following:
- Rename
errors.ts(and related) tocustomError.ts(singular) - Remove the class wrapping for
ErrorLogger, have this as plain methods - Export the
ErrorCodeenum astypes - Rename
throwErrortothrow
Then we can use this with a single-item import as follows:
import { customError } from 'ethereumjs-util'
customError.throw(customError.types.INVALID_PARAM)Not sure, we could also shorten and rename to just error, but I fear that this is too generic and might interfere in some unpredictable ways, it's also not so transparent regarding the use of our own custom error class.
There was a problem hiding this comment.
(then please also adopt the test file name)
| Error.captureStackTrace(error, this.throwError) | ||
| } | ||
|
|
||
| throw error |
There was a problem hiding this comment.
This is misplaced here, the name of the method says something else, can you please move this to throwError?
| return true | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
I am still highly sceptical on all this TypeScript magic TBH. The code is very hard to read (and therefore to maintain), I am also fearing a bit some unforseable JavaScript usage side effects, not sure if this is valid or not.
I have a - somewhat - stronger tendency to radically simplify here and throw all this sub-interfacing with InvalidParamError, UnknownError,... away, remove as much advanced TypeScript specifics as possible and just rely with a basic CustomError interface/class/whatever and solely keep the code property for distinction and integrate the ability from unknown-error to pass in an arbitrary list of key-value pairs with error information.
What do others think? @ryanio, @g11tech am I too conservative here? 🤔 I still do not feel settled on this code, especially this is something we would apply so broadly within the code base.
There was a problem hiding this comment.
I like lodestar error approach, similar to the code approach: generic lodestar error defined here (allows one to specify how to serialize it for log because we had the issue of dumping circular objects): https://github.com/ChainSafe/lodestar/blob/master/packages/utils/src/errors.ts
then finally extended to be used e.g. https://github.com/ChainSafe/lodestar/tree/master/packages/lodestar/src/chain/errors
There was a problem hiding this comment.
After reviewing Lodestar's approach, I quite like it and I feel like it would provide similar benefits to the approach used here but with simpler code.
There was a problem hiding this comment.
@gabrocheleau ok, nice, I've some additional comments I might be able to do later the day or tomorrow, then we should be ready to rework this and get to some final state to be applied throughout the code base! 🙂
There was a problem hiding this comment.
When "tomorrow" gets to 16 days, lol 😬 😜.
Anyhow.
Yes, I think this Lodestar approach is going very much into a good direction, I am also feeling a lot more confident on something like this in favor to defining our own custom "throwSomething()" function, remaining on dealing with original "Errors" or sub classes seems to be a lot more robust and side effects free to me.
So I would roughly suggest that:
- We do the generic code - similar to errors.ts in a dedicated
Utilspackage, as we already planned on the alternative layout
I think it then makes sense to define the respective errors like in the Lodestar errors folder on a "per package" basis on our side - e.g. for the block package - I don't see too much overlap on this between the packages. This simplifies things for structuring and drawing lines (and if we have some few redundancy here it also wouldn't hurt).
Additionally/eventually we can also define a few generic errors in Util as well, but rather as a second step if really necessary.
Just in between: here is a search for the Lodestar BlockErrorCode.PARENT_UNKNOWN error, just to get a glimpse on error usage.
If I get this right there are no "prose error strings" (like 'The parent hash of the uncle header is not part of the canonical chain') in Lodestar but just these capital AGGREGATOR_INDEX_TOO_HIGH = "ATTESTATION_ERROR_AGGREGATOR_INDEX_TOO_HIGH" aggregated error strings.
I would like to keep/stick to our prose approach, already for the reason that we have all this already written down. So I guess this would be one of the major deviations from the Lodestar approach.
WDYT? This is just a first round proposal - this would again need some draft implementation to verify - and is generally open for further discussion and evolution.
There was a problem hiding this comment.
i agree the more user friendly(readable) error strings are beneficial. 👍
|
Update: maybe wait with an update here, I have some additions thoughts and eventually an alternative suggestion on a simplified approach on this. |
|
Closing in favor of #1910 |
This PR implements a new error management pattern for a couple errors from the vm package (see #1440, #1439).
This new error management pattern adds an ErrorLogger to the
utilspackage. This ErrorLogger has athrowErrormethod that can be used as a replacement forthrow new Error('...'). The throwError method takes an ErrorCode (enum) as a first arg (defaults toUNKNOWN_ERROR), which allows for easier error identification & testing and also adjusts the shape of the second argument. For example, an ErrorCode can be linked to a particular error shape to allow additional error details to be provided (e.g. theInvalidParamErrorerror shape allows a user to pass in an additionalparamerror detail).Part of #1717