🥅 Feat: Good error messages #1186
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 214f242 The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 206 files out of 263 files are above the max files limit of 50. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the WalkthroughThe updates primarily focus on enhancing error handling and specificity across various modules. Custom error classes were introduced, replacing generic error messages with detailed ones. This change improves debugging and clarity. Additionally, specific dependencies were updated to support the new error handling mechanisms. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @roninjin10 and the rest of your teammates on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (4)
packages/errors/package.json (1)
5-5: The description "A typesafe library for writing forge scripts in typescript" seems generic. Consider specifying that this package is for error handling to improve clarity.packages/errors/src/ethereum/InternalErrorError.js (1)
41-77: TheInternalErrorclass implementation is consistent with other custom error classes in this package. Consider adding examples of when this error might be thrown in the documentation for better clarity.packages/errors/scripts/generate.ts (1)
1-2: Consider using ES module imports if Node.js version supports it.Using ES module syntax (
importinstead ofrequire) can make the code more modern and consistent with other JavaScript projects that use ES6+ features.packages/memory-client/src/test/viem/call.spec.ts (1)
Line range hint
19-41: Enhance error handling in test setup.- if (!deployResult.createdAddress) { - throw new Error('contract never deployed') - } - if (!deployResult.txHash) { - throw new Error('txHash not found') + if (!deployResult.createdAddress || !deployResult.txHash) { + throw new Error(`Deployment failed: ${!deployResult.createdAddress ? 'Address not created' : 'Transaction hash not found'}`) }Consolidating error checks can make the code cleaner and provide more specific error messages.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/memory-client/src/test/viem/__snapshots__/call.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (45)
- .changeset/dull-tables-invite.md (1 hunks)
- bundler-packages/unplugin/src/tevmUnplugin.js (1 hunks)
- packages/errors/package.json (1 hunks)
- packages/errors/scripts/generate.ts (1 hunks)
- packages/errors/src/TypedError.ts (1 hunks)
- packages/errors/src/ethereum/AccountLockedError.js (1 hunks)
- packages/errors/src/ethereum/BaseError.js (1 hunks)
- packages/errors/src/ethereum/BlockGasLimitExceededError.js (1 hunks)
- packages/errors/src/ethereum/ChainIdMismatchError.js (1 hunks)
- packages/errors/src/ethereum/ContractExecutionFailedError.js (1 hunks)
- packages/errors/src/ethereum/ExecutionErrorError.js (1 hunks)
- packages/errors/src/ethereum/GasLimitExceededError.js (1 hunks)
- packages/errors/src/ethereum/InsufficientFundsError.js (1 hunks)
- packages/errors/src/ethereum/InsufficientPermissionsError.js (1 hunks)
- packages/errors/src/ethereum/InternalErrorError.js (1 hunks)
- packages/errors/src/ethereum/InvalidAddressError.js (1 hunks)
- packages/errors/src/ethereum/InvalidGasPriceError.js (1 hunks)
- packages/errors/src/ethereum/InvalidOpcodeError.js (1 hunks)
- packages/errors/src/ethereum/InvalidParamsError.js (1 hunks)
- packages/errors/src/ethereum/InvalidRequestError.js (1 hunks)
- packages/errors/src/ethereum/InvalidSignatureError.js (1 hunks)
- packages/errors/src/ethereum/InvalidTransactionError.js (1 hunks)
- packages/errors/src/ethereum/LimitExceededError.js (1 hunks)
- packages/errors/src/ethereum/MethodNotFoundError.js (1 hunks)
- packages/errors/src/ethereum/MethodNotSupportedError.js (1 hunks)
- packages/errors/src/ethereum/NonceAlreadyUsedError.js (1 hunks)
- packages/errors/src/ethereum/NonceTooHighError.js (1 hunks)
- packages/errors/src/ethereum/NonceTooLowError.js (1 hunks)
- packages/errors/src/ethereum/OutOfGasError.js (1 hunks)
- packages/errors/src/ethereum/ParseErrorError.js (1 hunks)
- packages/errors/src/ethereum/PendingTransactionTimeoutError.js (1 hunks)
- packages/errors/src/ethereum/RateLimitExceededError.js (1 hunks)
- packages/errors/src/ethereum/ResourceNotFoundError.js (1 hunks)
- packages/errors/src/ethereum/ResourceUnavailableError.js (1 hunks)
- packages/errors/src/ethereum/RevertError.js (1 hunks)
- packages/errors/src/ethereum/TransactionRejectedError.js (1 hunks)
- packages/errors/src/ethereum/TransactionTooLargeError.js (1 hunks)
- packages/errors/src/ethereum/TransactionUnderpricedError.js (1 hunks)
- packages/errors/src/ethereum/UnknownBlockError.js (1 hunks)
- packages/errors/src/ethereum/UnsupportedChainError.js (1 hunks)
- packages/errors/src/requestErrors.ts (1 hunks)
- packages/memory-client/src/test/viem/call.spec.ts (17 hunks)
- packages/procedures/src/requestProcedure.js (1 hunks)
- packages/server/src/createHttpHandler.js (1 hunks)
- packages/zod/src/common/createError.js (1 hunks)
Files skipped from review due to trivial changes (1)
- bundler-packages/unplugin/src/tevmUnplugin.js
Additional context used
Biome
Additional comments not posted (68)
packages/zod/src/common/createError.js (1)
6-12: The refactoring increateErrorenhances clarity and error message formatting.packages/errors/src/TypedError.ts (1)
8-15: TheTypedErrortype definition is well-structured and aligns with the improved error handling objectives.packages/errors/src/ethereum/OutOfGasError.js (1)
41-77: TheOutOfGasErrorclass is implemented consistently with the other error classes. Ensure that the error code-32000is correctly mapped to this specific error type according to your error code documentation.packages/errors/src/ethereum/ParseErrorError.js (1)
41-64: The implementation ofParseErroris robust and well-documented. Good use of default parameters and property initialization.packages/errors/src/ethereum/NonceTooLowError.js (1)
41-64: The implementation ofNonceTooLowErroris robust and well-documented. Good use of default parameters and property initialization.packages/errors/src/ethereum/ExecutionErrorError.js (1)
41-64: The implementation ofExecutionErroris robust and well-documented. Good use of default parameters and property initialization.packages/errors/src/ethereum/NonceTooHighError.js (1)
41-64: The implementation ofNonceTooHighErroris robust and well-documented. Good use of default parameters and property initialization.packages/errors/src/ethereum/LimitExceededError.js (2)
48-58: Constructor implementation looks good and correctly initializes the base class with appropriate parameters.
70-76: Property overrides for_tagandnameare correctly implemented to facilitate specific error handling.packages/errors/src/ethereum/UnknownBlockError.js (2)
48-58: Constructor implementation is consistent with other custom errors and correctly initializes the base class.
70-76: Property overrides for_tagandnameare correctly implemented, consistent with other custom errors.packages/errors/src/ethereum/InvalidAddressError.js (2)
48-58: Constructor implementation is consistent and correctly initializes the base class with appropriate parameters.
70-76: Property overrides for_tagandnameare correctly implemented, facilitating specific error handling.packages/errors/src/ethereum/InvalidParamsError.js (2)
48-58: Constructor implementation is consistent and correctly initializes the base class with appropriate parameters.
70-76: Property overrides for_tagandnameare correctly implemented, facilitating specific error handling.packages/errors/src/ethereum/InvalidRequestError.js (2)
48-58: Constructor implementation looks good and properly initializes the error properties with sensible defaults and documentation links.
70-76: Proper use of_tagandnameproperties to ensure error type consistency.packages/errors/src/ethereum/GasLimitExceededError.js (2)
48-58: Constructor implementation is consistent and well-defined, correctly setting up the error properties.
70-76: Correct implementation of_tagandnameproperties, ensuring clear error identification.packages/errors/src/ethereum/InvalidOpcodeError.js (2)
48-58: Consistent and correct constructor implementation, effectively setting up the error properties.
70-76: Proper implementation of_tagandnameproperties, ensuring accurate error discrimination.packages/errors/src/ethereum/InvalidSignatureError.js (2)
48-58: Well-implemented constructor, correctly initializing error properties with consistent documentation references.
70-76: Correctly set_tagandnameproperties, facilitating accurate error identification.packages/errors/src/ethereum/UnsupportedChainError.js (2)
48-58: The constructor implementation forUnsupportedChainErroris well-defined, correctly initializing the base class with appropriate properties and a specific error code.
70-76: The properties_tagandnameare consistently set to 'UnsupportedChain', which is good for error type consistency and logging.packages/errors/src/ethereum/InvalidGasPriceError.js (2)
48-58: The constructor forInvalidGasPriceErroris correctly implemented, initializing the base class with the necessary properties and a specific error code.
70-76: The properties_tagandnameare set to 'InvalidGasPrice', ensuring clarity and consistency in error handling.packages/errors/src/ethereum/MethodNotFoundError.js (2)
48-58: The constructor forMethodNotFoundErroris well-implemented, correctly initializing the base class with the necessary properties and a specific error code.
70-76: The properties_tagandnameare set to 'MethodNotFound', which is appropriate and ensures consistency in error handling.packages/errors/src/ethereum/TransactionTooLargeError.js (2)
48-58: The constructor forTransactionTooLargeErroris correctly implemented, initializing the base class with the necessary properties and a specific error code.
70-76: The properties_tagandnameare set to 'TransactionTooLarge', ensuring clarity and consistency in error handling.packages/errors/src/ethereum/ChainIdMismatchError.js (2)
48-58: The constructor implementation forChainIdMismatchErroris correctly set up with appropriate default values and error codes. Good use of spreadingargsto allow extensibility.
70-76: Proper use of properties_tagandnameto ensure the error class is correctly identified in error handling mechanisms.packages/errors/src/ethereum/NonceAlreadyUsedError.js (2)
48-58: The constructor forNonceAlreadyUsedErroris well-defined, ensuring that all necessary information and defaults are correctly set.
70-76: Correct implementation of properties_tagandnameto uniquely identify theNonceAlreadyUsedError.packages/errors/src/ethereum/RateLimitExceededError.js (2)
48-58: Well-implemented constructor forRateLimitExceededErrorwith clear documentation links and appropriate error code.
70-76: Properly defined properties_tagandnameforRateLimitExceededError, ensuring accurate error identification.packages/errors/src/ethereum/ResourceNotFoundError.js (2)
48-58: The constructor forResourceNotFoundErroris correctly implemented, including all necessary default parameters and a specific error code.
70-76: Correct setup of properties_tagandnameforResourceNotFoundError, ensuring it is properly recognized in error handling.packages/errors/src/ethereum/InvalidTransactionError.js (2)
48-58: Constructor implementation looks good and aligns with the intended error handling enhancements.
70-76: Property definitions for_tagandnameare correctly implemented and marked as overrides.packages/errors/src/ethereum/InsufficientFundsError.js (2)
48-58: Constructor implementation is well-defined, ensuring proper initialization of error properties.
70-76: Properties_tagandnameare correctly set and marked as overrides, aligning with the class's purpose.packages/errors/src/ethereum/TransactionRejectedError.js (2)
48-58: Constructor implementation is correct, with appropriate error code and documentation paths initialized.
70-76: Property definitions for_tagandnameare correctly implemented and marked as overrides.packages/errors/src/ethereum/MethodNotSupportedError.js (2)
48-58: Constructor implementation is well-defined, with correct initialization of error properties and documentation paths.
70-76: Properties_tagandnameare correctly set and marked as overrides, aligning with the class's purpose.packages/errors/src/ethereum/AccountLockedError.js (2)
43-66: The implementation ofAccountLockedErroris robust and well-documented. Good use of ES6 features for default parameters and spreadingargs.
72-78: Proper use of class fields to define properties_tagandname. This enhances code clarity and maintainability.packages/errors/src/ethereum/BlockGasLimitExceededError.js (2)
41-64: The implementation ofBlockGasLimitExceededErroris robust and well-documented. Good use of ES6 features for default parameters and spreadingargs.
70-76: Proper use of class fields to define properties_tagandname. This enhances code clarity and maintainability.packages/errors/src/ethereum/ContractExecutionFailedError.js (2)
41-64: The implementation ofContractExecutionFailedErroris robust and well-documented. Good use of ES6 features for default parameters and spreadingargs.
70-76: Proper use of class fields to define properties_tagandname. This enhances code clarity and maintainability.packages/errors/src/ethereum/ResourceUnavailableError.js (2)
41-64: The implementation ofResourceUnavailableErroris robust and well-documented. Good use of ES6 features for default parameters and spreadingargs.
70-76: Proper use of class fields to define properties_tagandname. This enhances code clarity and maintainability.packages/errors/src/ethereum/TransactionUnderpricedError.js (3)
48-58: Constructor implementation looks good and correctly initializes the error properties.
70-76: Property definitions for_tagandnameare correctly overridden to reflect the specific error type.
16-40: Documentation is comprehensive and effectively explains the purpose and usage of theTransactionUnderpricedError.packages/errors/src/ethereum/InsufficientPermissionsError.js (3)
48-58: Constructor implementation looks good and correctly initializes the error properties.
70-76: Property definitions for_tagandnameare correctly overridden to reflect the specific error type.
16-40: Documentation is comprehensive and effectively explains the purpose and usage of theInsufficientPermissionsError.packages/errors/src/ethereum/PendingTransactionTimeoutError.js (3)
48-58: Constructor implementation looks good and correctly initializes the error properties.
70-76: Property definitions for_tagandnameare correctly overridden to reflect the specific error type.
16-40: Documentation is comprehensive and effectively explains the purpose and usage of thePendingTransactionTimeoutError.packages/errors/src/ethereum/BaseError.js (2)
41-97: Constructor implementation looks good and correctly initializes the error properties, including handling of error causes and documentation links.
104-119: Thewalkfunction is well-implemented and provides essential functionality for traversing error chains.packages/memory-client/src/test/viem/call.spec.ts (1)
Line range hint
72-85: Verify behavior for zero data and block number specific calls.Ensure that the tests for zero data and block number specific calls behave as expected under different conditions.
packages/procedures/src/requestProcedure.js (1)
1297-1297: Update error code to4200for unsupported methods.This change aligns with the PR's goal to improve error messages. Ensure that the new error code
4200is documented and consistently used across the project for similar errors.
.changeset/dull-tables-invite.md
Outdated
| "@tevm/unplugin": patch | ||
| --- | ||
|
|
||
| Fix: bug loading tsconfig.json" |
There was a problem hiding this comment.
Remove the extra quotation mark at the end of the description.
- Fix: bug loading tsconfig.json"
+ Fix: bug loading tsconfig.jsonCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fix: bug loading tsconfig.json" | |
| Fix: bug loading tsconfig.json |
| "access": "public" | ||
| } | ||
| "name": "@tevm/errors", | ||
| "version": "1.1.0-next.69", |
There was a problem hiding this comment.
Ensure versioning follows semantic versioning principles, especially for a -next release which implies a pre-release version.
| "types" | ||
| ], | ||
| "scripts": { | ||
| "all": "pnpm i && bun run build && bun lint && bun format && bun test:run && bun generate:docs && bun lint:deps && bun lint:package", |
There was a problem hiding this comment.
The script all seems to be doing too many things. Consider breaking it into smaller, more focused scripts to improve maintainability and readability.
| | -32004 // Method not supported | ||
| | -32005 // Limit exceeded | ||
| | -32006 // JSON-RPC version not supported | ||
| | -32042 // Method not found |
There was a problem hiding this comment.
Remove the duplicate error code for "Method not found."
- | -32042 // Method not foundCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | -32042 // Method not found |
| const outputDir = './errors' | ||
| if (!fs.existsSync(outputDir)) { | ||
| fs.mkdirSync(outputDir) | ||
| } |
There was a problem hiding this comment.
Ensure directory creation is handled correctly.
- if (!fs.existsSync(outputDir)) {
- fs.mkdirSync(outputDir)
+ if (!fs.existsSync(outputDir)) {
+ fs.mkdirSync(outputDir, { recursive: true })
}Adding { recursive: true } ensures that the directory is created along with any necessary parent directories.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const outputDir = './errors' | |
| if (!fs.existsSync(outputDir)) { | |
| fs.mkdirSync(outputDir) | |
| } | |
| const outputDir = './errors' | |
| if (!fs.existsSync(outputDir)) { | |
| fs.mkdirSync(outputDir, { recursive: true }) | |
| } |
| export class RevertError extends BaseError { | ||
| /** | ||
| * Constructs a RevertError. | ||
| * | ||
| * @param {string} message - Human-readable error message. | ||
| * @param {RevertErrorParameters} [args={}] - Additional parameters for the BaseError. | ||
| */ | ||
| constructor(message, args = {}) { | ||
| super( | ||
| message, | ||
| { | ||
| ...args, | ||
| docsBaseUrl: 'https://tevm.sh', | ||
| docsPath: '/reference/tevm/errors/classes/reverterror/', | ||
| }, | ||
| 'Revert', | ||
| -32000, | ||
| ) | ||
|
|
||
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta | ||
| } | ||
|
|
||
| /** | ||
| * @type {'Revert'} | ||
| * @override | ||
| */ | ||
| _tag = 'Revert' | ||
|
|
||
| /** | ||
| * @type {'Revert'} | ||
| * @override | ||
| */ | ||
| name = 'Revert' | ||
| } |
There was a problem hiding this comment.
The RevertError class is well-documented and follows good OOP practices. However, ensure that the _tag and name properties are necessary as they seem redundant.
| errors.forEach((error) => { | ||
| const content = `/** | ||
| * ${error.name} Error | ||
| * | ||
| * @type {TypedError<'${error.name}'>} | ||
| * @property {string} _tag - Same as name, used internally | ||
| * @property {string} name - The name of the error, analogous to code in JSON RPC | ||
| * @property {string} message - Human-readable error message | ||
| * @property {object} [meta] - Optional object containing additional information about the error | ||
| * @property {number} code - Error code analogous to the code in JSON RPC error | ||
| */ | ||
| export const ${error.name}Error: TypedError<'${error.name}'> = { | ||
| _tag: '${error.name}', | ||
| name: '${error.name}', | ||
| message: '${error.message}', | ||
| code: ${error.code} | ||
| }; | ||
| ` | ||
|
|
||
| fs.writeFileSync(path.join(outputDir, `${error.name}Error.ts`), content, 'utf8') | ||
| }) | ||
|
|
||
| console.log('Error files generated successfully.') |
There was a problem hiding this comment.
Refactor to use async file operations.
- fs.writeFileSync(path.join(outputDir, `${error.name}Error.ts`), content, 'utf8')
+ fs.promises.writeFile(path.join(outputDir, `${error.name}Error.ts`), content, 'utf8')Using asynchronous file operations can improve the performance of your script by not blocking the event loop.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| errors.forEach((error) => { | |
| const content = `/** | |
| * ${error.name} Error | |
| * | |
| * @type {TypedError<'${error.name}'>} | |
| * @property {string} _tag - Same as name, used internally | |
| * @property {string} name - The name of the error, analogous to code in JSON RPC | |
| * @property {string} message - Human-readable error message | |
| * @property {object} [meta] - Optional object containing additional information about the error | |
| * @property {number} code - Error code analogous to the code in JSON RPC error | |
| */ | |
| export const ${error.name}Error: TypedError<'${error.name}'> = { | |
| _tag: '${error.name}', | |
| name: '${error.name}', | |
| message: '${error.message}', | |
| code: ${error.code} | |
| }; | |
| ` | |
| fs.writeFileSync(path.join(outputDir, `${error.name}Error.ts`), content, 'utf8') | |
| }) | |
| console.log('Error files generated successfully.') | |
| errors.forEach(async (error) => { | |
| const content = `/** | |
| * ${error.name} Error | |
| * | |
| * @type {TypedError<'${error.name}'>} | |
| * @property {string} _tag - Same as name, used internally | |
| * @property {string} name - The name of the error, analogous to code in JSON RPC | |
| * @property {string} message - Human-readable error message | |
| * @property {object} [meta] - Optional object containing additional information about the error | |
| * @property {number} code - Error code analogous to the code in JSON RPC error | |
| */ | |
| export const ${error.name}Error: TypedError<'${error.name}'> = { | |
| _tag: '${error.name}', | |
| name: '${error.name}', | |
| message: '${error.message}', | |
| code: ${error.code} | |
| }; | |
| ` | |
| await fs.promises.writeFile(path.join(outputDir, `${error.name}Error.ts`), content, 'utf8') | |
| }) | |
| console.log('Error files generated successfully.') |
| responses.map((response, i) => { | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (requests[i]) | ||
| if (response.status === 'rejected') { | ||
| console.error(response.reason) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| return { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| } | ||
| return response.value | ||
| }) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(responses)) | ||
| } | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (parsedRequest.data) | ||
| try { | ||
| // TODO update this type to accept any jsonrpc request if a fork url pass through exists | ||
| // We don't officially support it until we explicitly implement all the endpoints instead of | ||
| // blindly passing through | ||
| const response = await client._tevm.send(/** @type any*/ (request)) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } catch (e) { | ||
| console.error(e) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| console.error(err) | ||
| const response = { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| res.writeHead(500, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } | ||
| }) | ||
| } | ||
| if (Array.isArray(parsedRequest.data)) { | ||
| /** | ||
| * @type {ReadonlyArray<import("@tevm/jsonrpc").JsonRpcRequest<string, object>>} | ||
| */ | ||
| const requests = parsedRequest.data | ||
| const responses = await Promise.allSettled( | ||
| requests.map((request) => client._tevm.send(/** @type any*/(request))), | ||
| ) | ||
| responses.map((response, i) => { | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (requests[i]) | ||
| if (response.status === 'rejected') { | ||
| console.error(response.reason) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| return { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| } | ||
| return response.value | ||
| }) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(responses)) | ||
| } | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (parsedRequest.data) | ||
| try { | ||
| // TODO update this type to accept any jsonrpc request if a fork url pass through exists | ||
| // We don't officially support it until we explicitly implement all the endpoints instead of | ||
| // blindly passing through | ||
| const response = await client._tevm.send(/** @type any*/(request)) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } catch (e) { | ||
| console.error(e) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| console.error(err) | ||
| const response = { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| res.writeHead(500, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Refactor to improve error handling and readability.
- if (!parsedRequest.success) {
- const err = new BadRequestError(JSON.stringify(parsedRequest.error.format()))
- res.writeHead(400, { 'Content-Type': 'application/json' })
- res.end(JSON.stringify({
- id: 'unknown',
- method: 'unknown',
- jsonrpc: '2.0',
- error: {
- code: err._tag,
- message: err.message,
- },
- }))
- return
- }
+ if (!parsedRequest.success) {
+ handleBadRequest(res, parsedRequest.error)
+ return
+ }Extracting the error handling into a separate function handleBadRequest can make the main function cleaner and easier to understand.
Committable suggestion was skipped due to low confidence.
roninjin10
force-pushed
the
06-09-_goal_net_feat_good_error_messages
branch
from
d0cab4d
to
928b086
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (39)
- packages/errors/package.json (1 hunks)
- packages/errors/src/actions/BaseCallError.ts (2 hunks)
- packages/errors/src/actions/ContractError.ts (2 hunks)
- packages/errors/src/actions/GetAccountError.ts (1 hunks)
- packages/errors/src/actions/SetAccountError.ts (1 hunks)
- packages/errors/src/ethereum/AccountNotFoundError.js (1 hunks)
- packages/errors/src/ethereum/BaseError.js (1 hunks)
- packages/errors/src/ethereum/ExecutionErrorError.js (1 hunks)
- packages/errors/src/ethereum/InvalidParamsError.js (1 hunks)
- packages/errors/src/ethereum/index.js (1 hunks)
- packages/errors/src/ethereum/index.ts (1 hunks)
- packages/errors/src/index.ts (1 hunks)
- packages/errors/src/input/InvalidAbiError.js (1 hunks)
- packages/errors/src/input/InvalidArgsError.js (1 hunks)
- packages/errors/src/input/InvalidBalanceError.js (1 hunks)
- packages/errors/src/input/InvalidBlobVersionedHashesError.js (1 hunks)
- packages/errors/src/input/InvalidBlockError.js (1 hunks)
- packages/errors/src/input/InvalidBytecodeError.js (1 hunks)
- packages/errors/src/input/InvalidCallerError.js (1 hunks)
- packages/errors/src/input/InvalidDataError.js (1 hunks)
- packages/errors/src/input/InvalidDeployedBytecodeError.js (1 hunks)
- packages/errors/src/input/InvalidDepthError.js (1 hunks)
- packages/errors/src/input/InvalidFunctionNameError.js (1 hunks)
- packages/errors/src/input/InvalidGasLimitError.js (1 hunks)
- packages/errors/src/input/InvalidGasPriceError.js (1 hunks)
- packages/errors/src/input/InvalidGasRefundError.js (1 hunks)
- packages/errors/src/input/InvalidNonceError.js (1 hunks)
- packages/errors/src/input/InvalidOriginError.js (1 hunks)
- packages/errors/src/input/InvalidRequestError.js (1 hunks)
- packages/errors/src/input/InvalidSaltError.js (1 hunks)
- packages/errors/src/input/InvalidSelfdestructError.js (1 hunks)
- packages/errors/src/input/InvalidSkipBalanceError.js (1 hunks)
- packages/errors/src/input/InvalidStorageRootError.js (1 hunks)
- packages/errors/src/input/InvalidToError.js (1 hunks)
- packages/errors/src/input/InvalidUrlError.js (1 hunks)
- packages/errors/src/input/InvalidValueError.js (1 hunks)
- packages/errors/src/input/index.ts (1 hunks)
- packages/errors/src/utils/DecodeFunctionDataError.js (1 hunks)
- packages/errors/src/utils/EncodeFunctionReturnDataError.js (1 hunks)
Files skipped from review due to trivial changes (2)
- packages/errors/src/ethereum/index.js
- packages/errors/src/index.ts
Files skipped from review as they are similar to previous changes (4)
- packages/errors/package.json
- packages/errors/src/ethereum/BaseError.js
- packages/errors/src/ethereum/ExecutionErrorError.js
- packages/errors/src/ethereum/InvalidParamsError.js
Additional context used
Biome
Additional comments not posted (28)
packages/errors/src/actions/GetAccountError.ts (1)
2-3: Ensure the new import paths are correct and resolve properly.packages/errors/src/actions/SetAccountError.ts (1)
2-2: Ensure the new import path forInvalidAddressErroris correct and resolves properly.Verification successful
The new import path for
InvalidAddressErrorinSetAccountError.tsresolves correctly and is used appropriately within the file.
- The file
InvalidAddressError.jsexists in the specified path.- The
InvalidAddressErroris imported and referenced correctly inSetAccountError.ts.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new import path in SetAccountError.ts resolves correctly. # Test: Check if the file exists. fd 'InvalidAddressError.js' packages/errors/src/ethereumLength of output: 108
Script:
#!/bin/bash # Description: Verify the usage of InvalidAddressError in SetAccountError.ts # Test: Check if InvalidAddressError is used within SetAccountError.ts rg 'InvalidAddressError' packages/errors/src/actions/SetAccountError.tsLength of output: 228
packages/errors/src/input/index.ts (1)
Line range hint
1-24: Verify that all exported error types are used elsewhere in the project and that their definitions are up-to-date.Verification successful
The verification confirms that all the exported error types from
index.tsare used elsewhere in the project and their definitions are up-to-date.
- The error types are referenced in various files, indicating active usage and correct implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all exported error types from index.ts are used elsewhere in the project. # Test: Search for usage of each exported type. rg --type ts 'InvalidAbiError|InvalidArgsError|InvalidBalanceError|InvalidBytecodeError|InvalidDataError|InvalidStorageRootError|InvalidFunctionNameError|InvalidDeployedBytecodeError|InvalidGasPriceError|InvalidGasLimitError|InvalidBlobVersionedHashesError|InvalidOriginError|InvalidRequestError|InvalidToError|InvalidCallerError|InvalidDepthError|InvalidGasRefundError|InvalidNonceError|InvalidSaltError|InvalidBlockError|InvalidValueError|InvalidSkipBalanceError|InvalidSelfdestructError|InvalidUrlError'Length of output: 11981
packages/errors/src/actions/BaseCallError.ts (2)
1-3: Imports are correctly scoped and relevant to the functionality ofBaseCallError.Also applies to: 22-22
28-29: The type definition forBaseCallErroris comprehensive and well-documented, covering a broad range of error scenarios relevant to call-based procedures.packages/errors/src/utils/DecodeFunctionDataError.js (2)
3-24: The documentation forDecodeFunctionDataErroris clear and informative, providing useful context and examples.
25-38: TheDecodeFunctionDataErrorclass is well-implemented, correctly extendingInvalidParamsErrorwith appropriate metadata initialization.packages/errors/src/utils/EncodeFunctionReturnDataError.js (2)
3-26: The documentation forEncodeFunctionReturnDataErroris clear and informative, providing useful context and examples.
28-41: TheEncodeFunctionReturnDataErrorclass is well-implemented, correctly extendingInvalidParamsErrorwith appropriate metadata initialization.packages/errors/src/input/InvalidAbiError.js (2)
3-38: The documentation forInvalidAbiErroris clear and informative, providing useful context and examples.
40-58: TheInvalidAbiErrorclass is well-implemented, correctly extendingInvalidParamsErrorwith appropriate metadata initialization.packages/errors/src/input/InvalidToError.js (1)
40-58: Well-structured constructor forInvalidToError. Consider adding a practical example in the documentation to illustrate how this error might be thrown and caught in real scenarios.packages/errors/src/input/InvalidUrlError.js (1)
40-58: Well-structured constructor forInvalidUrlError. Consider adding a practical example in the documentation to illustrate how this error might be thrown and caught in real scenarios.packages/errors/src/input/InvalidDataError.js (1)
40-58: Well-structured constructor forInvalidDataError. Consider adding a practical example in the documentation to illustrate how this error might be thrown and caught in real scenarios.packages/errors/src/input/InvalidSaltError.js (1)
40-58: Well-structured constructor forInvalidSaltError. Consider adding a practical example in the documentation to illustrate how this error might be thrown and caught in real scenarios.packages/errors/src/input/InvalidRequestError.js (1)
40-58: The constructor implementation forInvalidRequestErroris well-structured and effectively uses ES6 features for clean code. Good job on maintaining clarity and maintainability.packages/errors/src/input/InvalidCallerError.js (1)
40-58: The constructor implementation forInvalidCallerErroris consistent and well-implemented, similar to other custom errors in the codebase. It effectively uses modern JavaScript features for maintainability.packages/errors/src/input/InvalidOriginError.js (1)
40-58: Consistent with other error classes, the constructor forInvalidOriginErroris well-implemented and maintains the codebase's standards for error handling.packages/errors/src/ethereum/AccountNotFoundError.js (1)
40-58: The constructor forAccountNotFoundErroris correctly implemented, using ES6 features to enhance code clarity and maintainability. It aligns well with the error handling strategy across the codebase.packages/errors/src/input/InvalidBalanceError.js (1)
40-58: The implementation ofInvalidBalanceErrorcorrectly extendsInvalidParamsErrorand sets up specific properties and documentation links. Good use of spreadingargsto include additional properties.packages/errors/src/input/InvalidBlockError.js (1)
40-58: The implementation ofInvalidBlockErroris consistent with the other custom error classes, properly extendingInvalidParamsErrorand initializing specific properties and documentation links.packages/errors/src/input/InvalidGasLimitError.js (1)
40-58: The implementation ofInvalidGasLimitErrorfollows the established pattern of extendingInvalidParamsErrorand correctly initializes specific properties and documentation links.packages/errors/src/input/InvalidGasRefundError.js (1)
40-58: The implementation ofInvalidGasRefundErroris consistent with the other custom error classes, properly extendingInvalidParamsErrorand initializing specific properties and documentation links.packages/errors/src/input/InvalidBytecodeError.js (1)
40-59: The implementation ofInvalidBytecodeErroris consistent and well-documented. The use ofdocsBaseUrlanddocsPathto link directly to relevant documentation enhances usability and developer experience.packages/errors/src/input/InvalidGasPriceError.js (1)
40-59: TheInvalidGasPriceErrorclass is implemented consistently with clear documentation and appropriate error metadata. The constructor initialization is correct, ensuring that error instances will have all necessary information.packages/errors/src/input/InvalidFunctionNameError.js (1)
40-59: TheInvalidFunctionNameErrorclass is well-implemented with comprehensive documentation and consistent constructor behavior. The specific error documentation paths are correctly set, enhancing clarity and developer guidance.packages/errors/src/input/InvalidSkipBalanceError.js (1)
40-59: TheInvalidSkipBalanceErrorclass is correctly implemented with detailed documentation and consistent constructor behavior. The error class provides clear, actionable information and links to specific documentation, which is beneficial for developers.packages/errors/src/ethereum/index.ts (1)
1-47: All exports are correctly structured and follow consistent formatting.Ensure that all these exported classes are properly documented, especially regarding their parameters and usage within the system.
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta |
There was a problem hiding this comment.
Consider removing the redundant meta property definition in the constructor since it's already handled by the base class.
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta |
There was a problem hiding this comment.
Consider removing the redundant meta property definition in the constructor since it's already handled by the base class.
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta |
There was a problem hiding this comment.
Consider removing the redundant meta property definition in the constructor since it's already handled by the base class.
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta |
There was a problem hiding this comment.
Consider removing the redundant meta property definition in the constructor since it's already handled by the base class.
| export class InvalidDepthError extends InvalidParamsError { | ||
| /** | ||
| * Constructs an InvalidDepthError. | ||
| * | ||
| * @param {string} message - Human-readable error message. | ||
| * @param {InvalidDepthErrorParameters} [args={}] - Additional parameters for the InvalidDepthError. | ||
| */ | ||
| constructor(message, args = {}) { | ||
| super(message, { | ||
| ...args, | ||
| docsBaseUrl: 'https://tevm.sh', | ||
| docsPath: '/reference/tevm/errors/classes/invaliddeptherror/', | ||
| }) | ||
|
|
||
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta | ||
| } |
There was a problem hiding this comment.
Refine the constructor to ensure proper initialization of properties.
The constructor of InvalidDepthError effectively initializes several properties. However, the meta property is only conditionally set based on args.meta. It might be beneficial to ensure it always exists as an object to avoid potential issues when accessing properties on meta. Here's a suggested change:
constructor(message, args = {}) {
super(message, {
...args,
docsBaseUrl: 'https://tevm.sh',
docsPath: '/reference/tevm/errors/classes/invaliddeptherror/',
})
- this.meta = args.meta
+ this.meta = args.meta || {}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class InvalidDepthError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidDepthError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidDepthErrorParameters} [args={}] - Additional parameters for the InvalidDepthError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invaliddeptherror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta | |
| } | |
| export class InvalidDepthError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidDepthError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidDepthErrorParameters} [args={}] - Additional parameters for the InvalidDepthError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invaliddeptherror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta || {} | |
| } |
| export class InvalidNonceError extends InvalidParamsError { | ||
| /** | ||
| * Constructs an InvalidNonceError. | ||
| * | ||
| * @param {string} message - Human-readable error message. | ||
| * @param {InvalidNonceErrorParameters} [args={}] - Additional parameters for the InvalidNonceError. | ||
| */ | ||
| constructor(message, args = {}) { | ||
| super(message, { | ||
| ...args, | ||
| docsBaseUrl: 'https://tevm.sh', | ||
| docsPath: '/reference/tevm/errors/classes/invalidnonceerror/', | ||
| }) | ||
|
|
||
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta | ||
| } |
There was a problem hiding this comment.
Ensure consistent handling of optional properties in the constructor.
Similar to the previous file, the InvalidNonceError constructor could be improved by ensuring the meta property is always initialized as an object. This change ensures consistency and prevents potential runtime errors when accessing properties on meta:
constructor(message, args = {}) {
super(message, {
...args,
docsBaseUrl: 'https://tevm.sh',
docsPath: '/reference/tevm/errors/classes/invalidnonceerror/',
})
- this.meta = args.meta
+ this.meta = args.meta || {}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class InvalidNonceError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidNonceError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidNonceErrorParameters} [args={}] - Additional parameters for the InvalidNonceError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invalidnonceerror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta | |
| } | |
| export class InvalidNonceError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidNonceError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidNonceErrorParameters} [args={}] - Additional parameters for the InvalidNonceError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invalidnonceerror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta || {} | |
| } |
| export class InvalidValueError extends InvalidParamsError { | ||
| /** | ||
| * Constructs an InvalidValueError. | ||
| * | ||
| * @param {string} message - Human-readable error message. | ||
| * @param {InvalidValueErrorParameters} [args={}] - Additional parameters for the InvalidValueError. | ||
| */ | ||
| constructor(message, args = {}) { | ||
| super(message, { | ||
| ...args, | ||
| docsBaseUrl: 'https://tevm.sh', | ||
| docsPath: '/reference/tevm/errors/classes/invalidvalueerror/', | ||
| }) | ||
|
|
||
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta | ||
| } |
There was a problem hiding this comment.
Standardize the initialization of the meta property.
To maintain consistency across error classes and ensure robustness, the InvalidValueError constructor should always initialize the meta property as an object. This change avoids potential undefined access issues:
constructor(message, args = {}) {
super(message, {
...args,
docsBaseUrl: 'https://tevm.sh',
docsPath: '/reference/tevm/errors/classes/invalidvalueerror/',
})
- this.meta = args.meta
+ this.meta = args.meta || {}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class InvalidValueError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidValueError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidValueErrorParameters} [args={}] - Additional parameters for the InvalidValueError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invalidvalueerror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta | |
| } | |
| export class InvalidValueError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidValueError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidValueErrorParameters} [args={}] - Additional parameters for the InvalidValueError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invalidvalueerror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta || {} | |
| } |
| export class InvalidArgsError extends InvalidParamsError { | ||
| /** | ||
| * Constructs an InvalidArgsError. | ||
| * | ||
| * @param {string} message - Human-readable error message. | ||
| * @param {InvalidArgsErrorParameters} [args={}] - Additional parameters for the InvalidParamsError. | ||
| */ | ||
| constructor(message, args = {}) { | ||
| super(message, { | ||
| ...args, | ||
| docsBaseUrl: 'https://tevm.sh', | ||
| docsPath: '/reference/tevm/errors/classes/invalidargserror/', | ||
| }) | ||
|
|
||
| /** | ||
| * @type {object|undefined} | ||
| */ | ||
| this.meta = args.meta | ||
| } |
There was a problem hiding this comment.
Consistently initialize optional properties in the constructor.
To ensure that the meta property is always available as an object, even when not provided in args, the constructor of InvalidArgsError should be adjusted. This change enhances the robustness of error handling:
constructor(message, args = {}) {
super(message, {
...args,
docsBaseUrl: 'https://tevm.sh',
docsPath: '/reference/tevm/errors/classes/invalidargserror/',
})
- this.meta = args.meta
+ this.meta = args.meta || {}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class InvalidArgsError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidArgsError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidArgsErrorParameters} [args={}] - Additional parameters for the InvalidParamsError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invalidargserror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta | |
| } | |
| export class InvalidArgsError extends InvalidParamsError { | |
| /** | |
| * Constructs an InvalidArgsError. | |
| * | |
| * @param {string} message - Human-readable error message. | |
| * @param {InvalidArgsErrorParameters} [args={}] - Additional parameters for the InvalidParamsError. | |
| */ | |
| constructor(message, args = {}) { | |
| super(message, { | |
| ...args, | |
| docsBaseUrl: 'https://tevm.sh', | |
| docsPath: '/reference/tevm/errors/classes/invalidargserror/', | |
| }) | |
| /** | |
| * @type {object|undefined} | |
| */ | |
| this.meta = args.meta || {} | |
| } |
| @@ -1,8 +1,9 @@ | |||
| import type { ExecutionRevertedError } from 'viem' | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The ExecutionRevertedError type is defined in experimental/viem-effect/src-viem/errors/node.ts and not in viem. Therefore, the import path in ContractError.ts is incorrect.
packages/errors/src/actions/ContractError.ts: Update the import path forExecutionRevertedErrortoexperimental/viem-effect/src-viem/errors/node.
Analysis chain
Ensure the new import paths for ExecutionRevertedError and ExecutionError are correct and resolve properly.
Also applies to: 3-3
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new import paths in ContractError.ts resolve correctly.
# Test: Check if the files exist.
fd 'ExecutionRevertedError.js' packages/errors/src/ethereum
fd 'ExecutionErrorError.js' packages/errors/src/ethereum
Length of output: 168
Script:
#!/bin/bash
# Description: Verify the existence and correct import path of `ExecutionRevertedError`.
# Test: Search for the definition of `ExecutionRevertedError` in the codebase.
rg 'ExecutionRevertedError' --files-with-matches
Length of output: 1006
Script:
#!/bin/bash
# Description: Verify the definition of `ExecutionRevertedError`.
# Test: Search for the definition of `ExecutionRevertedError` in the codebase.
rg 'ExecutionRevertedError' -t typescript
Length of output: 76
Script:
#!/bin/bash
# Description: Verify the definition of `ExecutionRevertedError`.
# Test: Search for the definition of `ExecutionRevertedError` in the codebase.
rg 'ExecutionRevertedError' --type ts
Length of output: 2987
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (2)
packages/vm/src/actions/deepCopy.js (1)
Line range hint
15-15: Enhance error message for unsupported state manager.The error message could be more informative by suggesting potential next steps or checks.
- throw new Error('StateManager does not support deepCopy. Was a Tevm state manager used?') + throw new UnexpectedInternalServerError('StateManager does not support deepCopy. Ensure a compatible Tevm state manager is used.')packages/actions/src/eth/ethGetLogsHandler.js (1)
Line range hint
87-112: Ensure robust error handling when fetching logs from a forked chain.When fetching logs from a forked chain, ensure that errors are handled properly and that the user is informed of the specific issue. Consider adding more detailed error information or suggestions for resolution in the error messages.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (25)
- examples/vite/src/wagmi.ts (1 hunks)
- packages/actions/src/eth/ethGetLogsHandler.js (4 hunks)
- packages/actions/src/internal/forkAndCacheBlock.js (2 hunks)
- packages/errors/src/fork/ForkError.js (1 hunks)
- packages/errors/src/index.ts (1 hunks)
- packages/errors/src/input/InvalidBlockError.js (1 hunks)
- packages/errors/src/input/index.ts (1 hunks)
- packages/errors/src/utils/index.ts (1 hunks)
- packages/server/src/createHttpHandler.js (1 hunks)
- packages/state/package.json (1 hunks)
- packages/state/src/actions/deepCopy.js (2 hunks)
- packages/state/src/actions/generateCannonicalGenesis.js (2 hunks)
- packages/state/src/actions/putContractStorage.js (3 hunks)
- packages/state/src/createBaseState.js (2 hunks)
- packages/vm/package.json (1 hunks)
- packages/vm/src/actions/deepCopy.js (1 hunks)
- packages/zod/src/common/createError.js (1 hunks)
- packages/zod/src/validators/validateBaseCallParams.js (2 hunks)
- packages/zod/src/validators/validateCallParams.js (2 hunks)
- packages/zod/src/validators/validateContractParams.js (2 hunks)
- packages/zod/src/validators/validateGetAccountParams.js (2 hunks)
- packages/zod/src/validators/validateLoadStateParams.js (2 hunks)
- packages/zod/src/validators/validateMineParams.js (2 hunks)
- packages/zod/src/validators/validateScriptParams.js (2 hunks)
- packages/zod/src/validators/validateSetAccountParams.js (2 hunks)
Files skipped from review due to trivial changes (3)
- packages/errors/src/utils/index.ts
- packages/state/package.json
- packages/vm/package.json
Files skipped from review as they are similar to previous changes (3)
- packages/errors/src/input/InvalidBlockError.js
- packages/errors/src/input/index.ts
- packages/zod/src/common/createError.js
Additional context used
Biome
packages/actions/src/internal/forkAndCacheBlock.js
[error] 18-18: expected
,but instead found}(parse)Remove }
[error] 18-18: expected
,but instead found}(parse)Remove }
Additional comments not posted (28)
packages/zod/src/validators/validateGetAccountParams.js (1)
23-23: LGTM! The use ofInvalidRequestErrorenhances the specificity of error handling, aligning with the PR's objectives.packages/zod/src/validators/validateLoadStateParams.js (1)
25-25: LGTM! Consistent use ofInvalidRequestErrorfor handling validation errors effectively.packages/state/src/actions/deepCopy.js (1)
17-17: Excellent use ofUnexpectedInternalServerErrorto handle specific error scenarios, enhancing error clarity and management.examples/vite/src/wagmi.ts (1)
3-3: The configuration setup for wallet connections and RPC providers appears correct and well-structured.packages/state/src/actions/putContractStorage.js (1)
23-23: Good use of specific error messages for better clarity.This change helps in pinpointing the exact issue when the key length is incorrect, enhancing the error's usefulness in debugging scenarios.
packages/zod/src/validators/validateMineParams.js (3)
23-23: Correct use of specific error classes to enhance error specificity.Using
InvalidRequestErrorfor generic request errors is appropriate and improves the clarity of error reporting.
32-32: Appropriate use ofInvalidNonceErrorfor nonce-related validation failures.This specific error usage aids in quickly identifying issues related to nonce values in the mining parameters.
37-37: Correct application ofInvalidBalanceErrorfor balance-related validation issues.This enhances the specificity and clarity of error messages related to balance validation in mining parameters.
packages/zod/src/validators/validateCallParams.js (3)
26-26: Appropriate use ofInvalidSaltErrorfor salt-related validation failures.Using
InvalidSaltErrorspecifically for salt validation issues enhances the clarity and specificity of error messages.
31-31: Correct application ofInvalidDataErrorfor data-related validation issues.This specific error usage aids in quickly identifying issues related to data fields in the call parameters.
36-36: Proper use ofInvalidDeployedBytecodeErrorfor deployed bytecode validation failures.This enhances the specificity and clarity of error messages related to deployed bytecode validation in call parameters.
packages/zod/src/validators/validateContractParams.js (4)
1-1: Ensure all necessary error classes are imported correctly.
6-6: The typedef forValidateContractParamsErroris well-defined and clear.
11-11: The function signature is clear and the return type is well-documented.
26-26: Error handling invalidateContractParamsis robust, using specific error types for different validation failures.Also applies to: 31-31, 36-36, 41-41
packages/zod/src/validators/validateScriptParams.js (4)
1-1: Ensure all necessary error classes are imported correctly.
6-6: The typedef forValidateScriptParamsErroris well-defined and clear.
11-11: The function signature is clear and the return type is well-documented.
26-26: Error handling invalidateScriptParamsis robust, using specific error types for different validation failures.Also applies to: 31-31, 36-36, 41-41
packages/zod/src/validators/validateSetAccountParams.js (4)
1-8: Ensure all necessary error classes are imported correctly.
12-12: The typedef forValidateSetAccountParamsErroris well-defined and clear.
17-17: The function signature is clear and the return type is well-documented.
30-30: Error handling invalidateSetAccountParamsis robust, using specific error types for different validation failures.Also applies to: 34-34, 39-39, 44-44, 49-49, 54-54
packages/state/src/actions/generateCannonicalGenesis.js (1)
17-17: Improved error handling by usingUnexpectedInternalServerErrorfor uncommitted checkpoints.Consider adding a comment explaining why this specific error type is appropriate here, as it helps maintain code clarity and aids future maintainers.
packages/state/src/createBaseState.js (1)
43-43: Improved error handling by usingUnexpectedInternalServerErrorfor non-existing state roots.Consider adding a comment explaining why this specific error type is appropriate here, as it helps maintain code clarity and aids future maintainers.
packages/zod/src/validators/validateBaseCallParams.js (1)
35-107: Significantly improved error specificity by using various specific error classes.Consider adding comments explaining the choice of specific errors for each validation failure. This will enhance code clarity and help future maintainers understand the rationale behind these choices.
packages/errors/src/fork/ForkError.js (1)
60-106: Well-definedForkErrorclass with comprehensive documentation and appropriate properties.Consider adding unit tests for
ForkErrorto ensure that it behaves as expected under various scenarios.packages/errors/src/index.ts (1)
1-124: All error exports and their parameters are correctly structured and referenced.This comprehensive export structure enhances modularity and maintainability by centralizing error definitions, which can be easily imported and used across different packages.
| if (Array.isArray(parsedRequest.data)) { | ||
| /** | ||
| * @type {ReadonlyArray<import("@tevm/jsonrpc").JsonRpcRequest<string, object>>} | ||
| */ | ||
| const requests = parsedRequest.data | ||
| const responses = await Promise.allSettled( | ||
| requests.map((request) => client._tevm.send(/** @type any*/(request))), | ||
| ) | ||
| responses.map((response, i) => { | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (requests[i]) | ||
| if (response.status === 'rejected') { | ||
| console.error(response.reason) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| return { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| } | ||
| return response.value | ||
| }) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(responses)) | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling for batch requests.
Batch requests should handle individual request errors appropriately without failing the entire batch. Consider implementing a more robust error handling mechanism for each request in the batch.
| @@ -13,7 +14,7 @@ export const forkAndCacheBlock = async (client, block, executeBlock = false) => | |||
| const vm = await client.getVm() | |||
| // this can't happen making ts happy | |||
| if (!client.forkTransport) { | |||
| throw new Error('Cannot fork without a fork url') | |||
| throw new Internal(new Error('Cannot fork without a fork url') | |||
There was a problem hiding this comment.
Syntax error: missing closing parenthesis and comma in the throw statement.
- throw new Internal(new Error('Cannot fork without a fork url')
+ throw new Internal(new Error('Cannot fork without a fork url'),Committable suggestion was skipped due to low confidence.
| @@ -1,4 +1,5 @@ | |||
| import { createStateManager } from '@tevm/state' | |||
| import {InvalidAbiError} from '@tevm/errors' | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The InvalidAbiError is imported in forkAndCacheBlock.js but is not used within the file. It is, however, used in other parts of the codebase, specifically in validateContractParams.js and validateScriptParams.js.
forkAndCacheBlock.js:InvalidAbiErroris imported but not used.
This indicates that the import in forkAndCacheBlock.js is unnecessary and can be safely removed.
Analysis chain
Ensure InvalidAbiError is used appropriately or remove if not needed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of `InvalidAbiError` in `forkAndCacheBlock`.
# Test: Search for the usage of `InvalidAbiError`. Expect: No usage or appropriate usage.
rg --type js 'InvalidAbiError' packages/actions/src/internal/forkAndCacheBlock.js
Length of output: 126
Script:
#!/bin/bash
# Description: Verify usage of `InvalidAbiError` across the entire codebase.
# Test: Search for the usage of `InvalidAbiError`. Expect: No usage or appropriate usage.
rg --type js 'InvalidAbiError'
Length of output: 2261
| @@ -6,7 +7,7 @@ import { getAccount } from './getAccount.js' | |||
| */ | |||
| const stripZeros = (bytes) => { | |||
| if (!(bytes instanceof Uint8Array)) { | |||
| throw new Error('Unexpected type') | |||
| throw new UnexpectedInternalServerError('Unexpected type') | |||
There was a problem hiding this comment.
Ensure the error message is clear and actionable.
Consider providing more details in the error message to help developers understand what type was actually received, which can aid in debugging.
- throw new UnexpectedInternalServerError('Unexpected type')
+ throw new UnexpectedInternalServerError(`Unexpected type: expected Uint8Array, received ${typeof bytes}`)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new UnexpectedInternalServerError('Unexpected type') | |
| throw new UnexpectedInternalServerError(`Unexpected type: expected Uint8Array, received ${typeof bytes}`) |
| } | ||
|
|
||
| const account = await getAccount(baseState)(address) | ||
| if (!account) { | ||
| throw new Error('cannot putContractStorage on non existing acccount!') | ||
| throw new UnexpectedInternalServerError('cannot putContractStorage on non existing acccount! Consider checking if account exists first') |
There was a problem hiding this comment.
Enhance error message for non-existing account scenario.
The error message could be more informative by suggesting potential next steps or checks.
- throw new UnexpectedInternalServerError('cannot putContractStorage on non existing acccount! Consider checking if account exists first')
+ throw new UnexpectedInternalServerError('Attempted to putContractStorage on a non-existing account. Ensure the account address is correct and exists before retrying.')Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new UnexpectedInternalServerError('cannot putContractStorage on non existing acccount! Consider checking if account exists first') | |
| throw new UnexpectedInternalServerError('Attempted to putContractStorage on a non-existing account. Ensure the account address is correct and exists before retrying.') |
| }) | ||
| if (formattedErrors.blockCount) { | ||
| formattedErrors.blockCount._errors.forEach((error) => { | ||
| errors.push(createError('InvalidAddressError', error, String(action.blockCount))) | ||
| errors.push(new InvalidAddressError(error)) |
There was a problem hiding this comment.
Ensure error class matches the context of the error.
The error class InvalidAddressError seems misused for blockCount validation errors. Consider using a more appropriate error class or creating a new one if necessary.
- errors.push(new InvalidAddressError(error))
+ errors.push(new InvalidBlockCountError(error)) // Assuming InvalidBlockCountError is an appropriate error classCommittable suggestion was skipped due to low confidence.
| ) | ||
| responses.map((response, i) => { | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (requests[i]) | ||
| if (response.status === 'rejected') { | ||
| console.error(response.reason) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| return { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| } | ||
| return response.value | ||
| }) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(responses)) | ||
| } | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (parsedRequest.data) | ||
| try { | ||
| // TODO update this type to accept any jsonrpc request if a fork url pass through exists | ||
| // We don't officially support it until we explicitly implement all the endpoints instead of | ||
| // blindly passing through | ||
| const response = await client._tevm.send(/** @type any*/ (request)) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } catch (e) { | ||
| console.error(e) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| console.error(err) | ||
| const response = { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| res.writeHead(500, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } | ||
| }) | ||
| } | ||
| if (Array.isArray(parsedRequest.data)) { | ||
| /** | ||
| * @type {ReadonlyArray<import("@tevm/jsonrpc").JsonRpcRequest<string, object>>} | ||
| */ | ||
| const requests = parsedRequest.data | ||
| const responses = await Promise.allSettled( | ||
| requests.map((request) => client._tevm.send(/** @type any*/(request))), | ||
| ) | ||
| responses.map((response, i) => { | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (requests[i]) | ||
| if (response.status === 'rejected') { | ||
| console.error(response.reason) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| return { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| } | ||
| return response.value | ||
| }) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(responses)) | ||
| } | ||
| const request = /** @type {import("@tevm/jsonrpc").JsonRpcRequest<string, object>} */ (parsedRequest.data) | ||
| try { | ||
| // TODO update this type to accept any jsonrpc request if a fork url pass through exists | ||
| // We don't officially support it until we explicitly implement all the endpoints instead of | ||
| // blindly passing through | ||
| const response = await client._tevm.send(/** @type any*/(request)) | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } catch (e) { | ||
| console.error(e) | ||
| const err = new UnexpectedInternalServerError(request.method) | ||
| console.error(err) | ||
| const response = { | ||
| id: request.id, | ||
| method: request.method, | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: err._tag, | ||
| message: err.message, | ||
| }, | ||
| } | ||
| res.writeHead(500, { 'Content-Type': 'application/json' }) | ||
| return res.end(JSON.stringify(response)) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Refactor to improve error handling and readability.
- if (!parsedRequest.success) {
- const err = new BadRequestError(JSON.stringify(parsedRequest.error.format()))
- res.writeHead(400, { 'Content-Type': 'application/json' })
- res.end(JSON.stringify({
- id: 'unknown',
- method: 'unknown',
- jsonrpc: '2.0',
- error: {
- code: err._tag,
- message: err.message,
- },
- }))
- return
- }
+ if (!parsedRequest.success) {
+ handleBadRequest(res, parsedRequest.error)
+ return
+ }Extracting the error handling into a separate function handleBadRequest can make the main function cleaner and easier to understand.
Committable suggestion was skipped due to low confidence.
packages/vm/src/actions/deepCopy.js
Outdated
| @@ -1,5 +1,6 @@ | |||
| import { createEvm } from '@tevm/evm' | |||
| import { createBaseVm } from '../createBaseVm.js' | |||
| import {} from '@tevm/errors' | |||
There was a problem hiding this comment.
Remove unnecessary empty import.
The import statement does not import any modules and should be removed to clean up the code.
- import {} from '@tevm/errors'Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {} from '@tevm/errors' |
| throw new InvalidBlockError('safe not currently supported as block tag') | ||
| } | ||
| if (blockParam === 'latest' || blockParam === undefined) { | ||
| const safeBlock = blockchain.blocksByTag.get('latest') | ||
| // let's handle it here in case we forget to update it later | ||
| if (safeBlock) { | ||
| return safeBlock.header.number | ||
| } | ||
| throw new Error('latest block does not exist on chain') | ||
| throw new InvalidBlockError('latest block does not exist on chain') | ||
| } | ||
| if (blockParam === 'pending') { | ||
| // for pending we need to mine a new block and then handle it | ||
| // let's skip this functionality for now | ||
| throw new Error( | ||
| throw new InvalidBlockError( | ||
| 'Pending not yet supported but will be in future. Consider opening an issue or reaching out on telegram if you need this feature to expediate its release', | ||
| ) | ||
| } | ||
| if (blockParam === 'earliest') { | ||
| return BigInt(1) | ||
| } | ||
| if (blockParam === 'finalized') { | ||
| throw new Error('finalized noet yet supported for this feature') | ||
| throw new InvalidBlockError('finalized noet yet supported for this feature') | ||
| } | ||
| blockchain.logger.error({ blockParam }, 'Unknown block param pased to blockNumberHandler') | ||
| throw new Error(`Unknown block param ${blockParam} pased to blockNumberHandler`) | ||
| throw new InvalidBlockError(`Unknown block param ${blockParam} pased to blockNumberHandler`) |
There was a problem hiding this comment.
Improve error messages for better user guidance.
- throw new InvalidBlockError('safe not currently supported as block tag')
+ throw new InvalidBlockError('The "safe" block tag is not currently supported. Please use a specific block number or "latest".')
- throw new InvalidBlockError('latest block does not exist on chain')
+ throw new InvalidBlockError('The "latest" block does not exist on the chain. Ensure the blockchain is fully synchronized.')
- throw new InvalidBlockError('Pending not yet supported but will be in future...')
+ throw new InvalidBlockError('The "pending" block tag is not yet supported but will be in future releases. Stay tuned!')
- throw new InvalidBlockError('finalized not yet supported for this feature')
+ throw new InvalidBlockError('The "finalized" block tag is not yet supported for this feature.')
- throw new InvalidBlockError('Unknown block param ${blockParam} passed to blockNumberHandler')
+ throw new InvalidBlockError(`The block parameter "${blockParam}" is unknown. Please use a valid block identifier such as a block number, "latest", "earliest", or "pending".`)These changes make the error messages more informative and user-friendly, aligning with the PR's goal of improving error messages.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new InvalidBlockError('safe not currently supported as block tag') | |
| } | |
| if (blockParam === 'latest' || blockParam === undefined) { | |
| const safeBlock = blockchain.blocksByTag.get('latest') | |
| // let's handle it here in case we forget to update it later | |
| if (safeBlock) { | |
| return safeBlock.header.number | |
| } | |
| throw new Error('latest block does not exist on chain') | |
| throw new InvalidBlockError('latest block does not exist on chain') | |
| } | |
| if (blockParam === 'pending') { | |
| // for pending we need to mine a new block and then handle it | |
| // let's skip this functionality for now | |
| throw new Error( | |
| throw new InvalidBlockError( | |
| 'Pending not yet supported but will be in future. Consider opening an issue or reaching out on telegram if you need this feature to expediate its release', | |
| ) | |
| } | |
| if (blockParam === 'earliest') { | |
| return BigInt(1) | |
| } | |
| if (blockParam === 'finalized') { | |
| throw new Error('finalized noet yet supported for this feature') | |
| throw new InvalidBlockError('finalized noet yet supported for this feature') | |
| } | |
| blockchain.logger.error({ blockParam }, 'Unknown block param pased to blockNumberHandler') | |
| throw new Error(`Unknown block param ${blockParam} pased to blockNumberHandler`) | |
| throw new InvalidBlockError(`Unknown block param ${blockParam} pased to blockNumberHandler`) | |
| throw new InvalidBlockError('The "safe" block tag is not currently supported. Please use a specific block number or "latest".') | |
| } | |
| if (blockParam === 'latest' || blockParam === undefined) { | |
| const safeBlock = blockchain.blocksByTag.get('latest') | |
| // let's handle it here in case we forget to update it later | |
| if (safeBlock) { | |
| return safeBlock.header.number | |
| } | |
| throw new InvalidBlockError('The "latest" block does not exist on the chain. Ensure the blockchain is fully synchronized.') | |
| } | |
| if (blockParam === 'pending') { | |
| // for pending we need to mine a new block and then handle it | |
| // let's skip this functionality for now | |
| throw new InvalidBlockError( | |
| 'The "pending" block tag is not yet supported but will be in future releases. Stay tuned!', | |
| ) | |
| } | |
| if (blockParam === 'earliest') { | |
| return BigInt(1) | |
| } | |
| if (blockParam === 'finalized') { | |
| throw new InvalidBlockError('The "finalized" block tag is not yet supported for this feature.') | |
| } | |
| blockchain.logger.error({ blockParam }, 'Unknown block param pased to blockNumberHandler') | |
| throw new InvalidBlockError(`The block parameter "${blockParam}" is unknown. Please use a valid block identifier such as a block number, "latest", "earliest", or "pending".`) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (8)
- packages/actions/src/internal/forkAndCacheBlock.js (2 hunks)
- packages/actions/src/tevm/setAccountHandler.js (2 hunks)
- packages/errors/src/fork/NoForkUriSetError.js (1 hunks)
- packages/errors/src/fork/index.ts (1 hunks)
- packages/errors/src/index.ts (1 hunks)
- packages/tx/package.json (1 hunks)
- packages/utils/package.json (1 hunks)
- packages/utils/src/createMemoryDb.js (2 hunks)
Files skipped from review due to trivial changes (4)
- packages/actions/src/internal/forkAndCacheBlock.js
- packages/errors/src/fork/index.ts
- packages/tx/package.json
- packages/utils/package.json
Additional comments not posted (4)
packages/utils/src/createMemoryDb.js (1)
19-19: LGTM! Consider adding unit tests forunexpectedTypeErrorto ensure it handles various inputs as expected.packages/errors/src/fork/NoForkUriSetError.js (1)
29-48: Well-documented and correctly implemented error class. Consider adding unit tests to verify the behavior ofNoForkTransportSetErrorunder various scenarios.packages/actions/src/tevm/setAccountHandler.js (1)
3-3: The updates to imports and error handling are appropriate and improve clarity. Consider adding more detailed comments in complex sections to aid future maintainability.Also applies to: 10-10
packages/errors/src/index.ts (1)
1-129: The reorganization of exports is well-executed, enhancing modularity and clarity. This change facilitates easier maintenance and use across different modules.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/tx/src/__snapshots__/createImpersonatedTx.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (5)
- packages/errors/src/ethereum/BaseError.js (1 hunks)
- packages/errors/src/ethereum/GasLimitExceededError.js (1 hunks)
- packages/tx/package.json (1 hunks)
- packages/tx/src/createImpersonatedTx.js (1 hunks)
- packages/tx/src/createImpersonatedTx.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/errors/src/ethereum/BaseError.js
- packages/errors/src/ethereum/GasLimitExceededError.js
- packages/tx/package.json
Additional context used
Biome
Additional comments not posted (5)
packages/tx/src/createImpersonatedTx.js (2)
6-7: Excellent use of TypeScript for defining error types.
54-54: Ensure proper handling of all potential exceptions from theFeeMarketEIP1559Transactionconstructor.Verification successful
The
FeeMarketEIP1559Transactionconstructor is indeed wrapped in a try-catch block, ensuring that exceptions are caught and handled. The catch block logs the error and throws anInternalErrorif the caught exception is not an instance ofError.
packages/tx/src/createImpersonatedTx.js, around line 54Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all potential exceptions from the FeeMarketEIP1559Transaction constructor are handled. # Test: Search for the FeeMarketEIP1559Transaction constructor usage. Expect: Proper exception handling. rg --type javascript $'new FeeMarketEIP1559Transaction'Length of output: 91
Script:
#!/bin/bash # Description: Verify all potential exceptions from the FeeMarketEIP1559Transaction constructor are handled. # Test: Search for the FeeMarketEIP1559Transaction constructor usage. Expect: Proper exception handling. rg --type js 'new FeeMarketEIP1559Transaction'Length of output: 141
Script:
#!/bin/bash # Description: Check for exception handling around the FeeMarketEIP1559Transaction constructor usage. # Test: Search for try-catch blocks around the FeeMarketEIP1559Transaction constructor usage. rg -A 5 -B 5 'new FeeMarketEIP1559Transaction' packages/tx/src/createImpersonatedTx.jsLength of output: 321
packages/tx/src/createImpersonatedTx.spec.ts (3)
26-47: Good coverage of error scenarios in tests.
1-1: Ensure consistency in import statements.- import { afterEach, describe, expect, it, jest, mock } from 'bun:test' + import { afterEach, describe, expect, it, jest, mock } from 'bun:test'Likely invalid or redundant comment.
60-78: Ensure that all possible exceptions from theFeeMarketEIP1559Transactionconstructor are tested.
| @@ -1,13 +1,57 @@ | |||
| import { FeeMarketEIP1559Transaction } from '@ethereumjs/tx' | |||
| import { GasLimitExceededError, InternalError, InvalidGasLimitError } from '@tevm/errors' | |||
There was a problem hiding this comment.
Consider grouping imports from the same package.
- import { GasLimitExceededError, InternalError, InvalidGasLimitError } from '@tevm/errors'
- import { EthjsAddress, keccak256 } from '@tevm/utils'
+ import { GasLimitExceededError, InternalError, InvalidGasLimitError } from '@tevm/errors'
+ import { EthjsAddress, keccak256 } from '@tevm/utils'Committable suggestion was skipped due to low confidence.
| * @throws {CreateImpersonatedTxError} Error if the constructor for {@link FeeMarketEIP1559Transaction} throws | ||
| * @param {import("@ethereumjs/tx").FeeMarketEIP1559TxData & {impersonatedAddress: EthjsAddress}} txData | ||
| * @param {import("@ethereumjs/tx").TxOptions} [opts] | ||
| * @returns {import('./ImpersonatedTx.js').ImpersonatedTx} | ||
| */ | ||
| export const createImpersonatedTx = (txData, opts) => { | ||
| const tx = new FeeMarketEIP1559Transaction(txData, opts) | ||
| /** | ||
| * @type {FeeMarketEIP1559Transaction} | ||
| */ | ||
| let tx | ||
| try { | ||
| tx = new FeeMarketEIP1559Transaction(txData, opts) | ||
| } catch (e) { | ||
| console.error(e) | ||
| if (!(e instanceof Error)) { | ||
| throw new InternalError( | ||
| 'Unknown Error', | ||
| { cause: /** @type any*/(e)} | ||
| ) | ||
| } | ||
| if (e.message.includes('EIP-1559 not enabled on Common')) { | ||
| throw new InternalError( | ||
| 'EIP-1559 is not enabled on Common. Tevm currently only supports 1559 and it should be enabled by default', | ||
| { cause: e }, | ||
| ) | ||
| } | ||
| if (e.message.includes('gasLimit cannot exceed MAX_UINT64 (2^64-1)') || e.message.includes('gasLimit * maxFeePerGas cannot exceed MAX_INTEGER (2^256-1)')) { | ||
| throw new GasLimitExceededError(e.message, { cause: e }) | ||
| } | ||
| if ( | ||
| e.message.includes( | ||
| 'maxFeePerGas cannot be less than maxPriorityFeePerGas (The total must be the larger of the two)', | ||
| ) | ||
| ) { | ||
| throw new InvalidGasLimitError(e.message, { cause: e }) | ||
| } | ||
| throw new InternalError(e.message, { cause: e }) | ||
| } |
There was a problem hiding this comment.
Refactor error handling to reduce complexity and improve readability.
- if (!(e instanceof Error)) {
- throw new InternalError('Unknown Error', { cause: /** @type any*/(e) })
- }
- if (e.message.includes('EIP-1559 not enabled on Common')) {
- throw new InternalError('EIP-1559 is not enabled on Common. Tevm currently only supports 1559 and it should be enabled by default', { cause: e })
- }
- if (e.message.includes('gasLimit cannot exceed MAX_UINT64 (2^64-1)') || e.message.includes('gasLimit * maxFeePerGas cannot exceed MAX_INTEGER (2^256-1)')) {
- throw new GasLimitExceededError(e.message, { cause: e })
- }
- if (e.message.includes('maxFeePerGas cannot be less than maxPriorityFeePerGas (The total must be the larger of the two)')) {
- throw new InvalidGasLimitError(e.message, { cause: e })
- }
- throw new InternalError(e.message, { cause: e })
+ try {
+ tx = new FeeMarketEIP1559Transaction(txData, opts)
+ } catch (e) {
+ handleTransactionError(e)
+ }
+ function handleTransactionError(e) {
+ const errorMap = {
+ 'EIP-1559 not enabled on Common': () => new InternalError('EIP-1559 is not enabled on Common. Tevm currently only supports 1559 and it should be enabled by default', { cause: e }),
+ 'gasLimit cannot exceed MAX_UINT64 (2^64-1)': () => new GasLimitExceededError(e.message, { cause: e }),
+ 'gasLimit * maxFeePerGas cannot exceed MAX_INTEGER (2^256-1)': () => new GasLimitExceededError(e.message, { cause: e }),
+ 'maxFeePerGas cannot be less than maxPriorityFeePerGas (The total must be the larger of the two)': () => new InvalidGasLimitError(e.message, { cause: e })
+ };
+ const matchedError = Object.keys(errorMap).find(key => e.message.includes(key));
+ if (matchedError) {
+ throw errorMap[matchedError]();
+ }
+ throw new InternalError('Unknown Error', { cause: e });
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @throws {CreateImpersonatedTxError} Error if the constructor for {@link FeeMarketEIP1559Transaction} throws | |
| * @param {import("@ethereumjs/tx").FeeMarketEIP1559TxData & {impersonatedAddress: EthjsAddress}} txData | |
| * @param {import("@ethereumjs/tx").TxOptions} [opts] | |
| * @returns {import('./ImpersonatedTx.js').ImpersonatedTx} | |
| */ | |
| export const createImpersonatedTx = (txData, opts) => { | |
| const tx = new FeeMarketEIP1559Transaction(txData, opts) | |
| /** | |
| * @type {FeeMarketEIP1559Transaction} | |
| */ | |
| let tx | |
| try { | |
| tx = new FeeMarketEIP1559Transaction(txData, opts) | |
| } catch (e) { | |
| console.error(e) | |
| if (!(e instanceof Error)) { | |
| throw new InternalError( | |
| 'Unknown Error', | |
| { cause: /** @type any*/(e)} | |
| ) | |
| } | |
| if (e.message.includes('EIP-1559 not enabled on Common')) { | |
| throw new InternalError( | |
| 'EIP-1559 is not enabled on Common. Tevm currently only supports 1559 and it should be enabled by default', | |
| { cause: e }, | |
| ) | |
| } | |
| if (e.message.includes('gasLimit cannot exceed MAX_UINT64 (2^64-1)') || e.message.includes('gasLimit * maxFeePerGas cannot exceed MAX_INTEGER (2^256-1)')) { | |
| throw new GasLimitExceededError(e.message, { cause: e }) | |
| } | |
| if ( | |
| e.message.includes( | |
| 'maxFeePerGas cannot be less than maxPriorityFeePerGas (The total must be the larger of the two)', | |
| ) | |
| ) { | |
| throw new InvalidGasLimitError(e.message, { cause: e }) | |
| } | |
| throw new InternalError(e.message, { cause: e }) | |
| } | |
| * @throws {CreateImpersonatedTxError} Error if the constructor for {@link FeeMarketEIP1559Transaction} throws | |
| * @param {import("@ethereumjs/tx").FeeMarketEIP1559TxData & {impersonatedAddress: EthjsAddress}} txData | |
| * @param {import("@ethereumjs/tx").TxOptions} [opts] | |
| * @returns {import('./ImpersonatedTx.js').ImpersonatedTx} | |
| */ | |
| export const createImpersonatedTx = (txData, opts) => { | |
| /** | |
| * @type {FeeMarketEIP1559Transaction} | |
| */ | |
| let tx | |
| try { | |
| tx = new FeeMarketEIP1559Transaction(txData, opts) | |
| } catch (e) { | |
| handleTransactionError(e) | |
| } | |
| } | |
| function handleTransactionError(e) { | |
| const errorMap = { | |
| 'EIP-1559 not enabled on Common': () => new InternalError('EIP-1559 is not enabled on Common. Tevm currently only supports 1559 and it should be enabled by default', { cause: e }), | |
| 'gasLimit cannot exceed MAX_UINT64 (2^64-1)': () => new GasLimitExceededError(e.message, { cause: e }), | |
| 'gasLimit * maxFeePerGas cannot exceed MAX_INTEGER (2^256-1)': () => new GasLimitExceededError(e.message, { cause: e }), | |
| 'maxFeePerGas cannot be less than maxPriorityFeePerGas (The total must be the larger of the two)': () => new InvalidGasLimitError(e.message, { cause: e }) | |
| }; | |
| const matchedError = Object.keys(errorMap).find(key => e.message.includes(key)); | |
| if (matchedError) { | |
| throw errorMap[matchedError](); | |
| } | |
| throw new InternalError('Unknown Error', { cause: e }); | |
| } |
| it('should throw GasLimitExceededError if bigger than MAX_INTEGER', () => { | ||
| const impersonatedAddress = EthjsAddress.fromString(`0x${'42'.repeat(20)}`) | ||
| const data = '0x5234' | ||
| const ethjsError = new Error( | ||
| 'gasLimit cannot exceed MAX_UINT64 (2^64-1), given 374144419156711147060143317175368453031918731001855 (tx type=2 hash=not available (unsigned) nonce=0 value=0 signed=false hf=error maxFeePerGas=undefined maxPriorityFeePerGas=undefined)', | ||
| ) | ||
| expect(() => createImpersonatedTx({ impersonatedAddress, data, gasLimit: `0x${'ff'.repeat(21)}` })).toThrow( | ||
| new GasLimitExceededError(ethjsError.message, { cause: ethjsError }), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Consider adding more context to the test description for clarity.
- it('should throw GasLimitExceededError if bigger than MAX_INTEGER', () => {
+ it('should throw GasLimitExceededError if gasLimit exceeds MAX_UINT64 or gasLimit * maxFeePerGas exceeds MAX_INTEGER', () => {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should throw GasLimitExceededError if bigger than MAX_INTEGER', () => { | |
| const impersonatedAddress = EthjsAddress.fromString(`0x${'42'.repeat(20)}`) | |
| const data = '0x5234' | |
| const ethjsError = new Error( | |
| 'gasLimit cannot exceed MAX_UINT64 (2^64-1), given 374144419156711147060143317175368453031918731001855 (tx type=2 hash=not available (unsigned) nonce=0 value=0 signed=false hf=error maxFeePerGas=undefined maxPriorityFeePerGas=undefined)', | |
| ) | |
| expect(() => createImpersonatedTx({ impersonatedAddress, data, gasLimit: `0x${'ff'.repeat(21)}` })).toThrow( | |
| new GasLimitExceededError(ethjsError.message, { cause: ethjsError }), | |
| ) | |
| }) | |
| it('should throw GasLimitExceededError if gasLimit exceeds MAX_UINT64 or gasLimit * maxFeePerGas exceeds MAX_INTEGER', () => { | |
| const impersonatedAddress = EthjsAddress.fromString(`0x${'42'.repeat(20)}`) | |
| const data = '0x5234' | |
| const ethjsError = new Error( | |
| 'gasLimit cannot exceed MAX_UINT64 (2^64-1), given 374144419156711147060143317175368453031918731001855 (tx type=2 hash=not available (unsigned) nonce=0 value=0 signed=false hf=error maxFeePerGas=undefined maxPriorityFeePerGas=undefined)', | |
| ) | |
| expect(() => createImpersonatedTx({ impersonatedAddress, data, gasLimit: `0x${'ff'.repeat(21)}` })).toThrow( | |
| new GasLimitExceededError(ethjsError.message, { cause: ethjsError }), | |
| ) | |
| }) |
roninjin10
force-pushed
the
06-09-_goal_net_feat_good_error_messages
branch
from
7c1fa8f
to
214f242
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/tx/src/__snapshots__/createImpersonatedTx.spec.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!pnpm-lock.yaml
Files selected for processing (5)
- packages/errors/src/ethereum/BaseError.js (1 hunks)
- packages/errors/src/ethereum/GasLimitExceededError.js (1 hunks)
- packages/tx/package.json (1 hunks)
- packages/tx/src/createImpersonatedTx.js (1 hunks)
- packages/tx/src/createImpersonatedTx.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/errors/src/ethereum/BaseError.js
- packages/errors/src/ethereum/GasLimitExceededError.js
- packages/tx/package.json
- packages/tx/src/createImpersonatedTx.js
- packages/tx/src/createImpersonatedTx.spec.ts
roninjin10
force-pushed
the
06-09-_goal_net_feat_good_error_messages
branch
from
37ab16a
to
63d0a2b
Compare
Description
Concise description of proposed changes
Testing
Explain the quality checks that have been done on the code changes
Additional Information
Your ENS/address:
Summary by CodeRabbit
Bug Fixes
New Features
AccountNotFoundError,ExecutionError,InvalidParamsError, andForkError.Chores
package.jsonfiles to include@tevm/errorsdependency.Refactor
setAccountHandlerby removing redundant checks.