Fix issue #2858: Improve error messages in field validation functions #2961
Conversation
WalkthroughThis pull request modifies the validation behavior in the XRPL transaction model. The changes introduce an additional optional parameter, Changes
Possibly related issues
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/xrpl/test/models/transactions/common.test.ts (2)
1-2: Reorder imports as per project convention.The static analysis is flagging that imports should be ordered alphabetically. Following the project's import ordering convention improves consistency across the codebase.
-import { validateRequiredField, validateOptionalField } from '../../../src/models/transactions/common' -import { ValidationError } from '../../../src/errors' +import { ValidationError } from '../../../src/errors' +import { validateRequiredField, validateOptionalField } from '../../../src/models/transactions/common'🧰 Tools
🪛 ESLint
[error] 1-1: Replace
·validateRequiredField,·validateOptionalField·with⏎··validateRequiredField,⏎··validateOptionalField,⏎(prettier/prettier)
[error] 2-2:
../../../src/errorsimport should occur before import of../../../src/models/transactions/common(import/order)
13-14: Format multi-line function calls for better readability.The static analysis tool is suggesting to break the function call into multiple lines for better readability.
- expect(() => - validateRequiredField(txMock, 'amount', (val) => typeof val === 'string', 'string') - ).toThrow(new ValidationError('Payment: invalid field amount: expected string, received number')) + expect(() => + validateRequiredField( + txMock, + 'amount', + (val) => typeof val === 'string', + 'string', + ), + ).toThrow( + new ValidationError( + 'Payment: invalid field amount: expected string, received number', + ), + )🧰 Tools
🪛 ESLint
[error] 13-13: Replace
txMock,·'amount',·(val)·=>·typeof·val·===·'string',·'string')with⏎········txMock,⏎········'amount',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),(prettier/prettier)
[error] 14-14: Replace
new·ValidationError('Payment:·invalid·field·amount:·expected·string,·received·number')with⏎······new·ValidationError(⏎········'Payment:·invalid·field·amount:·expected·string,·received·number',⏎······),⏎····(prettier/prettier)
packages/xrpl/src/models/transactions/common.ts (2)
237-237: Fix line formatting for better readability.The static analysis tool is suggesting to break the line for better readability.
- `${tx.TransactionType}: invalid field ${paramName}${expectedType ? `: expected ${expectedType}, received ${actualType}` : ''}`, + `${tx.TransactionType}: invalid field ${paramName}${ + expectedType ? `: expected ${expectedType}, received ${actualType}` : '' + }`,🧰 Tools
🪛 ESLint
[error] 237-237: Replace
expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''with⏎········expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''⏎······(prettier/prettier)
260-260: Fix line formatting to match the other validation function.For consistency with the similar fix in validateRequiredField.
- `${tx.TransactionType}: invalid field ${paramName}${expectedType ? `: expected ${expectedType}, received ${actualType}` : ''}`, + `${tx.TransactionType}: invalid field ${paramName}${ + expectedType ? `: expected ${expectedType}, received ${actualType}` : '' + }`,🧰 Tools
🪛 ESLint
[error] 260-260: Replace
expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''with⏎········expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''⏎······(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/common.ts(5 hunks)packages/xrpl/test/models/transactions/common.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/xrpl/test/models/transactions/common.test.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:0-0
Timestamp: 2025-04-07T16:40:26.679Z
Learning: Test libraries like chai should not be used in source code. Use existing validation functions where available, or implement custom validation using ValidationError for runtime checks.
packages/xrpl/src/models/transactions/common.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2025-04-07T16:40:26.680Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
🪛 ESLint
packages/xrpl/test/models/transactions/common.test.ts
[error] 1-1: Replace ·validateRequiredField,·validateOptionalField· with ⏎··validateRequiredField,⏎··validateOptionalField,⏎
(prettier/prettier)
[error] 2-2: ../../../src/errors import should occur before import of ../../../src/models/transactions/common
(import/order)
[error] 13-13: Replace txMock,·'amount',·(val)·=>·typeof·val·===·'string',·'string') with ⏎········txMock,⏎········'amount',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),
(prettier/prettier)
[error] 14-14: Replace new·ValidationError('Payment:·invalid·field·amount:·expected·string,·received·number') with ⏎······new·ValidationError(⏎········'Payment:·invalid·field·amount:·expected·string,·received·number',⏎······),⏎····
(prettier/prettier)
[error] 19-19: Replace txMock,·'account',·(val)·=>·typeof·val·===·'string',·'string') with ⏎········txMock,⏎········'account',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),
(prettier/prettier)
[error] 25-25: Insert ,
(prettier/prettier)
[error] 31-31: Replace txMock,·'nonExistentField',·(val)·=>·typeof·val·===·'string') with ⏎········txMock,⏎········'nonExistentField',⏎········(val)·=>·typeof·val·===·'string',⏎······),
(prettier/prettier)
[error] 48-48: Replace txNoMemo,·'memo',·(val)·=>·typeof·val·===·'string',·'string') with ⏎········txNoMemo,⏎········'memo',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),
(prettier/prettier)
[error] 54-54: Replace txMock,·'memo',·(val)·=>·typeof·val·===·'string',·'string') with ⏎········txMock,⏎········'memo',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),
(prettier/prettier)
[error] 55-55: Replace new·ValidationError('Payment:·invalid·field·memo:·expected·string,·received·number') with ⏎······new·ValidationError(⏎········'Payment:·invalid·field·memo:·expected·string,·received·number',⏎······),⏎····
(prettier/prettier)
packages/xrpl/src/models/transactions/common.ts
[error] 237-237: Replace expectedType·?·:·expected·${expectedType},·received·${actualType}·:·'' with ⏎········expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''⏎······
(prettier/prettier)
[error] 260-260: Replace expectedType·?·:·expected·${expectedType},·received·${actualType}·:·'' with ⏎········expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''⏎······
(prettier/prettier)
🔇 Additional comments (6)
packages/xrpl/test/models/transactions/common.test.ts (2)
4-34: Great test coverage for validateRequiredField!The tests thoroughly cover all important scenarios for the enhanced error messages:
- When validation fails with expected and received types
- When validation succeeds
- When expectedType is not provided
- When a field is missing entirely
This ensures the validation function behaves correctly across different use cases.
🧰 Tools
🪛 ESLint
[error] 13-13: Replace
txMock,·'amount',·(val)·=>·typeof·val·===·'string',·'string')with⏎········txMock,⏎········'amount',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),(prettier/prettier)
[error] 14-14: Replace
new·ValidationError('Payment:·invalid·field·amount:·expected·string,·received·number')with⏎······new·ValidationError(⏎········'Payment:·invalid·field·amount:·expected·string,·received·number',⏎······),⏎····(prettier/prettier)
[error] 19-19: Replace
txMock,·'account',·(val)·=>·typeof·val·===·'string',·'string')with⏎········txMock,⏎········'account',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),(prettier/prettier)
[error] 25-25: Insert
,(prettier/prettier)
[error] 31-31: Replace
txMock,·'nonExistentField',·(val)·=>·typeof·val·===·'string')with⏎········txMock,⏎········'nonExistentField',⏎········(val)·=>·typeof·val·===·'string',⏎······),(prettier/prettier)
36-57: Good test coverage for validateOptionalField.The tests verify both core behaviors of the optional field validation:
- Skipping validation when the field is undefined
- Performing validation when the field is present but invalid
This ensures the enhanced error message functionality works correctly for optional fields too.
🧰 Tools
🪛 ESLint
[error] 48-48: Replace
txNoMemo,·'memo',·(val)·=>·typeof·val·===·'string',·'string')with⏎········txNoMemo,⏎········'memo',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),(prettier/prettier)
[error] 54-54: Replace
txMock,·'memo',·(val)·=>·typeof·val·===·'string',·'string')with⏎········txMock,⏎········'memo',⏎········(val)·=>·typeof·val·===·'string',⏎········'string',⏎······),(prettier/prettier)
[error] 55-55: Replace
new·ValidationError('Payment:·invalid·field·memo:·expected·string,·received·number')with⏎······new·ValidationError(⏎········'Payment:·invalid·field·memo:·expected·string,·received·number',⏎······),⏎····(prettier/prettier)
packages/xrpl/src/models/transactions/common.ts (4)
219-226: Excellent addition of expectedType parameter!The optional
expectedTypeparameter is a valuable enhancement that will significantly improve the developer experience by providing more informative error messages. The JSDoc comments have been appropriately updated to document this new parameter.
234-238: Great enhancement to error messages!This change significantly improves the error messages by including both the expected and actual types when validation fails. The code correctly handles the special case for null values and properly formats the message based on whether expectedType is provided.
🧰 Tools
🪛 ESLint
[error] 237-237: Replace
expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''with⏎········expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''⏎······(prettier/prettier)
257-261: Consistent implementation in validateOptionalField!The error message enhancement is consistently implemented in both validation functions, which maintains code symmetry and ensures users get the same helpful error format regardless of which validation method is used.
🧰 Tools
🪛 ESLint
[error] 260-260: Replace
expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''with⏎········expectedType·?·:·expected·${expectedType},·received·${actualType}·:·''⏎······(prettier/prettier)
365-399: Great update to validateBaseTransaction!The function calls to
validateRequiredFieldandvalidateOptionalFieldhave been consistently updated to include the type information, which ensures users get helpful error messages for all validation failures in the base transaction fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/src/models/transactions/common.ts (2)
220-224: Consider adding examples to the documentationWhile the parameter documentation is clear, it might be helpful to add examples of the error messages with and without the expectedType parameter to help developers understand how to use these functions effectively.
* @param checkValidity - The function to use to check the type. * @param expectedType - Optional. The expected type for more specific error messages. + * Example: With expectedType='string': "invalid field Account: expected string, received number" + * Example: Without expectedType: "invalid field Account" * @throws When the field is missing or invalid.Also applies to: 251-255
224-229: Consider leveraging existing domain knowledge for expected typeThe current implementation requires developers to manually specify the expected type as a string, which could lead to inconsistencies if the string doesn't match the actual type expected by the validation function.
You could consider a more type-safe approach by deriving the expected type from the checkValidity function's parameter type or return type constraints, potentially using TypeScript's type system to ensure consistency.
Also applies to: 255-260
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/src/models/transactions/common.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/xrpl/src/models/transactions/common.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2025-04-09T22:09:21.852Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
🔇 Additional comments (4)
packages/xrpl/src/models/transactions/common.ts (4)
213-214: Well-documented ESLint exceptions for improved error messagesThe ESLint disable/enable comments are clearly documented with rationale for both the max-params exception (to allow the new parameter) and the template-expressions exception (for including type information in errors).
Also applies to: 270-271
221-222: Good improvement to error reporting in validateRequiredFieldThe addition of the optional
expectedTypeparameter and enhanced error message formatting will make debugging validation issues much easier for developers. The implementation correctly handles null values separately from undefined and captures the actual type of the invalid field.Also applies to: 224-229, 236-242
252-253: Good improvement to error reporting in validateOptionalFieldSimilar to the changes in validateRequiredField, this update adds better type reporting to optional field validation errors. The implementation is consistent with the required field validation function and maintains backward compatibility by making the parameter optional.
Also applies to: 255-260, 261-267
373-407: Comprehensive update to all validation callsAll calls to the validation functions within validateBaseTransaction have been properly updated to include the expected type information, ensuring consistent error reporting throughout the validation process.
|
I prefer the solution in #2895, since that doesn't involve as much duplication in the function when writing it. |
High Level Overview of Change
Context of Change
The validation functions in the XRPL.js library previously provided generic error messages when field validation failed. This PR improves these error messages by including both the expected type and the actual type received, making it easier for developers to debug issues with their transactions.
The changes specifically enhance the
validateRequiredFieldandvalidateOptionalFieldfunctions in the transaction validation system to provide more descriptive error messages, and adds comprehensive test coverage for these improved messages.Type of Change
Did you update HISTORY.md?
Test Plan
The changes have been verified by:
packages/xrpl/test/models/transactions/common.test.tsthat verify the improved error messagesThe tests specifically verify: