refactor: improve typing of validateRequiredField, validateOptionalField, and DeliverMax
#2966
Conversation
WalkthroughThis update refines type safety and validation logic across transaction models, particularly focusing on runtime type guards, validation helpers, and Payment transaction handling. Several utility functions are added or improved, and function signatures are updated for better type inference. Validation logic for various transaction types is made more robust and explicit, especially for issued currencies and arrays. Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn EBADENGINE Unsupported engine { 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
There was a problem hiding this comment.
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/xrpl/src/client/index.ts:692
- Ensure that the new handling of the 'DeliverMax' property (assigning its value to 'tx.Amount' and later deleting it) correctly reflects the intended behavior under all Payment transaction scenarios and aligns with the updated type validations.
if (tx.TransactionType === 'Payment' && tx.DeliverMax != null) {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/xrpl/src/client/index.ts (1)
69-70: Code style improvement in imports orderingThe import for
calculateFeePerTransactionTypehas been reordered within the destructured import. This is a minor style improvement that doesn't affect functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/xrpl/src/client/index.ts(2 hunks)packages/xrpl/src/client/partialPayment.ts(1 hunks)packages/xrpl/src/models/transactions/AMMBid.ts(3 hunks)packages/xrpl/src/models/transactions/NFTokenCancelOffer.ts(2 hunks)packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts(1 hunks)packages/xrpl/src/models/transactions/XChainAddClaimAttestation.ts(1 hunks)packages/xrpl/src/models/transactions/XChainClaim.ts(1 hunks)packages/xrpl/src/models/transactions/XChainCommit.ts(1 hunks)packages/xrpl/src/models/transactions/accountDelete.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(8 hunks)packages/xrpl/src/models/transactions/depositPreauth.ts(1 hunks)packages/xrpl/src/models/transactions/escrowFinish.ts(1 hunks)packages/xrpl/src/models/transactions/oracleSet.ts(2 hunks)packages/xrpl/src/models/transactions/payment.ts(4 hunks)packages/xrpl/src/models/transactions/paymentChannelClaim.ts(1 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.ts(2 hunks)packages/xrpl/src/models/transactions/signerListSet.ts(2 hunks)packages/xrpl/src/sugar/autofill.ts(2 hunks)packages/xrpl/test/client/autofill.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/xrpl/test/client/autofill.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (13)
packages/xrpl/src/models/transactions/signerListSet.ts (1)
packages/xrpl/src/models/transactions/common.ts (1)
isArray(225-227)
packages/xrpl/src/client/partialPayment.ts (1)
packages/xrpl/src/models/common/index.ts (2)
Amount(28-28)MPTAmount(23-26)
packages/xrpl/src/models/transactions/depositPreauth.ts (1)
packages/xrpl/src/models/transactions/common.ts (2)
MAX_AUTHORIZED_CREDENTIALS(20-20)validateCredentialsList(491-533)
packages/xrpl/src/models/transactions/XChainClaim.ts (1)
packages/xrpl/src/models/transactions/common.ts (2)
isNumber(98-100)isString(88-90)
packages/xrpl/src/models/transactions/AMMBid.ts (1)
packages/xrpl/src/models/transactions/common.ts (1)
isArray(225-227)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (1)
packages/xrpl/src/models/transactions/common.ts (3)
validateRequiredField(239-259)isArray(225-227)validateCredentialsList(491-533)
packages/xrpl/src/models/transactions/XChainAddClaimAttestation.ts (1)
packages/xrpl/src/models/transactions/common.ts (4)
validateRequiredField(239-259)isXChainBridge(208-217)isNumber(98-100)isString(88-90)
packages/xrpl/src/models/transactions/XChainCommit.ts (1)
packages/xrpl/src/models/transactions/common.ts (2)
isNumber(98-100)isString(88-90)
packages/xrpl/src/models/transactions/NFTokenCancelOffer.ts (1)
packages/xrpl/src/models/transactions/common.ts (1)
isArray(225-227)
packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts (1)
packages/xrpl/src/models/transactions/common.ts (3)
validateRequiredField(239-259)isNumber(98-100)isString(88-90)
packages/xrpl/src/models/transactions/payment.ts (2)
packages/xrpl/src/models/common/index.ts (1)
Path(68-68)packages/xrpl/src/models/transactions/common.ts (1)
isArray(225-227)
packages/xrpl/src/models/transactions/oracleSet.ts (4)
packages/xrpl/src/models/transactions/common.ts (4)
validateRequiredField(239-259)isArray(225-227)isRecord(78-80)isNumber(98-100)packages/xrpl/src/models/common/index.ts (1)
PriceData(186-213)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/models/utils/index.ts (1)
isHex(36-38)
packages/xrpl/src/sugar/autofill.ts (3)
packages/xrpl/src/models/transactions/transaction.ts (1)
Transaction(193-193)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/utils/index.ts (2)
isValidXAddress(212-212)xAddressToClassicAddress(211-211)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: integration (20.x)
🔇 Additional comments (32)
packages/xrpl/src/models/transactions/signerListSet.ts (2)
4-4: Good improvement to type safety.Importing the
isArrayutility function from the common module enhances type safety through the generic type parameter.
59-59: Better type narrowing with type guard.Replacing
Array.isArraywith the importedisArraytype guard function improves TypeScript type narrowing. The custom type guard will properly narrowtx.SignerEntriesto an array type, providing better type safety and IDE support.packages/xrpl/src/models/transactions/escrowFinish.ts (1)
56-56: Removed redundant type assertion.Removing the
as stringtype assertion is a good improvement. Sincetx.TransactionTypeis already defined as a string literal type in the interface, TypeScript can correctly infer its type without the explicit assertion.packages/xrpl/src/models/transactions/depositPreauth.ts (2)
73-73: Removed unnecessary type assertion.Good cleanup of the redundant
as stringtype assertion. Thetx.TransactionTypeis already a string literal type in the interface, so TypeScript's type inference can handle this correctly.
80-80: Consistent type safety improvement.Same improvement as above, removing the unnecessary type assertion. This maintains consistency across the codebase and allows TypeScript to properly infer types.
packages/xrpl/src/models/transactions/accountDelete.ts (1)
55-55: Cleaner code with improved type safety.Properly removing the redundant
as stringtype assertion. This change follows the same pattern as in other files, making the codebase more consistent in its approach to type checking while maintaining type safety.packages/xrpl/src/models/transactions/XChainCommit.ts (1)
67-71: Good addition of explicit type predicateThe addition of the type predicate
(inp: unknown): inp is number | stringimproves type safety by explicitly telling TypeScript that the function is a type guard. This ensures that after validation, TypeScript knows theXChainClaimIDfield is definitely of typenumber | string, which enables better type checking and autocompletion in downstream code.packages/xrpl/src/models/transactions/NFTokenCancelOffer.ts (2)
3-3: Appropriate import of type guard utilityGood addition of the
isArrayutility import to standardize type checking across the codebase.
44-44: Improved type safety with isArray utilityReplacing the native
Array.isArraywith the importedisArrayutility function enhances type safety. The imported function serves as a proper TypeScript type guard that informs the compiler about the array type, improving type inference in the validation function.packages/xrpl/src/models/transactions/paymentChannelClaim.ts (1)
152-157: Simplified type handlingGood removal of the unnecessary
as stringtype assertion. This simplifies the code while maintaining type safety, suggesting that thevalidateCredentialsListfunction now properly handles the type oftx.TransactionTypewithout requiring an explicit assertion.packages/xrpl/src/client/partialPayment.ts (1)
99-102: Improved type assertion clarityGood refactoring of multiple ESLint/TypeScript directive comments into a single, clear comment with an explicit type assertion. This makes the code more maintainable while still ensuring proper type handling for the
DeliverMaxfield.packages/xrpl/src/models/transactions/XChainClaim.ts (1)
71-71: Improved type safety with explicit type predicateThe addition of an explicit TypeScript type predicate
(inp: unknown): inp is number | stringstrengthens type safety by ensuring that TypeScript correctly narrows the type ofXChainClaimIDafter validation. This change aligns with the field's declaration on line 35 and allows TypeScript to understand that after passing validation, the field is guaranteed to be either a number or string.packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts (2)
113-113: Added explicit type predicate for WasLockingChainSend validationThe addition of an explicit TypeScript type predicate
(inp: unknown): inp is 0 | 1enhances type safety by ensuring that TypeScript correctly narrows theWasLockingChainSendfield to the literal union type0 | 1after validation. This matches the field's declaration on line 70 and improves the developer experience without changing runtime behavior.
119-119: Added explicit type predicate for XChainAccountCreateCount validationThe addition of an explicit TypeScript type predicate
(inp: unknown): inp is number | stringenhances type safety by ensuring that TypeScript correctly narrows theXChainAccountCreateCountfield to the union typenumber | stringafter validation. This matches the field's declaration on line 75 and improves the developer experience without changing runtime behavior.packages/xrpl/src/models/transactions/AMMBid.ts (3)
7-7: Added isArray import for consistent type-checkingThe import of
isArrayfrom the common module provides a more TypeScript-friendly alternative to the nativeArray.isArrayfunction. This utility function likely includes proper generic typing, enhancing the type safety of array validations.
90-90: Switched to isArray utility for better type inferenceReplacing
Array.isArraywith the customisArrayutility function improves type inference and maintains consistency with the validation approach used throughout the codebase. This is a good practice that enhances code maintainability.
101-101: Removed unnecessary type assertionThe removal of the
as stringtype assertion fromtx.Accountsimplifies the code while maintaining type safety. This change relies on TypeScript's ability to correctly infer the type, making the code cleaner without sacrificing type checking.packages/xrpl/src/models/transactions/XChainAddClaimAttestation.ts (2)
107-107: Added explicit type predicate for WasLockingChainSend validationThe addition of an explicit TypeScript type predicate
(inp: unknown): inp is 0 | 1enhances type safety by ensuring that TypeScript correctly narrows theWasLockingChainSendfield to the literal union type0 | 1after validation. This matches the field's declaration on line 65 and improves the developer experience without changing runtime behavior.
115-115: Added explicit type predicate for XChainClaimID validationThe addition of an explicit TypeScript type predicate
(inp: unknown): inp is number | stringenhances type safety by ensuring that TypeScript correctly narrows theXChainClaimIDfield to the union typenumber | stringafter validation. This matches the field's declaration on line 76 and improves the developer experience without changing runtime behavior.packages/xrpl/src/client/index.ts (1)
697-699: Type safety improvement: Removed TypeScript expect-error annotationsThe removal of TypeScript ESLint directive comments and
@ts-expect-errorannotations suggests the code no longer needs type suppression for theDeliverMaxproperty. This is a positive improvement that maintains the same logic while making the code cleaner and more type-safe.packages/xrpl/src/models/transactions/permissionedDomainSet.ts (3)
10-11: Type safety improvement: Added isArray import for consistent type checkingImporting the
isArrayutility function from the common module standardizes array type checking across the codebase. This is part of a broader effort to improve type safety through consistent runtime type guards.
40-41: Type safety improvement: Using type guard function for array validationReplaced direct array check with the
isArraytype guard function. This improves type safety by ensuring the value is properly narrowed to an array type after validation.
44-45: Code improvement: Removed unnecessary type assertionRemoved the redundant
as stringtype assertion when passingtx.TransactionTypetovalidateCredentialsList. The TypeScript compiler can now properly infer the type without manual assertion.packages/xrpl/src/models/transactions/payment.ts (5)
17-18: Type safety improvement: Added isArray import for consistent type checkingImporting the
isArrayutility function from the common module standardizes array type checking across the codebase, improving type safety.
190-191: Code improvement: Removed unnecessary type assertionRemoved the redundant
as stringtype assertion when passingtx.TransactionTypetovalidateCredentialsList, allowing TypeScript to properly infer the type.
267-270: Type safety improvement: Enhanced type predicate in isPath functionThe
isPathfunction now acceptsunknowninput instead of assuming an array type, and correctly implements a type predicate (path is Path). This improves type safety by properly narrowing the type after validation.
279-281: Type safety improvement: Using isArray type guard in isPaths functionReplaced direct
Array.isArray()check with theisArraytype guard function for consistent type checking across the codebase. This improves type safety by ensuring proper type narrowing.
285-286: Type safety improvement: Using isArray for consistent type checkingReplaced another direct array check with the
isArraytype guard function, maintaining consistency in type checking throughout the codebase.packages/xrpl/src/models/transactions/oracleSet.ts (2)
7-10: Type safety improvement: Added type guard utilities for consistent type checkingImported
isArrayandisRecordutility functions from the common module to standardize type checking across the codebase, improving type safety.
91-193: Type safety improvement: Enhanced validation for PriceDataSeriesThe validation logic for
PriceDataSerieshas been significantly refactored to use proper type guard functions instead of direct type checks. This improves:
- Type safety - Using
isArrayandisRecordensures proper type narrowing- Error messages - More specific validation error messages
- Code organization - Better structure with nested validation logic
- Maintainability - Easier to understand the validation requirements
The validation maintains the same functional logic but with stronger type guarantees.
packages/xrpl/src/sugar/autofill.ts (1)
159-161: Guard againstundefinedtag comparison
inchecks only the property’s existence, not its value, so whentx[tagField]is explicitlyundefinedthis condition still evaluatestrueand the comparisonundefined !== tagwill always fail – even though the consumer may have intended “tag not set”.
Compare only when the field is defined:- if (tagField in tx && tx[tagField] !== tag) { + if (tx[tagField] !== undefined && tx[tagField] !== tag) {packages/xrpl/src/models/transactions/common.ts (1)
239-259: Consider stricter validation forAccountfield
validateBaseTransactionnow calls
validateRequiredField(common, 'Account', isString)– this only enforces “string”, whereasisAccount(already defined in the same file) additionally verifies canonical XRPL address formats. Unless there is a deliberate reason to relax the check, switching toisAccountrestores the stronger guarantee with zero runtime cost.- validateRequiredField(common, 'Account', isString) + validateRequiredField(common, 'Account', isAccount)
packages/xrpl/src/sugar/autofill.ts
Outdated
| classicAccount: account, | ||
| tag: expectedTag, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing classic‑address validation for non‑X‑addresses
The else branch returns the original account unchecked. Supplying a malformed classic address would therefore slip through, whereas X‑addresses are rigorously validated. Consider re‑using isValidClassicAddress for parity:
- return {
- classicAccount: account,
- tag: expectedTag,
- }
+ if (!isValidClassicAddress(account)) {
+ throw new ValidationError('Invalid classic account address')
+ }
+ return {
+ classicAccount: account,
+ tag: expectedTag,
+ }📝 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.
| classicAccount: account, | |
| tag: expectedTag, | |
| } | |
| if (!isValidClassicAddress(account)) { | |
| throw new ValidationError('Invalid classic account address') | |
| } | |
| return { | |
| classicAccount: account, | |
| tag: expectedTag, | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/xrpl/src/sugar/autofill.ts (2)
179-202: 🛠️ Refactor suggestionMissing classic-address validation for non-X-addresses
The
elsebranch at line 199-202 returns the original account without validation. This means malformed classic addresses would pass through unchecked, while X-addresses are rigorously validated.Consider validating classic addresses using the
isValidClassicAddressfunction:- return { - classicAccount: account, - tag: expectedTag, - } + if (!isValidClassicAddress(account)) { + throw new ValidationError('Invalid classic account address') + } + return { + classicAccount: account, + tag: expectedTag, + }
149-153:⚠️ Potential issueFix error type inconsistency
The current implementation uses a generic
Erroron line 157, while all other validation failures in this file throwValidationError. This inconsistency forces callers to handle different error types for similar validation failures.- throw new Error(`${accountField} must be a string`) + throw new ValidationError(`${accountField} must be a string`)Also applies to: 156-158
🧹 Nitpick comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
372-398: Well-designed extraction of DeliverMax handling logicThe new
handleDeliverMaxfunction effectively centralizes the logic for handling theDeliverMaxfield in Payment transactions. This improves code organization and maintainability.However, there's a performance concern with using the
deleteoperator on line 396.Consider setting the property to
undefinedinstead of usingdeletefor better performance:- delete tx.DeliverMax + tx.DeliverMax = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 396-397: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/xrpl/src/client/index.ts(3 hunks)packages/xrpl/src/models/transactions/AMMBid.ts(3 hunks)packages/xrpl/src/sugar/autofill.ts(4 hunks)packages/xrpl/test/integration/requests/accountTx.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/xrpl/test/integration/requests/accountTx.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/src/client/index.ts
- packages/xrpl/src/models/transactions/AMMBid.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/src/sugar/autofill.ts
[error] 396-397: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
packages/xrpl/src/sugar/autofill.ts (4)
149-162: Good addition of generic type parametersThe use of generics in the
validateAccountAddressfunction improves type safety by ensuring the correct field types are maintained throughout the validation process. This allows TypeScript to track the field types accurately after validation.
165-172: Improved type handling for tag fieldsThe updated type handling in the conditional check and subsequent assignment is more type-safe now. By using
tx[tagField]instead of direct property access, TypeScript can properly track the field types.
373-378: Excellent JSDoc documentation for new functionThe JSDoc comment for the
handleDeliverMaxfunction is clear, concise, and documents both the purpose of the function and the potential errors it might throw.
379-397:❌ Incorrect review comment
Verify potential impact on clients using DeliverMax directly
The function removes the
DeliverMaxfield after processing. This might affect clients that expect this field to persist after autofill.
🏁 Script executed:
#!/bin/bash # Search for usages of DeliverMax field after autofill rg "DeliverMax.*after.*autofill|after.*autofill.*DeliverMax" --type ts # Look for tests that rely on DeliverMax being present rg "DeliverMax.*test|test.*DeliverMax" --type tsLength of output: 377
🏁 Script executed:
#!/bin/bash rg -n "DeliverMax" -C 5 packages/xrpl/test/integration/transactions/payment.test.tsLength of output: 1726
🏁 Script executed:
#!/bin/bash # Locate definition of testTransaction to inspect how it handles transaction fields rg -n "function testTransaction" -C 5 --type tsLength of output: 936
No Action Required – DeliverMax Removal Is Intentional
handleDeliverMax deliberately deletes the non‑protocol DeliverMax field after copying its value into Amount. A search of both the codebase and integration tests shows no references to DeliverMax after autofill. Retaining DeliverMax would send an unsupported field to the XRP Ledger, so its removal is correct.
Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 396-397: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
200-202: Missing classic-address validation for non-X-addressesThe
elsebranch returns the originalaccountunchecked. Supplying a malformed classic address would slip through, whereas X-addresses are rigorously validated.#!/bin/bash # Check if isValidClassicAddress is used elsewhere in the codebase rg "isValidClassicAddress" --type ts
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/xrpl/src/client/index.ts(2 hunks)packages/xrpl/src/models/transactions/AMMBid.ts(4 hunks)packages/xrpl/src/models/transactions/AMMClawback.ts(3 hunks)packages/xrpl/src/models/transactions/AMMDelete.ts(2 hunks)packages/xrpl/src/models/transactions/AMMDeposit.ts(3 hunks)packages/xrpl/src/models/transactions/AMMVote.ts(2 hunks)packages/xrpl/src/models/transactions/AMMWithdraw.ts(3 hunks)packages/xrpl/src/models/transactions/checkCreate.ts(2 hunks)packages/xrpl/src/models/transactions/clawback.ts(2 hunks)packages/xrpl/src/models/transactions/common.ts(15 hunks)packages/xrpl/src/models/transactions/payment.ts(5 hunks)packages/xrpl/src/models/transactions/signerListSet.ts(3 hunks)packages/xrpl/src/models/transactions/transaction.ts(3 hunks)packages/xrpl/src/sugar/autofill.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/xrpl/src/models/transactions/signerListSet.ts
- packages/xrpl/src/client/index.ts
- packages/xrpl/src/models/transactions/payment.ts
- packages/xrpl/src/models/transactions/AMMBid.ts
- packages/xrpl/src/models/transactions/common.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
packages/xrpl/src/models/transactions/AMMDelete.ts (2)
packages/xrpl/src/models/transactions/common.ts (1)
isIssuedCurrencyAmount(129-139)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/AMMVote.ts (2)
packages/xrpl/src/models/transactions/common.ts (1)
isIssuedCurrencyAmount(129-139)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/AMMClawback.ts (2)
packages/xrpl/src/models/transactions/common.ts (4)
validateRequiredField(243-263)isIssuedCurrency(112-121)validateOptionalField(273-287)isIssuedCurrencyAmount(129-139)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/AMMDeposit.ts (2)
packages/xrpl/src/models/transactions/common.ts (1)
isIssuedCurrencyAmount(129-139)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/checkCreate.ts (1)
packages/xrpl/src/models/transactions/common.ts (1)
isIssuedCurrencyAmount(129-139)
packages/xrpl/src/models/transactions/clawback.ts (3)
packages/xrpl/src/models/transactions/common.ts (2)
isIssuedCurrencyAmount(129-139)isMPTAmount(165-172)packages/xrpl/src/models/transactions/index.ts (1)
isMPTAmount(1-1)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/AMMWithdraw.ts (2)
packages/xrpl/src/models/transactions/common.ts (1)
isIssuedCurrencyAmount(129-139)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
packages/xrpl/src/models/transactions/transaction.ts (3)
packages/xrpl/src/models/transactions/common.ts (4)
isArray(229-231)isRecord(82-84)isString(92-94)isIssuedCurrencyAmount(129-139)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/models/utils/index.ts (1)
isHex(36-38)
packages/xrpl/src/sugar/autofill.ts (4)
packages/xrpl/src/errors.ts (1)
ValidationError(156-156)packages/xrpl/src/utils/index.ts (2)
isValidXAddress(212-212)xAddressToClassicAddress(211-211)packages/xrpl/src/models/transactions/payment.ts (1)
Payment(115-161)packages/xrpl/src/models/common/index.ts (2)
Amount(28-28)MPTAmount(23-26)
🪛 Biome (1.9.4)
packages/xrpl/src/models/transactions/AMMDeposit.ts
[error] 9-9: Declarations inside of a import declaration may not have duplicates
a second declaration of isIssuedCurrencyAmount is not allowed
isIssuedCurrencyAmount is first declared here
(parse)
packages/xrpl/src/models/transactions/AMMWithdraw.ts
[error] 9-9: Declarations inside of a import declaration may not have duplicates
a second declaration of isIssuedCurrencyAmount is not allowed
isIssuedCurrencyAmount is first declared here
(parse)
packages/xrpl/src/sugar/autofill.ts
[error] 396-397: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (34)
packages/xrpl/src/sugar/autofill.ts (5)
6-13: Improved organization of importsThe imports have been nicely organized to group multiple types from the '../models' module together, which makes the dependencies clearer and easier to maintain.
149-158: Good use of generics to improve type safetyThe function signature has been enhanced with generic type parameters that ensure type safety. The explicit check for the string type of the account field value provides better runtime validation and error messages.
160-172: Improved type assertions with generic constraintsThe type assertions are now more type-safe due to the generic constraints established in the function signature. This ensures that the compiler can properly verify the types being used.
184-185: Parameter naming consistency improvementRenaming the parameter from
Accounttoaccountfollows common JavaScript/TypeScript naming conventions for function parameters and improves consistency across the codebase.
373-398:✅ Verification successful
Well-structured new utility function for handling DeliverMax field
The
handleDeliverMaxfunction encapsulates a specific business logic that was likely inline elsewhere. This improves code organization and separation of concerns.Note about line 396: While the static analysis tool flags the use of
deleteoperator as a potential performance issue, in this case it's necessary to completely remove the property rather than setting it to undefined, as downstream code might behave differently with an undefined property vs. a non-existent one.
🏁 Script executed:
#!/bin/bash # Check where handleDeliverMax is being used rg -B 3 -A 3 "handleDeliverMax" --type tsLength of output: 1435
Approved:
handleDeliverMaxImplementation and Usage Verified
- Verified that
handleDeliverMaxis only invoked inpackages/xrpl/src/client/index.tswhen handlingPaymenttransactions.- The use of
delete tx.DeliverMaxis justified to ensure downstream code treats the field as non‑existent rather thanundefined.- Static analysis suppressions around the
deleteand null‑checks are appropriate given the business logic.🧰 Tools
🪛 Biome (1.9.4)
[error] 396-397: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/AMMVote.ts (3)
5-9: Type safety enhancement with proper importsThe import has been updated to use the more specific
isIssuedCurrencyAmountfunction instead of the previousisCurrency. This aligns with the PR objective of improving typing throughout the codebase.
50-50: Improved type validation for Asset fieldChanged from using
isCurrencytoisIssuedCurrencyAmountfor more precise type checking. This function provides a proper TypeScript type predicate, ensuring that the Asset field has all required properties of an IssuedCurrencyAmount.
58-58: Improved type validation for Asset2 fieldChanged from using
isCurrencytoisIssuedCurrencyAmountfor more precise type checking. This function provides a proper TypeScript type predicate, ensuring that the Asset2 field has all required properties of an IssuedCurrencyAmount.packages/xrpl/src/models/transactions/AMMDelete.ts (3)
4-8: Type safety enhancement with proper importsThe import has been updated to use the more specific
isIssuedCurrencyAmountfunction instead of the previousisCurrency. This follows the same pattern as other files in this PR, improving type safety throughout the codebase.
48-48: Improved type validation for Asset fieldChanged from using
isCurrencytoisIssuedCurrencyAmountfor more precise type checking. This function provides a proper TypeScript type predicate, ensuring that the Asset field has all required properties of an IssuedCurrencyAmount.
56-56: Improved type validation for Asset2 fieldChanged from using
isCurrencytoisIssuedCurrencyAmountfor more precise type checking. This function provides a proper TypeScript type predicate, ensuring that the Asset2 field has all required properties of an IssuedCurrencyAmount.packages/xrpl/src/models/transactions/checkCreate.ts (2)
7-7: Type safety enhancement with proper importsThe import has been updated to use
isIssuedCurrencyAmountinstead of the previous utility. This is consistent with the PR's goal of improving type safety across the codebase.
67-67: Improved type validation for SendMax fieldChanged from using a more generic type check to the more specific
isIssuedCurrencyAmountfunction. This provides better type safety by ensuring that the SendMax field, when not a string, has all required properties of an IssuedCurrencyAmount.packages/xrpl/src/models/transactions/clawback.ts (4)
7-7: Type safety enhancement with proper importsThe import has been updated to use
isIssuedCurrencyAmountinstead of the previousisIssuedCurrency. This is consistent with other changes in this PR and improves the overall type safety.
51-51: Improved type validation for Amount fieldChanged from using
isIssuedCurrencytoisIssuedCurrencyAmountfor more precise type checking. This function ensures that the Amount field has all required properties of an IssuedCurrencyAmount when it's not an MPTAmount.
55-55: Enhanced conditional checkUpdated conditional to use
isIssuedCurrencyAmountinstead ofisIssuedCurrency, maintaining type safety throughout the validation flow. This ensures that theAmount.issuerproperty exists before accessing it.
63-63: Enhanced conditional checkUpdated conditional to use
isIssuedCurrencyAmountinstead ofisIssuedCurrency, ensuring proper type narrowing when checking if a Holder field is provided with a currency amount.packages/xrpl/src/models/transactions/AMMDeposit.ts (3)
92-94: Improved type safety with explicit type predicate.The validation now uses
isIssuedCurrencyAmountinstead of what was likelyisCurrencybefore. This is a more precise type check that ensures theAssetfield is an issued currency amount with the proper structure, improving type safety.
100-102: Enhanced validation for Asset2 field.Similar to the Asset field, this change improves type safety by using
isIssuedCurrencyAmountfor validation, ensuring that Asset2 has the correct structure with required properties.
114-118: Strengthened LPTokenOut validation.The validation for
LPTokenOutnow usesisIssuedCurrencyAmountinstead of what was likelyisIssuedCurrencybefore. This is more precise as it ensures the field has the correct structure including the 'value' property which is required for issued currency amounts.packages/xrpl/src/models/transactions/AMMWithdraw.ts (3)
90-92: Improved type safety with explicit type predicate.The validation now uses
isIssuedCurrencyAmountinstead of what was likelyisCurrencybefore. This is a more precise type check that ensures theAssetfield is an issued currency amount with the proper structure, improving type safety.
98-100: Enhanced validation for Asset2 field.Similar to the Asset field, this change improves type safety by using
isIssuedCurrencyAmountfor validation, ensuring that Asset2 has the correct structure with required properties.
108-112: Strengthened LPTokenIn validation.The validation for
LPTokenInnow usesisIssuedCurrencyAmountinstead of what was likelyisIssuedCurrencybefore. This is more precise as it ensures the field has the correct structure including the 'value' property which is required for issued currency amounts.packages/xrpl/src/models/transactions/AMMClawback.ts (5)
9-11: Added type-safe validation functions.Imported
isIssuedCurrencyandisIssuedCurrencyAmountfrom common.ts to enable more precise type checking with automatic type narrowing.
84-86: Improved type safety for Asset validation.Updated to use
validateRequiredFieldwith theisIssuedCurrencytype predicate. This not only validates that the Asset field exists and is correctly formatted, but also narrows its type for TypeScript, eliminating the need for type assertions in the code that follows.
100-102: Enhanced type validation for Asset2 and Amount fields.The code now uses:
validateRequiredFieldwithisIssuedCurrencyfor Asset2validateOptionalFieldwithisIssuedCurrencyAmountfor the optional Amount fieldThis improves type safety while also making the TypeScript compiler aware of the validated types.
105-109: Removed type assertion for Amount.currency check.Thanks to the use of
validateOptionalFieldwith a type predicate, TypeScript now knows that iftx.Amountexists, it matches theIssuedCurrencyAmountinterface. This eliminates the need for type assertions when accessingcurrencyproperty, making the code cleaner and safer.
111-115: Removed type assertion for Amount.issuer check.Similar to the currency check, the type assertion for accessing the
issuerproperty has been removed. The type is now properly narrowed by the validation function, allowing direct access to the property in a type-safe manner.packages/xrpl/src/models/transactions/transaction.ts (6)
22-28: Enhanced imports for better type checking.Replaced individual imports with a grouped import that includes additional utility functions:
isArray: For checking if a value is an arrayisIssuedCurrencyAmount: For validating issued currency amountsisRecord: For checking if a value is an objectisString: For verifying string typesThese utility functions will enable more precise type checking throughout the file.
231-241: Improved validation for Memos field.The validation for the
Memosfield has been significantly enhanced:
- Added proper array type check using
isArray- Added explicit validation that each memo item is an object using
isRecord- Added explicit validation that
memo.Memois an object usingisRecordThis provides more precise type checking and clearer error messages when validation fails.
243-245: Enhanced MemoData validation.Added an explicit string type check before validating the hex format. This provides a more complete validation and clearer error path when invalid data is encountered.
249-251: Enhanced MemoType validation.Added an explicit string type check before validating the hex format. This provides a more complete validation and clearer error path when invalid data is encountered.
255-257: Enhanced MemoFormat validation.Added an explicit string type check before validating the hex format. This provides a more complete validation and clearer error path when invalid data is encountered.
262-267: Improved currency code validation.This section replaces previous validation logic with a more streamlined approach:
- Extracts the value once per key for clarity
- Uses
isIssuedCurrencyAmountfor precise type checking- Directly extracts the currency from the validated field
This makes the validation more robust and the code more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
76-84: Implement the past recommendation to exclude arrays fromisRecordThe current implementation of
isRecordwill returntruefor arrays since they are also considered objects in JavaScript (typeof [] === 'object'). This can lead to false positives in validation logic and mask potential bugs.Apply this improvement to prevent Arrays from being considered Records:
export function isRecord(value: unknown): value is Record<string, unknown> { - return value !== null && typeof value === 'object' + return ( + value !== null && + typeof value === 'object' && + !Array.isArray(value) + ) }
🧹 Nitpick comments (2)
packages/xrpl/src/models/transactions/common.ts (1)
539-540: Consider improving the typecasting approachThe comment indicating a type assertion workaround suggests that the typing could be improved further. Consider refactoring this part to avoid needing the type assertion comment.
You could strengthen the typing of
validateCredentialsListby using generics to handle both string arrays and AuthorizeCredential arrays:export function validateCredentialsList<T extends string | AuthorizeCredential>( credentials: unknown, transactionType: string, isStringID: boolean, maxCredentials: number, ): asserts credentials is T[] { // Existing validation logic... // No need for type assertion with proper generic constraints if (containsDuplicates(credentials as T[])) { throw new ValidationError( `${transactionType}: Credentials cannot contain duplicate elements`, ) } }packages/xrpl/src/sugar/autofill.ts (1)
373-398: Consider replacingdeletewith assignment toundefinedThe new
handleDeliverMaxfunction is a good extraction of previously duplicated logic, but the use of thedeleteoperator at line 396 could impact performance.- delete tx.DeliverMax + tx.DeliverMax = undefinedThis change would maintain the same behavior while being more performant, as direct property assignment is faster than the
deleteoperator. This was also flagged by the Biome static analysis tool.🧰 Tools
🪛 Biome (1.9.4)
[error] 396-397: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/xrpl/src/models/transactions/AMMBid.ts(4 hunks)packages/xrpl/src/models/transactions/AMMDelete.ts(2 hunks)packages/xrpl/src/models/transactions/AMMDeposit.ts(3 hunks)packages/xrpl/src/models/transactions/AMMVote.ts(2 hunks)packages/xrpl/src/models/transactions/AMMWithdraw.ts(3 hunks)packages/xrpl/src/models/transactions/common.ts(15 hunks)packages/xrpl/src/models/transactions/signerListSet.ts(3 hunks)packages/xrpl/src/models/transactions/transaction.ts(3 hunks)packages/xrpl/src/sugar/autofill.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/xrpl/src/models/transactions/AMMDelete.ts
- packages/xrpl/src/models/transactions/AMMVote.ts
- packages/xrpl/src/models/transactions/AMMWithdraw.ts
- packages/xrpl/src/models/transactions/signerListSet.ts
- packages/xrpl/src/models/transactions/transaction.ts
- packages/xrpl/src/models/transactions/AMMBid.ts
- packages/xrpl/src/models/transactions/AMMDeposit.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/src/sugar/autofill.ts
[error] 396-397: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: snippets (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (18.x)
🔇 Additional comments (11)
packages/xrpl/src/models/transactions/common.ts (9)
25-47: Great type safety improvement inisMemofunctionThe refactoring of this function with proper type predicates and unknown input type enhances type safety while maintaining the same validation logic.
51-67: Well-structured type predicate forisSignerThe updated implementation provides stronger type guarantees through type predicates and consistent validation patterns.
106-114: Good addition of theisCurrencytype guardThis new helper function provides a clear and reusable type guard for Currency validation, improving code readability and maintainability.
122-131: Improved currency validation with consistent type guardsThe refactored currency-related validation functions now use consistent validation patterns with
isRecordandisString, making the validation logic more explicit and maintainable.Also applies to: 139-149, 227-229
233-241: Well-implemented genericisArraytype guardThis new utility function provides a type-safe way to check for arrays with generic type support, which is used consistently throughout the module.
253-273: Excellent TypeScript typing improvements for validation functionsThe refactoring of
validateRequiredFieldandvalidateOptionalFieldwith generic type parameters and assertion signatures greatly enhances type safety. This allows TypeScript to properly narrow the types after validation, which was the main goal of this PR.Also applies to: 283-297
307-307: Good extension of BaseTransaction interfaceExtending from
Record<string, unknown>allows for more flexible handling of transaction properties while maintaining type safety.
388-390: Improved type assertion in validateBaseTransactionThe function now properly uses assertion signatures to guarantee the type of the validated object, enhancing type safety.
413-416: Consistent array validation using the new isArray utilityThe validation of Memos and Signers now uses the new
isArraytype guard, improving consistency and type safety.Also applies to: 418-423
packages/xrpl/src/sugar/autofill.ts (2)
149-158: Great improvement to type safety!The function signature now uses generic type parameters to ensure type safety at compile time. This enhancement makes the function correctly validate field types from a TypeScript perspective, ensuring that:
T extends Transactionenforces the transaction typeK extends keyof T & stringensures the account field exists on the transactionK2 extends keyof T & stringensures the tag field exists on the transactionThis change significantly improves the library's type checking and aligns perfectly with the PR's objective of enhancing TypeScript typing.
160-172: Good use of type assertions for property updatesThe type assertions using
Record<K, string>andRecord<K2, number | false>provide proper type safety while allowing the function to update the transaction properties. This approach maintains runtime type checking while giving TypeScript better insight into the types.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
199-202: Missing classic-address validation for non-X-addresses.The
elsebranch returns the originalaccountunchecked. Supplying a malformed classic address would therefore slip through, whereas X-addresses are rigorously validated. Consider re-usingisValidClassicAddressfor parity.#!/bin/bash # Check if isValidClassicAddress function is available in the codebase rg -A 3 "function isValidClassicAddress" --type ts
🧹 Nitpick comments (2)
packages/xrpl/src/sugar/autofill.ts (2)
394-395: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance as it changes the object's underlying structure. While the performance impact is likely minimal in this context, using property assignment withundefinedis generally preferred.- delete tx.DeliverMax + tx.DeliverMax = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 394-395: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
379-395: Add a comment explaining why DeliverMax is removed.While the function handles the
DeliverMaxfield correctly, adding a comment explaining why it's removed after processing would improve code maintainability. This would help future developers understand that this is intentional behavior to normalize the transaction before submission.// eslint-disable-next-line no-param-reassign -- needed here + // Remove DeliverMax after processing to avoid redundancy, as Amount now contains the same value delete tx.DeliverMax🧰 Tools
🪛 Biome (1.9.4)
[error] 394-395: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/models/transactions/common.ts(15 hunks)packages/xrpl/src/sugar/autofill.ts(4 hunks)packages/xrpl/test/integration/transactions/payment.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/xrpl/test/integration/transactions/payment.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/models/transactions/common.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/src/sugar/autofill.ts
[error] 394-395: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: snippets (18.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (18.x)
- GitHub Check: snippets (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (18.x)
🔇 Additional comments (3)
packages/xrpl/src/sugar/autofill.ts (3)
149-173: Excellent type safety improvements with generic constraints.The refactoring of
validateAccountAddresswith generic type parameters significantly enhances type safety. The function now properly constrains theaccountFieldandtagFieldparameters to be valid keys of the transaction object, while ensuring proper runtime validation before processing. This eliminates potential type errors and makes the compiler aware of the field types after validation.
156-158: Good runtime type validation before processing.Adding explicit runtime type checking for the account field value ensures early failure with a clear error message, rather than allowing potential errors to propagate further.
371-396: Good extraction of DeliverMax handling logic into a dedicated function.Extracting this logic into a separate exported function improves code organization and reusability. The function correctly handles the relationship between
AmountandDeliverMaxfields in Payment transactions, ensuring consistency and proper validation.🧰 Tools
🪛 Biome (1.9.4)
[error] 394-395: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
198-200:⚠️ Potential issueMissing classic-address validation for non-X-addresses
The
elsebranch returns the originalaccountunchecked. Supplying a malformed classic address would therefore slip through, whereas X-addresses are rigorously validated. Consider re-usingisValidClassicAddressfor parity.- return { - classicAccount: account, - tag: expectedTag, - } + if (!isValidClassicAddress(account)) { + throw new ValidationError('Invalid classic account address') + } + return { + classicAccount: account, + tag: expectedTag, + }
🧹 Nitpick comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
392-393: Consider using undefined assignment instead of deleteUsing the
deleteoperator can impact performance as flagged by the static analyzer. Consider using an undefined assignment instead.- delete tx.DeliverMax + tx.DeliverMax = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 392-393: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/client/index.ts(2 hunks)packages/xrpl/src/models/transactions/payment.ts(7 hunks)packages/xrpl/src/sugar/autofill.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/src/client/index.ts
- packages/xrpl/src/models/transactions/payment.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/src/sugar/autofill.ts
[error] 392-393: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: snippets (20.x)
- GitHub Check: browser (18.x)
- GitHub Check: snippets (18.x)
- GitHub Check: snippets (22.x)
- GitHub Check: integration (18.x)
🔇 Additional comments (7)
packages/xrpl/src/sugar/autofill.ts (7)
6-11: Good consolidation of importsConsolidating imports from the
../modelsmodule improves code organization and readability.
147-151: Great improvement to type safety with genericsAdding generic type parameters to
validateAccountAddressis a significant improvement to type safety:
T extends Transactionproperly constrains the transaction typeK extends keyof T & stringensures the account field is a string key of TK2 extends keyof T & stringensures the tag field is a string key of TThis enables TypeScript to perform better static type checking when using this function.
154-156: Good addition of string validationAdding explicit validation that the account field value is a string is a good defensive programming practice that catches errors earlier.
158-160: Improved type handlingDirect usage of the return value from
getClassicAccountAndTagwith proper type assertion improves type safety.
163-169: Better type consistency in tag validationThe conditional check is more robust and the type assertion is properly handled for tag field assignment.
181-186: Standardized parameter namingStandardizing the parameter name to just
accountimproves consistency throughout the codebase.
369-394: Good extraction of DeliverMax handling logicExtracting the
DeliverMaxhandling logic into a dedicated function:
- Improves modularity and testability
- Centralizes this business logic in one place
- Makes the code more maintainable
The function correctly:
- Populates
AmountfromDeliverMaxwhenAmountis missing- Validates that
AmountandDeliverMaxare identical when both are provided- Removes the
DeliverMaxfield after processing🧰 Tools
🪛 Biome (1.9.4)
[error] 392-393: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
mvadari
changed the title
validateRequiredField and validateOptionalFieldvalidateRequiredField, validateOptionalField, and DeliverMax
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/xrpl/src/sugar/autofill.ts (2)
148-175: Classic-address format is still not validated
The earlier feedback about validating classic addresses when the input is not an X-address has not been addressed.
Supplying an invalid classic address (e.g. one with an incorrect checksum or length) will silently pass through this function and surface only at submission time.If
isValidClassicAddressis available in your utils (it is inripple-address-codec), reuse it here to keep validation parity with the X-address branch.This is a repeat of the previous comment.
184-205: Same validation gap insidegetClassicAccountAndTag
The helper still returns the rawaccountstring unchanged when it is not an X-address, without checking that the classic address is syntactically valid.
Coupling this with the above comment ensures a single validation point and prevents malformed addresses from propagating.Repeated from the former review as well.
🧹 Nitpick comments (1)
packages/xrpl/src/sugar/autofill.ts (1)
218-223: RedundantfieldName in txcheck inside already–guarded block
The outerif (fieldName in tx)already guarantees the property exists, so the inner check duplicates work and slightly hurts readability:- if (fieldName in tx && typeof account === 'string') { + if (typeof account === 'string') {A small clean-up, but worth considering.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/client/partialPayment.ts(1 hunks)packages/xrpl/src/models/transactions/common.ts(15 hunks)packages/xrpl/src/sugar/autofill.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xrpl/src/client/partialPayment.ts
- packages/xrpl/src/models/transactions/common.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/xrpl/src/sugar/autofill.ts
[error] 401-402: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/sugar/autofill.ts
Outdated
| export function handleDeliverMax(tx: Payment): void { | ||
| if (tx.DeliverMax != null) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | ||
| if (tx.Amount == null) { | ||
| // If only DeliverMax is provided, use it to populate the Amount field | ||
| // eslint-disable-next-line no-param-reassign -- known RPC-level property | ||
| tx.Amount = tx.DeliverMax | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | ||
| if (tx.Amount != null && tx.Amount !== tx.DeliverMax) { | ||
| throw new ValidationError( | ||
| 'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided', | ||
| ) | ||
| } | ||
|
|
||
| // eslint-disable-next-line no-param-reassign -- needed here | ||
| delete tx.DeliverMax | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
handleDeliverMax throws false negatives & uses slow delete
-
AmountandDeliverMaxare compared with strict reference equality (!==).
When both fields hold object Amounts that are value-identical but not the same reference, this check will incorrectly throw aValidationError. -
delete tx.DeliverMaxis flagged by linting tools for performance reasons.
Assigningundefined(keeping the key) avoids hidden class de-optimisations in V8.
Proposed fix:
-import deepEqual from 'fast-deep-equal' // or your internal util
+import deepEqual from 'fast-deep-equal' // lightweight deep compare
if (tx.Amount == null) {
// If only DeliverMax is provided, use it to populate the Amount field
tx.Amount = tx.DeliverMax
}
- if (tx.Amount != null && tx.Amount !== tx.DeliverMax) {
+ if (tx.Amount != null && !deepEqual(tx.Amount, tx.DeliverMax)) {
throw new ValidationError(
'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided',
)
}
- delete tx.DeliverMax
+ tx.DeliverMax = undefinedThis removes the erroneous reference check, performs a value comparison instead, and replaces the delete with an undefined assignment.
📝 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 function handleDeliverMax(tx: Payment): void { | |
| if (tx.DeliverMax != null) { | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | |
| if (tx.Amount == null) { | |
| // If only DeliverMax is provided, use it to populate the Amount field | |
| // eslint-disable-next-line no-param-reassign -- known RPC-level property | |
| tx.Amount = tx.DeliverMax | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | |
| if (tx.Amount != null && tx.Amount !== tx.DeliverMax) { | |
| throw new ValidationError( | |
| 'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided', | |
| ) | |
| } | |
| // eslint-disable-next-line no-param-reassign -- needed here | |
| delete tx.DeliverMax | |
| } | |
| } | |
| import deepEqual from 'fast-deep-equal' // lightweight deep compare | |
| export function handleDeliverMax(tx: Payment): void { | |
| if (tx.DeliverMax != null) { | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | |
| if (tx.Amount == null) { | |
| // If only DeliverMax is provided, use it to populate the Amount field | |
| // eslint-disable-next-line no-param-reassign -- known RPC-level property | |
| tx.Amount = tx.DeliverMax | |
| } | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- needed here | |
| if (tx.Amount != null && !deepEqual(tx.Amount, tx.DeliverMax)) { | |
| throw new ValidationError( | |
| 'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided', | |
| ) | |
| } | |
| // eslint-disable-next-line no-param-reassign -- known RPC-level property | |
| tx.DeliverMax = undefined | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 401-402: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
561-566: Consider strengthening the type assertion.While the type assertion is necessary here, the ESLint directive comment suggests the code could be further improved. The current assertion assumes that the validation done in the previous lines guarantees the type, but a more robust approach might be to create a specific type guard for this case.
- // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- checked above - if (containsDuplicates(credentials as string[] | AuthorizeCredential[])) { + // We've validated each credential individually, now we can safely assert the array type + const typedCredentials = isStringID + ? credentials as string[] + : credentials as AuthorizeCredential[]; + if (containsDuplicates(typedCredentials)) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/common.ts(15 hunks)packages/xrpl/src/models/transactions/transaction.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/models/transactions/transaction.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (18.x)
🔇 Additional comments (11)
packages/xrpl/src/models/transactions/common.ts (11)
25-38: Great improvement to theisMemofunction's type safety.The function has been updated to use proper type predicates and the newly added
isRecordutility function, which improves type narrowing. The code is now more concise and consistent with TypeScript best practices.
51-66: Improved type safety inisSignerfunction.The refactored
isSignerfunction correctly implements type predicates and uses the type-safeisRecordandisStringvalidators, which enhances TypeScript's ability to narrow types after validation.
76-84: Well-implementedisRecordutility function.This new utility function correctly implements a type predicate to check if a value is a proper record/object (not null, is an object, and not an array). This is a key addition that helps other validators perform proper type checking.
106-114: Improved currency validation with better type hierarchy.The renamed
isCurrencyfunction properly handles both string-based currencies and the more complexisIssuedCurrencyobjects, establishing a clear type hierarchy for validations.
233-241: Good addition of genericisArrayutility.The new
isArrayfunction with generic type parameter enhances type safety across the codebase, allowing for more precise type narrowing when validating arrays.
253-273: Excellent enhancement tovalidateRequiredFieldwith generics.The function now uses proper generic types and TypeScript's assertion syntax, which significantly improves type inference after validation. This allows TypeScript to correctly narrow the type of validated fields, eliminating the need for type assertions in calling code.
283-297: Great type safety improvement invalidateOptionalField.Similar to
validateRequiredField, this function now uses generics and assertion syntax to properly narrow field types after validation, handling the optional nature of the fields correctly.
393-400: Improved type safety invalidateBaseTransaction.The function now uses assertion syntax and properly validates that the input is a record before proceeding with further validation, which prevents runtime errors when non-object values are passed.
424-427: Simplified Memos validation using the new utilities.The
Memosvalidation now uses the newisArrayutility, making the code more consistent with the rest of the codebase while maintaining the same validation logic.
429-436: Cleaner Signers validation with improved readability.The validation of
Signershas been refactored to use the newisArrayutility, maintaining the validation requirements while improving code consistency and readability.
82-84: Excellent implementation ofisRecordutility.The
isRecordfunction now correctly excludes arrays, addressing a potential issue where arrays could be incorrectly identified as record objects. This is consistent with the past review comment that was marked as addressed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
561-562: Consider adding a type guard for AuthorizeCredential arraysThe code uses a type assertion with a comment explaining why, but a dedicated type guard function might be cleaner.
+function isAuthorizeCredentialOrStringArray( + arr: unknown +): arr is AuthorizeCredential[] | string[] { + return isArray(arr) && (arr.length === 0 || isString(arr[0]) || isAuthorizeCredential(arr[0])); +} // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- checked above -if (containsDuplicates(credentials as string[] | AuthorizeCredential[])) { +if (containsDuplicates(credentials)) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
packages/xrpl/src/models/transactions/common.ts(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: integration (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: snippets (22.x)
🔇 Additional comments (10)
packages/xrpl/src/models/transactions/common.ts (10)
76-84: Well-implemented type guard that excludes arrays from records!The
isRecordfunction correctly checks that a value is a non-null object and explicitly excludes arrays. This implementation is robust and prevents potential type confusion when validating records vs arrays.
25-38: Good refactoring to use the new type guards consistentlyThe
isMemofunction has been refactored to use the newisRecordtype guard and consistent string validation, making the code more robust and maintainable.
51-65: Consistent use of type guards in isSignerThe
isSignerfunction now uses the newisRecordtype guard and consistent string validation, improving type safety and readability.
106-114: Nice addition of a Currency type guardThe new
isCurrencytype guard provides a clean way to validate Currency types at runtime, leveraging both string and IssuedCurrency checks.
122-131: Improved IssuedCurrency type guardsThe refactored
isIssuedCurrencyandisIssuedCurrencyAmountfunctions now use theisRecordtype guard and consistent string validation, making them more robust and reliable.Also applies to: 139-149
233-241: Well-implemented generic array type guardThe new
isArraytype guard with generic type parameter increases type safety when working with arrays of specific types.
253-273: Excellent use of generics and type assertionsThe refactored
validateRequiredFieldandvalidateOptionalFieldfunctions now use generics and TypeScript'sassertsclause, which significantly improves type narrowing and static type checking.Also applies to: 283-297
393-401: Improved BaseTransaction validation with type assertionThe
validateBaseTransactionfunction now uses theassertsclause and theisRecordtype guard, providing better type safety and clearer error messages.
424-427: Consistent use of isArray across validationsAll array validations now use the newly introduced
isArraytype guard, providing a consistent approach to array validation throughout the codebase.Also applies to: 429-436, 534-534
1-607: Overall excellent type system improvementsThis refactoring greatly improves the TypeScript typing throughout the library by introducing proper type guards and leveraging TypeScript's type assertion capabilities. These changes will make the codebase more maintainable, reduce type-related bugs, and improve developer experience.
🧰 Tools
🪛 Biome (1.9.4)
[error] 302-302: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| * @returns Whether the Currency is properly formed. | ||
| */ | ||
| export function isCurrency(input: unknown): input is Currency { | ||
| return isString(input) || isIssuedCurrency(input) |
There was a problem hiding this comment.
what is the case that we would need this function as opposed to isissuedCurrency?
| if (credentials == null) { | ||
| return | ||
| } |
There was a problem hiding this comment.
since we now use isArray, could we delete the null check?
| if (!isArray(tx.Memos)) { | ||
| throw new ValidationError('Memo must be array') | ||
| } | ||
| tx.Memos.forEach((memo) => { | ||
| if (!isRecord(memo)) { | ||
| throw new ValidationError('Memo must be an object') | ||
| } |
There was a problem hiding this comment.
can we use isMemo here?
High Level Overview of Change
This PR allows
validateRequiredFieldandvalidateOptionalFieldto actually validate the fields from a TypeScript perspective - i.e. the types of the fields will be defined after they are called. This simplifies a lot of the typing in the library. A lot of other typing simplifications are included in this PR as well, as a downstream effect.Context of Change
Pulled out of #2801
Type of Change
Did you update HISTORY.md?
Test Plan
CI passes.