util: error management poc #1469
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| enum ErrorCode { | ||
| INVALID_PARAM = 'INVALID_PARAM', | ||
| UNKNOWN_ERROR = 'UNKNOWN_ERROR', | ||
| } | ||
|
|
||
| interface GeneralError<T extends ErrorCode = ErrorCode> extends Partial<Error> { | ||
| code?: T | ||
| } | ||
|
|
||
| interface InvalidParamError extends GeneralError<ErrorCode.INVALID_PARAM> { | ||
| param?: string | ||
| } | ||
|
|
||
| interface UnknownError extends GeneralError<ErrorCode.UNKNOWN_ERROR> { | ||
| [key: string]: any | ||
| } | ||
|
|
||
| // Convert an ErrorCode into its Typed Error | ||
| type CodedGeneralError<T> = T extends ErrorCode.INVALID_PARAM | ||
| ? InvalidParamError | ||
| : T extends ErrorCode.UNKNOWN_ERROR | ||
| ? UnknownError | ||
| : never | ||
|
|
||
| function isError<K extends ErrorCode, T extends CodedGeneralError<K>>( | ||
| error: GeneralError, | ||
| code: K | ||
| ): error is T { | ||
| if (error && (<GeneralError>error).code === code) { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| function isInvalidParamError(error: GeneralError): error is InvalidParamError { | ||
| return isError(error, ErrorCode.INVALID_PARAM) | ||
| } | ||
|
|
||
| function isUnknownError(error: GeneralError): error is UnknownError { | ||
| return isError(error, ErrorCode.UNKNOWN_ERROR) | ||
| } | ||
|
|
||
| export class ErrorLogger { | ||
| static errors = ErrorCode | ||
|
|
||
| makeError<T>(codedError: CodedGeneralError<T>): Error { | ||
| const { code, message, ...params } = codedError | ||
| const messageDetails: Array<string> = [] | ||
|
|
||
| if (isInvalidParamError(codedError) && typeof params.param !== 'undefined') { | ||
| messageDetails.push(`Invalid param=${codedError.param}`) | ||
| } | ||
|
|
||
| if (isUnknownError(codedError)) { | ||
| Object.keys(params).forEach((key) => { | ||
| const value = codedError[key] | ||
| try { | ||
| messageDetails.push(key + '=' + JSON.stringify(value)) | ||
| } catch { | ||
| messageDetails.push(key + '=' + JSON.stringify(codedError[key].toString())) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| messageDetails.push(`code=${code}`) | ||
|
|
||
| let errorMessage = message ?? '' | ||
|
|
||
| if (messageDetails.length) { | ||
| errorMessage += ' | Details: ' + messageDetails.join(', ') | ||
| } | ||
|
|
||
| const error = new Error(errorMessage) as CodedGeneralError<T> | ||
| error.code = codedError.code | ||
|
|
||
| Object.keys(params).forEach((key) => { | ||
| const typedKey = key as keyof typeof codedError | ||
| error[typedKey] = codedError[typedKey] | ||
| }) | ||
|
|
||
| // captureStackTrace is not defined in some browsers, notably Firefox | ||
| if (Error.captureStackTrace) { | ||
| 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<T extends ErrorCode>( | ||
| code?: T, | ||
| params?: Omit<CodedGeneralError<T>, 'code' | 'stack'> | ||
| ): void { | ||
| this.makeError({ | ||
| code: code ?? ErrorCode.UNKNOWN_ERROR, | ||
| ...params, | ||
| } as CodedGeneralError<T>) | ||
| } | ||
| } | ||
|
|
||
| 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:
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 There was a problem hiding this comment. (then please also adopt the test file name) |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| import tape from 'tape' | ||
| import { ErrorLogger } from '../src/errors' | ||
|
|
||
| tape('ErrorLogger', (t) => { | ||
| const errorLog = new ErrorLogger() | ||
|
|
||
| t.test('should assign the UNKNOWN_ERROR code to errors without code', (st) => { | ||
| let error: any | ||
|
|
||
| try { | ||
| errorLog.throwError() | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| st.equal(error.code, ErrorLogger.errors.UNKNOWN_ERROR) | ||
| st.end() | ||
| }), | ||
| t.test('should preserve the stack trace of the throwError context', (st) => { | ||
| let error: any | ||
|
|
||
| try { | ||
| errorLog.throwError() | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| // captureStackTrace is not defined in some browsers, notably Firefox, so behavior can't be implemented/tested there | ||
| // @ts-ignore | ||
| if (Error.captureStackTrace) { | ||
| st.ok(/Error: {2}\| Details: code=UNKNOWN_ERROR\n {4}at Test./.test(error.stack)) | ||
| } | ||
|
|
||
| st.end() | ||
| }), | ||
| t.test('should populate an error with UNKNOWN_ERROR code with all provided params', (st) => { | ||
| let error: any | ||
|
|
||
| try { | ||
| errorLog.throwError(ErrorLogger.errors.UNKNOWN_ERROR, { | ||
| errorInfo1: 'Information on the error', | ||
| errorInfo2: 'More information on the error', | ||
| }) | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| st.equal(error.code, ErrorLogger.errors.UNKNOWN_ERROR) | ||
| st.equal(error.errorInfo1, 'Information on the error') | ||
| st.equal(error.errorInfo2, 'More information on the error') | ||
| st.end() | ||
| }), | ||
| t.test( | ||
| 'should add all error params of UNKNOWN_ERROR to error mes21 02 2022 17:42:12.866:ERROR [launcher]: No binary for ChromeHeadless browser on your platform.sage details', | ||
| (st) => { | ||
| let error: any | ||
|
|
||
| try { | ||
| errorLog.throwError(ErrorLogger.errors.UNKNOWN_ERROR, { | ||
| errorInfo1: 'Information on the error', | ||
| errorInfo2: 'More information on the error', | ||
| }) | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| st.equal( | ||
| error.message, | ||
| ' | Details: errorInfo1="Information on the error", errorInfo2="More information on the error", code=UNKNOWN_ERROR' | ||
| ) | ||
| st.end() | ||
| } | ||
| ), | ||
| t.test('should append all error details to provided error message', (st) => { | ||
| let error: any | ||
|
|
||
| try { | ||
| errorLog.throwError(ErrorLogger.errors.UNKNOWN_ERROR, { | ||
| message: 'Error Message', | ||
| errorInfo1: 'Information on the error', | ||
| errorInfo2: 'More information on the error', | ||
| }) | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| st.equal( | ||
| error.message, | ||
| 'Error Message | Details: errorInfo1="Information on the error", errorInfo2="More information on the error", code=UNKNOWN_ERROR' | ||
| ) | ||
| st.end() | ||
| }) | ||
| t.test('should populate an error with INVALID_PARAM with the "param" prop', (st) => { | ||
| let error: any | ||
|
|
||
| try { | ||
| errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, { | ||
| param: 'difficulty', | ||
| }) | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| st.equal(error.param, 'difficulty') | ||
| st.end() | ||
| }), | ||
| t.test('should add the "param" prop to the INVALID_PARAM error message', (st) => { | ||
| let error: any | ||
|
|
||
| try { | ||
| errorLog.throwError(ErrorLogger.errors.INVALID_PARAM, { | ||
| message: 'Gas limit higher than maximum', | ||
| param: 'gasLimit', | ||
| }) | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| st.equal( | ||
| error.message, | ||
| 'Gas limit higher than maximum | Details: Invalid param=gasLimit, code=INVALID_PARAM' | ||
| ) | ||
| st.end() | ||
| }) | ||
| }) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 basicCustomErrorinterface/class/whatever and solely keep thecodeproperty 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like lodestar error approach, similar to the
codeapproach: 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.tsthen 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Utilspackage, as we already planned on the alternative layoutI 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
blockpackage - 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_UNKNOWNerror, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree the more user friendly(readable) error strings are beneficial. 👍