refactor: clean up transaction model validations #2895
Conversation
|
Warning Rate limit exceeded@mvadari has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request refactors the transaction model validation logic across the xrpl.js project. New utility functions— Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (28)
packages/xrpl/test/models/accountDelete.test.ts (1)
86-86: LGTM! Consider documenting error message formats.The error message change aligns with the standardization effort across transaction validations. However, to help developers understand and maintain this pattern, consider documenting the standard error message format (e.g., "invalid field [FieldName]") in a central location.
packages/xrpl/test/models/AMMWithdraw.test.ts (1)
104-104: LGTM! Consider enhancing test descriptions with type information.The standardization of error messages to "invalid field {fieldName}" aligns with the PR's objective to utilize newer helper functions. This change improves consistency across the codebase.
Consider updating the test descriptions to include the expected types, since this information is no longer in the error messages. For example:
- it(`throws w/ Asset must be a Currency`, function () { + it(`throws when Asset is not a Currency type`, function () {This would help maintain clarity about the validation requirements while keeping the standardized error messages.
Also applies to: 126-126, 163-163, 174-174, 186-186, 198-198
packages/xrpl/test/models/setRegularKey.test.ts (1)
35-48: Consider adding more edge cases for RegularKey validation.While the current test covers a basic invalid case (number instead of string), consider adding tests for other invalid scenarios such as:
- Malformed addresses
- Empty strings
- Special characters
it('throws w/ malformed RegularKey address', function () { account.RegularKey = 'invalid_address' assert.throws( () => validateSetRegularKey(account), ValidationError, 'SetRegularKey: invalid field RegularKey', ) }) it('throws w/ empty RegularKey string', function () { account.RegularKey = '' assert.throws( () => validateSetRegularKey(account), ValidationError, 'SetRegularKey: invalid field RegularKey', ) })packages/xrpl/test/models/clawback.test.ts (1)
23-24: LGTM! Consider extracting common test logic.The test additions provide good coverage for both validation functions. However, since both functions are expected to behave identically, consider reducing duplication by extracting the common test logic into a helper function.
Example refactor:
function assertValidationError(tx: any, expectedError: string): void { [validate, validateClawback].forEach((validationFn) => { assert.throws( () => validationFn(tx), ValidationError, expectedError ) }) }Also applies to: 52-56, 69-73, 92-96, 129-133, 151-155, 175-179
packages/xrpl/src/models/transactions/ticketCreate.ts (1)
44-46: Consider standardizing the error message format.For consistency with other validation messages in the codebase, consider updating the error message to follow the "invalid field" format.
- 'TicketCreate: TicketCount must be an integer from 1 to 250', + 'invalid field \'TicketCount\': must be an integer from 1 to 250',packages/xrpl/test/models/paymentChannelFund.test.ts (1)
97-109: Consider adding more test cases for Expiration validation.While the current test case verifies that string values are rejected, consider adding test cases for other edge cases such as:
- Negative numbers
- Floating-point numbers
- Very large numbers
- Empty string
- null/undefined values
This would ensure robust validation of the Expiration field.
packages/xrpl/test/models/baseTransaction.test.ts (1)
11-248: Consider adding edge cases for more thorough validation.While the test coverage is good, consider adding these edge cases:
- Empty strings for string fields
- Zero/negative values for numeric fields
- Malformed hex strings for fields expecting hex format
- Maximum/minimum allowed values for sequence fields
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
98-118: Consider extending the refactoring to offer-specific validations.The validation helper functions could also be used in
validateNFTokenSellOfferCasesandvalidateNFTokenBuyOfferCasesfor consistency with the main validation function.Example refactor for
validateNFTokenBuyOfferCases:function validateNFTokenBuyOfferCases(tx: Record<string, unknown>): void { - if (tx.Owner == null) { - throw new ValidationError( - 'NFTokenCreateOffer: Owner must be present for buy offers', - ) - } + validateRequiredField(tx, 'Owner', isAccount) if (parseAmountValue(tx.Amount) <= 0) { throw new ValidationError( 'NFTokenCreateOffer: Amount must be greater than 0 for buy offers', ) } }packages/xrpl/test/models/offerCreate.test.ts (2)
12-35: Consider adding proper type definitions instead of usingas any.While the test setup refactoring improves maintainability, consider replacing the
as anytype assertion with proper type definitions to improve type safety and documentation.- let offer + let offer: OfferCreate beforeEach(function () { offer = { // ... object properties ... - } as any + } })
89-147: Consider adding more edge cases to improve test coverage.While the error message updates improve consistency, consider adding test cases for:
nullandundefinedvalues- Additional invalid values (negative numbers, empty strings, etc.)
- Missing required fields
Example test case:
it('throws w/ null or undefined values', function () { // Test null offer.TakerPays = null assert.throws( () => validateOfferCreate(offer), ValidationError, 'OfferCreate: invalid field TakerPays' ) // Test undefined offer.TakerPays = undefined assert.throws( () => validateOfferCreate(offer), ValidationError, 'OfferCreate: invalid field TakerPays' ) })packages/xrpl/test/models/payment.test.ts (1)
74-74: Consider using undefined assignment instead of delete operator.While functionally correct, using the
deleteoperator can impact performance. Although this is test code where performance is less critical, consider using undefined assignment for consistency with best practices.- delete payment.Amount + payment.Amount = undefined - delete payment.Destination + payment.Destination = undefinedAlso applies to: 102-102
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/clawback.ts (1)
53-69: Consider standardizing error message format.The conditional logic is well-structured and handles both IssuedCurrency and MPTAmount cases appropriately. However, for consistency with the PR's objective of standardizing error messages, consider updating the error messages to follow the "invalid field ..." format.
- throw new ValidationError('Clawback: invalid holder Account') + throw new ValidationError('invalid field Account: cannot be same as issuer') - throw new ValidationError('Clawback: cannot have Holder for currency') + throw new ValidationError('invalid field Holder: not allowed for currency') - throw new ValidationError('Clawback: invalid holder Account') + throw new ValidationError('invalid field Account: cannot be same as Holder') - throw new ValidationError('Clawback: missing field Holder') + throw new ValidationError('invalid field Holder: required for MPT amount')packages/xrpl/test/models/AMMVote.test.ts (3)
42-47: Update test name to match the new error message format.The test name still references the old error message format. For consistency, consider updating it to match the new standardized format.
- it(`throws w/ Asset must be a Currency`, function () { + it(`throws w/ invalid Asset field`, function () {
56-61: Update test name to match the new error message format.Similar to the previous test, the name should be updated to reflect the new standardized error message format.
- it(`throws w/ Asset2 must be a Currency`, function () { + it(`throws w/ invalid Asset2 field`, function () {
70-75: Update test name and approve input value change.The test input value 'abcd' is a good example of an invalid value. However, the test name should be updated to match the new error message format.
- it(`throws w/ TradingFee must be a number`, function () { + it(`throws w/ invalid TradingFee field`, function () {packages/xrpl/src/models/transactions/AMMVote.ts (1)
52-57: Consider handling NaN case in TradingFee validation.While the
isNumbervalidation will catch most issues, theNumber()conversion could still produceNaN. Consider adding an explicit check for this edge case.const tradingFee = Number(tx.TradingFee) + if (Number.isNaN(tradingFee)) { + throw new ValidationError('AMMVote: TradingFee must be a valid number') + } if (tradingFee < 0 || tradingFee > AMM_MAX_TRADING_FEE) {packages/xrpl/src/models/transactions/trustSet.ts (1)
142-145: Consider enhancing flag validation based on past feedback.While the refactoring to use helper functions improves code clarity, consider implementing comprehensive flag validation rules as suggested in past feedback. The current implementation might miss important flag interdependencies.
Based on the retrieved learning from PR #2873, there are many interdependent validation rules associated with trust-set flags that should be validated comprehensively. For example:
tfSetDeepFreezeis only allowed if the trustline is already frozen or iftfSetFreezeis set in the same transactiontfSetfAuthcannot be unset once setWould you like me to help implement comprehensive flag validation?
packages/xrpl/src/models/transactions/signerListSet.ts (1)
56-82: Consider consolidating type assertions.While the current implementation works correctly, we could reduce the number of type assertions and improve type safety by consolidating the assertions.
Here's a suggested improvement:
- // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- checked above - const signerEntries = tx.SignerEntries as unknown[] + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- checked above that it's an array of SignerEntry + const signerEntries = tx.SignerEntries as SignerEntry[] if (signerEntries.length === 0) { throw new ValidationError( 'SignerListSet: need at least 1 member in SignerEntries', ) } if (signerEntries.length > MAX_SIGNERS) { throw new ValidationError( `SignerListSet: maximum of ${MAX_SIGNERS} members allowed in SignerEntries`, ) } for (const entry of signerEntries) { - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Should be a SignerEntry - const signerEntry = entry as SignerEntry - const { WalletLocator } = signerEntry.SignerEntry + const { WalletLocator } = entry.SignerEntry if ( WalletLocator !== undefined && !HEX_WALLET_LOCATOR_REGEX.test(WalletLocator) ) { throw new ValidationError( `SignerListSet: WalletLocator in SignerEntry must be a 256-bit (32-byte) hexadecimal value`, ) } }This change:
- Asserts the type as
SignerEntry[]directly- Eliminates the need for a second type assertion in the loop
- Maintains the same validation logic with cleaner type handling
packages/xrpl/src/models/transactions/payment.ts (1)
182-188: Consider enhancing Paths validation error messages.While the validation logic is thorough, the error message could be more specific about what makes the Paths field invalid (e.g., empty array, invalid path step, etc.).
Consider updating the error message to be more descriptive:
- throw new ValidationError('Payment: invalid field Paths') + throw new ValidationError('Payment: invalid field Paths - must be a non-empty array of valid path steps')packages/xrpl/src/models/transactions/NFTokenCancelOffer.ts (1)
50-52: Consider using type guard for safer type assertion.The current type assertion could be replaced with a type guard for better type safety.
- // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- checked above - const offers = tx.NFTokenOffers as unknown[] - if (offers.length < 1) { + if ((tx.NFTokenOffers as unknown[]).length < 1) {packages/xrpl/src/models/transactions/AMMCreate.ts (1)
61-63: Consider adding type assertion for TradingFee.The current implementation assumes
tx.TradingFeeis convertible to a number. Consider adding a type assertion or validation to ensure safe conversion.- const tradingFee = Number(tx.TradingFee) + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + const tradingFee = Number(tx.TradingFee as number | string)packages/xrpl/src/models/transactions/AMMBid.ts (1)
71-101: Consider refactoring AuthAccounts validation.The AuthAccounts validation logic could be simplified by extracting the validation into a helper function and using early returns to reduce nesting.
- if (tx.AuthAccounts != null) { - if (!Array.isArray(tx.AuthAccounts)) { - throw new ValidationError( - `AMMBid: AuthAccounts must be an AuthAccount array`, - ) - } - if (tx.AuthAccounts.length > MAX_AUTH_ACCOUNTS) { - throw new ValidationError( - `AMMBid: AuthAccounts length must not be greater than ${MAX_AUTH_ACCOUNTS}`, - ) - } - validateAuthAccounts( - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Only used by JS - tx.Account as string, - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Only used by JS - tx.AuthAccounts as Array<Record<string, unknown>>, - ) - } + validateOptionalField(tx, 'AuthAccounts', (accounts) => { + if (!Array.isArray(accounts)) { + throw new ValidationError(`AMMBid: invalid field AuthAccounts`) + } + if (accounts.length > MAX_AUTH_ACCOUNTS) { + throw new ValidationError( + `AMMBid: AuthAccounts length must not be greater than ${MAX_AUTH_ACCOUNTS}`, + ) + } + return validateAuthAccounts( + tx.Account as string, + accounts as Array<Record<string, unknown>>, + ) + })packages/xrpl/test/models/paymentChannelCreate.test.ts (1)
134-134: Consider using more diverse test data.The test cases for invalid fields all use 'abcd' as the invalid value. Consider using different types of invalid data to ensure robust validation:
- For
SettleDelay: Try using negative numbers or decimals- For
DestinationTag: Try using negative numbers or objects- For
CancelAfter: Try using negative numbers or arraysAlso applies to: 164-164, 179-179
packages/xrpl/src/models/transactions/paymentChannelClaim.ts (1)
153-157: LGTM! Validation logic refactored using helper functions.The changes effectively use the new helper functions
validateRequiredFieldandvalidateOptionalField, making the code more maintainable and consistent with other transaction validations.Consider grouping related field validations together for better readability:
validateRequiredField(tx, 'Channel', isString) +validateOptionalField(tx, 'PublicKey', isString) +validateOptionalField(tx, 'Signature', isString) validateOptionalField(tx, 'Balance', isString) validateOptionalField(tx, 'Amount', isString) -validateOptionalField(tx, 'Signature', isString) -validateOptionalField(tx, 'PublicKey', isString)packages/xrpl/src/models/transactions/common.ts (1)
464-464: Consider adding a comment explaining why max-params is necessary.While disabling the max-params eslint rule is acceptable here, it would be helpful to add a brief comment explaining why the function needs multiple parameters and why separating the logic would add unnecessary complexity.
-// eslint-disable-next-line max-params -- separating logic further will add unnecessary complexity +// eslint-disable-next-line max-params -- Function requires multiple parameters to validate credentials list. +// Separating the logic would add unnecessary complexity without improving maintainability.packages/xrpl/src/models/transactions/depositPreauth.ts (1)
45-87: Consider extracting business rule validations.The validation function mixes type checking with business rules. Consider extracting the self-authorization checks into a separate function for better maintainability and readability.
+function validateSelfAuthorization(account: string, target: string, action: 'preauthorize' | 'unauthorize'): void { + if (account === target) { + throw new ValidationError( + `DepositPreauth: Account can't ${action} its own address`, + ) + } +} + export function validateDepositPreauth(tx: Record<string, unknown>): void { validateBaseTransaction(tx) validateSingleAuthorizationFieldProvided(tx) if (tx.Authorize !== undefined) { - if (typeof tx.Authorize !== 'string') { - throw new ValidationError('DepositPreauth: invalid field Authorize') - } - - if (tx.Account === tx.Authorize) { - throw new ValidationError( - "DepositPreauth: Account can't preauthorize its own address", - ) - } + validateOptionalField(tx, 'Authorize', 'string') + validateSelfAuthorization(tx.Account as string, tx.Authorize as string, 'preauthorize') } else if (tx.Unauthorize !== undefined) { - if (typeof tx.Unauthorize !== 'string') { - throw new ValidationError('DepositPreauth: invalid field Unauthorize') - } - - if (tx.Account === tx.Unauthorize) { - throw new ValidationError( - "DepositPreauth: Account can't unauthorize its own address", - ) - } + validateOptionalField(tx, 'Unauthorize', 'string') + validateSelfAuthorization(tx.Account as string, tx.Unauthorize as string, 'unauthorize') } else if (tx.AuthorizeCredentials !== undefined) { validateCredentialsList( tx.AuthorizeCredentials,packages/xrpl/test/models/CredentialDelete.test.ts (1)
5-5: LGTM! Import path update aligns with refactoring objectives.The change from PascalCase to camelCase in the import path is consistent with the PR's goal of cleaning up transaction model validations.
Consider adding the
.tsextension to the import path to resolve the ESLint warning:-import { validateCredentialDelete } from '../../src/models/transactions/credentialDelete' +import { validateCredentialDelete } from '../../src/models/transactions/credentialDelete.ts'🧰 Tools
🪛 ESLint
[error] 5-5: Missing file extension for "../../src/models/transactions/credentialDelete"
(import/extensions)
packages/xrpl/test/models/CredentialCreate.test.ts (1)
5-5: LGTM! Import path update aligns with refactoring objectives.The change from PascalCase to camelCase in the import path is consistent with the PR's goal of cleaning up transaction model validations.
Consider adding the
.tsextension to the import path to resolve the ESLint warning:-import { validateCredentialCreate } from '../../src/models/transactions/credentialCreate' +import { validateCredentialCreate } from '../../src/models/transactions/credentialCreate.ts'🧰 Tools
🪛 ESLint
[error] 5-5: Missing file extension for "../../src/models/transactions/credentialCreate"
(import/extensions)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/AMMBid.ts(3 hunks)packages/xrpl/src/models/transactions/AMMCreate.ts(2 hunks)packages/xrpl/src/models/transactions/AMMDelete.ts(2 hunks)packages/xrpl/src/models/transactions/AMMDeposit.ts(2 hunks)packages/xrpl/src/models/transactions/AMMVote.ts(2 hunks)packages/xrpl/src/models/transactions/AMMWithdraw.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenAcceptOffer.ts(3 hunks)packages/xrpl/src/models/transactions/NFTokenCancelOffer.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(2 hunks)packages/xrpl/src/models/transactions/accountSet.ts(2 hunks)packages/xrpl/src/models/transactions/checkCancel.ts(2 hunks)packages/xrpl/src/models/transactions/checkCash.ts(2 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(5 hunks)packages/xrpl/src/models/transactions/depositPreauth.ts(2 hunks)packages/xrpl/src/models/transactions/escrowCancel.ts(2 hunks)packages/xrpl/src/models/transactions/escrowCreate.ts(2 hunks)packages/xrpl/src/models/transactions/escrowFinish.ts(2 hunks)packages/xrpl/src/models/transactions/index.ts(1 hunks)packages/xrpl/src/models/transactions/offerCancel.ts(2 hunks)packages/xrpl/src/models/transactions/offerCreate.ts(2 hunks)packages/xrpl/src/models/transactions/payment.ts(4 hunks)packages/xrpl/src/models/transactions/paymentChannelClaim.ts(2 hunks)packages/xrpl/src/models/transactions/paymentChannelCreate.ts(3 hunks)packages/xrpl/src/models/transactions/paymentChannelFund.ts(2 hunks)packages/xrpl/src/models/transactions/setRegularKey.ts(2 hunks)packages/xrpl/src/models/transactions/signerListSet.ts(2 hunks)packages/xrpl/src/models/transactions/ticketCreate.ts(2 hunks)packages/xrpl/src/models/transactions/transaction.ts(1 hunks)packages/xrpl/src/models/transactions/trustSet.ts(2 hunks)packages/xrpl/test/integration/transactions/credentialDelete.test.ts(1 hunks)packages/xrpl/test/models/AMMBid.test.ts(6 hunks)packages/xrpl/test/models/AMMCreate.test.ts(3 hunks)packages/xrpl/test/models/AMMDelete.test.ts(2 hunks)packages/xrpl/test/models/AMMDeposit.test.ts(6 hunks)packages/xrpl/test/models/AMMVote.test.ts(3 hunks)packages/xrpl/test/models/AMMWithdraw.test.ts(6 hunks)packages/xrpl/test/models/CredentialAccept.test.ts(1 hunks)packages/xrpl/test/models/CredentialCreate.test.ts(1 hunks)packages/xrpl/test/models/CredentialDelete.test.ts(1 hunks)packages/xrpl/test/models/NFTokenAcceptOffer.test.ts(1 hunks)packages/xrpl/test/models/NFTokenCreateOffer.test.ts(2 hunks)packages/xrpl/test/models/accountDelete.test.ts(1 hunks)packages/xrpl/test/models/accountSet.test.ts(7 hunks)packages/xrpl/test/models/baseTransaction.test.ts(6 hunks)packages/xrpl/test/models/checkCancel.test.ts(1 hunks)packages/xrpl/test/models/checkCash.test.ts(4 hunks)packages/xrpl/test/models/checkCreate.test.ts(5 hunks)packages/xrpl/test/models/clawback.test.ts(8 hunks)packages/xrpl/test/models/depositPreauth.test.ts(4 hunks)packages/xrpl/test/models/escrowCancel.test.ts(2 hunks)packages/xrpl/test/models/escrowCreate.test.ts(2 hunks)packages/xrpl/test/models/escrowFinish.test.ts(3 hunks)packages/xrpl/test/models/offerCancel.test.ts(1 hunks)packages/xrpl/test/models/offerCreate.test.ts(3 hunks)packages/xrpl/test/models/oracleDelete.test.ts(1 hunks)packages/xrpl/test/models/oracleSet.test.ts(3 hunks)packages/xrpl/test/models/payment.test.ts(5 hunks)packages/xrpl/test/models/paymentChannelClaim.test.ts(6 hunks)packages/xrpl/test/models/paymentChannelCreate.test.ts(7 hunks)packages/xrpl/test/models/paymentChannelFund.test.ts(4 hunks)packages/xrpl/test/models/setRegularKey.test.ts(1 hunks)packages/xrpl/test/models/signerListSet.test.ts(1 hunks)packages/xrpl/test/models/ticketCreate.test.ts(1 hunks)packages/xrpl/test/models/trustSet.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/xrpl/test/models/checkCancel.test.ts
- packages/xrpl/test/models/NFTokenAcceptOffer.test.ts
🧰 Additional context used
🧠 Learnings (5)
packages/xrpl/src/models/transactions/trustSet.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
packages/xrpl/test/models/clawback.test.ts (1)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
packages/xrpl/src/models/transactions/offerCreate.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/NFTokenMint.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
🪛 Biome (1.9.4)
packages/xrpl/test/models/payment.test.ts
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 102-102: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 ESLint
packages/xrpl/src/models/transactions/index.ts
[error] 40-40: Missing file extension for "./credentialAccept"
(import/extensions)
[error] 41-41: Missing file extension for "./credentialCreate"
(import/extensions)
[error] 42-42: Missing file extension for "./credentialDelete"
(import/extensions)
packages/xrpl/src/models/transactions/transaction.ts
[error] 23-23: Missing file extension for "./credentialAccept"
(import/extensions)
[error] 24-24: Missing file extension for "./credentialCreate"
(import/extensions)
[error] 25-25: Missing file extension for "./credentialDelete"
(import/extensions)
packages/xrpl/test/integration/transactions/credentialDelete.test.ts
[error] 9-9: Missing file extension for "../../../src/models/transactions/credentialDelete"
(import/extensions)
packages/xrpl/test/models/CredentialAccept.test.ts
[error] 5-5: Missing file extension for "../../src/models/transactions/credentialAccept"
(import/extensions)
packages/xrpl/test/models/CredentialCreate.test.ts
[error] 5-5: Missing file extension for "../../src/models/transactions/credentialCreate"
(import/extensions)
packages/xrpl/test/models/CredentialDelete.test.ts
[error] 5-5: Missing file extension for "../../src/models/transactions/credentialDelete"
(import/extensions)
🪛 GitHub Actions: Node.js CI
packages/xrpl/src/models/transactions/transaction.ts
[error] 23-23: Cannot find module './credentialAccept' or its corresponding type declarations.
🔇 Additional comments (117)
packages/xrpl/test/models/oracleDelete.test.ts (1)
34-39: LGTM! Good improvement to the test case.Using 'abcd' instead of '1234' is a better choice for testing invalid input as it's more obviously invalid and provides clearer validation coverage.
packages/xrpl/test/models/escrowCancel.test.ts (2)
56-62: LGTM! Error message standardization looks good.The updated error messages for missing OfferSequence follow the new standardized format by including the "field" keyword, making them more descriptive and consistent with other validation messages.
86-92: LGTM! Error message standardization looks good.The updated error messages for invalid OfferSequence follow the new standardized format by including the "field" keyword, making them more descriptive and consistent with other validation messages.
packages/xrpl/test/models/oracleSet.test.ts (2)
1-196: Well-structured and comprehensive test coverage!The test suite provides excellent coverage of validation scenarios for the OracleSet transaction type, including:
- Required field validation
- Data type validation
- Array structure validation
- Nested object validation
- Range validation for Scale field
- Edge cases and error conditions
🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 122-122: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 138-138: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 146-146: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 154-154: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 162-162: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
50-50: Consistent use of invalid test values.Good choice using 'abcd' as the invalid value for numeric field validations. This standardization:
- Makes the test cases more consistent
- Clearly demonstrates the type validation behavior
- Aligns with the PR's objective of cleaning up transaction model validations
Also applies to: 64-64, 170-170, 177-177
packages/xrpl/src/models/transactions/checkCancel.ts (2)
1-6: LGTM! Import changes align with validation refactoring.The updated imports reflect the transition to using the new helper functions for validation, which is consistent with the PR's objective of enhancing validation functions.
34-34: LGTM! Validation logic refactored effectively.The validation has been simplified by using the new helper function while maintaining the same validation requirements. This change:
- Makes the code more maintainable
- Standardizes error handling
- Aligns with the project-wide validation refactoring effort
packages/xrpl/src/models/transactions/setRegularKey.ts (2)
1-6: LGTM! Clean import refactoring.The imports have been properly updated to use the new validation helper functions, removing the direct ValidationError dependency.
30-34: LGTM! Clean validation refactoring.The validation logic has been effectively simplified using the new helper function while maintaining the same behavior. The code is now more consistent with other transaction validations.
packages/xrpl/test/models/offerCancel.test.ts (3)
42-42: LGTM!The test data change to 'abcd' is appropriate for verifying type validation of the
OfferSequencefield.
46-46: LGTM!The error message has been standardized to follow the new "invalid field" format, which aligns with the PR's objective of consistent error messaging.
51-51: LGTM!The error message standardization is consistently applied across both validation functions, maintaining uniform error reporting.
packages/xrpl/test/models/NFTokenCreateOffer.test.ts (3)
138-138: LGTM! Error message follows the new standardized format.The updated error message "NFTokenCreateOffer: invalid field Amount" aligns with the PR's objective of standardizing error messages across the codebase.
157-157: LGTM! Error message follows the new standardized format.The updated error message "NFTokenCreateOffer: missing field Amount" aligns with the PR's objective of standardizing error messages across the codebase.
13-217: LGTM! Test coverage is comprehensive.The test suite thoroughly covers all aspects of NFTokenCreateOffer validation:
- Valid buy and sell offers
- Zero amount validation
- Required field validation
- Field type validation
- Business rule validation
packages/xrpl/test/models/setRegularKey.test.ts (1)
41-41: LGTM! Error message updates align with standardization efforts.The changes to error messages follow the new standardized format of "invalid field [FieldName]", which aligns with the PR's objective of cleaning up transaction model validations.
Also applies to: 46-46
packages/xrpl/test/models/ticketCreate.test.ts (1)
41-53: LGTM! Error message standardization aligns with refactoring goals.The updated error message format follows the new standardized pattern using the newer helper functions, which improves consistency across the codebase. The test case properly validates both the specific
validateTicketCreateand genericvalidatefunctions.packages/xrpl/test/models/clawback.test.ts (2)
4-4: LGTM!Clean import statement following TypeScript best practices.
50-51: LGTM! Error messages follow the new standardized format.The error messages have been consistently updated to use the standardized format of "invalid field" or "missing field", improving clarity and maintainability.
Also applies to: 67-68, 149-150
packages/xrpl/src/models/transactions/ticketCreate.ts (2)
3-8: LGTM! Clean import organization.The addition of
isNumberandvalidateRequiredFieldimports aligns well with the PR's objective to utilize newer helper functions for validation.
37-38: LGTM! Improved validation using helper functions.The refactoring improves code quality by:
- Using the standardized
validateRequiredFieldhelper- Following consistent variable naming conventions
packages/xrpl/src/models/transactions/checkCreate.ts (2)
11-12: LGTM! Clean import additions.The new imports
isAmountandisStringare correctly added and align with the validation requirements.
60-64: LGTM! Clean validation refactor.The validation logic has been effectively refactored to use the new helper functions while maintaining the same validation requirements. This change:
- Simplifies the code by using
validateRequiredFieldandvalidateOptionalField- Maintains consistent validation behavior
- Aligns with the broader refactoring effort across the codebase
packages/xrpl/test/models/paymentChannelFund.test.ts (2)
43-44: LGTM! Error messages for missing fields are now more descriptive.The updated error messages now clearly indicate that fields are missing, which improves error clarity and debugging experience.
Also applies to: 48-49, 58-59, 63-64
73-74: LGTM! Error messages for invalid fields are now more descriptive.The updated error messages now clearly indicate that fields have invalid values, which improves error clarity and debugging experience.
Also applies to: 78-79, 88-89, 93-94, 103-104, 108-109
packages/xrpl/src/models/transactions/NFTokenAcceptOffer.ts (3)
4-11: LGTM! Import changes align with validation refactoring.The addition of utility functions
isAmount,isString, andvalidateOptionalFieldsupports the PR's objective of enhancing validation using newer helper functions.
79-81: LGTM! Error message follows standardized format.The error message now follows the standardized "invalid field" format while maintaining clear error context.
106-109: LGTM! Enhanced field validation while preserving core logic.The addition of
validateOptionalFieldcalls for each optional field improves type validation while maintaining the existing business rules. This aligns perfectly with the PR's refactoring objectives.packages/xrpl/test/models/baseTransaction.test.ts (3)
89-89: LGTM! Improved test cases for sequence fields.The changes to use non-numeric strings ('abcd') for testing invalid sequence fields are more appropriate than using numeric strings, as they better validate the type checking logic.
Also applies to: 117-117, 159-159
193-193: LGTM! Standardized error messages for better clarity.The error messages for Signers and Memos validation now follow the standardized format and correctly identify these as base transaction fields.
Also applies to: 211-211, 232-232
236-247: LGTM! Well-structured test for NetworkID validation.The NetworkID validation test follows the same pattern as other field validations and uses the standardized error message format.
packages/xrpl/src/models/transactions/paymentChannelFund.ts (3)
1-8: LGTM! Import changes align with refactoring goals.The imports have been updated to use the new validation helper functions, which aligns with the PR's objective of refactoring transaction model validations.
17-40: LGTM! Interface is well-defined and documented.The PaymentChannelFund interface is properly structured with clear type definitions and comprehensive JSDoc documentation for each field.
48-54: LGTM! Validation refactoring improves code maintainability.The validation logic has been successfully refactored to use the new helper functions, which:
- Maintains the same validation checks
- Improves code consistency across transaction models
- Standardizes error messaging
- Reduces code duplication
packages/xrpl/test/models/depositPreauth.test.ts (3)
115-116: LGTM! Error message standardization for Authorize field.The error message update aligns with the project's standardization efforts while maintaining clear validation feedback.
Also applies to: 120-121
138-139: LGTM! Error message standardization for Unauthorize field.The error message update maintains consistency with the project's standardization efforts.
Also applies to: 143-144
162-162: LGTM! Error message standardization for Credentials fields.The error message update appropriately standardizes validation feedback for both AuthorizeCredentials and UnauthorizeCredentials fields.
Also applies to: 174-174
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (2)
14-16: LGTM! Good use of helper functions.The addition of validation helper functions aligns with the project's direction to standardize validation logic.
141-145: LGTM! Excellent refactoring of validation logic.The changes improve code quality by:
- Using dedicated validators for required fields (NFTokenID, Amount)
- Consistent validation approach for optional fields
- Enhanced type safety through specialized type checkers
This aligns well with the project's goal of standardizing validation logic across transaction models.
packages/xrpl/test/models/offerCreate.test.ts (1)
36-87: LGTM! Good test coverage for valid scenarios.The test cases effectively verify different valid combinations of TakerGets/TakerPays fields, including:
- Currency to XRP
- XRP to Currency
- Currency to Currency
packages/xrpl/test/models/checkCreate.test.ts (5)
70-70: LGTM! Error message standardization for SendMax validation.The error message updates align with the standardization effort mentioned in the PR objectives, making validation errors more consistent across the codebase.
Also applies to: 75-75
88-88: LGTM! Improved test case for DestinationTag validation.The change from a numeric string ('1') to a non-numeric string ('abcd') provides better test coverage by ensuring proper type validation without potential numeric coercion.
110-110: LGTM! Improved test case and standardized error messages for Expiration validation.The changes enhance test coverage with a clear invalid value and standardize error messages for consistency.
Also applies to: 120-120, 125-125
144-144: LGTM! Error message standardization for InvoiceID validation.The error message updates align with the standardization effort, maintaining consistency across validation errors.
Also applies to: 149-149
11-152: LGTM! Well-structured test suite with comprehensive coverage.The test suite demonstrates:
- Comprehensive validation coverage
- Consistent test patterns
- Clear test descriptions
- Proper validation of both direct and model-level checks
packages/xrpl/src/models/transactions/accountSet.ts (3)
7-8: LGTM! Helper functions imported for improved type checking.The addition of
isNumberandisStringimports aligns with the PR's objective to utilize newer helper functions for validation.
190-193: LGTM! Improved validation using helper functions.The refactoring to use
validateOptionalFieldwith type-checking helpers (isString,isNumber) improves code consistency and maintainability.Also applies to: 203-203
183-183: LGTM! Standardized error messages.Error messages have been updated to consistently use the format "invalid field [FieldName]" for better clarity and consistency.
Also applies to: 186-186, 196-196, 199-199, 207-207, 213-213
packages/xrpl/test/models/accountSet.test.ts (2)
38-38: LGTM! Updated test assertions with standardized error messages.Test assertions have been updated to match the new standardized error message format "invalid field [FieldName]".
Also applies to: 43-43, 53-53, 58-58, 68-68, 73-73, 83-83, 88-88, 98-98, 103-103, 113-113, 118-118, 128-128, 133-133, 143-143, 148-148
123-123: LGTM! Improved test case for TransferRate validation.Changed test data from numeric string '1000000001' to non-numeric string 'abcd' provides a clearer test case for type validation.
packages/xrpl/test/models/payment.test.ts (3)
13-13: LGTM! Variable renaming improves readability.The renaming from
paymentTransactiontopaymentand the updated test description make the code more concise and easier to understand.Also applies to: 16-16, 39-41
262-274: LGTM! Good test coverage for MPT Payment.The test case effectively validates the Multi-Protocol Token Payment functionality with appropriate assertions.
276-337: LGTM! Comprehensive validation for CredentialIDs.The test suite thoroughly covers various validation scenarios for CredentialIDs, including:
- Invalid type checks
- Length validation
- Empty array validation
- Duplicate entry validation
The error messages are clear and descriptive.
packages/xrpl/src/models/transactions/clawback.ts (2)
11-11: LGTM! Import of new validation helper.The addition of
validateRequiredFieldaligns with the PR's objective to utilize newer helper functions for validation.
47-51: LGTM! Improved Amount field validation.The use of
validateRequiredFieldsimplifies the validation logic while maintaining the same validation rules.packages/xrpl/test/models/AMMVote.test.ts (1)
1-91: LGTM! The error message standardization improves consistency.The changes successfully standardize the error messages across validation tests while maintaining comprehensive test coverage. This aligns well with the PR's objective of refactoring transaction model validations.
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 64-64: 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 (2)
5-11: LGTM! Clean import organization.The imports are well-structured and correctly include all necessary validation utilities, aligning with the PR's objective of utilizing newer helper functions.
48-50: LGTM! Clean validation refactoring.The use of
validateRequiredFieldwith appropriate type checkers improves code consistency and reduces duplication.packages/xrpl/src/models/transactions/trustSet.ts (1)
6-11: LGTM! Clean import organization.The imports are well-organized, bringing in the necessary validation helper functions and type checks.
packages/xrpl/test/models/trustSet.test.ts (2)
57-62: LGTM! Consistent error message format.The error messages now follow a consistent format using "invalid field", which improves clarity and maintainability.
67-67: LGTM! More explicit invalid test values.The test values for QualityIn and QualityOut have been updated to use non-numeric strings ('abcd' and 'dcba') instead of numeric strings, making the invalid cases more explicit and easier to understand.
Also applies to: 81-81
packages/xrpl/src/models/transactions/signerListSet.ts (2)
4-9: LGTM! Clean import organization.The addition of validation helper functions aligns well with the PR's objective to enhance validation logic.
47-54: LGTM! Improved validation approach.The refactored validation logic using
validateRequiredFieldis more maintainable while preserving the same level of type safety. The validation order (SignerQuorum before SignerEntries) is logical as it allows early returns for SignerQuorum = 0.packages/xrpl/src/models/transactions/payment.ts (3)
16-16: LGTM!The addition of
isStringimport is appropriate for the new validation logic.
177-180: Great refactoring of validation logic!The use of utility functions
validateRequiredFieldandvalidateOptionalFieldimproves code consistency and maintainability. The validation order is logical, handling required fields first, followed by optional fields.Also applies to: 190-191
187-187: LGTM! Error messages are consistent and clear.The error messages follow a standardized format, making them more user-friendly and maintainable.
Also applies to: 208-208, 221-221, 226-226
packages/xrpl/src/models/transactions/NFTokenMint.ts (2)
9-10: LGTM! Good use of validation helper functions.The addition of validation utility imports aligns with the project's direction to standardize validation across transaction models.
Also applies to: 13-13
127-131: LGTM! Clean validation refactor using helper functions.The validation logic has been effectively refactored using helper functions, making the code more maintainable and consistent with other transaction models.
packages/xrpl/test/models/paymentChannelClaim.test.ts (2)
29-42: LGTM! Comprehensive test coverage for valid scenarios.The test suite thoroughly covers both complete and minimal valid PaymentChannelClaim transactions, ensuring robustness of the validation logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
44-132: LGTM! Consistent error message format across all validation scenarios.The error messages have been standardized to follow a clear pattern:
- Missing fields: "PaymentChannelClaim: missing field {FieldName}"
- Invalid fields: "PaymentChannelClaim: invalid field {FieldName}"
This change improves error message consistency and aligns with the PR's refactoring objectives.
Let's verify that this error message pattern is consistent across other transaction models:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for validation error messages in other transaction model tests rg -A 1 "ValidationError,\n.*'.*: (missing|invalid) field" packages/xrpl/test/models/Length of output: 282
I'll run a new script using the
--multilineflag to correctly capture error messages from all transaction model tests. This should help us confirm that error messages follow the expected pattern.
🏁 Script executed:
#!/bin/bash # Search for error messages matching the expected format in the transaction model tests rg -U -o "'[^']+: (missing|invalid) field [^']+'" packages/xrpl/test/models/Length of output: 43840
Consistent Error Message Format Verified
The output confirms that error messages in all transaction model tests follow the standardized pattern ("[TransactionType]: missing field ..." and "[TransactionType]: invalid field ..."). This confirms consistency across PaymentChannelClaim and other transaction models.
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/escrowCreate.ts (3)
7-7: LGTM! Import changes align with the refactoring.The addition of
isAmountandisStringimports supports the new validation approach using helper functions.Also applies to: 9-9
63-68: LGTM! Validation refactoring improves code consistency.The changes effectively utilize the new helper functions while maintaining the same validation rules. This aligns with the PR's objective to standardize validation across transaction models.
60-81: Verify validation consistency across transaction models.Let's ensure the validation pattern is consistent across other transaction files.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check validation patterns across transaction models # Look for other transaction files using similar validation patterns # Search for files with similar validation patterns echo "Files using validateRequiredField:" rg "validateRequiredField\(tx, '(Amount|Destination)'" "packages/xrpl/src/models/transactions/" echo -e "\nFiles using validateOptionalField with CancelAfter/FinishAfter:" rg "validateOptionalField\(tx, '(CancelAfter|FinishAfter)'" "packages/xrpl/src/models/transactions/" echo -e "\nFiles with similar validation error messages:" rg "Either.*must be specified" "packages/xrpl/src/models/transactions/"Length of output: 3274
Validation logic in escrowCreate is implemented consistently with our transaction models.
- All required fields ("Amount" and "Destination") and appropriate optional fields (including "CancelAfter", "FinishAfter", "Condition", and "DestinationTag") are validated as expected.
- The business rules, ensuring that either "CancelAfter" or "FinishAfter" is set and that either "Condition" or "FinishAfter" is provided, are unique to EscrowCreate and conform to our intended validation patterns.
- Searches across the transaction models confirm similar use of our validation helpers, with escrowCreate standing out only in its additional business rule checks necessary for escrow logic.
packages/xrpl/test/models/escrowCreate.test.ts (4)
85-85: LGTM! Error message standardization for Amount field.The error message update aligns with the standardization effort across the codebase.
Also applies to: 90-90
95-95: LGTM! Improved test case and error message for CancelAfter field.The test case effectively validates type checking with a clear invalid input, and the error message follows the new standard format.
Also applies to: 100-100, 105-105
125-125: LGTM! Error message standardization for Condition field.The error message update aligns with the standardization effort across the codebase.
Also applies to: 130-130
135-135: LGTM! Improved test case for DestinationTag field.The test case effectively validates type checking with a clear invalid input.
packages/xrpl/src/models/transactions/escrowCancel.ts (2)
1-8: LGTM!The import changes align with the new validation approach, adding the necessary
isNumberfunction.
35-36: LGTM!The validation logic has been simplified using the new helper function while maintaining type safety.
packages/xrpl/src/models/transactions/offerCancel.ts (2)
1-6: LGTM!The imports are well-organized and include only the necessary functions.
33-33: LGTM!The validation logic is consistent with other transaction validations, using the new helper function effectively.
packages/xrpl/src/models/transactions/AMMDelete.ts (2)
3-8: LGTM!The imports are well-organized and include all necessary validation functions.
44-45: LGTM!The validation logic for AMM-specific fields follows the consistent pattern using the new helper functions.
packages/xrpl/src/models/transactions/escrowFinish.ts (2)
1-12: LGTM!The imports comprehensively cover both required and optional field validations.
63-65: LGTM!The validation logic effectively uses both required and optional field validations, maintaining type safety while simplifying the code.
packages/xrpl/src/models/transactions/NFTokenCancelOffer.ts (1)
3-7: LGTM! Improved validation using helper function.The refactoring improves code consistency by utilizing the new
validateRequiredFieldhelper function for type validation.Also applies to: 48-48
packages/xrpl/src/models/transactions/checkCash.ts (1)
4-11: LGTM! Enhanced validation with helper functions.The refactoring improves code consistency and readability by:
- Utilizing
validateRequiredFieldfor mandatory fields.- Using
validateOptionalFieldfor optional fields.- Leveraging type-specific validation functions (
isString,isAmount).Also applies to: 50-52
packages/xrpl/test/models/AMMDelete.test.ts (1)
48-48: LGTM! Standardized error messages.The error messages have been updated to follow a consistent format using "invalid field" prefix, which aligns with the project's standardization efforts.
Also applies to: 70-70
packages/xrpl/src/models/transactions/AMMCreate.ts (1)
4-10: LGTM! Enhanced validation with helper functions.The refactoring improves code consistency by utilizing the new validation helper functions for type checking.
Also applies to: 57-59
packages/xrpl/src/models/transactions/AMMWithdraw.ts (2)
11-12: LGTM! Good modularization of validation logic.The addition of
validateOptionalFieldandvalidateRequiredFieldhelpers improves code maintainability and reduces duplication.
88-93: LGTM! Clean validation implementation.The validation logic is well-structured and maintains all necessary checks while being more concise.
packages/xrpl/src/models/transactions/AMMDeposit.ts (2)
11-12: LGTM! Consistent use of validation helpers.The imports align with the project-wide validation refactoring pattern.
90-95: LGTM! Well-structured validation logic.The validation implementation maintains all necessary checks while being more maintainable.
packages/xrpl/test/models/checkCash.test.ts (2)
37-38: LGTM! Consistent error message format.The standardized "invalid field" prefix improves error message consistency across the codebase.
Also applies to: 42-43, 58-59, 63-64, 101-102, 106-107
72-72: Verify the DeliverMin type change.The DeliverMin value has been changed from a number to a string. While this appears intentional, please verify that this aligns with the expected type in the XRPL specification.
✅ Verification successful
🌐 Web query:
What is the correct type for DeliverMin field in XRPL CheckCash transaction?💡 Result:
The
DeliverMinfield in an XRPLCheckCashtransaction has two possible formats depending on whether the payment is in XRP or tokens:
For XRP:
- A string specifying drops of XRP (e.g.,
"20000000"for 20 XRP).- Example:
"DeliverMin": "20000000"[1][4].For tokens:
- An object with three fields:
currency: The token’s currency code (e.g.,"USD").issuer: The token’s issuing account address.value: The minimum token amount (as a string).- Example:
[1][2][4]."DeliverMin": { "currency": "USD", "issuer": "rhub8VRN55s94qWKDv6jmDy1pUykJzF3wq", "value": "100" }This structure ensures compatibility with both XRP and issued currencies while enforcing validation rules (e.g., matching the Check’s original currency/issuer) [1][2].
Citations:
- 1: https://xrpl.org/docs/tutorials/how-tos/use-specialized-payment-types/use-checks/cash-a-check-for-a-flexible-amount
- 2: https://xrpl.org/docs/concepts/payment-types/checks
- 3: https://pkg.go.dev/github.com/andreimerlescu/xrpl-go/models
- 4: https://xrpl.org/docs/tutorials/how-tos/use-specialized-payment-types/use-checks/cash-a-check-for-an-exact-amount
- 5: 0056 XLS-56d: Atomic/Batch Transactions XRPL-Standards#162
- 6: https://javadoc.io/doc/org.xrpl/xrpl4j-model/2.0.0/org/xrpl/xrpl4j/model/transactions/ImmutablePayment.html
- 7: https://xrpl.org/docs/references/protocol/transactions/types/payment
- 8: https://pub.dev/documentation/xrpl_dart/latest/xrpl_dart/
- 9: Rename Amount Field to DeliverMax (to prevent incorrect handling of Partial Payments) rippled#3484
- 10: https://xrpl-py.readthedocs.io/en/stable/source/xrpl.models.transactions.html
DeliverMin Type for CheckCash Confirmed as String for XRP Payments
- The DeliverMin field is correctly set as a string for XRP transactions, per the XRPL specification.
packages/xrpl/test/models/AMMCreate.test.ts (2)
47-47: LGTM! Consistent error message format.The standardized "invalid field" prefix improves error message consistency across the test suite.
Also applies to: 69-69, 91-91
90-91: LGTM! Improved test case.Using a string value ('abcd') for TradingFee provides better test coverage for type validation.
packages/xrpl/src/models/transactions/offerCreate.ts (1)
8-10: LGTM! Great refactoring of validation logic.The changes effectively utilize the new helper functions
validateRequiredFieldandvalidateOptionalFieldto streamline the validation process. This improves code maintainability and consistency across the codebase.Also applies to: 122-125
packages/xrpl/src/models/transactions/AMMBid.ts (1)
9-10: LGTM! Good use of validation helpers.The changes effectively utilize the new helper functions to validate required and optional fields.
Also applies to: 66-69
packages/xrpl/test/models/paymentChannelCreate.test.ts (1)
49-55: LGTM! Good standardization of error messages.The changes consistently use the "invalid field" format for error messages, improving clarity and maintainability.
Also applies to: 109-115, 139-145, 184-190
packages/xrpl/test/models/escrowFinish.test.ts (1)
70-71: LGTM! Good standardization of error messages.The changes consistently use the "invalid field" format for error messages, improving clarity and maintainability across the test suite.
Also applies to: 85-86, 100-101, 113-113
packages/xrpl/test/models/AMMBid.test.ts (1)
75-75: LGTM! Error message standardization improves consistency.The changes standardize error messages to use the "invalid field" format, which aligns with the PR's objective of improving validation functions. This makes error messages more consistent and maintainable across the codebase.
Also applies to: 89-89, 103-103, 131-131, 140-140, 149-149, 160-160
packages/xrpl/test/models/AMMDeposit.test.ts (1)
91-91: LGTM! Error message standardization improves consistency.The changes successfully standardize error messages to use the "invalid field" format, maintaining consistency with other transaction validation tests while preserving test coverage.
Also applies to: 113-113, 160-160, 171-171, 183-183, 195-195
packages/xrpl/test/models/signerListSet.test.ts (1)
85-85: LGTM! Error message standardization improves consistency.The changes successfully standardize error messages for SignerEntries validation to use the "invalid field" format, maintaining consistency with other transaction validation tests.
Also applies to: 90-90
packages/xrpl/src/models/transactions/common.ts (3)
92-93: LGTM: Enhanced number validation logic.The change improves flexibility by allowing string numbers while maintaining type safety through proper validation.
375-376: LGTM: Standardized error message format.The error message now follows the consistent "invalid field" format, improving clarity and maintainability.
474-475: LGTM: Standardized error message format.The error message now follows the consistent "invalid field" format, improving clarity and maintainability.
packages/xrpl/HISTORY.md (1)
14-15: LGTM: Clear and concise changelog entry.The entry accurately reflects the improvements made to error handling in model validation, providing good documentation for users.
packages/xrpl/src/models/transactions/paymentChannelCreate.ts (4)
6-6: LGTM! Import changes align with validation refactoring.The addition of
isStringimport supports the new validation approach using helper functions.
32-36: LGTM! Well-documented interface extension.The
DestinationTagfield is properly typed and documented, following XRPL patterns for destination specification.
69-74: LGTM! Validation refactoring improves maintainability.The validation logic has been successfully refactored to use the new helper functions while maintaining complete coverage of all fields. The validation order matches the interface definition, making it easy to verify completeness.
18-75: Verify complete field validation coverage.Let's ensure all interface fields are properly validated:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify that all interface fields have corresponding validation # Get interface fields echo "Interface fields:" rg -A 20 "export interface PaymentChannelCreate" packages/xrpl/src/models/transactions/paymentChannelCreate.ts | rg "^\s+\w+:?\??" --multiline # Get validation calls echo -e "\nValidation calls:" rg "validate(Required|Optional)Field" packages/xrpl/src/models/transactions/paymentChannelCreate.tsLength of output: 815
Field validations are comprehensive.
- The specific fields for PaymentChannelCreate—Amount, Destination, DestinationTag, SettleDelay, PublicKey, and CancelAfter—are explicitly validated.
- The TransactionType field is managed by the call to validateBaseTransaction(tx), ensuring base fields are verified.
packages/xrpl/src/models/transactions/transaction.ts (1)
23-25: LGTM!The import paths have been updated to match the new camelCase file naming convention, maintaining consistency with the changes in index.ts.
🧰 Tools
🪛 ESLint
[error] 23-23: Missing file extension for "./credentialAccept"
(import/extensions)
[error] 24-24: Missing file extension for "./credentialCreate"
(import/extensions)
[error] 25-25: Missing file extension for "./credentialDelete"
(import/extensions)
🪛 GitHub Actions: Node.js CI
[error] 23-23: Cannot find module './credentialAccept' or its corresponding type declarations.
packages/xrpl/test/integration/transactions/credentialDelete.test.ts (1)
9-9: LGTM! Comprehensive test coverage.The import path has been updated to match the new camelCase convention. The test file provides excellent coverage of the credential lifecycle, including create, accept, and delete operations.
🧰 Tools
🪛 ESLint
[error] 9-9: Missing file extension for "../../../src/models/transactions/credentialDelete"
(import/extensions)
packages/xrpl/test/models/CredentialAccept.test.ts (1)
5-5: LGTM! Thorough validation testing.The import path has been updated to match the new camelCase convention. The test file provides excellent coverage of validation scenarios, including:
- Valid credential acceptance
- Missing required fields
- Invalid field types
- Field length constraints
- Field format validation
🧰 Tools
🪛 ESLint
[error] 5-5: Missing file extension for "../../src/models/transactions/credentialAccept"
(import/extensions)
packages/xrpl/test/models/CredentialDelete.test.ts (1)
27-170: Excellent test coverage for credential deletion validation.The test suite thoroughly covers all validation scenarios including:
- Valid cases
- Missing required fields
- Invalid field types
- Field length constraints
- Field format validations
packages/xrpl/test/models/CredentialCreate.test.ts (1)
28-229: Excellent test coverage for credential creation validation.The test suite thoroughly covers all validation scenarios including:
- Valid cases
- Missing required fields
- Invalid field types
- Field length constraints
- Field format validations
- Additional fields specific to credential creation (Expiration, URI)
| @@ -121,23 +124,22 @@ export interface NFTokenMintMetadata extends TransactionMetadataBase { | |||
| export function validateNFTokenMint(tx: Record<string, unknown>): void { | |||
| validateBaseTransaction(tx) | |||
|
|
|||
| validateRequiredField(tx, 'NFTokenTaxon', isNumber) | |||
| validateOptionalField(tx, 'Issuer', isAccount) | |||
| validateOptionalField(tx, 'TransferFee', isNumber) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add range validation for TransferFee.
The TransferFee field should be between 0 and 50000 inclusive (as per the field documentation). Consider adding range validation.
- validateOptionalField(tx, 'TransferFee', isNumber)
+ validateOptionalField(tx, 'TransferFee', (value): boolean => {
+ if (!isNumber(value)) {
+ return false
+ }
+ return value >= 0 && value <= 50000
+ })📝 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.
| validateOptionalField(tx, 'TransferFee', isNumber) | |
| validateOptionalField(tx, 'TransferFee', (value): boolean => { | |
| if (!isNumber(value)) { | |
| return false | |
| } | |
| return value >= 0 && value <= 50000 | |
| }) |
There was a problem hiding this comment.
@mvadari Thank you for implementing the suggested range validation for the TransferFee field! This will ensure that values stay within the valid range of 0-50000 (0.000% to 50.000%) as specified in the documentation.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| } else if (tx.Unauthorize !== undefined) { | ||
| if (typeof tx.Unauthorize !== 'string') { | ||
| throw new ValidationError('DepositPreauth: Unauthorize must be a string') | ||
| throw new ValidationError('DepositPreauth: invalid field Unauthorize') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the new helper functions for field validation.
Similar to the Authorize field, consider replacing the manual validation with validateOptionalField for better consistency and maintainability.
- } else if (tx.Unauthorize !== undefined) {
- if (typeof tx.Unauthorize !== 'string') {
- throw new ValidationError('DepositPreauth: invalid field Unauthorize')
- }
+ } else if (tx.Unauthorize !== undefined) {
+ validateOptionalField(tx, 'Unauthorize', 'string')📝 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.
| } else if (tx.Unauthorize !== undefined) { | |
| if (typeof tx.Unauthorize !== 'string') { | |
| throw new ValidationError('DepositPreauth: Unauthorize must be a string') | |
| throw new ValidationError('DepositPreauth: invalid field Unauthorize') | |
| } | |
| } else if (tx.Unauthorize !== undefined) { | |
| validateOptionalField(tx, 'Unauthorize', 'string') |
There was a problem hiding this comment.
@achowdhry-ripple I'll implement the suggested change to use validateOptionalField for the Unauthorize field validation. This will make the code more consistent with the rest of the PR's refactoring approach.
(⌐■_■)
| if (tx.Authorize !== undefined) { | ||
| if (typeof tx.Authorize !== 'string') { | ||
| throw new ValidationError('DepositPreauth: Authorize must be a string') | ||
| throw new ValidationError('DepositPreauth: invalid field Authorize') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the new helper functions for field validation.
Since this PR aims to utilize newer helper functions, consider replacing the manual validation with validateOptionalField for better consistency and maintainability.
- if (tx.Authorize !== undefined) {
- if (typeof tx.Authorize !== 'string') {
- throw new ValidationError('DepositPreauth: invalid field Authorize')
- }
+ if (tx.Authorize !== undefined) {
+ validateOptionalField(tx, 'Authorize', 'string')📝 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.
| if (tx.Authorize !== undefined) { | |
| if (typeof tx.Authorize !== 'string') { | |
| throw new ValidationError('DepositPreauth: Authorize must be a string') | |
| throw new ValidationError('DepositPreauth: invalid field Authorize') | |
| } | |
| if (tx.Authorize !== undefined) { | |
| validateOptionalField(tx, 'Authorize', 'string') |
There was a problem hiding this comment.
I agree with this, seems like this field might have been skipped in the refactor
There was a problem hiding this comment.
@achowdhry-ripple Yes, it looks like this field and a few others in this file were missed during the refactoring. In addition to Authorize, the validation for Unauthorize, AuthorizeCredentials, and UnauthorizeCredentials could also benefit from using the helper functions for consistency.
Would you like me to provide a more complete refactoring suggestion that includes all these fields?
🐰 curious rabbit pondering overlooked code
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (5)
packages/xrpl/test/models/NFTokenCancelOffer.test.ts (2)
1-21:⚠️ Potential issueAdd missing imports for test dependencies.
The test is using
assertandvalidatebut they are not imported. Add the following imports at the beginning of the file:+import { assert } from 'chai' +import { validate } from '../validate'🧰 Tools
🪛 ESLint
[error] 1-48: This module could be parsed as a valid script.
(import/unambiguous)
🪛 GitHub Actions: Node.js CI
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
23-33:⚠️ Potential issueAdd import for assertInvalid helper.
The test is using the new
assertInvalidhelper but it's not imported. Add the following import:+import { assertInvalid } from '../testUtils'The error message format aligns with the PR's objective of standardizing validation messages.
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 32-32: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/clawback.test.ts (1)
81-81:⚠️ Potential issueFix async function call in MPT Clawback test.
Similar to the previous issue, this test case also needs to handle the async validation correctly.
Apply this diff to fix the async issues:
- assert.doesNotThrow(() => validate(validClawback)) + await assert.doesNotReject(async () => validate(validClawback))Also update the test function to be async:
- it(`verifies valid MPT Clawback`, function () { + it(`verifies valid MPT Clawback`, async function () {🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 81-81: Functions that return promises must be async
[error] 81-81: Unsafe return of an
anytyped valuepackages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts (1)
1-2:⚠️ Potential issueAdd missing imports to fix pipeline failures.
The file is using undefined functions that need to be imported.
Add the following imports at the beginning of the file:
+import { validate } from '../../src/models' +import { validateMPTokenIssuanceDestroy } from '../../src/models/transactions/MPTokenIssuanceDestroy' +import { assertTxIsValid, assertTxValidationError } from '../testUtils' const TOKEN_ID = '000004C463C52827307480341125DA0577DEFC38405B0E3E'Also, define the utility functions:
+const assertValid = (tx: any): void => + assertTxIsValid(tx, validateMPTokenIssuanceDestroy) +const assertInvalid = (tx: any, message: string): void => + assertTxValidationError(tx, validateMPTokenIssuanceDestroy, message)🧰 Tools
🪛 ESLint
[error] 1-31: This module could be parsed as a valid script.
(import/unambiguous)
packages/xrpl/test/models/NFTokenBurn.test.ts (1)
1-3:⚠️ Potential issueAdd missing imports to fix pipeline failures.
The file is using undefined functions that need to be imported.
Add the following imports and utility functions:
+import { validate } from '../../src/models' +import { validateNFTokenBurn } from '../../src/models/transactions/NFTokenBurn' +import { assertTxIsValid, assertTxValidationError } from '../testUtils' const TOKEN_ID = '00090032B5F762798A53D543A014CAF8B297CFF8F2F937E844B17C9E00000003' +const assertValid = (tx: any): void => + assertTxIsValid(tx, validateNFTokenBurn) +const assertInvalid = (tx: any, message: string): void => + assertTxValidationError(tx, validateNFTokenBurn, message)🧰 Tools
🪛 ESLint
[error] 1-35: This module could be parsed as a valid script.
(import/unambiguous)
🧹 Nitpick comments (37)
packages/xrpl/test/models/AMMWithdraw.test.ts (1)
88-152: Consider using undefined assignment instead of delete operator.While the validation refactoring looks good, the use of
deleteoperator in test cases could impact performance.Apply this diff to improve performance:
- delete withdraw.Asset + withdraw.Asset = undefined- delete withdraw.Asset2 + withdraw.Asset2 = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 101-101: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts (2)
14-14: Consider using a more specific type instead ofany.Using
anytype bypasses TypeScript's type checking. Consider defining an interface for the AMMVote transaction type to improve type safety.interface AMMVoteTransaction { TransactionType: 'AMMVote' Account: string Asset: { currency: string } Asset2: { currency: string issuer: string } TradingFee: number Sequence: number }
37-37: Consider using undefined assignment instead of delete operator.For better performance, consider replacing
deleteoperations with undefined assignments:-delete vote.Asset +vote.Asset = undefined -delete vote.Asset2 +vote.Asset2 = undefined -delete vote.TradingFee +vote.TradingFee = undefinedAlso applies to: 49-49, 61-61
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts (1)
29-29: Consider avoiding the delete operator.Using the
deleteoperator can impact performance. Consider using an undefined assignment instead.- delete tx.OracleDocumentID + tx.OracleDocumentID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts (3)
4-7: Consider using proper types instead ofany.The helper functions could benefit from proper type definitions to maintain type safety. Consider using the transaction type from the validation function.
-const assertValid = (tx: any): void => +const assertValid = (tx: XChainAccountCreateCommit): void => assertTxIsValid(tx, validateXChainAccountCreateCommit) -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: Partial<XChainAccountCreateCommit>, message: string): void => assertTxValidationError(tx, validateXChainAccountCreateCommit, message)
15-15: Consider using proper type instead ofany.For better type safety and IDE support, consider using the proper transaction type.
-let tx: any +let tx: XChainAccountCreateCommit
45-45: Consider alternatives to thedeleteoperator.The
deleteoperator can impact performance. Consider using undefined assignment instead.-delete tx.XChainBridge +tx.XChainBridge = undefined -delete tx.SignatureReward +tx.SignatureReward = undefined -delete tx.Destination +tx.Destination = undefined -delete tx.Amount +tx.Amount = undefinedAlso applies to: 57-57, 75-75, 87-87
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDeposit.test.ts (3)
7-9: Consider using more specific types instead ofany.While
anyworks, using more specific types would improve type safety and maintainability. Consider creating an interface for the transaction type.-const assertValid = (tx: any): void => assertTxIsValid(tx, validateAMMDeposit) -const assertInvalid = (tx: any, message: string): void => +interface AMMDepositTransaction { + TransactionType: 'AMMDeposit' + Account: string + Asset: any // Replace with specific type + Asset2: any // Replace with specific type + [key: string]: any +} +const assertValid = (tx: AMMDepositTransaction): void => assertTxIsValid(tx, validateAMMDeposit) +const assertInvalid = (tx: Partial<AMMDepositTransaction>, message: string): void =>
22-22: Consider using the specific transaction type instead ofany.Replace
anywith the suggestedAMMDepositTransactiontype to improve type safety.-let deposit: any +let deposit: AMMDepositTransaction
77-93: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. Consider using undefined assignment instead.-delete deposit.Asset +deposit.Asset = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCreate.test.ts (2)
14-14: Consider using proper transaction type instead ofany.Replace
anywith the proper transaction type to improve type safety and catch potential issues at compile time.- let escrow: any + let escrow: EscrowCreate
36-36: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. Consider using undefined assignment instead:- delete escrow.Amount + escrow.Amount = undefined - delete escrow.Destination + escrow.Destination = undefined - delete escrow.CancelAfter - delete escrow.FinishAfter + escrow.CancelAfter = undefined + escrow.FinishAfter = undefined - delete escrow.Condition - delete escrow.FinishAfter + escrow.Condition = undefined + escrow.FinishAfter = undefinedAlso applies to: 42-42, 84-85, 94-95
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelClaim.test.ts (2)
4-7: Consider using a more specific type instead ofany.While
anyworks, using a more specific type would improve type safety. Consider creating an interface or type that represents the expected transaction structure.-const assertValid = (tx: any): void => +interface PaymentChannelClaimTx { + Account?: string + TransactionType: 'PaymentChannelClaim' + Channel?: string + Balance?: string + Amount?: string + Signature?: string + PublicKey?: string +} +const assertValid = (tx: PaymentChannelClaimTx): void => assertTxIsValid(tx, validatePaymentChannelClaim) -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: PaymentChannelClaimTx, message: string): void => assertTxValidationError(tx, validatePaymentChannelClaim, message)
15-15: Use the specific transaction type for better type safety.Replace
anywith the specific transaction type to improve type safety and code maintainability.- let channel: any + let channel: PaymentChannelClaimTxpackages/xrpl/test/models/offerCancel.test.ts (1)
47-48: Consider alternative todeleteoperator.While the error message update follows the new consistent format, static analysis flags the use of the
deleteoperator due to potential performance impact.Although the performance impact is negligible in test context, you could consider using undefined assignment instead:
-delete offer.OfferSequence +offer.OfferSequence = undefinedHowever, using
deletemore clearly expresses the intent to test a missing field. The choice is yours based on whether you prioritize performance or clarity in tests.🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelCreate.test.ts (2)
4-7: Consider using a more specific type for thetxparameter.Instead of using
any, consider creating an interface or type that represents the expected shape of a PaymentChannelCreate transaction. This would provide better type safety and documentation.-const assertValid = (tx: any): void => +interface PaymentChannelCreateTransaction { + Account: string + TransactionType: 'PaymentChannelCreate' + Amount: string + Destination: string + SettleDelay: number + PublicKey: string + CancelAfter?: number + DestinationTag?: number + SourceTag?: number +} +const assertValid = (tx: Partial<PaymentChannelCreateTransaction>): void => assertTxIsValid(tx, validatePaymentChannelCreate) -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: Partial<PaymentChannelCreateTransaction>, message: string): void => assertTxValidationError(tx, validatePaymentChannelCreate, message)
15-15: Use the specific transaction type instead ofany.Replace
anywith the suggestedPaymentChannelCreateTransactiontype for better type safety.- let channel: any + let channel: PaymentChannelCreateTransactionpackages/xrpl/test/models/DIDSet.test.ts (2)
4-6: Consider using a more specific type instead ofany.While
anyworks, using a more specific type would improve type safety. Consider creating an interface for the transaction type or using the existing transaction type from the codebase.-const assertValid = (tx: any): void => assertTxIsValid(tx, validateDIDSet) -const assertInvalid = (tx: any, message: string): void => +interface DIDSetTransaction { + Account: string + Data?: string + DIDDocument?: string + URI?: string + Fee: string + Flags: number + Sequence: number + TransactionType: 'DIDSet' +} +const assertValid = (tx: DIDSetTransaction): void => assertTxIsValid(tx, validateDIDSet) +const assertInvalid = (tx: DIDSetTransaction, message: string): void =>
51-60: Consider using undefined assignment instead of delete operator.While the current implementation works, the delete operator can have performance implications. Although this is test code where performance is less critical, for consistency with best practices, consider using undefined assignments.
- delete tx.Data - delete tx.DIDDocument - delete tx.URI + tx.Data = undefined + tx.DIDDocument = undefined + tx.URI = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 54-54: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/trustSet.test.ts (2)
14-14: Consider using a more specific type instead ofany.Using
anytype bypasses TypeScript's type checking. Consider creating an interface or type for the TrustSet transaction to improve type safety.Example:
interface TrustSetTransaction { TransactionType: 'TrustSet' Account: string LimitAmount: { currency: string issuer: string value: string } QualityIn?: number QualityOut?: number Flags?: { tfSetFreeze?: boolean tfSetDeepFreeze?: boolean } }
40-41: Consider using undefined assignment instead of delete operator.Using the
deleteoperator can impact performance. Consider using an undefined assignment instead.-delete trustSet.LimitAmount +trustSet.LimitAmount = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/NFTokenModify.test.ts (1)
11-61: Consider adding test for URI length validation.While the test coverage is good for basic URI validation, consider adding a test case for maximum URI length validation, which is a common requirement for NFT metadata.
Here's a suggested test case:
it(`throws w/ URI exceeding maximum length`, function () { const invalid = { TransactionType: 'NFTokenModify', Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm', NFTokenID: TOKEN_ID, Fee: '5000000', Sequence: 2470665, URI: stringToHex('x'.repeat(257)), // Assuming 256 bytes is the maximum } as any assertInvalid(invalid, 'NFTokenModify: URI length cannot exceed 256 bytes') })🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/baseTransaction.test.ts (1)
11-248: Consider adding more test cases for edge cases.The test coverage is comprehensive, but consider adding test cases for:
- Multiple invalid fields in a single transaction
- Boundary values for numeric fields
- Empty strings vs null/undefined values
Would you like me to help generate these additional test cases?
packages/xrpl/test/models/permissionedDomainDelete.test.ts (1)
31-31: Replace delete operator with undefined assignment.The
deleteoperator can impact performance. Consider using an undefined assignment instead.-delete tx.DomainID +tx.DomainID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDelete.test.ts (1)
37-38: Consider using undefined assignment instead of delete operator.For better performance, consider replacing
deleteoperations with undefined assignments:- delete ammDelete.Asset + ammDelete.Asset = undefined - delete ammDelete.Asset2 + ammDelete.Asset2 = undefinedAlso applies to: 49-50
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts (1)
29-30: Consider using undefined assignment instead of delete operator.For better performance, consider replacing the
deleteoperation with an undefined assignment:- delete ticketCreate.TicketCount + ticketCreate.TicketCount = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelFund.test.ts (1)
33-34: Consider using undefined assignments instead of delete operators.For better performance, consider replacing multiple
deleteoperations with undefined assignments:- delete channel.Expiration + channel.Expiration = undefined - delete channel.Amount + channel.Amount = undefined - delete channel.Channel + channel.Channel = undefinedAlso applies to: 39-40, 45-46
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainModifyBridge.test.ts (1)
44-45: Consider using undefined assignment instead of delete operator.For better performance, consider replacing the
deleteoperation with an undefined assignment:- delete tx.XChainBridge + tx.XChainBridge = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateBridge.test.ts (1)
44-44: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. While this is less critical in test code, consider using undefined assignment for consistency with static analysis recommendations.- delete tx.XChainBridge + tx.XChainBridge = undefined - delete tx.SignatureReward + tx.SignatureReward = undefinedAlso applies to: 56-56
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCommit.test.ts (1)
43-43: Consider using undefined assignment instead of delete operator.Similar to other test files, replace
deletewith undefined assignments for better performance.- delete tx.XChainBridge + tx.XChainBridge = undefined - delete tx.XChainClaimID + tx.XChainClaimID = undefined - delete tx.Amount + tx.Amount = undefinedAlso applies to: 55-55, 73-73
🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMCreate.test.ts (1)
36-36: Consider using undefined assignment instead of delete operator.Replace
deletewith undefined assignments for consistency with static analysis recommendations.- delete ammCreate.Amount + ammCreate.Amount = undefined - delete ammCreate.Amount2 + ammCreate.Amount2 = undefined - delete ammCreate.TradingFee + ammCreate.TradingFee = undefinedAlso applies to: 48-48, 60-60
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainClaim.test.ts (1)
44-44: Consider using undefined assignment instead of delete operator.Replace
deletewith undefined assignments for better performance and consistency.- delete tx.XChainBridge + tx.XChainBridge = undefined - delete tx.XChainClaimID + tx.XChainClaimID = undefined - delete tx.Destination + tx.Destination = undefined - delete tx.Amount + tx.Amount = undefinedAlso applies to: 56-56, 68-68, 86-86
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMClawback.test.ts (1)
45-45: Consider using undefined assignment for better performance.Multiple instances of the
deleteoperator are used to remove properties. While acceptable in tests, consider using undefined assignment for better performance:-delete ammClawback.Amount +ammClawback.Amount = undefinedAlso applies to: 55-55, 73-73, 91-91
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts (1)
68-68: Consider using undefined assignment for better performance.Similar to other test files, replace delete operator with undefined assignment:
-delete bid.Asset +bid.Asset = undefinedAlso applies to: 80-80
🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts (1)
4-7: LGTM! Consider performance improvements.The introduction of helper functions and explicit typing improves code maintainability and type safety.
For better performance, consider replacing
deleteoperations withundefinedassignments in test cases:-delete tx.Amount +tx.Amount = undefinedThis change would address the static analysis warnings about the
deleteoperator's performance impact.Also applies to: 15-15
packages/xrpl/test/testUtils.ts (2)
60-69: Consider improving type safety and documentation.The function implementation looks good, but consider these improvements:
- Replace
anywith a more specific type for thetxparameter- Add example usage in the JSDoc comment
/** * Check that a transaction error validation fails properly. * + * @example + * ```typescript + * const tx = { TransactionType: 'Payment', ... } + * assertTxIsValid(tx, validatePayment) + * ``` * * @param tx The transaction that should fail validation. * @param validateTx The transaction-specific validation function (e.g. `validatePayment`). */ -export function assertTxIsValid(tx: any, validateTx: (tx: any) => void): void { +export function assertTxIsValid( + tx: Record<string, unknown>, + validateTx: (tx: Record<string, unknown>) => void +): void { assert.doesNotThrow(() => validateTx(tx)) assert.doesNotThrow(() => validate(tx)) }
71-85: Consider improving type safety and documentation.The function implementation looks good, but consider these improvements:
- Replace
anywith a more specific type for thetxparameter- Add example usage in the JSDoc comment
/** * Check that a transaction error validation fails properly. * + * @example + * ```typescript + * const tx = { TransactionType: 'Payment' } // missing required field + * assertTxValidationError(tx, validatePayment, 'invalid field') + * ``` * * @param tx The transaction that should fail validation. * @param validateTx The transaction-specific validation function (e.g. `validatePayment`). * @param errorMessage The error message that should be included in the error. */ export function assertTxValidationError( - tx: any, - validateTx: (tx: any) => void, + tx: Record<string, unknown>, + validateTx: (tx: Record<string, unknown>) => void, errorMessage: string, ): void { assert.throws(() => validateTx(tx), ValidationError, errorMessage) assert.throws(() => validate(tx), ValidationError, errorMessage) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
.vscode/settings.json(1 hunks)packages/xrpl/test/models/AMMBid.test.ts(5 hunks)packages/xrpl/test/models/AMMClawback.test.ts(2 hunks)packages/xrpl/test/models/AMMCreate.test.ts(2 hunks)packages/xrpl/test/models/AMMDelete.test.ts(2 hunks)packages/xrpl/test/models/AMMDeposit.test.ts(5 hunks)packages/xrpl/test/models/AMMVote.test.ts(2 hunks)packages/xrpl/test/models/AMMWithdraw.test.ts(5 hunks)packages/xrpl/test/models/CredentialAccept.test.ts(2 hunks)packages/xrpl/test/models/CredentialCreate.test.ts(2 hunks)packages/xrpl/test/models/CredentialDelete.test.ts(2 hunks)packages/xrpl/test/models/DIDDelete.test.ts(2 hunks)packages/xrpl/test/models/DIDSet.test.ts(2 hunks)packages/xrpl/test/models/MPTokenAuthorize.test.ts(6 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(9 hunks)packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts(1 hunks)packages/xrpl/test/models/MPTokenIssuanceSet.test.ts(3 hunks)packages/xrpl/test/models/NFTokenAcceptOffer.test.ts(6 hunks)packages/xrpl/test/models/NFTokenBurn.test.ts(1 hunks)packages/xrpl/test/models/NFTokenCancelOffer.test.ts(2 hunks)packages/xrpl/test/models/NFTokenCreateOffer.test.ts(9 hunks)packages/xrpl/test/models/NFTokenMint.test.ts(6 hunks)packages/xrpl/test/models/NFTokenModify.test.ts(5 hunks)packages/xrpl/test/models/XChainAccountCreateCommit.test.ts(2 hunks)packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts(2 hunks)packages/xrpl/test/models/XChainAddClaimAttestation.test.ts(2 hunks)packages/xrpl/test/models/XChainClaim.test.ts(2 hunks)packages/xrpl/test/models/XChainCommit.test.ts(2 hunks)packages/xrpl/test/models/XChainCreateBridge.test.ts(2 hunks)packages/xrpl/test/models/XChainCreateClaimID.test.ts(2 hunks)packages/xrpl/test/models/XChainModifyBridge.test.ts(2 hunks)packages/xrpl/test/models/accountDelete.test.ts(5 hunks)packages/xrpl/test/models/accountSet.test.ts(2 hunks)packages/xrpl/test/models/baseTransaction.test.ts(8 hunks)packages/xrpl/test/models/checkCancel.test.ts(3 hunks)packages/xrpl/test/models/checkCash.test.ts(5 hunks)packages/xrpl/test/models/checkCreate.test.ts(7 hunks)packages/xrpl/test/models/clawback.test.ts(8 hunks)packages/xrpl/test/models/depositPreauth.test.ts(9 hunks)packages/xrpl/test/models/escrowCancel.test.ts(2 hunks)packages/xrpl/test/models/escrowCreate.test.ts(3 hunks)packages/xrpl/test/models/escrowFinish.test.ts(5 hunks)packages/xrpl/test/models/offerCancel.test.ts(2 hunks)packages/xrpl/test/models/offerCreate.test.ts(3 hunks)packages/xrpl/test/models/oracleDelete.test.ts(2 hunks)packages/xrpl/test/models/oracleSet.test.ts(3 hunks)packages/xrpl/test/models/payment.test.ts(5 hunks)packages/xrpl/test/models/paymentChannelClaim.test.ts(3 hunks)packages/xrpl/test/models/paymentChannelCreate.test.ts(2 hunks)packages/xrpl/test/models/paymentChannelFund.test.ts(2 hunks)packages/xrpl/test/models/permissionedDomainDelete.test.ts(2 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(2 hunks)packages/xrpl/test/models/setRegularKey.test.ts(2 hunks)packages/xrpl/test/models/signerListSet.test.ts(3 hunks)packages/xrpl/test/models/ticketCreate.test.ts(1 hunks)packages/xrpl/test/models/trustSet.test.ts(2 hunks)packages/xrpl/test/testUtils.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/xrpl/test/models/checkCreate.test.ts
- packages/xrpl/test/models/setRegularKey.test.ts
- packages/xrpl/test/models/escrowFinish.test.ts
- packages/xrpl/test/models/accountSet.test.ts
- packages/xrpl/test/models/signerListSet.test.ts
- packages/xrpl/test/models/checkCash.test.ts
- packages/xrpl/test/models/CredentialDelete.test.ts
- packages/xrpl/test/models/checkCancel.test.ts
- packages/xrpl/test/models/NFTokenAcceptOffer.test.ts
- packages/xrpl/test/models/CredentialAccept.test.ts
- packages/xrpl/test/models/depositPreauth.test.ts
- packages/xrpl/test/models/NFTokenCreateOffer.test.ts
- packages/xrpl/test/models/accountDelete.test.ts
- packages/xrpl/test/models/CredentialCreate.test.ts
- packages/xrpl/test/models/offerCreate.test.ts
🧰 Additional context used
🧠 Learnings (9)
packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/clawback.test.ts (1)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/permissionedDomainSet.test.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
packages/xrpl/test/models/XChainCommit.test.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:18-18
Timestamp: 2025-01-16T04:11:37.316Z
Learning: Test libraries like chai should not be used in source code. For runtime checks in browser-compatible code, use vanilla JS error throwing instead of assertion libraries.
packages/xrpl/test/models/MPTokenAuthorize.test.ts (3)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
packages/xrpl/test/models/permissionedDomainDelete.test.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
packages/xrpl/test/models/AMMClawback.test.ts (1)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
🪛 GitHub Actions: Node.js CI
packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
[error] 16-16: Cannot find name 'assert'.
[error] 16-16: Cannot find name 'validate'.
[error] 25-25: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/NFTokenBurn.test.ts
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/clawback.test.ts
[warning] 4-4: 'assertValid' is assigned a value but never used. Allowed unused vars must match /^_/u
[error] 25-25: Functions that return promises must be async
[error] 25-25: Unsafe return of an any typed value
[error] 81-81: Functions that return promises must be async
[error] 81-81: Unsafe return of an any typed value
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts
[error] 19-19: Cannot find name 'assert'.
[error] 19-19: Cannot find name 'validate'.
[error] 29-29: Cannot find name 'assert'.
[error] 29-29: Cannot find name 'validate'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 65-65: Cannot find name 'assertInvalid'.
[error] 69-69: Cannot find name 'assertInvalid'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 19-19: Cannot find name 'assert'.
[error] 19-19: Cannot find name 'validate'.
[error] 29-29: Cannot find name 'assert'.
[error] 29-29: Cannot find name 'validate'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 65-65: Cannot find name 'assertInvalid'.
[error] 69-69: Cannot find name 'assertInvalid'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 19-19: Cannot find name 'assert'.
[error] 19-19: Cannot find name 'validate'.
[error] 29-29: Cannot find name 'assert'.
[error] 29-29: Cannot find name 'validate'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 65-65: Cannot find name 'assertInvalid'.
[error] 69-69: Cannot find name 'assertInvalid'.
[error] 39-39: Cannot find name 'assert'.
[error] 39-39: Cannot find name 'validate'.
packages/xrpl/test/models/NFTokenMint.test.ts
[error] 4-4: Unable to resolve path to module '../../src/models/transactions/nftokenMint'
[error] 4-4: Missing file extension for '../../src/models/transactions/nftokenMint'
[error] 4-4: '../../src/models/transactions/nftokenMint' is not found
packages/xrpl/test/models/NFTokenCancelOffer.test.ts
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 20-20: Cannot find name 'assert'.
[error] 20-20: Cannot find name 'validate'.
[error] 32-32: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/MPTokenAuthorize.test.ts
[error] 2-2: Cannot find module '../../src/models/transactions/mptokenAuthorize' or its corresponding type declarations.
[error] 2-2: Cannot find module '../../src/models/transactions/mptokenAuthorize' or its corresponding type declarations.
[error] 2-2: Cannot find module '../../src/models/transactions/mptokenAuthorize' or its corresponding type declarations.
packages/xrpl/test/models/NFTokenModify.test.ts
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
[error] 22-22: Cannot find name 'assert'.
[error] 22-22: Cannot find name 'validate'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 46-46: Cannot find name 'assertInvalid'.
[error] 59-59: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/baseTransaction.test.ts
[error] 4-4: Cannot find module '../../src/models/transactions/BaseTransaction' or its corresponding type declarations.
🪛 Biome (1.9.4)
packages/xrpl/test/models/AMMWithdraw.test.ts
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 101-101: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainClaim.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateBridge.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainModifyBridge.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/DIDSet.test.ts
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 54-54: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 61-61: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 87-87: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 80-80: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 65-65: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 83-83: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 137-137: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 149-149: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 167-167: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCommit.test.ts
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainDelete.test.ts
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCreate.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 84-84: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 85-85: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateClaimID.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMClawback.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 71-71: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 143-143: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 197-197: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 215-215: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 233-233: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/payment.test.ts
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 83-83: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMCreate.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 60-60: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDelete.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDeposit.test.ts
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCancel.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/offerCancel.test.ts
[error] 47-47: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleSet.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 114-114: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 128-128: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 135-135: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 142-142: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 149-149: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelClaim.test.ts
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelCreate.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 63-63: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelFund.test.ts
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/trustSet.test.ts
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 ESLint
packages/xrpl/test/models/NFTokenMint.test.ts
[error] 4-4: Missing file extension for "../../src/models/transactions/nftokenMint"
(import/extensions)
packages/xrpl/test/models/MPTokenAuthorize.test.ts
[error] 2-2: Missing file extension for "../../src/models/transactions/mptokenAuthorize"
(import/extensions)
packages/xrpl/test/models/baseTransaction.test.ts
[error] 4-4: Missing file extension for "../../src/models/transactions/BaseTransaction"
(import/extensions)
🪛 Gitleaks (8.21.2)
packages/xrpl/test/models/MPTokenAuthorize.test.ts
3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (70)
packages/xrpl/test/models/AMMWithdraw.test.ts (2)
3-9: LGTM! Great refactoring of test utilities.The introduction of
assertTxIsValidandassertTxValidationErrorhelper functions improves code maintainability and readability by centralizing validation logic.
22-86: LGTM! Clean refactoring of validation assertions.The changes consistently apply the new validation pattern across all test cases while maintaining comprehensive test coverage.
packages/xrpl/test/models/NFTokenCancelOffer.test.ts (1)
35-46: LGTM! Test case properly validates empty NFTokenOffers field.The test case correctly validates the empty NFTokenOffers field scenario and follows the standardized error message format.
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
[error] 45-45: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/AMMVote.test.ts (3)
4-6: LGTM! Helper functions improve code maintainability.The introduction of
assertValidandassertInvalidfunctions effectively reduces code duplication and improves readability.
16-30: LGTM! Well-structured test setup.The
beforeEachblock provides a comprehensive setup with all required fields and valid values for testing AMMVote transactions.
32-82: LGTM! Comprehensive test coverage.The test suite thoroughly covers all validation scenarios including valid transactions, missing fields, invalid types, and boundary conditions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 61-61: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateClaimID.test.ts (4)
2-7: LGTM! Well-structured helper functions.The introduction of
assertValidandassertInvalidhelper functions improves code maintainability by encapsulating common validation logic. This aligns well with the PR's objective of enhancing validation functions.
14-37: LGTM! Comprehensive test setup.The test setup provides a complete valid transaction object with all required fields, and the explicit
anytype annotation improves type clarity.
39-78: LGTM! Comprehensive test coverage.The test cases thoroughly validate both successful and error scenarios with consistent error message formatting. This aligns well with the PR's objective of enhancing validation functions.
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
44-44: Regarding thedeleteoperator usage in tests.While static analysis flags the use of
deleteoperator for performance reasons, in this test context:
- Performance impact is negligible as this is test code.
- Using
deleteclearly communicates the intent of testing missing fields.- Using
undefinedassignment might not trigger the same validation behavior.Therefore, I recommend keeping the current implementation.
Also applies to: 56-56, 68-68
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts (4)
2-6: LGTM! Good use of utility functions.The introduction of
assertTxIsValidandassertTxValidationErroralong with helper functions improves code reusability and readability. The type annotations are properly used.
14-14: LGTM! Good type annotation.Adding the type annotation for the test case variable improves code clarity.
25-25: LGTM! Good use of the utility function.Using
assertValidsimplifies the test case and makes it more readable.
31-31: LGTM! Good test case improvements.The test cases are more readable with the
assertInvalidutility function, and using 'abcd' as an invalid value makes the test more explicit.Also applies to: 35-37
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts (1)
40-96: LGTM! Comprehensive test coverage.The test cases thoroughly validate:
- Valid transaction structure
- Required fields (XChainBridge, SignatureReward, Destination, Amount)
- Field type validations
- Consistent error message format
The refactored assertions using
assertValidandassertInvalidimprove code readability.🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 75-75: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 87-87: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDeposit.test.ts (2)
40-75: LGTM! Well-structured positive test cases.The refactoring improves readability while maintaining comprehensive test coverage for valid transaction scenarios.
77-146: LGTM! Well-structured negative test cases with standardized error messages.The refactoring improves readability and maintains consistent error message format across all validation failures.
🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 90-90: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCreate.test.ts (2)
2-6: LGTM! Well-structured utility functions.The introduction of
assertValidandassertInvalidutility functions improves code reusability and readability while following the DRY principle.
65-69: Add missing validation test for FinishAfter.The test case only checks
validateEscrowCreate()but notvalidate(). For consistency with other test cases, add the missing validation test.Apply this diff to add the missing test:
it(`invalid FinishAfter`, function () { escrow.FinishAfter = 'abcd' assertInvalid(escrow, 'EscrowCreate: invalid field FinishAfter') + assert.throws( + () => validate(escrow), + ValidationError, + 'EscrowCreate: invalid field FinishAfter', + ) })packages/xrpl/test/models/paymentChannelClaim.test.ts (1)
45-79: LGTM! The test cases are comprehensive and well-structured.Note: The static analysis warning about using the
deleteoperator can be safely ignored in test code. While usingundefinedassignment would be more performant, thedeleteoperator better expresses the intent of removing fields for testing purposes, and the performance impact is negligible in test scenarios.🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCancel.test.ts (6)
2-6: LGTM! Good use of utility functions.The introduction of
assertValidandassertInvalidhelper functions improves code reusability and makes the test cases more maintainable.
14-14: LGTM! Appropriate use ofletdeclaration.The change from
consttoletis correct as the variable is modified in test cases.
26-26: LGTM! Good use of the new helper function.The change to use
assertValidmakes the test case more concise and readable.
32-32: LGTM! Consistent use of helper functions.The change maintains consistency with other test cases.
38-38: LGTM! Consistent error message format.The error messages now follow a consistent format: "missing/invalid field {fieldName}".
Also applies to: 44-44, 50-50, 56-56
36-36: Note aboutdeleteoperator usage.Static analysis flagged the use of
deleteoperator for potential performance impact. However, in test files:
- Performance is not critical
deleteprovides clearer intent than setting to undefined- Tests often need to simulate missing properties
Therefore, the current implementation is acceptable.
Also applies to: 42-42
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/offerCancel.test.ts (5)
2-6: LGTM! Good refactoring of test utilities.The introduction of
assertValidandassertInvalidutility functions improves code readability and follows the DRY principle by encapsulating common validation logic.
14-14: LGTM! Type annotation is appropriate for test context.Using
anytype provides the necessary flexibility for testing various scenarios.
29-29: No issues with this minor formatting change.
33-33: LGTM! Test cases are now more concise.The use of
assertValidutility function improves test readability while maintaining the same validation logic.Also applies to: 38-38
42-43: LGTM! Test case and error message improvements.The changes improve test clarity by:
- Using a more obvious invalid value ('abcd' instead of a number)
- Following the new consistent error message format
packages/xrpl/test/models/paymentChannelCreate.test.ts (2)
32-102: Well-structured test cases with consistent error messages!The test cases are comprehensive and follow a consistent pattern for error messages. They cover:
- Required fields (Amount, Destination, SettleDelay, PublicKey)
- Optional fields (CancelAfter, DestinationTag, SourceTag)
- Both valid and invalid scenarios
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 63-63: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
37-39: Regarding thedeleteoperator usage in tests.The static analysis tool flags the use of the
deleteoperator for potential performance impacts. However, since this is test code:
- The performance impact is negligible
- Using
deletemakes the intent clearer than setting fields toundefined- It actually removes the property, which is what we want to test
Therefore, we can safely ignore these warnings.
Also applies to: 45-45, 51-51, 57-57, 63-63
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/DIDSet.test.ts (2)
14-31: LGTM! Clean test setup and valid case verification.The test setup provides a comprehensive valid transaction object, and the assertion is clear and concise.
33-49: LGTM! Comprehensive invalid case testing.The test cases effectively verify validation for each field with consistent error message formatting.
packages/xrpl/test/models/trustSet.test.ts (5)
2-6: LGTM! Great refactoring of test assertions.The introduction of
assertTxIsValidandassertTxValidationErrorhelper functions improves code reusability and maintainability across test files.
36-36: LGTM! Clean test assertion.The use of
assertValidhelper function makes the test more readable and consistent with other test files.
46-46: LGTM! Consistent error message format.The error message follows the new standardized format: "invalid field ...".
50-51: LGTM! Clear test case for non-numeric QualityIn.The test value 'abcd' clearly demonstrates the validation of non-numeric input, and the error message follows the standardized format.
55-56: LGTM! Clear test case for non-numeric QualityOut.The test value 'dcba' clearly demonstrates the validation of non-numeric input, and the error message follows the standardized format.
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (1)
48-51: LGTM! Good use of the new assertInvalid helper.The changes align with the PR objective by utilizing the newer
assertInvalidhelper function for validation testing. The error messages are consistent and descriptive.Also applies to: 65-65, 69-69
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 48-48: Cannot find name 'assertInvalid'.
[error] 48-48: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/NFTokenMint.test.ts (3)
7-9: LGTM! Well-designed helper functions.The new helper functions
assertValidandassertInvalidimprove code maintainability by:
- Reducing code duplication
- Providing consistent validation behavior
- Making test cases more readable
29-29: LGTM! Consistent URI handling.The URI field is consistently converted to hex format using
stringToHexacross all test cases, improving code consistency.Also applies to: 44-44, 60-60
32-32: LGTM! Clear and consistent error handling.The test cases effectively use the new helper functions with clear, descriptive error messages that follow a consistent format:
- Valid case verification
- Missing field validation
- Account/Issuer equality check
- URI format validation
Also applies to: 47-47, 63-63, 79-79, 95-95
packages/xrpl/test/models/NFTokenModify.test.ts (2)
19-19: LGTM! Consistent use of string conversion utility.The change from
convertStringToHextostringToHexaligns with the standardization effort across the codebase.
33-33: LGTM! Improved error handling consistency.The changes to use
assertInvalidwith descriptive error messages align with the standardization of error handling across transaction tests.Also applies to: 46-46, 59-59
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 33-33: Cannot find name 'assertInvalid'.
[error] 33-33: Cannot find name 'assertInvalid'.
packages/xrpl/test/models/MPTokenAuthorize.test.ts (3)
5-8: LGTM! Well-structured helper functions.The helper functions
assertValidandassertInvalidare well-designed:
- Improve code reusability and readability
- Follow TypeScript best practices with type annotations
- Encapsulate validation logic effectively
10-16: LGTM! Clear test setup and documentation.The test setup is well-structured with:
- Clear constant naming
- Descriptive JSDoc comments
- Appropriate scope for the test suite
18-63: LGTM! Comprehensive test coverage.The test cases are well-structured and provide good coverage:
- Test valid transaction variations
- Verify required field validation
- Use realistic test data
- Follow the new assertion pattern consistently
This aligns with the learnings that indicate we don't need to test certain negative cases (e.g., invalid address formats) as they're covered elsewhere.
packages/xrpl/test/models/baseTransaction.test.ts (6)
89-89: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the Sequence field, which is a better test for type validation.
117-117: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the LastLedgerSequence field, which is a better test for type validation.
159-159: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the TicketSequence field, which is a better test for type validation.
193-193: LGTM! Error messages standardized.The error messages have been updated to use the standardized format "invalid field Signers", which improves clarity and consistency.
Also applies to: 211-211
232-232: LGTM! Error message standardized.The error message has been updated to use the standardized format "invalid field Memos", which improves clarity and consistency.
240-240: LGTM! Test case updated to use non-numeric string.The test case now uses a non-numeric string to validate the NetworkID field, which is a better test for type validation.
packages/xrpl/test/models/permissionedDomainDelete.test.ts (1)
15-24: LGTM! Type casting approach aligns with best practices.The use of
as anyfor type casting aligns with the preferred approach for validation tests, as it better represents real-world scenarios where the validate method needs to handle arbitrary JSON input from users.packages/xrpl/test/models/AMMDelete.test.ts (1)
44-45: LGTM! Consistent error message format.The error messages follow a consistent pattern using "invalid field" format, which aligns with the PR's refactoring goals.
Also applies to: 56-57
packages/xrpl/test/models/ticketCreate.test.ts (1)
40-43: LGTM! Clear validation boundaries in error messages.The error messages clearly specify the valid range for TicketCount (1 to 250), which helps developers understand the constraints.
packages/xrpl/test/models/paymentChannelFund.test.ts (1)
41-42: LGTM! Consistent error handling pattern.The error messages follow a consistent pattern using "missing field" or "invalid field" format, which aligns with the PR's refactoring goals.
Also applies to: 47-48, 53-54, 59-60, 65-66
packages/xrpl/test/models/XChainModifyBridge.test.ts (1)
46-47: LGTM! Consistent error handling pattern.The error messages follow a consistent pattern using "missing field" or "invalid field" format, which aligns with the PR's refactoring goals.
Also applies to: 52-53, 58-59, 64-67
packages/xrpl/test/models/XChainCreateBridge.test.ts (1)
4-7: LGTM! Helper functions improve test readability.The introduction of
assertValidandassertInvalidhelper functions enhances code organization by encapsulating validation logic.packages/xrpl/test/models/AMMCreate.test.ts (1)
71-81: LGTM! Comprehensive boundary testing for TradingFee.Good test coverage for TradingFee validation, including both upper and lower bounds.
packages/xrpl/test/models/XChainClaim.test.ts (1)
79-83: LGTM! Good validation for DestinationTag type.The test case properly validates that DestinationTag must be a number.
packages/xrpl/test/models/permissionedDomainSet.test.ts (2)
7-10: LGTM! Test utilities improve code maintainability.The introduction of
assertValidandassertInvalidutility functions enhances test readability and reduces code duplication.
18-18: Type declaration aligns with testing best practices.Using
anytype for test input is appropriate here as it better represents real-world scenarios where validation needs to handle arbitrary JSON input.packages/xrpl/test/models/AMMBid.test.ts (1)
37-58: LGTM! Comprehensive validation testing for AuthAccounts.The test cases thoroughly cover various scenarios for AuthAccounts validation, including:
- Array length limits
- Invalid account types
- Null/undefined handling
- Sender address validation
packages/xrpl/test/models/oracleSet.test.ts (1)
6-8: LGTM! Well-structured test refactoring.The helper functions and explicit typing enhance code maintainability while preserving test coverage.
Also applies to: 16-16
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts (1)
4-11: LGTM! Consistent with codebase standards.The helper functions and explicit typing align with the established patterns across the test suite.
Also applies to: 19-19
packages/xrpl/test/models/payment.test.ts (1)
7-9: LGTM! Improved naming and consistent structure.The renaming from
paymentTransactiontopaymentenhances readability, while the helper functions and explicit typing maintain consistency with other test files.Also applies to: 17-17
packages/xrpl/test/testUtils.ts (1)
8-13: LGTM!The imports are well-organized and include the necessary dependencies for the new validation assertion functions.
| it('throws on invalid DIDDelete', function () { | ||
| tx.FakeField = 'blah' | ||
| assert.doesNotThrow(() => validateDIDDelete(tx)) | ||
| assert.doesNotThrow(() => validate(tx)) | ||
| assertValid(tx) | ||
| }) |
There was a problem hiding this comment.
Fix incorrect assertion in invalid field test.
The test case is checking for invalid fields but using assertValid instead of assertInvalid. This will not properly verify that invalid fields are rejected.
Define and use assertInvalid function to properly test invalid fields:
+const assertInvalid = (tx: any, message: string): void =>
+ assertTxValidationError(tx, validateDIDDelete, message)
it('throws on invalid DIDDelete', function () {
tx.FakeField = 'blah'
- assertValid(tx)
+ assertInvalid(tx, 'DIDDelete: invalid field FakeField')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('throws on invalid DIDDelete', function () { | |
| tx.FakeField = 'blah' | |
| assert.doesNotThrow(() => validateDIDDelete(tx)) | |
| assert.doesNotThrow(() => validate(tx)) | |
| assertValid(tx) | |
| }) | |
| const assertInvalid = (tx: any, message: string): void => | |
| assertTxValidationError(tx, validateDIDDelete, message) | |
| it('throws on invalid DIDDelete', function () { | |
| tx.FakeField = 'blah' | |
| assertInvalid(tx, 'DIDDelete: invalid field FakeField') | |
| }) |
There was a problem hiding this comment.
Is there a way in JS to ensure that an object doesn't have any other keys?
There was a problem hiding this comment.
@mvadari Yes, there are a few ways to ensure an object only has specific keys in JavaScript:
- Compare keys with expected set:
function validateKeys(obj, expectedKeys) {
const actualKeys = Object.keys(obj)
const hasOnlyExpectedKeys = actualKeys.every(key => expectedKeys.includes(key))
const hasAllExpectedKeys = expectedKeys.every(key => actualKeys.includes(key))
return hasOnlyExpectedKeys && hasAllExpectedKeys
}- Using TypeScript with strict object literal checks:
interface ValidTransaction {
Account: string;
Fee: string;
Flags: number;
Sequence: number;
TransactionType: string;
}
// TypeScript will error if extra properties are present
const tx: ValidTransaction = {
Account: "...",
Fee: "10",
Flags: 2147483648,
Sequence: 4,
TransactionType: "DIDDelete",
extraField: "invalid" // Error: Object literal may only specify known properties
}- Create a sanitized object with only allowed keys:
function sanitizeObject(obj, allowedKeys) {
return Object.fromEntries(
Object.entries(obj).filter(([key]) => allowedKeys.includes(key))
)
}For your test cases, I'd recommend using TypeScript interfaces with strict checks as it provides compile-time validation, or implementing a validation function that explicitly checks for unexpected keys if you need runtime validation.
✏️ Learnings added
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/DIDDelete.test.ts:28-31
Timestamp: 2025-02-12T23:30:40.497Z
Learning: In JavaScript/TypeScript transaction validation tests, object key validation can be performed using:
1. Object.keys() comparison with expected set
2. TypeScript interfaces with strict object literal checks
3. Object sanitization by filtering to allowed keys only
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
packages/xrpl/test/models/NFTokenCreateOffer.test.ts (1)
5-8: Consider adding type constraints to helper functions.The helper functions use
anytype which might be too permissive. Consider adding a more specific type to improve type safety.-const assertValid = (tx: any): void => +const assertValid = (tx: Partial<NFTokenCreateOffer>): void => -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: Partial<NFTokenCreateOffer>, message: string): void =>packages/xrpl/test/models/NFTokenAcceptOffer.test.ts (1)
9-12: Consider documenting test data and adding edge cases.The hardcoded offer IDs (
NFTOKEN_BUY_OFFERandNFTOKEN_SELL_OFFER) could benefit from documentation explaining their significance or format.Consider adding test cases for:
- Maximum allowed broker fee
- Invalid offer ID formats
packages/xrpl/test/models/NFTokenBurn.test.ts (1)
1-6: Consider using a more specific type instead ofany.The utility functions use the
anytype which reduces type safety. Consider using a more specific type that represents the transaction structure.-const assertValid = (tx: any): void => assertTxIsValid(tx, validateNFTokenBurn) -const assertInvalid = (tx: any, message: string): void => +interface NFTokenBurnTransaction { + TransactionType: 'NFTokenBurn' + NFTokenID?: string + Account?: string + Fee?: string + Sequence?: number + Flags?: number +} + +const assertValid = (tx: NFTokenBurnTransaction): void => assertTxIsValid(tx, validateNFTokenBurn) +const assertInvalid = (tx: NFTokenBurnTransaction, message: string): void =>packages/xrpl/test/models/NFTokenModify.test.ts (1)
21-28: Consider using proper TypeScript types instead ofany.Using
as anybypasses TypeScript's type checking. Consider creating an interface for the NFTokenModify transaction type.+interface NFTokenModify { + TransactionType: 'NFTokenModify' + Account: string + NFTokenID: string + Fee: string + Sequence: number + URI?: string +} - const validNFTokenModify = { + const validNFTokenModify: NFTokenModify = { TransactionType: 'NFTokenModify', Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm', NFTokenID: TOKEN_ID, Fee: '5000000', Sequence: 2470665, URI: stringToHex('http://xrpl.org'), - } as any + }packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
147-153: Consider improving type handling to avoid type assertion.The type assertion with eslint disable could be avoided by properly narrowing the type of
tx.Flags.Consider this alternative implementation:
- } else if (typeof tx.Flags === 'object') { - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- Needed here - isSellOffer = (tx.Flags as Record<string, unknown>).tfSellNFToken === true + } else if (tx.Flags !== null && typeof tx.Flags === 'object') { + const flags = tx.Flags as { tfSellNFToken?: boolean } + isSellOffer = flags.tfSellNFToken === truepackages/xrpl/test/models/MPTokenIssuanceSet.test.ts (2)
5-8: Consider using a more specific type than 'any'.While the helper functions are well-structured, using
anytype reduces type safety. Consider creating an interface that represents the common structure of transaction objects.-const assertValid = (tx: any): void => +interface BaseTransaction { + TransactionType: string + Account: string + [key: string]: unknown +} +const assertValid = (tx: BaseTransaction): void =>
49-77: Consider adding more edge cases.While the current test coverage is good, consider adding tests for:
- Invalid MPTokenIssuanceID format
- Invalid Account address format
- Invalid Holder address format
- Maximum/minimum allowed flag values
Would you like me to help generate additional test cases for these scenarios?
packages/xrpl/test/models/clawback.test.ts (1)
15-23: Improve type safety in test data.The test data uses
as anytype assertions which bypasses TypeScript's type checking. Consider using proper types to improve type safety.Apply this diff to improve type safety:
+import { IssuedCurrencyAmount, MPTokenAmount } from '../../src/models/common' +import { ClawbackTransaction } from '../../src/models/transactions/clawback' - } as any + } as Partial<ClawbackTransaction & { Amount: IssuedCurrencyAmount }> - } as any + } as Partial<ClawbackTransaction & { Amount: MPTokenAmount }>Also applies to: 70-78
packages/xrpl/test/models/NFTokenCancelOffer.test.ts (3)
4-7: Consider using a more specific type instead ofany.The helper functions could benefit from a more specific type annotation instead of
any. This would provide better type safety and documentation.-const assertValid = (tx: any): void => +const assertValid = (tx: NFTokenCancelOffer): void => -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: Partial<NFTokenCancelOffer>, message: string): void =>
18-29: LGTM! Consider improving type safety.The test case is well-structured and covers a valid scenario. However, the type assertion
as anycould be replaced with proper typing.- } as any + } as NFTokenCancelOffer
31-54: Consider adding more edge cases and improving type safety.The test cases effectively cover basic validation scenarios. However:
- Type assertions could be more specific
- Additional edge cases could be tested, such as:
- Invalid NFTokenOffers format
- Duplicate offers
- Maximum number of offers
Would you like me to help generate additional test cases for these scenarios?
packages/xrpl/test/models/NFTokenMint.test.ts (1)
7-9: Consider using a more specific type instead ofany.The helper functions use the
anytype which bypasses TypeScript's type checking. Consider creating an interface or type that represents the expected transaction structure.-const assertValid = (tx: any): void => assertTxIsValid(tx, validateNFTokenMint) -const assertInvalid = (tx: any, message: string): void => +interface NFTokenMintTransaction { + TransactionType: 'NFTokenMint' + Account: string + Fee: string + Sequence: number + Flags?: any + NFTokenTaxon?: number + Issuer?: string + TransferFee?: number + URI?: string +} + +const assertValid = (tx: NFTokenMintTransaction): void => assertTxIsValid(tx, validateNFTokenMint) +const assertInvalid = (tx: NFTokenMintTransaction, message: string): void =>packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (2)
1-10: Consider using a more specific type instead ofany.While the utility functions are well-structured, using
anytype reduces type safety. Consider creating an interface or type that represents the expected transaction structure.-const assertValid = (tx: any): void => +interface MPTokenIssuanceCreateTransaction { + TransactionType: 'MPTokenIssuanceCreate' + Account: string + MaximumAmount?: string + AssetScale?: number + TransferFee?: number + Flags?: number + MPTokenMetadata?: string +} +const assertValid = (tx: MPTokenIssuanceCreateTransaction): void => assertTxIsValid(tx, validateMPTokenIssuanceCreate) -const assertInvalid = (tx: any, message: string): void => +const assertInvalid = (tx: Partial<MPTokenIssuanceCreateTransaction>, message: string): void => assertTxValidationError(tx, validateMPTokenIssuanceCreate, message)
61-85: Consider adding test cases for decimal values in MaximumAmount.While the current test cases cover integer bounds and invalid formats, consider adding test cases for decimal values to ensure complete validation coverage.
it('throws w/ decimal MaximumAmount', function () { const invalid = { TransactionType: 'MPTokenIssuanceCreate', Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm', MaximumAmount: '100.5', } as any assertInvalid(invalid, 'MPTokenIssuanceCreate: Invalid MaximumAmount') })packages/xrpl/src/models/utils/index.ts (1)
29-31: Consider throwing an error for invalid flag types.Silently returning
falsewhen an object is passed might mask programming errors. Consider throwing a descriptive error to help developers identify incorrect usage during development.- if (typeof flags === 'object') { - return false - } + if (typeof flags === 'object') { + throw new Error('isFlagEnabled expects a number, received an object') + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(1 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts(1 hunks)packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts(2 hunks)packages/xrpl/src/models/transactions/common.ts(5 hunks)packages/xrpl/src/models/utils/index.ts(1 hunks)packages/xrpl/test/models/CredentialAccept.test.ts(2 hunks)packages/xrpl/test/models/CredentialCreate.test.ts(2 hunks)packages/xrpl/test/models/CredentialDelete.test.ts(2 hunks)packages/xrpl/test/models/MPTokenAuthorize.test.ts(6 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(9 hunks)packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts(3 hunks)packages/xrpl/test/models/MPTokenIssuanceSet.test.ts(6 hunks)packages/xrpl/test/models/NFTokenAcceptOffer.test.ts(11 hunks)packages/xrpl/test/models/NFTokenBurn.test.ts(3 hunks)packages/xrpl/test/models/NFTokenCancelOffer.test.ts(4 hunks)packages/xrpl/test/models/NFTokenCreateOffer.test.ts(12 hunks)packages/xrpl/test/models/NFTokenMint.test.ts(6 hunks)packages/xrpl/test/models/NFTokenModify.test.ts(5 hunks)packages/xrpl/test/models/baseTransaction.test.ts(7 hunks)packages/xrpl/test/models/clawback.test.ts(9 hunks)packages/xrpl/test/models/offerCreate.test.ts(4 hunks)packages/xrpl/test/models/signerListSet.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts
- packages/xrpl/test/models/baseTransaction.test.ts
- packages/xrpl/test/models/signerListSet.test.ts
- packages/xrpl/test/models/offerCreate.test.ts
- packages/xrpl/test/models/CredentialAccept.test.ts
- packages/xrpl/test/models/CredentialCreate.test.ts
- packages/xrpl/test/models/CredentialDelete.test.ts
- packages/xrpl/src/models/transactions/common.ts
🧰 Additional context used
🧠 Learnings (7)
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/MPTokenAuthorize.test.ts (3)
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/clawback.test.ts (2)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (28)
packages/xrpl/test/models/NFTokenCreateOffer.test.ts (1)
18-205: LGTM! Well-structured test cases with comprehensive coverage.The test suite effectively validates:
- Valid buyside and sellside offers
- Account/Owner/Destination relationship rules
- Required fields validation
- Amount validation for buy/sell offers
packages/xrpl/test/models/NFTokenAcceptOffer.test.ts (2)
1-8: LGTM! Well-structured helper functions.The introduction of
assertValidandassertInvalidhelper functions improves code reusability and readability. The TypeScript type annotations are properly used.
19-174: LGTM! Comprehensive test coverage with consistent error messaging.The test suite thoroughly covers all essential validation scenarios for NFTokenAcceptOffer transactions, including:
- Valid cases with buy/sell offers
- Required field validation
- Broker fee validation
- Edge cases and error conditions
The error messages follow a consistent format, aligning with the broader standardization effort.
packages/xrpl/test/models/NFTokenBurn.test.ts (2)
27-27: LGTM!The refactored assertion using
assertValidimproves code readability while maintaining the same test coverage.
39-39: LGTM!The refactored assertion using
assertInvalidimproves code readability while maintaining proper error validation.packages/xrpl/test/models/NFTokenModify.test.ts (3)
1-4: Add missing imports to fix pipeline failures.The
assertimport from 'chai' is still missing, which was mentioned in the previous review.
6-9: LGTM! Well-structured utility functions.The utility functions are concise, properly typed, and follow the single responsibility principle.
19-69: LGTM! Comprehensive test coverage.The test suite effectively covers:
- Happy path with valid data
- Required field validation
- Empty string validation
- Format validation
The error messages are clear and descriptive.
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (3)
14-16: LGTM! Imports align with refactoring objectives.The addition of helper function imports aligns with the PR objectives to utilize newer validation functions.
141-145: LGTM! Validation refactoring improves consistency.The use of helper functions for both required and optional field validation improves code consistency and maintainability. This aligns with the retrieved learning about utilizing existing helper functions.
155-159: LGTM! Control flow is more explicit.The use of
isSellOffermakes the control flow more readable while maintaining the original validation behavior.packages/xrpl/test/models/MPTokenAuthorize.test.ts (3)
5-8: LGTM! Helper functions improve test readability.The introduction of
assertValidandassertInvalidhelper functions enhances code maintainability by encapsulating common validation patterns.
25-25: LGTM! Test cases effectively validate MPTokenAuthorize scenarios.The test cases cover various valid configurations:
- Basic MPTokenAuthorize
- MPTokenAuthorize with Holder
- MPTokenAuthorize with Flags
- MPTokenAuthorize with both Holder and Flags
Also applies to: 34-34, 43-43, 53-53
62-62: LGTM! Error case is properly tested.The test case verifies that the transaction fails when the required
MPTokenIssuanceIDfield is missing.packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
72-72: LGTM! Improved flag initialization with nullish coalescing.The use of the nullish coalescing operator (
??) ensures thatflagsdefaults to0whentx.Flagsis undefined, preventing potential runtime issues.packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
160-162: LGTM! Improved flag initialization with nullish coalescing.The use of the nullish coalescing operator (
??) ensures thatflagsdefaults to0whentx.Flagsis undefined, preventing potential runtime issues.packages/xrpl/test/models/MPTokenIssuanceSet.test.ts (2)
1-3: LGTM! Clean import refactoring.The imports have been nicely refactored to use specific validation utilities instead of generic assertions, aligning with the PR objectives of utilizing newer helper functions.
18-47: LGTM! Well-structured test cases for valid scenarios.The test cases effectively cover various valid configurations including:
- Basic transaction with flags
- Transaction with holder field
- Transaction without flags
packages/xrpl/test/models/clawback.test.ts (2)
4-6: Improve type safety and remove unused code.The utility functions have type safety issues and potential unused code.
Apply this diff to improve type safety and remove unused code:
-const assertValid = (tx: any): void => assertTxIsValid(tx, validateClawback) -const assertInvalid = (tx: any, message: string): void => +import { ClawbackTransaction } from '../../src/models/transactions/clawback' + +// Remove unused function if not needed +const assertInvalid = (tx: Partial<ClawbackTransaction>, message: string): void => assertTxValidationError(tx, validateClawback, message)
13-124: Test implementation looks good!The test cases:
- Cover essential validation scenarios
- Use correct synchronous validation as per learnings
- Have consistent error messages
- Include both regular and MPT Clawback scenarios
packages/xrpl/test/models/NFTokenMint.test.ts (2)
4-4: Fix the import path to resolve pipeline failures.The import path is causing pipeline failures. Add the file extension to resolve the module.
-import { validateNFTokenMint } from '../../src/models/transactions/NFTokenMint' +import { validateNFTokenMint } from '../../src/models/transactions/NFTokenMint.ts'
16-97: Test implementation looks good!The test cases are well-structured and comprehensive:
- Clear test descriptions
- Good coverage of validation scenarios
- Consistent use of
stringToHexfor URI fields- Clear and descriptive error messages
packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts (3)
18-31: LGTM! Comprehensive test case for valid MPTokenIssuanceCreate.The test case effectively validates all required fields with appropriate values and uses the new
stringToHexutility for MPTokenMetadata.
33-59: LGTM! Well-structured validation tests for MPTokenMetadata.The test cases properly validate empty string and non-hex format scenarios with clear error messages.
87-132: LGTM! Thorough validation tests for TransferFee.The test cases effectively cover:
- Negative values
- Values exceeding the maximum limit
- Missing flag validation
- Invalid flag combinations
packages/xrpl/src/models/utils/index.ts (3)
21-23: LGTM! Documentation updates are clear and consistent.The parameter documentation has been updated to match the new parameter name and remains clear and concise.
33-33: LGTM! Consistent use of BigInt for bitwise operations.The conversion of all operands to BigInt ensures consistent handling of large numbers in bitwise operations.
25-28: Verify the architectural decision to accept objects.While the parameter renaming to
flagsfollows camelCase conventions, expanding the type to accept objects seems to contradict the function's purpose of performing bitwise operations on flags. This might indicate that the function is being used in contexts where flag operations aren't appropriate.Let's check how this function is being used in the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all usages of isFlagEnabled to understand the contexts where objects might be passed rg "isFlagEnabled\(" -A 2Length of output: 2797
Architectural Decision Confirmation:
isFlagEnabledAccepting Object TypesThe investigation confirms that
isFlagEnabledis consistently used with both numeric and object flag representations. For example:
- In
partialPayment.tsandpayment.ts, the function is invoked withtx.Flags, which may be a number in some cases.- In
NFTokenCreateOffer.ts, there is explicit handling where an object is passed and its type is checked.- Additional usages in files like
MPTokenIssuanceSet.tsandMPTokenIssuanceCreate.tsalso follow this pattern.This indicates an intentional architectural decision to support heterogeneous flag representations. While the function’s name may imply bitwise operations on numbers, extending the type to include objects aligns with how flag data is structured across the codebase.
Recommendations:
- Ensure the implementation of
isFlagEnabledcan safely handle both numbers and objects.- Consider adding documentation or comments to clarify the dual role of the function and guide future maintenance.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
91-99: 🛠️ Refactor suggestionUpdate documentation to reflect new behavior
The function now accepts strings that can be converted to numbers, but this isn't reflected in the documentation or type signature. Consider:
- Updating the JSDoc to document this behavior
- Renaming the function to
isNumberLikeor similar- Updating the return type to reflect that the input might need conversion
/** * Verify the form and type of a number at runtime. + * Accepts both numbers and strings that can be converted to numbers. * * @param num - The object to check the form and type of. + * @example + * isNumber(123) // true + * isNumber("123") // true + * isNumber("abc") // false * @returns Whether the number is properly formed. */ -export function isNumber(num: unknown): num is number { +export function isNumberLike(num: unknown): num is number | string { return typeof num === 'number' || !Number.isNaN(Number(num)) }
🧹 Nitpick comments (2)
packages/xrpl/src/models/transactions/common.ts (2)
378-382: Add validation for GlobalFlags structureWhen
Flagsis a record, consider validating its structure against theGlobalFlagsinterface to prevent invalid flag objects.validateOptionalField( common, 'Flags', - (inp) => isNumber(inp) || isRecord(inp), + (inp) => isNumber(inp) || (isRecord(inp) && validateGlobalFlags(inp)), ) +function validateGlobalFlags(flags: Record<string, unknown>): boolean { + // Add validation logic for GlobalFlags interface + return Object.keys(flags).every((key) => /* validate flag properties */); +}
475-481: Consider using a configuration object patternInstead of disabling the
max-paramsrule, consider refactoring to use a configuration object. This would improve maintainability and make the function easier to extend.-export function validateCredentialsList( - credentials: unknown, - transactionType: string, - isStringID: boolean, - maxCredentials: number, -): void { +interface CredentialsValidationConfig { + credentials: unknown; + transactionType: string; + isStringID: boolean; + maxCredentials: number; +} + +export function validateCredentialsList(config: CredentialsValidationConfig): void { + const { credentials, transactionType, isStringID, maxCredentials } = config;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/ripple-binary-codec/src/types/st-array.ts(1 hunks)packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts(2 hunks)packages/xrpl/src/models/transactions/common.ts(6 hunks)packages/xrpl/src/models/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/models/utils/index.ts
🧰 Additional context used
🧠 Learnings (1)
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
🔇 Additional comments (7)
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (4)
13-18: LGTM! Good use of helper functions.The addition of validation helper functions aligns with the PR objectives and follows the established pattern of utilizing existing helper functions for type checking and validation.
142-146: LGTM! Improved validation logic.The refactoring enhances the validation by:
- Using
validateRequiredFieldfor required fields (NFTokenID, Amount)- Using
validateOptionalFieldfor optional fields (Owner, Expiration, Destination)- Standardizing error handling and type checking
148-153: LGTM! Enhanced flag handling.The changes improve the flag handling by:
- Using a clear boolean variable for sell offer status
- Adding type-safe checks for both number and record flag types
- Making the logic more explicit and maintainable
155-159: LGTM! Simplified control flow.The use of the
isSellOfferboolean variable makes the code more readable and maintainable while preserving the original logic.packages/ripple-binary-codec/src/types/st-array.ts (1)
15-26:✅ Verification successful
LGTM! Enhanced type safety with null check.
The addition of the null check improves the robustness of the type guard by preventing potential null pointer exceptions in subsequent validation steps.
Let's verify the impact of this change on existing code:
🏁 Script executed:
#!/bin/bash # Search for any tests or code that might be affected by the stricter null check rg -l "STArray.from\(" | xargs rg "null|undefined" -C 3Length of output: 1825
Enhanced null safety in type guard verified.
The addition of the null check in the
isObjectsfunction indeed makes the validation more robust by guarding against null/undefined values. Our verification (using the script output that searched for usage patterns aroundSTArray.from) did not reveal any problematic instances where the stricter null check might negatively affect existing behaviors. Please proceed with confidence, but do keep an eye on areas that might pass null values intoSTArray.fromduring further integration testing.packages/xrpl/src/models/transactions/common.ts (2)
71-79: Well-documented type guard function!The addition of JSDoc comments and exporting the
isRecordfunction improves code clarity and reusability.
387-387: Consistent error message formatThe standardization of error messages to include "field" improves consistency and clarity.
Also applies to: 397-397, 486-486
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/xrpl/tools/createValidateTests.js (2)
131-131: Consider using template literals for error messages.The error messages could be more maintainable using template literals instead of string concatenation.
- assertInvalid(tx, '${txName}: missing field ${param}') + assertInvalid(tx, `${txName}: missing field ${param}`) - assertInvalid(tx, '${txName}: invalid field ${param}') + assertInvalid(tx, `${txName}: invalid field ${param}`)Also applies to: 141-141
112-113: Address the TODO comment for flag checking.The comment indicates that flag checking support is missing. This could be important for complete validation coverage.
Would you like me to help implement the flag checking support or create an issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/src/models/transactions/common.ts(6 hunks)packages/xrpl/test/models/baseTransaction.test.ts(9 hunks)packages/xrpl/tools/createValidateTests.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/src/models/transactions/common.ts
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: snippets (22.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: integration (18.x)
- GitHub Check: unit (22.x)
- GitHub Check: browser (18.x)
- GitHub Check: unit (20.x)
- GitHub Check: unit (18.x)
- GitHub Check: build-and-lint (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
packages/xrpl/test/models/baseTransaction.test.ts (5)
3-4: LGTM! Import path updated correctly.The import path has been updated to use the common module, which aligns with the refactoring objectives.
62-69: LGTM! Good addition of flag map validation test.The new test case ensures compatibility with object-based flag representations, which is a valuable addition to the test suite.
98-98: LGTM! Consistent use of invalid test inputs.The test cases now consistently use 'abcd' as an invalid input for numeric fields (Sequence, LastLedgerSequence, TicketSequence, NetworkID), which improves test readability and maintainability.
Also applies to: 126-126, 182-182, 263-263
150-162: LGTM! Good addition of invalid flags test.The new test case for invalid flags complements the flag map validation test, ensuring robust validation of the Flags field.
216-216: LGTM! Consistent error message format.Error messages for Signers and Memos have been updated to consistently use the "invalid field" format, improving error message clarity.
Also applies to: 234-234, 255-255
packages/xrpl/tools/createValidateTests.js (2)
150-155: LGTM! Well-structured validation helpers.The introduction of
assertValidandassertInvalidhelper functions with TypeScript annotations improves code organization and reusability. The transaction-specific import also enhances maintainability.
22-29:✅ Verification successful
Verify test generation for all transaction models.
Let's ensure the test generation works correctly for all transaction models.
🏁 Script executed:
#!/bin/bash # Description: Verify test generation for all transaction models # Find all transaction model files echo "Checking test generation for all transaction models..." fd --type f ".*\.ts$" "packages/xrpl/src/models/transactions" --exec sh -c ' model=$(basename {} .ts) echo "Testing $model..." node packages/xrpl/tools/createValidateTests.js "$model" > /dev/null if [ $? -eq 0 ]; then echo "✓ $model" else echo "✗ $model failed" exit 1 fi 'Length of output: 2977
Verification Passed: Test Generation for All Transaction Models is Working Correctly
The verification script executed against all transaction models produced successful results without any failures, confirming that the
createValidateTests.jstool is generating tests as expected. There are no further changes required.
- All transaction models in
packages/xrpl/src/models/transactionswere processed without error.- The current implementation of
main(modelName)(lines 22-29) accurately loads and processes the models for test generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/xrpl/test/models/AMMCreate.test.ts (3)
4-6: Consider using a more specific type for thetxparameter.The
anytype bypasses TypeScript's type checking. Consider using a more specific type that represents the transaction structure.-const assertValid = (tx: any): void => assertTxIsValid(tx, validateAMMCreate) -const assertInvalid = (tx: any, message: string): void => +const assertValid = (tx: Partial<AMMCreate>): void => assertTxIsValid(tx, validateAMMCreate) +const assertInvalid = (tx: Partial<AMMCreate>, message: string): void => assertTxValidationError(tx, validateAMMCreate, message)
14-14: Consider using a more specific type for theammCreatevariable.Using
anytype reduces type safety. Consider using a more specific type that represents the AMMCreate transaction structure.- let ammCreate: any + let ammCreate: Partial<AMMCreate>
36-36: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. Consider using undefined assignment instead.- delete ammCreate.Amount + ammCreate.Amount = undefined - delete ammCreate.Amount2 + ammCreate.Amount2 = undefined - delete ammCreate.TradingFee + ammCreate.TradingFee = undefinedAlso applies to: 48-48, 60-60
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts (2)
14-14: Consider using a more specific type instead ofany.Using
anyreduces type safety. Consider creating an interface or type for the AMMVote transaction structure.Example:
interface AMMVoteTransaction { TransactionType: 'AMMVote' Account: string Asset: { currency: string } Asset2: { currency: string issuer: string } TradingFee: number Sequence: number } let vote: Partial<AMMVoteTransaction>
37-37: Consider using undefined assignment instead of delete operator.The
deleteoperator can impact performance. While this is test code and performance is less critical, consider using undefined assignments for consistency with best practices.Example:
-delete vote.Asset +vote.Asset = undefinedAlso applies to: 49-49, 61-61
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts (1)
14-14:❓ Verification inconclusive
Consider using a more specific type instead of
any.While the explicit type declaration improves clarity, using
anybypasses TypeScript's type checking benefits. Consider using a more specific type that represents the expected structure of a TicketCreate transaction.Let me help you find the appropriate type:
🏁 Script executed:
#!/bin/bash # Search for TicketCreate type definition ast-grep --pattern 'interface $name { TransactionType: "TicketCreate" $$$ }'Length of output: 82
Action: Consider replacing
anywith a more specific TicketCreate type
- In
packages/xrpl/test/models/ticketCreate.test.tsat line 14, the variableticketCreateis currently typed asany.- While making the type explicit improves readability, using
anybypasses TypeScript’s safety features.- Our initial search for an existing
TicketCreateinterface (withTransactionType: "TicketCreate") returned no results. Please manually verify if a dedicated type exists elsewhere in the codebase.- If no such type is found, consider defining a specific interface (for example,
interface TicketCreateTx { TransactionType: "TicketCreate"; ... }) that accurately reflects the transaction’s structure.packages/xrpl/src/models/transactions/common.ts (1)
105-122: Add missing parameter descriptions in JSDoc.The JSDoc comments are missing descriptions for the
lowerandupperparameters./** * Verify the form and type of a number at runtime. Includes * numbers in the form of strings (e.g. `"7"`). * - * @param num - The object to check the form and type of. - * @param lower - * @param upper + * @param num - The object to check the form and type of + * @param lower - The lower bound (inclusive) for the number + * @param upper - The upper bound (inclusive) for the number * @returns Whether the number is properly formed. */🧰 Tools
🪛 ESLint
[error] 109-109: Expected @param names to be "lower, upper". Got "num, lower, upper".
(jsdoc/check-param-names)
[error] 110-110: Missing JSDoc @param "lower" description.
(jsdoc/require-param-description)
[error] 111-111: Missing JSDoc @param "upper" description.
(jsdoc/require-param-description)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/AMMCreate.ts(2 hunks)packages/xrpl/src/models/transactions/AMMVote.ts(2 hunks)packages/xrpl/src/models/transactions/accountSet.ts(2 hunks)packages/xrpl/src/models/transactions/common.ts(7 hunks)packages/xrpl/src/models/transactions/ticketCreate.ts(2 hunks)packages/xrpl/test/models/AMMCreate.test.ts(2 hunks)packages/xrpl/test/models/AMMVote.test.ts(2 hunks)packages/xrpl/test/models/accountSet.test.ts(2 hunks)packages/xrpl/test/models/ticketCreate.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/xrpl/src/models/transactions/ticketCreate.ts
- packages/xrpl/test/models/accountSet.test.ts
- packages/xrpl/src/models/transactions/accountSet.ts
- packages/xrpl/src/models/transactions/AMMVote.ts
- packages/xrpl/HISTORY.md
- packages/xrpl/src/models/transactions/AMMCreate.ts
🧰 Additional context used
🪛 ESLint
packages/xrpl/src/models/transactions/common.ts
[error] 109-109: Expected @param names to be "lower, upper". Got "num, lower, upper".
(jsdoc/check-param-names)
[error] 110-110: Missing JSDoc @param "lower" description.
(jsdoc/require-param-description)
[error] 111-111: Missing JSDoc @param "upper" description.
(jsdoc/require-param-description)
🪛 Biome (1.9.4)
packages/xrpl/test/models/AMMCreate.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 60-60: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 61-61: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts
[error] 29-29: 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 (9)
- GitHub Check: snippets (22.x)
- GitHub Check: unit (18.x)
- GitHub Check: snippets (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: browser (18.x)
- GitHub Check: integration (20.x)
- GitHub Check: snippets (18.x)
- GitHub Check: integration (18.x)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (13)
packages/xrpl/test/models/AMMCreate.test.ts (1)
31-81: LGTM! Comprehensive test coverage.The test cases thoroughly cover:
- Valid transaction validation
- Required field validation
- Type validation for Amount, Amount2, and TradingFee
- Boundary value checks for TradingFee
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 48-48: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 60-60: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts (2)
2-6: LGTM! Great refactoring of test utilities.The introduction of
assertValidandassertInvalidutility functions improves code reusability and readability while reducing duplication. The TypeScript type annotations enhance type safety.
44-45: Consider maintaining specific error messages for better debugging.While standardizing error messages to "invalid field" format aligns with the PR objectives, it reduces the specificity of error messages. This could make debugging more challenging as developers won't immediately know why a field is invalid.
Consider keeping specific error messages or adding error codes to maintain debuggability while still standardizing the format. For example:
const errorMessage = 'AMMVote: invalid field Asset (must be a Currency)'Also applies to: 56-57, 68-69, 74-75, 80-81
packages/xrpl/test/models/ticketCreate.test.ts (6)
2-2: LGTM!The import of test utility functions aligns with the PR objective of utilizing newer helper functions for validation.
4-6: LGTM! Well-structured utility functions.The utility functions provide a clean abstraction for validation assertions, reducing code duplication and improving maintainability.
25-25: LGTM!The test case is simplified using the new utility function while maintaining the same validation logic.
30-30: LGTM! Consistent error handling.The error test cases are well-structured with:
- Consistent use of the new
assertInvalidfunction- Standardized error message format
- Comprehensive coverage of edge cases
Also applies to: 35-35, 40-40, 45-45, 50-50
29-29: LGTM! Keep usingdeletefor clarity.While the static analysis tool flags the use of the
deleteoperator for performance reasons, in test code, the clarity of intent is more important than performance. Thedeleteoperator clearly expresses the test's purpose of validating missing field behavior.🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
21-21: LGTM!The addition of a trailing comma is a good practice for cleaner diffs when adding new properties.
packages/xrpl/src/models/transactions/common.ts (4)
71-79: LGTM! Good documentation improvement.The addition of JSDoc comments and exporting the
isRecordfunction improves code documentation and reusability.
396-400: LGTM! Good improvements to validation and error messages.The changes improve the code in several ways:
- More flexible Flags validation that accepts both number and record types
- Standardized error messages for better consistency
Also applies to: 405-405, 415-415
493-505: LGTM! Good error message standardization.The error message has been standardized to match the project's format, and the ESLint disable comment is well-justified.
98-103:❓ Verification inconclusive
Verify the impact of relaxed number validation.
The function now accepts both numbers and string numbers (e.g., "7"). While this makes the function more flexible, we should verify that this change doesn't affect existing code that might rely on stricter type checking.
🏁 Script executed:
#!/bin/bash # Search for all usages of isNumber to verify the impact rg -A 5 "isNumber\(" --type tsLength of output: 5239
Action Required: Verify relaxed number validation across usage sites
The updated
isNumberfunction now accepts both numbers and numeric strings (e.g.,"7"). Our search shows that many call sites already account for flexible inputs (e.g., usingisNumber(inp) || isString(inp)), suggesting the change was intentional in those contexts. However, notice that in some locations—such as in the validation of fields like'DestinationTag'and in tests expecting strict number types—this relaxed check might inadvertently accept strings where a strict number is expected.
- Verify that functions like
validateOptionalField(tx, 'DestinationTag', isNumber)and assertions (e.g.,assert.isNumber(seq)) correctly handle string inputs or safely convert them.- Revisit any use sites where a strict numeric type is assumed to ensure no side effects are introduced by converting valid string representations into numbers.
Please run thorough tests to confirm that these changes maintain the intended behavior across the codebase.
| throw new ValidationError(`AMMBid: invalid AuthAccounts`) | ||
| throw new ValidationError(`AMMBid: invalid field AuthAccounts`) | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- used for null check | ||
| // @ts-expect-error -- used for null check | ||
| if (authAccount.AuthAccount.Account == null) { | ||
| throw new ValidationError(`AMMBid: invalid AuthAccounts`) | ||
| throw new ValidationError(`AMMBid: invalid field AuthAccounts`) | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment -- used for null check | ||
| // @ts-expect-error -- used for null check | ||
| if (typeof authAccount.AuthAccount.Account !== 'string') { | ||
| throw new ValidationError(`AMMBid: invalid AuthAccounts`) | ||
| throw new ValidationError(`AMMBid: invalid field AuthAccounts`) |
There was a problem hiding this comment.
Is it worth wrapping all this in an isAuthAccounts function and then passing it into validateOptionalField?
There was a problem hiding this comment.
That sounds like a good idea, but I'd prefer to leave that for a separate PR. This PR was just about getting the low hanging fruit.
| let isSellOffer = false | ||
| if (isNumber(tx.Flags)) { | ||
| isSellOffer = isFlagEnabled(tx.Flags, NFTokenCreateOfferFlags.tfSellNFToken) | ||
| } else if (isRecord(tx.Flags)) { | ||
| isSellOffer = tx.Flags.tfSellNFToken === true |
There was a problem hiding this comment.
seems like this flag check is happening a lot, might be worth making a helper function for
const isTfMPTUnlock = typeof flags === 'number' ? isFlagEnabled(flags, MPTokenIssuanceSetFlags.tfMPTUnlock) : flags.tfMPTUnlock ?? false : flags.tfMPTUnlock === true
There was a problem hiding this comment.
If thats out of the scope for this PR, then maybe as a nit we could reformat this flag check to model the ternery syntax from earlier files for consistency
There was a problem hiding this comment.
Left a TODO for now
|
Thanks a ton for this PR, its going to be really helpful |
There was a problem hiding this comment.
Overall LGTM with a few small comments, so won't block this PR. My main concern is the shift towards more generic and less descriptive error messages. I see that it saves a lot of extra code in our tests, but it also seems like the developer experience might be negatively impacted during debugging. Maybe as part of a new issue we could design some sort of logging helper that clarifies invalid field types and some other common mistakes
|
@achowdhry-ripple made the error messages more descriptive - the diff is rather large so you should re-review |
mvadari
dismissed
achowdhry-ripple’s stale review
Made a lot of changes post-approval
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (30)
packages/xrpl/test/models/escrowCreate.test.ts (1)
35-39: Consider alternatives to the delete operatorStatic analysis tools flagged the use of the
deleteoperator as a potential performance concern. While the impact is negligible in tests, consider using undefined assignments instead for consistency with modern JavaScript practices.- delete escrow.Amount + escrow.Amount = undefined - delete escrow.Destination + escrow.Destination = undefined - delete escrow.CancelAfter - delete escrow.FinishAfter + escrow.CancelAfter = undefined + escrow.FinishAfter = undefined - delete escrow.Condition - delete escrow.FinishAfter + escrow.Condition = undefined + escrow.FinishAfter = undefinedAlso applies to: 41-45, 101-109, 111-119
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCancel.test.ts (1)
36-36: Consider using property assignment instead of delete operator.The static analysis tool suggests avoiding the
deleteoperator for performance reasons. While this is a test file where performance is less critical, you could consider using an alternative approach.- delete cancel.Owner + cancel.Owner = undefined- delete cancel.OfferSequence + cancel.OfferSequence = undefinedAlso applies to: 42-42
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts (1)
29-29: Consider using property assignment instead of delete operator.The static analysis tool suggests avoiding the
deleteoperator for performance reasons. While this is a test file where performance is less critical, you could consider using an alternative approach.- delete tx.OracleDocumentID + tx.OracleDocumentID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/DIDSet.ts (1)
10-11: Add documentation for DIDSetThere's a TODO comment indicating missing documentation for this transaction type. Consider adding comprehensive documentation explaining the purpose and usage of the DIDSet transaction type.
packages/xrpl/test/models/paymentChannelFund.test.ts (1)
33-33: Consider using undefined assignment instead of delete operatorStatic analysis tools flag performance concerns with the
deleteoperator. Consider using undefined assignment instead:- delete channel.Expiration + channel.Expiration = undefined- delete channel.Amount + channel.Amount = undefined- delete channel.Channel + channel.Channel = undefinedAlso applies to: 39-39, 45-45
🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (1)
9-9: Updated field validation to require hex string formatThe validation of the
DomainIDfield has been enhanced from accepting any string to specifically requiring a hexadecimal string format. This change strengthens type checking and ensures the field conforms to the expected format in the XRPL protocol.Consider adding a comment explaining the hex string requirement for
DomainIDto improve documentation for developers.Also applies to: 38-38
packages/xrpl/test/models/AMMDeposit.test.ts (1)
78-78: Performance consideration with delete operatorThe static analysis tools flagged the use of the
deleteoperator which can impact performance.Although this is test code where performance is less critical, consider using undefined assignment instead:
- delete deposit.Asset + deposit.Asset = undefined- delete deposit.Asset2 + deposit.Asset2 = undefinedAlso applies to: 91-91
🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/DIDSet.test.ts (1)
55-57: Performance consideration with delete operatorThe static analysis tools flagged multiple uses of the
deleteoperator which can impact performance.Consider using undefined assignment instead:
- delete tx.Data - delete tx.DIDDocument - delete tx.URI + tx.Data = undefined + tx.DIDDocument = undefined + tx.URI = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts (1)
8-8: Enhanced field validation for cryptographic elementsThe validation for
PublicKeyandSignaturefields has been strengthened to specifically require hexadecimal string format, which is appropriate for cryptographic elements.Consider adding JSDoc comments to the interface properties specifying the expected hex string format.
Also applies to: 104-104, 106-106
packages/xrpl/test/models/trustSet.test.ts (1)
37-40: Consider using assignment instead of delete operatorThe static analysis tool flagged the use of the
deleteoperator which can impact performance.- delete trustSet.LimitAmount + trustSet.LimitAmount = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDelete.test.ts (1)
37-40: Consider using assignment instead of delete operatorThe static analysis tool flagged the use of the
deleteoperator which can impact performance.- delete ammDelete.Asset + ammDelete.Asset = undefinedAnd similarly for Asset2:
- delete ammDelete.Asset2 + ammDelete.Asset2 = undefinedAlso applies to: 50-53
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelCreate.test.ts (1)
37-39: Consider using undefined assignment instead of delete.The static analyzer flagged potential performance issues with using the
deleteoperator.- delete channel.CancelAfter - delete channel.DestinationTag - delete channel.SourceTag + channel.CancelAfter = undefined + channel.DestinationTag = undefined + channel.SourceTag = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelClaim.test.ts (1)
46-46: Consider using undefined assignment instead of delete.Similar to the other test file, the static analyzer suggests avoiding the
deleteoperator for performance reasons.- delete channel.Channel + channel.Channel = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts (1)
68-68: Consider using undefined assignment instead of delete.The
deleteoperator can have performance implications. Consider using undefined assignment instead.- delete bid.Asset + bid.Asset = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleSet.test.ts (1)
45-45: Consider using undefined assignment instead of delete.The
deleteoperator can have performance implications. Consider using undefined assignment instead.- delete tx.OracleDocumentID + tx.OracleDocumentID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/offerCancel.test.ts (1)
50-50: Consider using undefined assignment instead of delete.The
deleteoperator can have performance implications. Consider using undefined assignment instead.- delete offer.OfferSequence + offer.OfferSequence = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainClaim.test.ts (1)
44-44: Consider using assignment instead of delete for better performanceStatic analysis suggests avoiding the
deleteoperator due to potential performance impacts.- delete tx.XChainBridge + tx.XChainBridge = undefinedSame applies to other delete operations in this file.
Also applies to: 59-59, 74-74, 98-98
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainModifyBridge.test.ts (1)
44-44: Consider using assignment instead of delete for better performanceStatic analysis suggests avoiding the
deleteoperator due to potential performance impacts.- delete tx.XChainBridge + tx.XChainBridge = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCommit.test.ts (1)
43-43: Consider using assignment instead of delete for better performanceStatic analysis suggests avoiding the
deleteoperator due to potential performance impacts.- delete tx.XChainBridge + tx.XChainBridge = undefinedSame applies to other delete operations in this file.
Also applies to: 58-58, 82-82
🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
49-49: Consider using assignment instead of delete for better performanceStatic analysis suggests avoiding the
deleteoperator due to potential performance impacts.- delete tx.AcceptedCredentials + tx.AcceptedCredentials = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainDelete.test.ts (1)
31-31: Consider using assignment instead of delete operator.The
deleteoperator can have performance implications.- delete tx.DomainID + tx.DomainID = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMWithdraw.test.ts (1)
89-89: Consider using assignment instead of delete operator.The
deleteoperator can have performance implications in both instances.- delete withdraw.Asset + withdraw.Asset = undefinedAnd similarly for the Asset2 field.
Also applies to: 102-102
🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts (1)
29-29: Consider using assignment instead of delete operator.The
deleteoperator can have performance implications.- delete ticketCreate.TicketCount + ticketCreate.TicketCount = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateClaimID.test.ts (1)
44-47: Consider using undefined assignment instead of delete.While functionally correct, the
deleteoperator can impact performance.- delete tx.XChainBridge + tx.XChainBridge = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMCreate.test.ts (1)
36-39: Consider using undefined assignment instead of delete.The static analyzer flagged multiple uses of the
deleteoperator, which can impact performance. While this is less critical in test code, consider using undefined assignment for consistency with best practices.- delete ammCreate.Amount + ammCreate.Amount = undefinedAlso applies to: 49-52, 62-65
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts (1)
53-56: Consider using undefined assignment instead of delete.Multiple uses of the
deleteoperator were flagged by static analysis. While functional in test code, consider using undefined assignment as a performance best practice.- delete tx.Amount + tx.Amount = undefinedAlso applies to: 68-73, 86-91, 113-118, 131-134, 146-149, 161-166, 179-182, 194-197
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
148-153: Consider centralizing flag parsing logic.
The approach to handle numeric vs. record-basedFlagsis correct. As a good-to-have refactor, you could wrap this pattern in a helper function that returnsisSellOfferdirectly, reducing duplication across transactions.packages/xrpl/test/models/payment.test.ts (3)
17-17: Use oflet payment: anyis understandable for tests.
For strongly-typed scenarios, you could define a Payment interface to ensure type safety, though it’s not strictly required here.
72-75: Avoid using thedeleteoperator for performance reasons.
Static analysis suggests assigningpayment.Amount = undefinedinstead ofdelete payment.Amount.🧰 Tools
🪛 Biome (1.9.4)
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
85-89: Avoid usingdeleteonDestination.
Similarly, setpayment.Destination = undefinedfor improved performance per static analysis.🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: 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 (86)
.vscode/settings.json(1 hunks)packages/xrpl/.eslintrc.js(1 hunks)packages/xrpl/src/models/common/index.ts(2 hunks)packages/xrpl/src/models/transactions/DIDSet.ts(2 hunks)packages/xrpl/src/models/transactions/MPTokenAuthorize.ts(2 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts(5 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceDestroy.ts(2 hunks)packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenAcceptOffer.ts(3 hunks)packages/xrpl/src/models/transactions/NFTokenBurn.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenModify.ts(2 hunks)packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts(3 hunks)packages/xrpl/src/models/transactions/XChainAddClaimAttestation.ts(3 hunks)packages/xrpl/src/models/transactions/XChainClaim.ts(2 hunks)packages/xrpl/src/models/transactions/XChainCommit.ts(2 hunks)packages/xrpl/src/models/transactions/accountSet.ts(2 hunks)packages/xrpl/src/models/transactions/checkCancel.ts(2 hunks)packages/xrpl/src/models/transactions/checkCash.ts(2 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(13 hunks)packages/xrpl/src/models/transactions/credentialAccept.ts(2 hunks)packages/xrpl/src/models/transactions/credentialCreate.ts(2 hunks)packages/xrpl/src/models/transactions/credentialDelete.ts(2 hunks)packages/xrpl/src/models/transactions/escrowCreate.ts(2 hunks)packages/xrpl/src/models/transactions/escrowFinish.ts(2 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(2 hunks)packages/xrpl/src/models/transactions/paymentChannelCreate.ts(3 hunks)packages/xrpl/src/models/transactions/paymentChannelFund.ts(2 hunks)packages/xrpl/src/models/transactions/permissionedDomainDelete.ts(2 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.ts(2 hunks)packages/xrpl/src/models/transactions/setRegularKey.ts(2 hunks)packages/xrpl/src/models/utils/index.ts(2 hunks)packages/xrpl/test/models/AMMBid.test.ts(5 hunks)packages/xrpl/test/models/AMMClawback.test.ts(2 hunks)packages/xrpl/test/models/AMMCreate.test.ts(2 hunks)packages/xrpl/test/models/AMMDelete.test.ts(2 hunks)packages/xrpl/test/models/AMMDeposit.test.ts(5 hunks)packages/xrpl/test/models/AMMVote.test.ts(2 hunks)packages/xrpl/test/models/AMMWithdraw.test.ts(5 hunks)packages/xrpl/test/models/CredentialAccept.test.ts(2 hunks)packages/xrpl/test/models/CredentialCreate.test.ts(2 hunks)packages/xrpl/test/models/CredentialDelete.test.ts(2 hunks)packages/xrpl/test/models/DIDSet.test.ts(2 hunks)packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts(9 hunks)packages/xrpl/test/models/NFTokenAcceptOffer.test.ts(11 hunks)packages/xrpl/test/models/NFTokenCreateOffer.test.ts(12 hunks)packages/xrpl/test/models/NFTokenMint.test.ts(6 hunks)packages/xrpl/test/models/NFTokenModify.test.ts(5 hunks)packages/xrpl/test/models/XChainAccountCreateCommit.test.ts(2 hunks)packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts(2 hunks)packages/xrpl/test/models/XChainAddClaimAttestation.test.ts(2 hunks)packages/xrpl/test/models/XChainClaim.test.ts(2 hunks)packages/xrpl/test/models/XChainCommit.test.ts(2 hunks)packages/xrpl/test/models/XChainCreateBridge.test.ts(2 hunks)packages/xrpl/test/models/XChainCreateClaimID.test.ts(2 hunks)packages/xrpl/test/models/XChainModifyBridge.test.ts(2 hunks)packages/xrpl/test/models/accountDelete.test.ts(5 hunks)packages/xrpl/test/models/accountSet.test.ts(2 hunks)packages/xrpl/test/models/checkCancel.test.ts(3 hunks)packages/xrpl/test/models/checkCash.test.ts(6 hunks)packages/xrpl/test/models/checkCreate.test.ts(7 hunks)packages/xrpl/test/models/clawback.test.ts(10 hunks)packages/xrpl/test/models/depositPreauth.test.ts(9 hunks)packages/xrpl/test/models/escrowCancel.test.ts(2 hunks)packages/xrpl/test/models/escrowCreate.test.ts(3 hunks)packages/xrpl/test/models/escrowFinish.test.ts(5 hunks)packages/xrpl/test/models/offerCancel.test.ts(2 hunks)packages/xrpl/test/models/offerCreate.test.ts(4 hunks)packages/xrpl/test/models/oracleDelete.test.ts(2 hunks)packages/xrpl/test/models/oracleSet.test.ts(3 hunks)packages/xrpl/test/models/payment.test.ts(5 hunks)packages/xrpl/test/models/paymentChannelClaim.test.ts(3 hunks)packages/xrpl/test/models/paymentChannelCreate.test.ts(2 hunks)packages/xrpl/test/models/paymentChannelFund.test.ts(2 hunks)packages/xrpl/test/models/permissionedDomainDelete.test.ts(2 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(2 hunks)packages/xrpl/test/models/setRegularKey.test.ts(2 hunks)packages/xrpl/test/models/ticketCreate.test.ts(1 hunks)packages/xrpl/test/models/trustSet.test.ts(2 hunks)packages/xrpl/test/testUtils.ts(2 hunks)packages/xrpl/test/wallet/index.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (29)
- packages/xrpl/test/models/setRegularKey.test.ts
- packages/xrpl/src/models/transactions/setRegularKey.ts
- packages/xrpl/test/models/checkCancel.test.ts
- packages/xrpl/test/models/escrowFinish.test.ts
- packages/xrpl/src/models/transactions/accountSet.ts
- .vscode/settings.json
- packages/xrpl/test/models/checkCreate.test.ts
- packages/xrpl/src/models/transactions/payment.ts
- packages/xrpl/src/models/transactions/escrowCreate.ts
- packages/xrpl/test/models/checkCash.test.ts
- packages/xrpl/src/models/transactions/paymentChannelFund.ts
- packages/xrpl/test/models/NFTokenMint.test.ts
- packages/xrpl/src/models/transactions/checkCash.ts
- packages/xrpl/src/models/transactions/escrowFinish.ts
- packages/xrpl/test/models/CredentialAccept.test.ts
- packages/xrpl/test/models/accountSet.test.ts
- packages/xrpl/src/models/transactions/paymentChannelCreate.ts
- packages/xrpl/test/models/MPTokenIssuanceCreate.test.ts
- packages/xrpl/test/models/clawback.test.ts
- packages/xrpl/src/models/transactions/checkCancel.ts
- packages/xrpl/test/models/accountDelete.test.ts
- packages/xrpl/src/models/transactions/NFTokenAcceptOffer.ts
- packages/xrpl/test/models/NFTokenModify.test.ts
- packages/xrpl/test/models/offerCreate.test.ts
- packages/xrpl/src/models/transactions/paymentChannelClaim.ts
- packages/xrpl/test/models/CredentialDelete.test.ts
- packages/xrpl/src/models/transactions/checkCreate.ts
- packages/xrpl/src/models/transactions/NFTokenMint.ts
- packages/xrpl/src/models/utils/index.ts
🧰 Additional context used
🧠 Learnings (14)
packages/xrpl/src/models/transactions/XChainClaim.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2796
File: packages/xrpl/src/models/transactions/XChainClaim.ts:36-36
Timestamp: 2024-11-12T12:37:49.114Z
Learning: The codebase defines `XChainClaimID` as `number | string` in some places and `string` only in others; this inconsistency is known and currently accepted.
packages/xrpl/src/models/transactions/credentialAccept.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
packages/xrpl/src/models/transactions/MPTokenIssuanceDestroy.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/XChainCommit.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2796
File: packages/xrpl/src/models/transactions/XChainClaim.ts:36-36
Timestamp: 2024-11-12T12:37:49.114Z
Learning: The codebase defines `XChainClaimID` as `number | string` in some places and `string` only in others; this inconsistency is known and currently accepted.
packages/xrpl/test/models/AMMClawback.test.ts (2)
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `@ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
packages/xrpl/src/models/transactions/XChainAddClaimAttestation.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2796
File: packages/xrpl/src/models/transactions/XChainClaim.ts:36-36
Timestamp: 2024-11-12T12:37:49.114Z
Learning: The codebase defines `XChainClaimID` as `number | string` in some places and `string` only in others; this inconsistency is known and currently accepted.
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/MPTokenAuthorize.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/test/models/permissionedDomainDelete.test.ts (2)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/common.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
packages/xrpl/test/models/XChainCommit.test.ts (1)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:18-18
Timestamp: 2025-01-16T04:11:37.316Z
Learning: Test libraries like chai should not be used in source code. For runtime checks in browser-compatible code, use vanilla JS error throwing instead of assertion libraries.
packages/xrpl/test/models/permissionedDomainSet.test.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
🪛 Biome (1.9.4)
packages/xrpl/test/models/AMMClawback.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 93-93: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelFund.test.ts
[error] 33-33: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainDelete.test.ts
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 92-92: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 128-128: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 146-146: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 164-164: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 182-182: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 200-200: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 218-218: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 236-236: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMWithdraw.test.ts
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 102-102: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleSet.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 58-58: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 140-140: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 147-147: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 154-154: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCreate.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 102-102: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 103-103: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/payment.test.ts
[error] 73-73: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 81-81: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMCreate.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 62-62: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDelete.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMDeposit.test.ts
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMVote.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 63-63: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/DIDSet.test.ts
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 60-60: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 78-78: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 93-93: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 113-113: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 131-131: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 146-146: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 194-194: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainClaim.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 98-98: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCommit.test.ts
[error] 43-43: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 58-58: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 82-82: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateBridge.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainCreateClaimID.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainModifyBridge.test.ts
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/escrowCancel.test.ts
[error] 36-36: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 42-42: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/offerCancel.test.ts
[error] 50-50: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleDelete.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelClaim.test.ts
[error] 46-46: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/paymentChannelCreate.test.ts
[error] 37-37: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 38-38: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 39-39: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 57-57: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 63-63: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/ticketCreate.test.ts
[error] 29-29: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/trustSet.test.ts
[error] 40-40: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (184)
packages/xrpl/.eslintrc.js (1)
110-110: Appropriate addition to test file type safety exceptions.Adding
'@typescript-eslint/no-unsafe-return': 'off'is consistent with the other disabled TypeScript safety rules for tests (lines 107-109, 113). This change allows tests to return values without strict type checking, which is necessary when testing the newly refactored validation functions mentioned in the PR objectives.packages/xrpl/test/models/escrowCreate.test.ts (4)
2-6: Good refactoring with test utility functions!The introduction of utility functions
assertTxIsValidandassertTxValidationErroralong with their wrapper functionsassertValidandassertInvalidsignificantly improves code readability and maintainability. This approach reduces repetition and makes the tests more concise.
32-32: Clean test assertion!Replacing the direct validation call with the
assertValidutility function makes the test more readable while maintaining the same functionality.
38-38: Improved error message consistencyThe standardized error messages follow a consistent pattern and provide more descriptive information about expected types, making it easier to understand validation failures. For example, specifying "expected a valid number" or "expected a valid hex string" gives developers clearer guidance on how to fix issues.
Also applies to: 44-44, 50-53, 59-62, 68-71, 77-80, 86-89, 95-98, 105-108, 115-118
47-54: More descriptive validation error messageThe improved error message for invalid Destination now specifically indicates that a "valid account address" is expected, which makes troubleshooting easier for developers.
packages/xrpl/src/models/transactions/NFTokenBurn.ts (1)
50-50: Enhanced validation for NFTokenID field.The validation now ensures that NFTokenID is a valid hexadecimal string rather than just any string, which is more appropriate for this field type and provides better error detection.
packages/xrpl/src/models/transactions/permissionedDomainDelete.ts (1)
27-27: Enhanced validation for DomainID field.The validation now ensures that DomainID is a valid hexadecimal string rather than just any string, which is more appropriate for this field type and provides better error detection.
packages/xrpl/src/models/transactions/MPTokenIssuanceDestroy.ts (2)
3-3: Improved type validation by using more specific validator.Changed from
isStringtoisHexStringimport, which will provide more precise validation for hexadecimal fields. This is aligned with the feedback from achowdhry-ripple about utilizing appropriate helper functions for validation.
33-33: Enhanced validation for MPTokenIssuanceID field.The validation now ensures that MPTokenIssuanceID is a valid hexadecimal string rather than just any string, which is more appropriate for this field type and provides better error detection.
packages/xrpl/src/models/common/index.ts (2)
19-20: Improved type safety with dedicated XRPAmount type.Adding a specific type for XRP amounts improves code readability and makes the intent clearer.
30-30: Enhanced type definition for Amount.Replacing the generic string with the more specific XRPAmount type improves type safety and makes the code more self-documenting.
packages/xrpl/src/models/transactions/XChainClaim.ts (1)
71-72: Improved validation for XChainClaimID field.The change from using
isStringtoisHexStringstrengthens the validation by ensuring that the XChainClaimID is either a number or specifically a hex string, rather than any arbitrary string. The added error message also provides clearer guidance to developers.Note that while the interface still declares
XChainClaimID: number | stringat line 35, the validation is now more restrictive. This is consistent with the known pattern in the codebase, as mentioned in the retrieved learning.packages/xrpl/src/models/transactions/credentialDelete.ts (1)
48-48: More precise validation for Account-related fields.Good improvement replacing generic string validation with the more specific
isAccountvalidation for Account, Subject, and Issuer fields. This ensures that these fields are properly validated as XRP Ledger account addresses, which follows best practices for type safety while maintaining the string type in the interface.Also applies to: 52-52, 54-54
packages/xrpl/test/models/escrowCancel.test.ts (2)
2-6: Well-structured test refactoring with utility functions.The introduction of
assertTxIsValidandassertTxValidationErrorutility functions with local wrappersassertValidandassertInvalidimproves code organization and maintainability. This approach will make tests more consistent across the codebase.
38-38: More descriptive and consistent error messages.The error messages now follow a consistent format ("missing field X" or "invalid field X, expected Y"), which aligns with the PR objective of making error messages more descriptive and standardized across the codebase.
Also applies to: 44-44, 51-53, 59-62
packages/xrpl/test/models/oracleDelete.test.ts (2)
2-6: Consistent test refactoring with utility functions.The introduction of test utility functions improves consistency across the test suite and reduces code duplication. This refactoring approach matches what was done in other test files, creating a standardized testing pattern.
35-39: Improved test case with better invalid input example.Changing the test value from '1234' to 'abcd' for the invalid OracleDocumentID test is a good improvement. The previous value '1234' could potentially be parsed as a number and pass validation, while 'abcd' clearly cannot be converted to a valid number, making it a better test case.
packages/xrpl/src/models/transactions/XChainCommit.ts (2)
8-8: Good addition of isHexString importThis import is now being properly utilized in your validation function to ensure XChainClaimID follows the required format.
70-71: Improved validation with better error messagingGood change from using generic
isStringto the more specificisHexStringvalidation for XChainClaimID. The added error message is also helpful for debugging.This change properly aligns with the known codebase behavior where XChainClaimID can be either a number or a hex string as mentioned in the retrieved learnings.
packages/xrpl/src/models/transactions/XChainAddClaimAttestation.ts (3)
8-8: Good addition of isHexString importThis import is now being properly utilized in your validation function to ensure fields adhere to hex string format requirements.
100-102: Enhanced validation for cryptographic fieldsGood change to use
isHexStringfor PublicKey and Signature fields. These fields should always be hex-encoded, so this validation is more appropriate than the previous generic string check.
115-116: Improved XChainClaimID validation with better error messagingGood enhancement from generic
isStringto more specificisHexStringvalidation for XChainClaimID. The added error message improves developer experience when debugging.This change correctly maintains compatibility with the existing type definition where XChainClaimID can be either a number or a hex string.
packages/xrpl/src/models/transactions/MPTokenIssuanceSet.ts (4)
12-12: Good addition of isHexString importThis import supports the improved validation in this file.
68-68: Enhanced validation for MPTokenIssuanceIDGood change to use
isHexStringinstead of a generic string validator, ensuring MPTokenIssuanceID follows the required hex format.
72-72: Improved flags initializationUsing the nullish coalescing operator is a cleaner approach for defaulting flags to 0.
73-77: Better flag checking logicExplicitly comparing flag values to
truemakes the code's intent clearer. This avoids potential issues with falsy non-undefined values being treated differently than expected.Also applies to: 78-82
packages/xrpl/test/wallet/index.test.ts (1)
1187-1187: Improved error message formatThe error message now follows a consistent pattern: "NFTokenMint: invalid field URI, expected a valid hex string" instead of the previous generic message. This is part of the larger effort to standardize error messages across model validations.
packages/xrpl/src/models/transactions/MPTokenAuthorize.ts (1)
9-9: Great upgrade to hex string validation!Switching from
isStringtoisHexStringforMPTokenIssuanceIDvalidation improves type safety by ensuring the field contains a properly formatted hexadecimal string. This change aligns with the PR objective of enhancing validation functions.Also applies to: 65-65
packages/xrpl/src/models/transactions/NFTokenModify.ts (1)
10-10: Improved validation for NFToken fieldsThe change from
isStringtoisHexStringvalidation for bothNFTokenIDandURIfields properly enforces hex encoding requirements, making validation more precise and consistent with the field documentation (line 36-37 mentions the URI must be hex-encoded).Also applies to: 54-56
packages/xrpl/src/models/transactions/DIDSet.ts (1)
5-5: Enhanced validation for DID fieldsSwitching validation from generic strings to hex strings for
Data,DIDDocument, andURIfields ensures proper formatting for these fields, which should contain hex-encoded data. This change is part of the broader standardization of validation across transaction types.Also applies to: 34-38
packages/xrpl/test/models/paymentChannelFund.test.ts (3)
2-7: Good test refactoring with utility functionsThe introduction of
assertValidandassertInvalidutility functions improves code readability and maintainability by abstracting the validation logic. This approach is consistent with the PR's objective to refactor and clean up validation code.
15-15: Added type annotation for better type safetyAdding the type annotation for the
channelvariable improves code clarity and type safety.
29-29: Improved test assertions with clear error messagesThe test assertions now use the utility functions with more descriptive error messages that follow a consistent format (e.g., "missing field" or "invalid field"). This makes test failures more informative and easier to debug.
Also applies to: 35-35, 41-41, 47-47, 53-56, 62-65, 71-74
packages/xrpl/test/models/AMMDeposit.test.ts (3)
3-9: Test utilities refactoring improves maintainabilityThe test has been improved by introducing helper functions
assertValidandassertInvalidthat encapsulate the validation logic, making the tests more readable and reducing code duplication.
22-22: Type clarity improvementExplicitly typing the
depositvariable asanyimproves code clarity.
43-43: Standardized error handling and assertionsTests now use the newly introduced assertion helpers for consistent validation and error handling.
Also applies to: 49-49, 60-60, 67-67, 74-74, 80-80, 87-87, 93-93, 100-100, 105-105, 121-121, 127-128, 133-135, 141-143, 149-151
packages/xrpl/test/models/DIDSet.test.ts (3)
2-6: Test utilities refactoring improves maintainabilitySimilar to other test files, the introduction of helper functions
assertValidandassertInvalidimproves code readability and reduces duplication.
14-14: Type clarity improvementExplicitly typing the
txvariable asanyimproves code clarity.
30-30: Standardized assertions and error messagingTests now use the newly introduced assertion helpers with standardized error formatting, making the test suite more consistent.
Also applies to: 36-36, 42-44, 51-51, 59-61
packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts (1)
119-119: Improved type validation for XChainAccountCreateCountThe validation for
XChainAccountCreateCountnow specifically requires either a number or a hexadecimal string, tightening validation and preventing potential issues with non-hex strings.packages/xrpl/src/models/transactions/oracleSet.ts (1)
79-83: Improved field validation for hex string data types.The change from
isStringtoisHexStringforProvider,URI, andAssetClassfields is appropriate as these fields are expected to contain hex-encoded data according to their documentation. This makes the validation more specific and accurate.packages/xrpl/test/models/trustSet.test.ts (2)
2-6: Good refactoring with utility assertion functions.The introduction of
assertTxIsValidandassertTxValidationErrorutility functions and the creation of local helpersassertValidandassertInvalidimproves code maintainability and readability.
41-65: Improved error message consistency and specificity.The updated error messages now follow a consistent pattern of "invalid field X, expected a valid Y", which provides clearer guidance to developers about validation failures. The test cases for
QualityInandQualityOutnow properly test string values to validate the error handling.packages/xrpl/test/models/AMMDelete.test.ts (3)
2-6: Good refactoring with utility assertion functions.Consistent with other test files, the introduction of assertion utility functions improves code organization and maintainability.
44-46: Improved error message clarity.The updated error message format "invalid field Asset, expected a valid Currency" provides clearer guidance about the validation requirement.
57-59: Consistent error message formatting.This follows the same improved error message pattern as the other validation messages in the codebase.
packages/xrpl/src/models/transactions/credentialAccept.ts (1)
39-41: Improved field validation with more specific type checking.The change from
isStringtoisAccountfor theAccountandIssuerfields provides more precise validation, ensuring these fields contain properly formatted account identifiers rather than just any string.packages/xrpl/test/models/paymentChannelCreate.test.ts (4)
2-7: Good abstraction for test utilities!These utility functions improve test readability and maintainability by abstracting away the validation logic and providing consistent assertion patterns.
33-33: LGTM! Clean test assertion.The new assertion pattern improves readability while maintaining the same validation logic.
Also applies to: 41-41
47-47: Great consistency in error messages.The error messages now follow a standardized format: "missing field [FieldName]" which is more consistent and easier to understand.
Also applies to: 53-53, 59-59, 65-65
71-74: Improved error message clarity.Error messages now follow a consistent pattern: "invalid field [FieldName], expected [ExpectedType]", which provides clearer guidance to developers about what went wrong and how to fix it.
Also applies to: 80-83, 89-92, 98-101, 107-110, 116-119
packages/xrpl/src/models/transactions/clawback.ts (3)
47-52: Good use of validateRequiredField helper.Utilizing the helper function reduces code duplication and creates more consistent validation patterns across the codebase.
54-70: Improved validation logic structure.The refactored code clearly separates validation for different Amount types (IssuedCurrency vs MPTAmount), making the logic easier to follow.
64-64: Consider making the error message more specific.As noted in a previous review, this error message could be more descriptive to help users debug the issue faster.
- throw new ValidationError('Clawback: invalid holder Account') + throw new ValidationError('Clawback: Account and Holder cannot be the same')packages/xrpl/src/models/transactions/credentialCreate.ts (3)
5-7: Good improvement in type checking specificity.Using
isAccountandisHexStringinstead of more generic validators provides better type safety and clearer validation.
49-51: Enhanced Account validation.Using
isAccountinstead of generic string validation ensures proper formatting of account addresses.
57-71: Good URI validation integration.The inline validation logic for URI length is clear and provides specific error messages. This is a good refactoring of what was likely a separate validation function.
packages/xrpl/test/models/paymentChannelClaim.test.ts (4)
2-7: Good test utility abstraction.These utility functions improve test consistency across different transaction types and make the tests more readable.
33-33: LGTM! Clean test assertion pattern.The new assertion pattern makes the tests more concise and focuses on the validation logic rather than the assertion mechanism.
Also applies to: 42-42
48-48: Improved error message format for missing fields.The standardized format "missing field [FieldName]" provides better consistency across error messages.
54-57: Consistent error messaging for invalid fields.The error messages now follow a consistent pattern: "invalid field [FieldName], expected [ExpectedType]", providing clearer guidance to developers.
Also applies to: 63-66, 72-75, 81-84, 90-93
packages/xrpl/test/models/AMMBid.test.ts (4)
2-6: Clear improvements in test structure and organization.The introduction of
assertTxIsValidandassertTxValidationErrorutility functions, along with the helper functionsassertValidandassertInvalid, significantly improves code readability and maintainability by standardizing validation patterns.
14-14: Good type clarification.Explicitly typing
bidasanyimproves code clarity and reduces TypeScript ambiguity.
64-64: Simplified test verification.Replacing direct assertions with the utility function call provides cleaner and more consistent validation.
75-77: Improved standardized error message format.The error message is now more consistent and follows the format "invalid field X, expected a valid Y", which provides clearer information about validation failures.
packages/xrpl/test/models/oracleSet.test.ts (4)
4-8: Good test structure refactoring.The addition of helper functions
assertValidandassertInvalidimproves code clarity and makes tests more maintainable.
16-16: Good type clarification.Explicitly typing
txasanyimproves code clarity and reduces TypeScript ambiguity.
51-54: Consistent test case improvement.The test now uses a string value to properly test type validation, and the error message follows the standard format.
161-162: Improved test reliability.Using a string value for
AssetPriceprovides better coverage for type validation testing.packages/xrpl/test/models/depositPreauth.test.ts (3)
3-10: Good test utility structure implementation.Adding
assertValidandassertInvalidhelper functions improves code clarity and makes tests more maintainable.
18-18: Good type clarification.Explicitly typing
depositPreauthasanyimproves code clarity and reduces TypeScript ambiguity.
112-114: Standardized error message format.The error message now follows the consistent pattern used throughout the codebase.
packages/xrpl/test/models/offerCancel.test.ts (4)
2-6: Good test utility implementation.The addition of helper functions
assertValidandassertInvalidimproves code clarity and makes tests more maintainable.
14-14: Good type clarification.Explicitly typing
offerasanyimproves code clarity and reduces TypeScript ambiguity.
32-34: Simplified test verification.Replacing direct assertions with the utility function call provides cleaner and more consistent validation.
42-46: Improved validation testing.Using a string value instead of a number properly tests the type validation, and the error message now follows the standard format.
packages/xrpl/test/models/XChainClaim.test.ts (3)
2-6: Helper functions improve test maintainabilityGreat refactoring! The introduction of
assertTxIsValidandassertTxValidationErrorutilities with wrapper functionsassertValidandassertInvalidsimplifies the test cases and makes them more readable and maintainable.
14-14: Type clarity improvementGood addition of explicit type annotation for the
txvariable. This improves code clarity and type safety.
40-109: Test assertions refactored consistentlyThe test assertions have been consistently refactored to use the new helper functions, making the code more concise and readable while maintaining the same test coverage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 44-44: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 98-98: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainModifyBridge.test.ts (2)
2-7: Helper functions improve test maintainabilityThe introduction of helper functions
assertValidandassertInvalidstreamlines the test assertions and improves code readability.
15-15: Type clarity improvementExplicit typing of
txasanyenhances code clarity.packages/xrpl/test/models/XChainCommit.test.ts (2)
2-6: Helper functions improve test maintainabilityWell-structured helper functions that improve test readability and maintainability.
14-14: Type clarity improvementExplicit typing of
txasanyimproves code clarity.packages/xrpl/test/models/permissionedDomainSet.test.ts (4)
3-10: Improved imports and helper functions for validationGood refactoring to use specific validation functions and helper methods. The
assertValidandassertInvalidpattern is consistently applied across all test files, which improves maintainability.
18-18: Type clarity with explicit anyExplicit typing of
txasanyaligns with the retrieved learning that generic JSON input is preferred for validation tests as it better represents real-world scenarios.
33-33: Object literal simplifiedGood cleanup removing the unnecessary
as anytype assertion.
43-45: Consistent error assertion patternThe refactored error assertions using
assertInvalidfollow a consistent pattern across all test files, making the code more readable and maintainable.Also applies to: 57-61, 66-68, 74-76, 82-84
packages/xrpl/test/models/permissionedDomainDelete.test.ts (3)
2-7: Good refactoring to standardize test validation pattern.The introduction of utility functions
assertValidandassertInvalidsimplifies test cases and creates a consistent pattern throughout the codebase.
15-15: Adding explicit typing improves code clarity.Good practice to explicitly type
txasanyto clearly indicate it's intentionally using a loosely typed variable.
27-27: Simplified test assertions improve readability.The refactored assertions make the test cases more concise and easier to understand.
Also applies to: 32-34, 38-40
packages/xrpl/test/models/AMMWithdraw.test.ts (3)
3-9: Good refactoring to standardize test validation pattern.The introduction of utility functions
assertValidandassertInvalidsimplifies test cases and creates a consistent pattern. The one-liner implementation forassertValidis clean and concise.
22-22: Adding explicit typing improves code clarity.Good practice to explicitly type
withdrawasanyto clearly indicate it's intentionally using a loosely typed variable.
43-43: Standardized validation error handling across the codebase.The refactored assertions make the test cases more consistent, with clear error messages that follow a standard format.
Also applies to: 49-50, 60-61, 67-68, 74-75, 80-81, 85-86, 91-92, 97-98, 104-105, 110-112, 121-122, 127-128, 133-135, 139-142, 147-150, 155-158
packages/xrpl/test/models/CredentialCreate.test.ts (3)
3-9: Good refactoring with standardized imports and validation utilities.The import path has been updated to use camelCase, and the introduction of utility functions
assertValidandassertInvalidsimplifies test cases.
17-17: Adding explicit typing improves code clarity.Good practice to explicitly type
credentialCreateasanyto clearly indicate it's intentionally using a loosely typed variable.
33-33: Standardized validation error handling with better error messages.The refactored assertions improve consistency in error handling and provide more descriptive error messages that clearly indicate what's expected.
Also applies to: 39-40, 44-47, 52-53, 57-60, 65-66, 72-73, 79-80, 84-86, 90-93, 97-100, 105-106, 111-112, 116-119
packages/xrpl/test/models/ticketCreate.test.ts (3)
2-6: Good refactoring to standardize test validation pattern.The introduction of utility functions
assertValidandassertInvalidsimplifies test cases and creates a consistent pattern. The implementation is concise and clear.
14-14: Improving type clarity in the test setup.Adding explicit typing for
ticketCreateasanyand removing redundant type casting improves code clarity.Also applies to: 21-21
25-25: Standardized validation error handling with consistent patterns.The validation assertions now follow a consistent pattern across all test cases, improving code readability and maintainability.
Also applies to: 30-31, 34-38, 43-46, 51-54, 59-62
packages/xrpl/test/models/XChainCreateBridge.test.ts (3)
2-7: Well-structured test utilities implementation!The introduction of
assertTxIsValidandassertTxValidationErrorutility functions with the wrapper functionsassertValidandassertInvalidis a great improvement to the codebase. This approach centralizes validation logic, makes tests more readable, and standardizes error handling.
15-15: Good explicit typing for better type safetyExplicitly typing
txasanyimproves type clarity in the test suite and aligns with TypeScript best practices.
40-40: Improved test assertions with consistent patternThe consistent use of helper functions for validation creates a more maintainable and readable test suite. This approach makes error messages clearer and tests more concise.
Also applies to: 46-46, 52-55, 61-61, 67-70, 76-79
packages/xrpl/test/models/NFTokenCreateOffer.test.ts (2)
1-8: Excellent standardization of test utilitiesThe replacement of direct assertions with utility functions improves consistency across the test suite. The helper functions
assertValidandassertInvalidmake the validation logic more centralized and easier to maintain.
32-32: Consistent validation approach across all test casesThe uniform application of the new assertion pattern across all test cases creates a more predictable and maintainable test structure. Error messages are now more descriptive and follow a consistent format, which will make debugging easier.
Also applies to: 50-50, 66-66, 81-84, 100-103, 118-118, 134-137, 152-152, 168-171, 185-188, 203-206
packages/xrpl/test/models/NFTokenAcceptOffer.test.ts (3)
1-7: Well-implemented test utilities for consistent validationThe new utility functions create a consistent validation approach across the test suite, improving maintainability and readability.
172-175: Improved error message specificityThe error message for invalid NFTokenBrokerFee has been enhanced to be more descriptive, following the format "invalid field X, expected Y". This standardization makes error messages more helpful for debugging.
30-30: Consistent test assertion patternAll test assertions now follow a uniform pattern using the helper functions, which improves code consistency and readability.
Also applies to: 43-43, 55-58, 72-75, 89-92, 106-106, 121-121, 136-139, 154-157
packages/xrpl/test/models/AMMClawback.test.ts (3)
5-9: Well-structured test utilities implementationThe introduction of utility functions for validation assertions creates a more maintainable and consistent test structure.
17-17: Improved type clarityExplicitly typing
ammClawbackasanyand removing redundant type assertions improves type safety and code clarity.Also applies to: 37-37
41-41: Consistent error validation patternThe uniform application of the
assertValidandassertInvalidfunctions creates a more consistent and maintainable test suite. The error messages are now more standardized and descriptive.Also applies to: 46-46, 51-51, 57-57, 64-65, 70-70, 76-76, 82-84, 89-89, 95-95, 100-102, 107-109, 116-116, 122-122
packages/xrpl/test/models/XChainCreateClaimID.test.ts (2)
2-7: Well-implemented test utility functions.The refactoring to use helper functions
assertValidandassertInvalidcreates a more maintainable and readable test structure. This approach centralizes the validation logic and provides consistent error handling across test cases.
40-41: Clean assertion refactoring.The simplified assertion pattern improves readability while maintaining the same test coverage.
packages/xrpl/test/testUtils.ts (3)
8-13: LGTM - Appropriate imports added.The import statement has been correctly expanded to include
validateandValidationError, which are necessary for the new utility functions.
60-69: Great addition of utility function for valid transactions.The
assertTxIsValidfunction is well-documented and clearly encapsulates the logic for validating that a transaction passes both its specific validation function and the general validate function.
71-95: Excellent implementation of error validation utility.The
assertTxValidationErrorfunction is well-designed with:
- Clear documentation
- Proper error handling
- Robust regex matching for error messages
- Validation against both transaction-specific and general validation functions
This significantly improves test consistency and reduces code duplication across test files.
packages/xrpl/test/models/AMMCreate.test.ts (2)
2-6: Good refactoring to use helper functions.The introduction of
assertValidandassertInvalidhelper functions streamlines the test code and improves readability.
43-46: Improved error message consistency.The standardized format for error messages ("invalid field X, expected a valid Y") makes the errors more consistent and clear.
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts (2)
2-7: Good refactoring to use utility functions.The introduction of
assertValidandassertInvalidhelper functions creates more readable and consistent test cases.
61-64: Improved error messages with better context.The error messages now provide more specific information about what validation failed and what was expected, which will make debugging easier.
Also applies to: 79-82, 97-100, 106-109, 124-127, 139-142, 154-157, 187-190, 202-205
packages/xrpl/test/models/AMMVote.test.ts (3)
2-6: Great refactoring with utility functions.The implementation of
assertValidandassertInvalidutility functions helps improve code readability and maintainability by centralizing validation logic. This approach also makes the tests more concise and consistent.
14-14: Good type clarification.Explicitly typing
voteasanyimproves type safety and clarifies intent in the test.
33-33: Clean assertion replacement.Excellent job replacing direct assertions with the new utility functions. This makes the tests more consistent and maintainable, while also standardizing error handling patterns.
Also applies to: 39-39, 45-47, 57-59, 65-65, 70-72, 77-79, 84-86
packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (5)
2-2: Improved imports organization.The addition of
isHexStringimport helps support the enhanced validation approach. This aligns with the effort to utilize more specific validation helpers.Also applies to: 11-11
110-110: Type definition simplification.Good simplification of the
MPTokenMetadatatype by removing thenulloption. This makes the interface cleaner and more straightforward.
130-130: Enhanced validation with specialized helper.Using
isHexStringinstead of the more genericisStringimproves type safety and provides more accurate validation for hex format data. This aligns with the retrieved learning to utilize existing helper functions for type checking.
134-134: Consistent validation approach.The change from
isStringtoisHexStringin the condition ensures consistency with the validation above. Good attention to detail.
155-155: Improved flags handling.The nullish coalescing operator (
??) provides cleaner default handling for undefined flags. The direct comparison withtruein the flag check is more explicit and readable than the previous approach.Also applies to: 159-159
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts (3)
2-11: Well-structured test utility functions.Excellent implementation of the
assertValidandassertInvalidhelper functions. This approach centralizes validation logic and makes tests more maintainable. The multi-line formatting for the longer function improves readability.
19-19: Explicit typing for test object.Adding the explicit
anytype to thetxvariable improves code clarity and aligns with TypeScript best practices.
55-55: Consistent error message formatting.The error messages now follow a more consistent pattern, specifying both the invalid field and the expected type. This improves the developer experience by providing more specific feedback during validation failures.
Also applies to: 61-70, 85-89, 121-124, 157-160, 175-178, 193-195, 211-213, 229-231, 247-249
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts (3)
2-7: Clean utility function implementation.Good job implementing the
assertValidandassertInvalidutility functions. The single-line variant ofassertInvalid(compared to the multi-line version in other files) maintains consistency with the function's complexity.
15-15: Proper type annotation.Adding the explicit
anytype to thetxvariable improves code clarity and type safety.
53-56: Improved error messaging.The error messages now provide more specific information about the expected types, which helps with debugging and understanding validation failures.
Also applies to: 71-74, 86-89, 101-104
packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts (4)
14-17: Imports look consistent.
This set of imports for validation helpers aligns with the usage below, offering a concise and reusable approach.
142-143: Good use ofvalidateRequiredFieldfor mandatory fields.
Ensures thatNFTokenIDis a valid hex string andAmountis properly formed, reducing repetitive checks.
145-146: Optional field validations are clear and consistent.
UsingvalidateOptionalFieldforExpirationandDestinationimproves readability and maintainability.
155-159: Sell vs. buy offer handling is neatly organized.
Routing to different validators based onisSellOfferis straightforward and clear.packages/xrpl/src/models/transactions/common.ts (21)
72-77: Doc comment forisRecordis clear.
It clarifies the function’s purpose and usage, enabling better understanding of type guards.
92-101: Doc comment and implementation forisHexString.
Correctly checks for string inputs and hex format, aligning well with transaction field validations.
102-114: Integer-only check may need confirmation.
Currently,isNumberenforces integer values (Number.isInteger(Number(num))). If decimals are required for some transactions, consider relaxing that restriction or clarifying the function name and doc comment.
116-134:isNumberWithBoundsfunction is well-documented.
This utility cleanly enforces integer range constraints for numeric fields.
152-165:isXRPAmountcorrectly enforces integer drops.
Requiring integer values fits XRP drops usage. This is consistent with the ledger’s representation of XRP.
266-282:invalidMessagesMapimproves error clarity.
Providing descriptive messages for each validator offers more targeted debugging feedback.
291-292: Optional custom invalid message parameter is well-explained.
Enhances flexibility invalidateRequiredField.
294-319:validateRequiredFieldlogic is solid.
It supplies a fallback message frominvalidMessagesMapif no custom invalid message is provided, ensuring consistent error reporting.
322-329: Doc enhancements forvalidateOptionalFieldare clear.
Expanding the parameter doc withinvalidMessagefosters standardized error handling.
339-350:validateOptionalFieldnow aligns with required-field logic.
Mirroring custom message handling preserves consistency.
443-446: ValidatingTransactionTypewithisStringand known types.
This step ensures valid transaction definitions right from the start.
450-450: EnsuringAccountis present and valid.
UsingvalidateRequiredFieldwithisAccountcentralizes the account check.
454-454: OptionalSequencevalidated as a number.
Matches the ledger requirement that Sequence be numeric when present.
456-456: Hex string verification forAccountTxnID.
Ensures this field is properly formed without burdening each transaction-specific validator.
460-465: Mixed flag types supported (numberorrecord).
Flexibly handling both numeric and object flags is a neat approach to accommodate various usage patterns.
469-470: Improved “Memos” error message.
This explicit “invalid field Memos” phrase keeps error reporting consistent with the rest of the code.
479-480: Consistent “Signers” error message.
Aligns with the new standardized wording throughout.
484-485: Hex validation forSigningPubKey.
Appropriate for verifying public key formats.
488-489: Hex validation forTxnSignature.
Ensures the signature is expressed in a correct hex form.
525-527: Hex validation enforced forCredentialType.
Makes the requirement explicit in the error message, aiding debug.
551-551:validateCredentialsListdoc block is helpful.
Properly indicates usage and constraints, boosting code readability.packages/xrpl/test/models/payment.test.ts (26)
3-3: New import ofPaymentFlagsrecognized.
Facilitates test coverage of partial payment scenarios.
5-5: Centralized test utilities imported.
Enhances readability by reusingassertTxIsValidandassertTxValidationError.
7-9: Helper assertions streamline test code.
assertValidandassertInvalidkeep tests concise and self-explanatory.
12-12: Clear documentation for Payment tests.
Provides a good overview and rationale for coverage.
20-40: Initial test fixture is comprehensive.
Populates numerous fields to confirm multi-field validation. This ensures thorough coverage.
43-45: Test for a valid Payment.
Confirms that the fixture’s default Payment object passes validation without error.
48-57: Correct memo acceptance check.
Demonstrates that a minimal Memo structure remains valid.
60-69: Ensures invalid memo format triggers an error.
UsingassertInvalidconfirms that numericMemoTypeis caught.
78-83: Checks that non-string Amount is invalid.
Ensures the validator blocks integer-literal amounts.
90-96: Rejects numericDestination.
Demonstrates that the validator correctly flags an invalid address.
107-109: Accepts valid X-address forDestination.
Ensures coverage of alternative XRPL address formats.
112-117: Rejects empty string forDestination.
Covers boundary conditions in account validation.
120-125: EnsuresDestinationTagis numeric if present.
Helps avoid downstream confusion with string-based tags.
128-133: Rejects non-stringInvoiceID.
Validates that ID fields must remain hex-encoded strings.
136-138: InvalidPathsstructure fails correctly.
This verifies that path elements must match the known payment path shape.
141-146:SendMaxmust be a validAmount.
Keeps the test suite consistent with the model’s validation approach.
149-152: Partial payment flag as number is recognized.
Confirms that numericFlagscan settfPartialPaymenteffectively.
155-158: Partial payment flag as an object is recognized.
Ensures usage parity whetherFlagsis numeric or object-based.
160-167: Rejects integer-literalDeliverMin.
Aligns with requiring string-literal amounts.
170-175: Flags must include partial payment ifDeliverMinis used.
Prevents silent acceptance of a partial payment condition without the associated flag.
177-188: MPT Payment is validated successfully.
Confirms that MPT amounts pass standard checks.
191-198: Invalid type forCredentialIDs.
Rejects string-literal usage where arrays are required.
201-216: Rejects oversizedCredentialIDsarray.
Ensures array length constraints are enforced.
219-224: Rejects emptyCredentialIDs.
Prevents meaningless credentials usage.
227-235: Non-string credential IDs are invalid.
Protects the field from unpredictable data types.
238-247: Detects duplicate credential IDs.
Warns that array elements must be unique.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
packages/ripple-binary-codec/src/types/amount.ts (1)
260-260: Improved validation logic for XRP amounts.The updated validation method uses
Number.isInteger(Number(amount))which is more robust than the string-based decimal point check. This change correctly handles edge cases like "1.0" (which is mathematically an integer despite containing a decimal point), providing more accurate validation.packages/xrpl/test/models/oracleSet.test.ts (1)
45-48: Consider replacing delete operator with undefined assignments.Static analysis tools indicate that using the
deleteoperator can impact performance. While this is less critical in test code, consider using undefined assignments instead.- delete tx.OracleDocumentID + tx.OracleDocumentID = undefinedAlso applies to: 58-61, 119-123, 133-137, 140-147, 154-158
🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts (1)
68-71: Consider replacing delete operator with undefined assignments.Static analysis tools indicate that using the
deleteoperator can impact performance. While this is less critical in test code, consider using undefined assignments instead.- delete bid.Asset + bid.Asset = undefinedAlso applies to: 81-84
🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainSet.test.ts (1)
49-49: Consider alternative to delete operator for performance.While appropriate for test code, using the delete operator can impact performance in production code.
- delete tx.AcceptedCredentials + tx.AcceptedCredentials = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMWithdraw.test.ts (1)
89-89: Consider alternative to delete operator.While acceptable in test code, consider using assignment to undefined instead of the delete operator for better performance.
- delete withdraw.Asset + withdraw.Asset = undefined - delete withdraw.Asset2 + withdraw.Asset2 = undefinedAlso applies to: 102-102
🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts (1)
53-53: Consider replacingdeletewithundefinedassignment for better performance.Static analysis identified potential performance issues with the
deleteoperator. While this is less critical in test code, consider replacing instances ofdelete tx.Propertywithtx.Property = undefinedfor better performance.- delete tx.Amount + tx.Amount = undefinedAlso applies to: 68-68, 86-86, 113-113, 131-131, 146-146, 161-161, 179-179, 194-194
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts (1)
59-59: Consider replacingdeletewithundefinedassignment for better performance.Static analysis identified potential performance issues with the
deleteoperator. While less critical in test code, consider replacing instances ofdelete tx.Propertywithtx.Property = undefinedfor better performance.- delete tx.Amount + tx.Amount = undefinedAlso applies to: 74-74, 92-92, 110-110, 128-128, 146-146, 164-164, 182-182, 200-200, 218-218, 236-236
🧰 Tools
🪛 Biome (1.9.4)
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/src/models/transactions/common.ts (1)
87-89: SimplifiedisArrayfunction could use direct reference.The
isArrayfunction is a thin wrapper around the nativeArray.isArraymethod. Consider using the native method directly in validation functions to reduce one level of indirection.-export function isArray(value: unknown): value is unknown[] { - return Array.isArray(value) -}And update usage sites to use the native method directly:
-if (!isArray(memos)) { +if (!Array.isArray(memos)) {packages/xrpl/test/models/payment.test.ts (1)
76-76: Consider using undefined assignment instead of delete operator.The delete operator can impact performance. Consider assigning undefined instead.
-delete payment.Amount +payment.Amount = undefined -delete payment.Destination +payment.Destination = undefinedAlso applies to: 89-89
🧰 Tools
🪛 Biome (1.9.4)
[error] 76-76: 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 (25)
packages/ripple-binary-codec/src/types/amount.ts(1 hunks)packages/xrpl/src/models/transactions/AMMBid.ts(3 hunks)packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts(2 hunks)packages/xrpl/src/models/transactions/NFTokenMint.ts(2 hunks)packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts(2 hunks)packages/xrpl/src/models/transactions/XChainAddClaimAttestation.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/depositPreauth.ts(2 hunks)packages/xrpl/src/models/transactions/oracleSet.ts(4 hunks)packages/xrpl/src/models/transactions/payment.ts(5 hunks)packages/xrpl/src/models/transactions/permissionedDomainSet.ts(2 hunks)packages/xrpl/src/models/transactions/signerListSet.ts(2 hunks)packages/xrpl/test/models/AMMBid.test.ts(5 hunks)packages/xrpl/test/models/AMMWithdraw.test.ts(5 hunks)packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts(2 hunks)packages/xrpl/test/models/XChainAddClaimAttestation.test.ts(2 hunks)packages/xrpl/test/models/baseTransaction.test.ts(10 hunks)packages/xrpl/test/models/clawback.test.ts(10 hunks)packages/xrpl/test/models/depositPreauth.test.ts(9 hunks)packages/xrpl/test/models/oracleSet.test.ts(3 hunks)packages/xrpl/test/models/payment.test.ts(5 hunks)packages/xrpl/test/models/permissionedDomainSet.test.ts(2 hunks)packages/xrpl/test/models/signerListSet.test.ts(5 hunks)packages/xrpl/test/wallet/index.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/xrpl/src/models/transactions/XChainAddAccountCreateAttestation.ts
- packages/xrpl/src/models/transactions/depositPreauth.ts
- packages/xrpl/src/models/transactions/signerListSet.ts
- packages/xrpl/test/wallet/index.test.ts
- packages/xrpl/src/models/transactions/payment.ts
- packages/xrpl/test/models/clawback.test.ts
- packages/xrpl/src/models/transactions/AMMBid.ts
- packages/xrpl/src/models/transactions/NFTokenCreateOffer.ts
- packages/xrpl/test/models/depositPreauth.test.ts
- packages/xrpl/src/models/transactions/XChainAddClaimAttestation.ts
- packages/xrpl/src/models/transactions/oracleSet.ts
- packages/xrpl/test/models/signerListSet.test.ts
🧰 Additional context used
🧠 Learnings (3)
packages/xrpl/src/models/transactions/NFTokenMint.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
packages/xrpl/src/models/transactions/common.ts (1)
Learnt from: achowdhry-ripple
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/transactions/CredentialAccept.ts:36-44
Timestamp: 2024-12-18T20:54:40.935Z
Learning: The validateCredentialType function already checks that CredentialType is a properly hex-encoded string within the allowed length.
packages/xrpl/test/models/permissionedDomainSet.test.ts (3)
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: For PermissionedDomain feature (XLS-80d), complex error cases like invalid credential format, duplicate credentials, and non-existent DomainID are tested in the rippled C++ implementation rather than being duplicated across SDK implementations like xrpl.js.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR #5161) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
🪛 Biome (1.9.4)
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts
[error] 59-59: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 74-74: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 92-92: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 110-110: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 128-128: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 146-146: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 164-164: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 182-182: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 200-200: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 218-218: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 236-236: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/payment.test.ts
[error] 76-76: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMBid.test.ts
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 81-81: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/AMMWithdraw.test.ts
[error] 89-89: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 102-102: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts
[error] 53-53: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 86-86: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 113-113: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 131-131: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 146-146: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 179-179: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 194-194: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/oracleSet.test.ts
[error] 45-45: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 58-58: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 119-119: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 133-133: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 140-140: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 147-147: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 154-154: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/xrpl/test/models/permissionedDomainSet.test.ts
[error] 49-49: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (45)
packages/xrpl/src/models/transactions/clawback.ts (3)
11-11: Good addition of validateRequiredField import.This aligns with the PR objective of using newer helper functions for validation.
47-52: Improved validation logic using new helper function.Using
validateRequiredFieldfor the Amount field simplifies the code and standardizes the validation approach, which helps with code maintainability and consistency. The validation predicate and error message are clear and descriptive.
54-74: Cleaner conditional validation logic with improved error messages.The refactored code provides better structure by clearly separating validation logic for different cases (issued currency vs MPT amount). The error messages are more specific and user-friendly, especially the message "Account and Holder cannot be the same" which addresses the previous review comment about making error messages more specific.
packages/xrpl/test/models/oracleSet.test.ts (3)
4-8: Good abstraction with test helper functions.Introducing utility functions for validation testing improves code consistency and readability. The wrapper functions
assertValidandassertInvalidmake the test cases cleaner and easier to maintain.
16-16: Explicit typing for better code clarity.Adding explicit
anytype to thetxvariable improves code clarity and makes the type intention clear.
51-54: Updated error messages with consistent format.The error messages now follow a consistent format: "OracleSet: invalid field X, expected Y", making them more standardized and descriptive.
Also applies to: 64-67, 72-74, 85-88, 161-164, 168-171
packages/xrpl/test/models/AMMBid.test.ts (3)
2-6: Good abstraction with test helper functions.Introducing utility functions for validation testing improves code consistency and readability. The wrapper functions
assertValidandassertInvalidmake the test cases cleaner and easier to maintain.
14-14: Explicit typing for better code clarity.Adding explicit
anytype to thebidvariable improves code clarity and makes the type intention clear.
75-77: Updated error messages with consistent format.The error messages now follow a consistent format, making them more standardized and descriptive. This is in line with the PR objective of making error messages more descriptive.
Also applies to: 88-90, 95-96, 101-102, 126-128, 135-137, 144-146, 155-156
packages/xrpl/test/models/permissionedDomainSet.test.ts (4)
3-5: Appropriate imports for refactored validation approach.The imports now correctly reference the new validation function and test utilities, which aligns with the PR's objective of enhancing transaction model validations.
7-10: Well-designed helper functions for test assertions.These helper functions provide a clean abstraction for test assertions, improving readability and reducing duplication across test cases. The approach aligns with the PR's goal of streamlining validation processes.
18-18: Appropriate type declaration for test input.Using
anytype for the test transaction object aligns with the approach mentioned in the retrieved learnings, which suggests using generic JSON input is preferred for testing validation functions, as it better represents real-world scenarios.
37-38: Consistent test assertions using helper functions.The refactored test assertions using the new helper functions improve readability and maintainability. The standardized error message format makes it easier to understand validation failures.
Also applies to: 44-46, 52-53, 58-61, 66-69, 74-77, 82-85, 90-91
packages/xrpl/test/models/AMMWithdraw.test.ts (4)
3-9: Standardized test utility pattern across transaction tests.The implementation of
assertValidandassertInvalidhelper functions follows the same pattern as in other transaction test files, creating a consistent approach to validation testing across the codebase.
22-22: Consistent type declaration for test input.Using
anytype for the test transaction object provides flexibility for testing with various input formats, consistent with the approach in other test files.
43-43: Improved positive test assertions.The use of
assertValidfor positive test cases replaces the previous assertion methods, leading to more consistent and readable test code.Also applies to: 49-49, 60-60, 67-67, 74-74, 80-80, 85-85
91-91: Enhanced error message consistency.The error messages now follow a standardized format: "TransactionType: invalid field FieldName, expected a valid Type", making it easier to understand validation failures across different fields. This is a significant improvement in the error reporting quality.
Also applies to: 97-98, 111-111, 121-121, 127-127, 134-141, 147-149, 155-157
packages/xrpl/test/models/baseTransaction.test.ts (5)
6-15: Comprehensive helper functions with robust error message handling.These helper functions include detailed regex handling to properly escape special characters in error messages, showing careful attention to error validation details. The implementation is robust and will handle various error message formats correctly.
24-24: Streamlined validation testing for base transaction.The explicit typing and use of
assertValidsimplifies the test code while maintaining the same validation checks.Also applies to: 69-69
72-79: Added test coverage for flag map validation.This new test case ensures that transactions with flag objects (not just numeric flags) are validated correctly, improving test coverage for different flag formats.
107-107: Consistent test cases for invalid field types.Using non-numeric strings ('abcd') consistently for testing invalid numeric fields creates a more uniform approach to type validation testing across different fields.
Also applies to: 133-133, 185-185, 261-261
97-100: Standardized error message format.Error messages now follow a consistent format: "TransactionType: invalid field FieldName, expected a valid Type", which greatly improves error diagnostics and aligns with the format used in other transaction validation tests.
Also applies to: 110-113, 123-126, 136-139, 149-153, 162-165, 175-178, 188-191, 201-204, 214-217, 231-234, 251-254, 263-266
packages/xrpl/src/models/transactions/permissionedDomainSet.ts (2)
9-9: Updated import for more specific validation.The import now includes
isHexStringinstead of what was likelyisString, indicating a move toward more specific type validation for hex strings.
39-39: Simplified array validation.Using
Array.isArraydirectly instead of a custom function is a more idiomatic approach in JavaScript/TypeScript, leading to cleaner and more maintainable code.packages/xrpl/test/models/XChainAddClaimAttestation.test.ts (3)
4-7: Good refactoring with utility functions.The introduction of
assertValidandassertInvalidutility functions is a great improvement that simplifies test assertions and makes the code more maintainable. This pattern enhances readability by centralizing validation logic.
15-15: Explicitly typingtxasanyimproves clarity.Adding explicit typing for the
txvariable clarifies its intended usage. Whileanytypes are generally discouraged in TypeScript, in test code where the object needs flexible manipulation, this approach is reasonable.
49-49: Consistent validation assertions improve test code quality.The refactored code uses a consistent pattern for assertions, making the tests more readable and maintainable. The improved error messages with field-specific details will help with debugging.
Also applies to: 55-55, 61-64, 70-72, 79-82, 88-90, 97-99, 106-109, 115-118, 124-127, 133-133, 139-142, 148-148, 154-157, 163-165, 172-175, 181-182, 187-190, 196-197, 202-205
packages/xrpl/src/models/transactions/NFTokenMint.ts (2)
8-13: Good use of utility functions and constants.The code now properly imports utility functions and defines a constant for
MAX_TRANSFER_FEE. This follows the best practices learned from previous PRs and enhances code maintainability.Also applies to: 17-17
129-137: Well-implemented field validation with improved helper functions.The refactored validation logic leverages the newer helper functions as mentioned in the PR objectives. This implementation correctly validates required and optional fields with appropriate type checking.
The
TransferFeevalidation now properly includes bounds checking (0 to 50000), addressing a previous review comment.packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts (2)
4-11: Good refactoring with utility functions.Similar to other test files, the introduction of
assertValidandassertInvalidutility functions improves code maintainability and readability by centralizing validation logic.
19-19: Explicitly typingtxasanyimproves clarity.Adding explicit typing for the
txvariable clarifies its intended usage in a testing context.packages/xrpl/src/models/transactions/common.ts (9)
107-109: Good implementation ofisHexStringfunction.The new
isHexStringfunction provides a clear way to validate hex strings and is used consistently throughout the codebase.
118-123: ImprovedisNumberfunction with integer validation.The updated
isNumberfunction now properly handles string representations of numbers and ensures they are integers, which is important for transaction field validation.
134-142: Well-designedisNumberWithBoundsfunction enhances validation.The new
isNumberWithBoundsfunction provides a clean way to create validators with range constraints. This is particularly useful for fields likeTransferFeethat have specific valid ranges.
275-291: Centralized error message mapping improves consistency.The
invalidMessagesMapcentralizes error messages for different validation functions, ensuring consistent error reporting across the codebase.Note: The entry for
isNumberWithBoundsInternalmight need to be adjusted if the internal function name changes, as it references an implementation detail.
304-330: Enhanced validation functions with improved error messages.The updated
validateRequiredFieldandvalidateOptionalFieldfunctions now include an optionalinvalidMessageparameter and generate more descriptive error messages using theinvalidMessagesMap. This addresses the previous comment about making error messages more specific for troubleshooting.Also applies to: 341-361
452-472: ImprovedvalidateBaseTransactionwith cleaner validation.The refactored function uses the enhanced validation helpers for a more consistent approach to field validation. The validation for
Flagsnow properly handles both number and object types.
475-486: Consistent array validation for Memos and Signers.The validation logic for
MemosandSignersnow follows a consistent pattern: first checking if the value is an array, then validating each element. The error messages also now consistently mention "field" in their descriptions.Also applies to: 491-502
543-543: UpdatedvalidateCredentialTypeto useisHexString.The function now uses the
isHexStringutility instead of a direct type check, consistent with the overall refactoring approach.
579-582: Consistent error message formatting invalidateCredentialsList.The error message now includes "field" in the description, matching the format used throughout the codebase.
packages/xrpl/test/models/payment.test.ts (5)
7-9: Good utility function introduction for test code simplification.These helper functions significantly improve code readability and maintainability by abstracting the common validation patterns. This refactoring aligns well with the PR's goal of streamlining validation functions across the codebase.
16-17: Good renaming from PaymentTransaction to Payment.This renaming makes the code more consistent with the transaction type naming in the XRPL. The variable renaming from
paymentTransactiontopaymentmakes the code more concise while maintaining clarity.
80-85: Improved error message clarity and consistency.The error messages have been updated to follow a more consistent format: "invalid field X, expected Y". This makes the errors more descriptive and helpful for debugging, which aligns with the PR author's comment about making error messages more descriptive.
Also applies to: 93-98, 101-106, 123-127, 130-135, 138-143, 146-151, 166-171
43-45: Good test description updates.The test descriptions have been updated to use "Payment" instead of "PaymentTransaction", maintaining consistency with the renaming pattern throughout the file.
Also applies to: 183-194
69-72: Error message updated for BaseTransaction validation.This error message references
BaseTransactionrather thanPayment, which is consistent with how hierarchical validation would work - base validation errors come from the base class. Good consistency in error handling hierarchy.
| 'AcceptedCredentials', | ||
| () => tx.AcceptedCredentials instanceof Array, | ||
| ) | ||
| validateOptionalField(tx, 'DomainID', isHexString) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Enhanced validation for DomainID field.
The validation for DomainID now uses isHexString instead of isString, which enforces that the field must be a valid hexadecimal string rather than any string.
This is a more restrictive validation - let's verify this change aligns with XRPL specifications for the DomainID field:
🏁 Script executed:
#!/bin/bash
# Check the rippled source code or documentation for DomainID format requirements
rg -A 3 "DomainID" --glob "*.md" || echo "No explicit documentation found"
# Check if existing tests use hex strings for DomainID
rg -A 3 "DomainID.*['\"]([0-9A-Fa-f]+)['\"]" --glob "*.{ts,js,json}" | head -n 10Length of output: 193
DomainID Validation Restriction – Please Verify Against XRPL Specs
- The updated validation in
packages/xrpl/src/models/transactions/permissionedDomainSet.tsnow usesisHexStringfor the DomainID field. - Our script searches did not find explicit XRPL documentation or test cases requiring DomainID to strictly be a hexadecimal string.
- Since this change is more restrictive than before, please double-check that XRPL standards or internal design decisions indeed mandate a hex string format. If confirmed, update or add relevant tests and documentation; if not, consider whether this validation might be inadvertently rejecting valid values.
| const errorMessage = | ||
| 'Payment: invalid field Credentials, expected a valid array' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure consistency in error message collection.
This error message for CredentialIDs has been updated to use the new format, but the error messages in the test cases below (lines 219, 227, 238, 249-250) are still using the old format. Consider updating these for consistency or verify that they're intentionally different.
🏁 Script executed:
#!/bin/bash
# Look for error message patterns in other credential-related validation code
rg -A 5 -B 5 "Credentials cannot be an empty array|Credentials cannot contain duplicate elements|Credentials length cannot exceed" --glob "*.ts"Length of output: 16391
Action: Update error message expectations for Payment validations
It appears that in packages/xrpl/test/models/payment.test.ts the error message on lines 200–201 has been updated to use the new format, but the subsequent test cases (lines 219, 227, 238, 249–250) still expect messages referencing “Credentials” rather than “CredentialIDs”. Since the payment object uses the property CredentialIDs, the error messages should be aligned with that naming. Please update the expected error messages in these test cases to use the new format (e.g. replacing occurrences of “Credentials” with “CredentialIDs”) unless the difference is intentional.
- File Impacted:
packages/xrpl/test/models/payment.test.ts - Locations: Lines 200–201 (new error message) vs. Lines 219, 227, 238, 249–250 (old format)
High Level Overview of Change
This PR cleans up the validation functions for transactions, to use newer helper functions. It also adds some helper functions for easier model validation test writing. This refactor saves >2000 lines of code.
This process uncovered a few bugs, which are fixed.
The integration tests have not been touched at all, which shows that nothing major was changed.
Note for reviewers: the vast majority of the changes were made with regex find-and-replace, so while there are a lot of changes, most of them (especially in the test files) are pretty formulaic.
Context of Change
The
validateRequiredFieldandvalidateOptionalFieldhelper functions were added somewhat recently, and the older models don't use it.Type of Change
Did you update HISTORY.md?
Test Plan
Fixed tests. CI passes.