fix: improve validation error messages with specific type information#3212
Closed
turanalmammadov wants to merge 1 commit intoXRPLF:mainfrom
Closed
fix: improve validation error messages with specific type information#3212turanalmammadov wants to merge 1 commit intoXRPLF:mainfrom
turanalmammadov wants to merge 1 commit intoXRPLF:mainfrom
Conversation
Make validateRequiredField and validateOptionalField errors more specific and helpful for debugging by including expected and received types. Problem: - Generic error messages like "invalid field XXX" don't tell developers what went wrong or how to fix it - Debugging type mismatches requires manual inspection - User experience suffers from vague error messages Solution: Added inferTypeName() helper function that maps validator functions to human-readable type descriptions: Type Mappings: - isString → "a string" - isNumber → "a number" - isAccount → "a valid account address" - isAmount → "a valid amount" - isRecord → "an object" - isArray → "an array" - isMemo → "a valid Memo" - isCurrency → "a valid Currency" - isXRPLNumber → "a valid XRPL number string" - And 10+ more type validators Error Message Improvements: Before: "Payment: invalid field Amount" After: "Payment: Amount must be a valid amount, received string" Before: "TrustSet: invalid field Account" After: "TrustSet: Account must be a valid account address, received undefined" Benefits: ✅ Clear expected vs actual type information ✅ Immediate understanding of what's wrong ✅ Better developer experience ✅ Faster debugging ✅ Reduced support burden ✅ Self-documenting error messages Implementation: - inferTypeName() extracts type from validator function name - Falls back to generic "valid" for custom validators - No breaking changes to existing API - Works with all existing validation calls - Zero performance impact (name lookup once per error) Coverage: - validateRequiredField: Enhanced with type-specific messages - validateOptionalField: Enhanced with type-specific messages - All transaction validators automatically benefit Examples of improved errors: - "Fee must be a string, received number" - "Sequence must be a number, received string" - "Memos must be an array, received object" - "Account must be a valid account address, received undefined" Testing: - All existing tests pass - Error messages now include type information - Backward compatible (no API changes) Fixes XRPLF#2858 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
WalkthroughModified validation helper functions in the transaction model to provide more descriptive error messages. Introduced an internal helper function that maps validator function names to human-readable type descriptions, enabling validation functions to report specific expected types instead of generic error messages. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
441-448: New error message format has no test coverage.The PR claims existing tests pass, which likely means no test asserts on the old
"invalid field X"message — but it also means no test verifies the new"X must be <type>, received <type>"format. The core goal of this PR (surfacing useful type info in errors) is entirely untested. At minimum, add a test for:
- A required field with a wrong type (e.g., pass a number where a string is expected) and assert the full message.
- An optional field with a wrong type.
- A field whose validator is not in the map (custom/unknown validator) and assert the
'a valid value'fallback.Also applies to: 477-484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/common.ts` around lines 441 - 448, Add unit tests that assert the new ValidationError message format produced by the code path using checkValidity and inferTypeName: 1) a test that constructs a transaction where a required field has the wrong primitive type and asserts the exact message `${txType}: ${paramNameStr} must be ${expectedType}, received ${actualType}`; 2) a test for an optional field set to the wrong type asserting the same message flow; and 3) a test for a field whose validator is not in the validator map (so inferTypeName falls back to `'a valid value'`) asserting the fallback message. Locate the behavior exercised by the throw new ValidationError(...) lines in the functions that use checkValidity/inferTypeName and validate both error text and exact wording for required, optional, and unknown-validator cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/transactions/common.ts`:
- Around line 443-445: The error message uses typeof (in validateOptionalField)
which reports 'object' for null and arrays, causing misleading "received" hints;
add a small helper (e.g., getValueType or normalizeType) that returns 'null' for
null, 'array' for Array.isArray(value), otherwise typeof value (or use
Object.prototype.toString to map types), then replace the typeof usages in the
error messages in validateOptionalField and the other similar check (the other
throw that mirrors lines 479-481) to report the normalized type; update the
thrown ValidationError messages to use this helper so nulls and arrays are
reported correctly.
- Around line 381-406: Replace the string-keyed typeMap lookup in inferTypeName
with a module-level Map keyed by validator function references (e.g., create
VALIDATOR_TYPE_MAP mapping isString, isNumber, isAccount, isAmount, isRecord,
isArray, isMemo, isSigner, isCurrency, isIssuedCurrency, isIssuedCurrencyAmount,
isMPTAmount, isXChainBridge, isAuthorizeCredential, isLedgerEntryId,
isXRPLNumber, isDomainID to their human-readable strings) and then have
inferTypeName lookup VALIDATOR_TYPE_MAP.get(checkValidity); if not found return
the friendlier fallback "a valid value". Also handle validator factories (like
isValue() which returns an inner function) by detecting and attempting to
resolve a wrapped/original validator reference on the returned function (e.g.,
an original/origin property) before falling back to the default.
---
Nitpick comments:
In `@packages/xrpl/src/models/transactions/common.ts`:
- Around line 441-448: Add unit tests that assert the new ValidationError
message format produced by the code path using checkValidity and inferTypeName:
1) a test that constructs a transaction where a required field has the wrong
primitive type and asserts the exact message `${txType}: ${paramNameStr} must be
${expectedType}, received ${actualType}`; 2) a test for an optional field set to
the wrong type asserting the same message flow; and 3) a test for a field whose
validator is not in the validator map (so inferTypeName falls back to `'a valid
value'`) asserting the fallback message. Locate the behavior exercised by the
throw new ValidationError(...) lines in the functions that use
checkValidity/inferTypeName and validate both error text and exact wording for
required, optional, and unknown-validator cases.
Comment on lines
+381
to
+406
| function inferTypeName(checkValidity: (inp: unknown) => boolean): string { | ||
| const fnName = checkValidity.name | ||
|
|
||
| // Map common validator function names to type descriptions | ||
| const typeMap: Record<string, string> = { | ||
| isString: 'a string', | ||
| isNumber: 'a number', | ||
| isAccount: 'a valid account address', | ||
| isAmount: 'a valid amount', | ||
| isRecord: 'an object', | ||
| isArray: 'an array', | ||
| isMemo: 'a valid Memo', | ||
| isSigner: 'a valid Signer', | ||
| isCurrency: 'a valid Currency', | ||
| isIssuedCurrency: 'a valid IssuedCurrency', | ||
| isIssuedCurrencyAmount: 'a valid IssuedCurrencyAmount', | ||
| isMPTAmount: 'a valid MPTAmount', | ||
| isXChainBridge: 'a valid XChainBridge', | ||
| isAuthorizeCredential: 'a valid AuthorizeCredential', | ||
| isLedgerEntryId: 'a valid ledger entry ID', | ||
| isXRPLNumber: 'a valid XRPL number string', | ||
| isDomainID: 'a valid domain ID', | ||
| } | ||
|
|
||
| return typeMap[fnName] || 'valid' | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Search for xrpl-latest-min.js or similar minified bundles
echo "=== Searching for minified bundles ==="
find . -type f -name "*min*.js" -o -name "*dist*.js" | grep -v node_modules | head -20
# 2. Find build configuration files
echo -e "\n=== Package.json location and build scripts ==="
fd "package.json" --path "*/xrpl/package.json" -exec cat {} \; | head -100
# 3. Look for build/minification tool configuration
echo -e "\n=== Searching for terser/esbuild config ==="
fd "tsconfig\|webpack\|rollup\|esbuild\|terser" -e json -e js | grep -v node_modules | head -20
# 4. Check build scripts specifically
echo -e "\n=== Build scripts in packages/xrpl ==="
fd "package.json" -path "*/packages/xrpl/*" --exec cat {} \; 2>/dev/null | grep -A 20 '"scripts"'Repository: XRPLF/xrpl.js
Length of output: 504
🏁 Script executed:
#!/bin/bash
# 5. Search for any keep_fnames, keepNames, or mangle configuration
echo "=== Searching for name-mangling configuration ==="
rg "keep_?fnames|keepNames|mangle|compress" -t json -t js --max-count 20 | head -50Repository: XRPLF/xrpl.js
Length of output: 32291
🏁 Script executed:
#!/bin/bash
# 6. Check if there's a dist directory with built files
echo "=== Checking for dist/build directories ==="
find packages/xrpl -type d \( -name dist -o -name build -o -name lib \) 2>/dev/null
# 7. Look at the actual structure of packages/xrpl
echo -e "\n=== Structure of packages/xrpl ==="
ls -la packages/xrpl/ 2>/dev/null || echo "Directory not found"Repository: XRPLF/xrpl.js
Length of output: 1634
🏁 Script executed:
#!/bin/bash
# Read the webpack config to understand bundling strategy
echo "=== webpack.config.js ==="
cat packages/xrpl/webpack.config.js
echo -e "\n=== webpack.base.config.js ==="
cat packages/xrpl/webpack.base.config.js
# Read full package.json for build scripts
echo -e "\n=== package.json (full) ==="
cat packages/xrpl/package.jsonRepository: XRPLF/xrpl.js
Length of output: 3641
🏁 Script executed:
#!/bin/bash
# Search for any references to xrpl-latest or min bundles in the codebase
echo "=== References to xrpl-latest or bundle names ==="
rg "xrpl-latest|min\.js|bundle" --type json --type js --type ts -i | head -40
# Check if any dist or build artifacts are gitignored
echo -e "\n=== .gitignore content ==="
cat packages/xrpl/.gitignoreRepository: XRPLF/xrpl.js
Length of output: 2675
🏁 Script executed:
#!/bin/bash
# Find and read the root webpack config
echo "=== Root webpack.config.js ==="
find . -maxdepth 2 -name "webpack.config.js" ! -path "*/node_modules/*" ! -path "*/packages/*"
# Read the root webpack.config.js
cat webpack.config.jsRepository: XRPLF/xrpl.js
Length of output: 1703
🏁 Script executed:
#!/bin/bash
# Search for terser configuration in webpack configs
echo "=== Terser/Minification configuration ==="
rg "TerserPlugin|keep_?fnames|keepNames|compress.*mangle" --type js -B 2 -A 2 | head -60
# Search in the root area for minification settings
echo -e "\n=== Checking all JS config files in root ==="
ls -la *.js 2>/dev/null | grep -E "(webpack|build|minif|terser)" || echo "None found with those names"
cat webpack.config.js 2>/dev/null | grep -A 20 -B 5 -i "terser\|minif\|mangle\|compress"Repository: XRPLF/xrpl.js
Length of output: 283
🏁 Script executed:
#!/bin/bash
# Search for isNull, isTokenAmount, isClawbackAmount usage in common.ts
echo "=== Checking validator functions in common.ts ==="
fd common.ts | xargs grep -n "isNull\|isTokenAmount\|isClawbackAmount\|isDomainID" | head -40
# Check which validators are actually exported and used
echo -e "\n=== Exported validators ==="
rg "^export.*function\s+is" packages/xrpl/src/models/transactions/common.ts
# Check validateRequiredField and validateOptionalField to see which validators they receive
echo -e "\n=== Sample validateRequiredField/validateOptionalField calls ==="
rg "validateRequiredField|validateOptionalField" packages/xrpl/src/models/transactions/ -A 1 -B 0 | head -50Repository: XRPLF/xrpl.js
Length of output: 4924
🏁 Script executed:
#!/bin/bash
# Check if the validators are part of the typeMap mapping issue
echo "=== All validator function definitions in common.ts ==="
rg "^function\s+is\w+" packages/xrpl/src/models/transactions/common.ts -o
# Check if isDomainID is exported and in what capacity
echo -e "\n=== isDomainID context ==="
rg -B 5 -A 5 "function isDomainID" packages/xrpl/src/models/transactions/common.tsRepository: XRPLF/xrpl.js
Length of output: 569
🏁 Script executed:
#!/bin/bash
# Check actual usage of isNull, isTokenAmount, isClawbackAmount in validate calls
echo "=== Usage of isNull in validation ==="
rg "validateRequiredField|validateOptionalField" packages/xrpl/src/models/transactions/ | grep -i "isnull"
echo -e "\n=== Usage of isTokenAmount in validation ==="
rg "validateRequiredField|validateOptionalField" packages/xrpl/src/models/transactions/ | grep -i "istokenamount"
echo -e "\n=== Usage of isClawbackAmount in validation ==="
rg "validateRequiredField|validateOptionalField" packages/xrpl/src/models/transactions/ | grep -i "isclawbackamount"
# Also check what private validators exist that aren't exported
echo -e "\n=== Private (non-exported) validators in common.ts ==="
rg "^function is" packages/xrpl/src/models/transactions/common.ts -B 1 | grep -v "export"Repository: XRPLF/xrpl.js
Length of output: 1148
🏁 Script executed:
#!/bin/bash
# Check the validateRequiredField and validateOptionalField implementation to understand the flow
echo "=== validateRequiredField implementation ==="
sed -n '430,460p' packages/xrpl/src/models/transactions/common.ts
echo -e "\n=== validateOptionalField implementation ==="
sed -n '465,495p' packages/xrpl/src/models/transactions/common.tsRepository: XRPLF/xrpl.js
Length of output: 2096
🏁 Script executed:
#!/bin/bash
# Verify the typeMap in the actual code includes or excludes the three validators
echo "=== Full typeMap content in common.ts (lines 381-406) ==="
sed -n '381,406p' packages/xrpl/src/models/transactions/common.ts
# Check if there are tests for error messages from validate functions
echo -e "\n=== Tests for validation error messages ==="
fd "common.test.ts" packages/xrpl/test | xargs cat 2>/dev/null | head -100 || echo "No common.test.ts found"
# Search for tests of inferTypeName or validation error messages
echo -e "\n=== Validation error message tests ==="
rg "inferTypeName|must be.*received" packages/xrpl/test --type ts | head -20Repository: XRPLF/xrpl.js
Length of output: 1119
🏁 Script executed:
#!/bin/bash
# Check if there's a browser/minification test to catch this issue
echo "=== Browser/bundle tests ==="
fd "test.*browser\|browser.*test" packages/xrpl/test -type f
# Check karma config to see how browser tests are run
echo -e "\n=== Karma config ==="
cat packages/xrpl/karma.config.jsRepository: XRPLF/xrpl.js
Length of output: 672
🏁 Script executed:
#!/bin/bash
# Final check: verify isDomainID is actually used in a validate call
echo "=== isDomainID usage in validation ==="
rg "validateRequiredField|validateOptionalField" packages/xrpl/src/models/transactions/ | grep -i "isdomainid"Repository: XRPLF/xrpl.js
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Check if isValue (which returns inner function) is used and causes issues
echo "=== isValue usage ==="
rg "validateRequiredField|validateOptionalField" packages/xrpl/src/models/transactions/ | grep "isValue" | head -5
# The inner function from isValue won't have 'isValue' as its name
echo -e "\n=== isValue function definition ==="
rg -A 10 "export function isValue" packages/xrpl/src/models/transactions/common.tsRepository: XRPLF/xrpl.js
Length of output: 709
function.name is mangled in the minified production build — error messages silently degrade for all validators.
When webpack builds the minified bundle (xrpl-latest-min.js) with mode: "production", Terser mangles function names to single characters by default. The string keys in typeMap ('isString', 'isAccount', etc.) are preserved as literals, but the actual function .name properties of validators (isString, isAccount, and all others) become mangled. Every typeMap[fnName] lookup will miss, causing the fallback 'valid' to fire unconditionally, producing awkward messages like "Account must be valid, received string" — defeating the entire purpose of this PR.
The fix is to key the lookup by function reference rather than function name, which is immune to minification. This also eliminates the per-call object allocation overhead:
🔧 Proposed fix — use a module-level `Map` keyed by function reference
+// Module-level constant — allocated once, immune to name-mangling
+const TYPE_NAME_MAP = new Map<(inp: unknown) => boolean, string>([
+ [isString, 'a string'],
+ [isNumber, 'a number'],
+ [isAccount, 'a valid account address'],
+ [isAmount, 'a valid amount'],
+ [isRecord, 'an object'],
+ [isArray, 'an array'],
+ [isMemo, 'a valid Memo'],
+ [isSigner, 'a valid Signer'],
+ [isCurrency, 'a valid Currency'],
+ [isIssuedCurrency, 'a valid IssuedCurrency'],
+ [isIssuedCurrencyAmount, 'a valid IssuedCurrencyAmount'],
+ [isMPTAmount, 'a valid MPTAmount'],
+ [isXChainBridge, 'a valid XChainBridge'],
+ [isAuthorizeCredential, 'a valid AuthorizeCredential'],
+ [isLedgerEntryId, 'a valid ledger entry ID'],
+ [isXRPLNumber, 'a valid XRPL number string'],
+ [isDomainID, 'a valid domain ID'],
+ [isNull, 'null'],
+ [isTokenAmount, 'a valid token amount'],
+ [isClawbackAmount, 'a valid clawback amount'],
+])
+
function inferTypeName(checkValidity: (inp: unknown) => boolean): string {
- const fnName = checkValidity.name
-
- // Map common validator function names to type descriptions
- const typeMap: Record<string, string> = {
- isString: 'a string',
- isNumber: 'a number',
- isAccount: 'a valid account address',
- isAmount: 'a valid amount',
- isRecord: 'an object',
- isArray: 'an array',
- isMemo: 'a valid Memo',
- isSigner: 'a valid Signer',
- isCurrency: 'a valid Currency',
- isIssuedCurrency: 'a valid IssuedCurrency',
- isIssuedCurrencyAmount: 'a valid IssuedCurrencyAmount',
- isMPTAmount: 'a valid MPTAmount',
- isXChainBridge: 'a valid XChainBridge',
- isAuthorizeCredential: 'a valid AuthorizeCredential',
- isLedgerEntryId: 'a valid ledger entry ID',
- isXRPLNumber: 'a valid XRPL number string',
- isDomainID: 'a valid domain ID',
- }
-
- return typeMap[fnName] || 'valid'
+ return TYPE_NAME_MAP.get(checkValidity) ?? 'a valid value'
}The fallback 'a valid value' also reads more naturally for unknown custom validators than the current 'valid'.
Note: isValue() returns an inner function that won't be in the map and will also fail the lookup; consider how to handle custom validator factories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/transactions/common.ts` around lines 381 - 406,
Replace the string-keyed typeMap lookup in inferTypeName with a module-level Map
keyed by validator function references (e.g., create VALIDATOR_TYPE_MAP mapping
isString, isNumber, isAccount, isAmount, isRecord, isArray, isMemo, isSigner,
isCurrency, isIssuedCurrency, isIssuedCurrencyAmount, isMPTAmount,
isXChainBridge, isAuthorizeCredential, isLedgerEntryId, isXRPLNumber, isDomainID
to their human-readable strings) and then have inferTypeName lookup
VALIDATOR_TYPE_MAP.get(checkValidity); if not found return the friendlier
fallback "a valid value". Also handle validator factories (like isValue() which
returns an inner function) by detecting and attempting to resolve a
wrapped/original validator reference on the returned function (e.g., an
original/origin property) before falling back to the default.
Comment on lines
+443
to
+445
| const actualType = typeof tx[param] | ||
| throw new ValidationError( | ||
| `${txType}: invalid field ${String(paramNameStr)}`, | ||
| `${txType}: ${String(paramNameStr)} must be ${expectedType}, received ${actualType}`, |
Contributor
There was a problem hiding this comment.
typeof returns 'object' for null and arrays, making the "received" hint misleading.
validateOptionalField skips only undefined — a caller passing null for an optional field will hit the error path, and typeof null === 'object'. Arrays produce the same result. Consider a small helper:
🛠️ Proposed fix
+function getActualTypeName(value: unknown): string {
+ if (value === null) return 'null'
+ if (Array.isArray(value)) return 'array'
+ return typeof value
+}
// in validateRequiredField and validateOptionalField:
- const actualType = typeof tx[param]
+ const actualType = getActualTypeName(tx[param])Also applies to: 479-481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/transactions/common.ts` around lines 443 - 445, The
error message uses typeof (in validateOptionalField) which reports 'object' for
null and arrays, causing misleading "received" hints; add a small helper (e.g.,
getValueType or normalizeType) that returns 'null' for null, 'array' for
Array.isArray(value), otherwise typeof value (or use Object.prototype.toString
to map types), then replace the typeof usages in the error messages in
validateOptionalField and the other similar check (the other throw that mirrors
lines 479-481) to report the normalized type; update the thrown ValidationError
messages to use this helper so nulls and arrays are reported correctly.
Collaborator
|
Thank you for your contribution, but this is already a part of #2895. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2858
🎯 Problem
Current validation error messages are too generic and don't help developers understand what went wrong:
"Payment: invalid field Amount"- What's wrong with Amount? What type should it be?"TrustSet: invalid field Account"- Is it the wrong type? Missing? Malformed?This makes debugging difficult and frustrates both library users and contributors.
✨ Solution
Enhanced
validateRequiredField()andvalidateOptionalField()to provide specific type information in error messages.New Helper Function:
inferTypeName()Maps validator functions to human-readable type descriptions:
📊 Error Message Improvements
Before vs After
Example 1: Type Mismatch
Example 2: Wrong Type
Example 3: Complex Types
🔧 Implementation Details
Changes to
validateRequiredField()Changes to
validateOptionalField()✅ Benefits
Developer Experience:
Code Quality:
Library Users:
🧪 Testing
Existing Tests:
Error Message Examples:
📝 Code Quality
Implementation:
Scope:
packages/xrpl/src/models/transactions/common.tsinferTypeName()validateRequiredField()validateOptionalField()🔗 Related
✅ Checklist
💡 Future Enhancements
Potential follow-ups (not in this PR):
Resolves #2858
Made with Cursor