fix Autofill validation of DeliverMax and Amount
#2857
Conversation
…en both passed as objects
WalkthroughThis pull request updates the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/xrpl/test/client/autofill.test.ts (1)
Line range hint
102-143: Add test cases for additional amount comparison scenariosWhile the current test coverage is good, consider adding these scenarios:
- Mixed type comparison (string vs object)
- MPT amount comparison
Here are the additional test cases to add:
it('Validate Payment transaction API v2: Payment Transaction: differing DeliverMax and Amount fields using mixed types', async function () { // @ts-expect-error -- DeliverMax is a non-protocol, RPC level field in Payment transactions paymentTx.DeliverMax = '1000' paymentTx.Amount = { currency: 'USD', value: '1000', issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X', } await assertRejects(testContext.client.autofill(paymentTx), ValidationError) }) it('Validate Payment transaction API v2: Payment Transaction: identical DeliverMax and Amount fields using MPT amounts', async function () { // @ts-expect-error -- DeliverMax is a non-protocol, RPC level field in Payment transactions paymentTx.DeliverMax = { value: '1000', mpt_issuance_id: 'abc123' } paymentTx.Amount = { value: '1000', mpt_issuance_id: 'abc123' } const txResult = await testContext.client.autofill(paymentTx) assert.strictEqual('DeliverMax' in txResult, false) })packages/xrpl/HISTORY.md (1)
Line range hint
1-24: Improve consistency in changelog entry formattingFor better traceability and documentation, consider:
- Adding a link to the PR/commit that fixed the amount validation issue
- Maintaining consistent formatting with other entries that include links
- Following the established pattern of categorizing changes under "Fixed" or "Bug fixes" sections
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/client/index.ts(2 hunks)packages/xrpl/src/models/transactions/common.ts(2 hunks)packages/xrpl/test/client/autofill.test.ts(3 hunks)
🔇 Additional comments (2)
packages/xrpl/src/client/index.ts (1)
703-703: LGTM! Improved validation logic
The change correctly replaces the direct comparison with the new areAmountsEqual function, improving the robustness of amount validation.
packages/xrpl/HISTORY.md (1)
17-17: Verify that the changelog entry matches the PR objectives
The changelog entry "autofill function in client not validating amounts correctly" aligns with the PR objectives which describe a bug fix for amount validation in the Client's autofill function. However, for better clarity and traceability, consider expanding the entry to explicitly mention:
- The specific fields affected (
DeliverMaxandAmount) - The root cause (JavaScript object comparison behavior)
| /** | ||
| * Check if two amounts are equal. | ||
| * | ||
| * @param amount1 - The first amount to compare. | ||
| * @param amount2 - The second amount to compare. | ||
| * @returns Whether the two amounts are equal. | ||
| * @throws When the amounts are not valid. | ||
| */ | ||
| export function areAmountsEqual(amount1: unknown, amount2: unknown): boolean { | ||
| const isAmount1Invalid = !isAmount(amount1) | ||
| if (isAmount1Invalid || !isAmount(amount2)) { | ||
| throw new ValidationError( | ||
| `Amount: invalid field. Expected Amount but received ${JSON.stringify( | ||
| isAmount1Invalid ? amount1 : amount2, | ||
| )}`, | ||
| ) | ||
| } | ||
|
|
||
| if (isString(amount1) && isString(amount2)) { | ||
| return new BigNumber(amount1).eq(amount2) | ||
| } | ||
|
|
||
| if (isRecord(amount1) && isRecord(amount2)) { | ||
| return Object.entries(amount1).every( | ||
| ([key, value]) => amount2[key] === value, | ||
| ) | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance amount comparison logic for better type safety and edge cases
The areAmountsEqual function has a few areas that could be improved:
- The object comparison is shallow and may miss nested differences
- Type narrowing is not leveraged after
isRecordcheck - Mixed type comparisons (string vs object) are not explicitly handled
Consider this improved implementation:
export function areAmountsEqual(amount1: unknown, amount2: unknown): boolean {
const isAmount1Invalid = !isAmount(amount1)
if (isAmount1Invalid || !isAmount(amount2)) {
throw new ValidationError(
`Amount: invalid field. Expected Amount but received ${JSON.stringify(
isAmount1Invalid ? amount1 : amount2,
)}`,
)
}
if (isString(amount1) && isString(amount2)) {
return new BigNumber(amount1).eq(amount2)
}
- if (isRecord(amount1) && isRecord(amount2)) {
- return Object.entries(amount1).every(
- ([key, value]) => amount2[key] === value,
- )
+ if (isIssuedCurrency(amount1) && isIssuedCurrency(amount2)) {
+ return (
+ amount1.currency === amount2.currency &&
+ amount1.issuer === amount2.issuer &&
+ new BigNumber(amount1.value).eq(amount2.value)
+ )
+ }
+
+ if (isMPTAmount(amount1) && isMPTAmount(amount2)) {
+ return (
+ amount1.mpt_issuance_id === amount2.mpt_issuance_id &&
+ new BigNumber(amount1.value).eq(amount2.value)
+ )
}
return false
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
This appears to be a reasonable suggestion. Can it be incorporated into the PR?
There was a problem hiding this comment.
@ckeshava Sure! Below is an example of how you could update the areAmountsEqual function to incorporate the improved checks:
export function areAmountsEqual(amount1: unknown, amount2: unknown): boolean {
const isAmount1Invalid = !isAmount(amount1)
if (isAmount1Invalid || !isAmount(amount2)) {
throw new ValidationError(
`Amount: invalid field. Expected Amount but received ${JSON.stringify(
isAmount1Invalid ? amount1 : amount2,
)}`,
)
}
if (isString(amount1) && isString(amount2)) {
return new BigNumber(amount1).eq(amount2)
}
if (isIssuedCurrency(amount1) && isIssuedCurrency(amount2)) {
return (
amount1.currency === amount2.currency &&
amount1.issuer === amount2.issuer &&
new BigNumber(amount1.value).eq(amount2.value)
)
}
if (isMPTAmount(amount1) && isMPTAmount(amount2)) {
return (
amount1.mpt_issuance_id === amount2.mpt_issuance_id &&
new BigNumber(amount1.value).eq(amount2.value)
)
}
return false
}This approach adds clearer checks for each amount variant and leverages BigNumber for numeric comparisons. Let me know if you need anything else!
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.
I don't understand what is the bug in the existing codebase.
I have incorporated the identical Amount and DeliverMax field test case into the main branch of the code base. However, I did not get any errors.
The test where Amount and DeliverMax differ will throw a ValidationError. What is the need for your changes?
| /** | ||
| * Check if two amounts are equal. | ||
| * | ||
| * @param amount1 - The first amount to compare. | ||
| * @param amount2 - The second amount to compare. | ||
| * @returns Whether the two amounts are equal. | ||
| * @throws When the amounts are not valid. | ||
| */ | ||
| export function areAmountsEqual(amount1: unknown, amount2: unknown): boolean { | ||
| const isAmount1Invalid = !isAmount(amount1) | ||
| if (isAmount1Invalid || !isAmount(amount2)) { | ||
| throw new ValidationError( | ||
| `Amount: invalid field. Expected Amount but received ${JSON.stringify( | ||
| isAmount1Invalid ? amount1 : amount2, | ||
| )}`, | ||
| ) | ||
| } | ||
|
|
||
| if (isString(amount1) && isString(amount2)) { | ||
| return new BigNumber(amount1).eq(amount2) | ||
| } | ||
|
|
||
| if (isRecord(amount1) && isRecord(amount2)) { | ||
| return Object.entries(amount1).every( | ||
| ([key, value]) => amount2[key] === value, | ||
| ) | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
This appears to be a reasonable suggestion. Can it be incorporated into the PR?
|
@JordiParraCrespo Here is my branch with the concerned test case: https://github.com/XRPLF/xrpl.js/compare/main...ckeshava:xrpl.js:IssuedAmountPaymentTest?expand=1 Let me know what I'm missing |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/transactions/common.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/HISTORY.md
🔇 Additional comments (1)
packages/xrpl/src/models/transactions/common.ts (1)
3-3: LGTM! Good choice using BigNumber for amount comparisons.Using BigNumber ensures precise decimal arithmetic, which is crucial for financial calculations.
| /** | ||
| * Check if two amounts are equal. | ||
| * | ||
| * @param amount1 - The first amount to compare. | ||
| * @param amount2 - The second amount to compare. | ||
| * @returns Whether the two amounts are equal. | ||
| * @throws When the amounts are not valid. | ||
| */ | ||
| export function areAmountsEqual(amount1: unknown, amount2: unknown): boolean { | ||
| const isAmount1Invalid = !isAmount(amount1) | ||
| if (isAmount1Invalid || !isAmount(amount2)) { | ||
| throw new ValidationError( | ||
| `Amount: invalid field. Expected Amount but received ${JSON.stringify( | ||
| isAmount1Invalid ? amount1 : amount2, | ||
| )}`, | ||
| ) | ||
| } | ||
|
|
||
| if (isString(amount1) && isString(amount2)) { | ||
| return new BigNumber(amount1).eq(amount2) | ||
| } | ||
|
6DAF
|
||
| if (isIssuedCurrency(amount1) && isIssuedCurrency(amount2)) { | ||
| return ( | ||
| amount1.currency === amount2.currency && | ||
| amount1.issuer === amount2.issuer && | ||
| new BigNumber(amount1.value).eq(amount2.value) | ||
| ) | ||
| } | ||
|
|
||
| if (isMPTAmount(amount1) && isMPTAmount(amount2)) { | ||
| return ( | ||
| amount1.mpt_issuance_id === amount2.mpt_issuance_id && | ||
| new BigNumber(amount1.value).eq(amount2.value) | ||
| ) | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for the areAmountsEqual function
This utility function handles critical amount comparisons for different currency types (XRP, IssuedCurrency, MPTAmount) and is used in transaction validation, but appears to lack dedicated test coverage. Consider adding comprehensive tests to verify:
- Comparisons between all amount type combinations
- Edge cases and validation error scenarios
- Numeric precision handling with BigNumber
🔗 Analysis chain
Implementation looks solid and addresses the original bug.
The function correctly handles all amount types (XRP, IssuedCurrency, MPTAmount) with proper validation and precise numeric comparisons using BigNumber. The implementation aligns with the PR objectives to fix amount validation issues.
Let's verify the test coverage for this new function:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering areAmountsEqual
rg -l "areAmountsEqual" "test_.*\.(ts|js)$"
Length of output: 101
Script:
#!/bin/bash
# Find test files and search for areAmountsEqual usage
fd -e ts -e js . | rg "test|spec" | xargs rg "areAmountsEqual" -B 2 -A 2
# Also search for import statements
fd -e ts -e js . | rg "test|spec" | xargs rg "import.*areAmountsEqual" -B 1 -A 1
Length of output: 155
Script:
#!/bin/bash
# Verify the source file location
fd common.ts
# Find test files in the same directory and parent directories
fd -e ts -e js . packages/xrpl/src/models/transactions/
# Search all files for areAmountsEqual references
rg "areAmountsEqual" -g "!node_modules" -g "!dist" -g "!build"
Length of output: 3762
There was a problem hiding this comment.
Hello! Can you add unit tests to the areAmountsEqual method?
There was a problem hiding this comment.
🧩 Analysis chain
Task: Add comprehensive unit tests for areAmountsEqual method
- Find the appropriate location for the new test file
- Create test cases covering all amount types and comparison scenarios
- Include validation error testing and edge cases
- Follow existing test patterns and conventions
@ckeshava, I'll create comprehensive unit tests for the areAmountsEqual method. This function handles critical amount comparisons for different currency types and needs thorough testing coverage. Let me set up the tests to cover all scenarios including validation errors, edge cases, and numeric precision handling.
🏁 Script executed:
#!/bin/bash
# Find existing test files for the common.ts module
fd -e ts . packages/xrpl/test/ | rg "common" -A 2 -B 2
# Check the test directory structure for models/transactions
fd -t d . packages/xrpl/test/models/
# Look for existing test patterns in the models directory
ls -la packages/xrpl/test/models/transactions/ 2>/dev/null || echo "Directory not found"
fd -e ts . packages/xrpl/test/models/ | head -5
Length of output: 457
Based on the codebase structure, I'll create comprehensive unit tests for the areAmountsEqual method in a new file packages/xrpl/test/models/common.test.ts. This will follow the existing test patterns and cover all the different amount types and edge cases.
Task: Create comprehensive unit tests for areAmountsEqual method
- Create new test file following existing patterns
- Cover all amount types: string (XRP), IssuedCurrencyAmount, MPTAmount
- Include validation error testing and edge cases
- Test mixed type comparisons and precision handling
The tests will be created at packages/xrpl/test/models/common.test.ts and will provide comprehensive coverage for the areAmountsEqual function, including all amount types, validation scenarios, and edge cases. Another agent will execute these changes for you.
🧬 Code Graph Analysis Results
Relevant snippets from packages/xrpl/src/models/transactions/common.ts (lines 120-165 approx)
/**
* Check if two amounts are equal.
*
* @param amount1 - The first amount to compare.
* @param amount2 - The second amount to compare.
* @returns Whether the two amounts are equal.
* @throws When the amounts are not valid.
*/
export function areAmountsEqual(amount1: unknown, amount2: unknown): boolean {
const isAmount1Invalid = !isAmount(amount1)
if (isAmount1Invalid || !isAmount(amount2)) {
throw new ValidationError(
`Amount: invalid field. Expected Amount but received ${JSON.stringify(
isAmount1Invalid ? amount1 : amount2,
)}`,
)
}
if (isString(amount1) && isString(amount2)) {
return new BigNumber(amount1).eq(amount2)
}
if (isIssuedCurrency(amount1) && isIssuedCurrency(amount2)) {
return (
amount1.currency === amount2.currency &&
amount1.issuer === amount2.issuer &&
new BigNumber(amount1.value).eq(amount2.value)
)
}
if (isMPTAmount(amount1) && isMPTAmount(amount2)) {
return (
amount1.mpt_issuance_id === amount2.mpt_issuance_id &&
new BigNumber(amount1.value).eq(amount2.value)
)
}
return false
}Relevant snippets from packages/xrpl/src/models/transactions/common.ts (lines 80-110 approx)
These are helper type guards used by areAmountsEqual:
/**
* Verify the form and type of an Amount at runtime.
*
* @param amount - The object to check the form and type of.
* @returns Whether the Amount is properly formed.
*/
export function isAmount(amount: unknown): amount is Amount {
return (
typeof amount === 'string' ||
isIssuedCurrency(amount) ||
isMPTAmount(amount)
)
}
/**
* Verify the form and type of an IssuedCurrencyAmount at runtime.
*
* @param input - The input to check the form and type of.
* @returns Whether the IssuedCurrencyAmount is properly formed.
*/
export function isIssuedCurrency(
input: unknown,
): input is IssuedCurrencyAmount {
return (
isRecord(input) &&
Object.keys(input).length === ISSUED_CURRENCY_SIZE &&
typeof input.value === 'string' &&
typeof input.issuer === 'string' &&
typeof input.currency === 'string'
)
}
/**
* Verify the form and type of an MPT at runtime.
*
* @param input - The input to check the form and type of.
* @returns Whether the MPTAmount is properly formed.
*/
export function isMPTAmount(input: unknown): input is MPTAmount {
return (
isRecord(input) &&
Object.keys(input).length === MPTOKEN_SIZE &&
typeof input.value === 'string' &&
typeof input.mpt_issuance_id === 'string'
)
}Relevant snippet from packages/xrpl/src/errors.ts (line 156)
export class ValidationError extends Error {
// Implementation of ValidationError used in areAmountsEqual for throwing errors on invalid input
}Relevant snippet from packages/xrpl/src/models/common/index.ts (lines 19-28)
These define the types used by areAmountsEqual:
export interface IssuedCurrencyAmount extends IssuedCurrency {
value: string
}
export interface MPTAmount {
mpt_issuance_id: string
value: string
}
export type Amount = IssuedCurrencyAmount | string | MPTAmountRelevant snippet from packages/xrpl/src/models/utils/index.ts (lines 11-16)
export function onlyHasFields(
obj: Record<string, unknown>,
fields: string[],
): boolean {
return Object.keys(obj).every((key: string) => fields.includes(key))
}This utility is used in type guards like isIssuedCurrency and isMPTAmount indirectly via isRecord.
These snippets provide the full context of the areAmountsEqual method, its input validation, type guards for the amount types it supports, and the error class it throws. This context is essential for writing unit tests for areAmountsEqual.
🧠 Learnings used
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: ckeshava
PR: XRPLF/xrpl.js#3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like `number` vs `string`) in ledger objects such as `LockedAmount` vs `OutstandingAmount` in `MPTokenIssuance` are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25
F438
.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.
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.
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.
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.
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: ckeshava
PR: XRPLF/xrpl.js#2812
File: packages/xrpl/test/integration/transactions/payment.test.ts:41-43
Timestamp: 2024-10-30T01:05:33.583Z
Learning: In tests involving fee calculations for XRPL, avoid using default fee amounts. If unable to retrieve fee values from the latest validated ledger, throw an error instead of defaulting to a specific amount.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
PR: XRPLF/xrpl.js#2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
⏳ Preparing changes...
There was a problem hiding this comment.
A summary of the changes CodeRabbit can apply:
- Add a new comprehensive unit test file
common.test.tsinpackages/xrpl/test/models/that thoroughly tests theareAmountsEqualfunction for string, IssuedCurrencyAmount, and MPTAmount types, covering equality, inequality, mixed types, invalid inputs (throwing ValidationError), and edge cases including zero, negative, high precision, scientific notation, and very large numbers.
A summary of the context that CodeRabbit is considering across the codebase:
I need to understand the test directory structure and existing patterns to create the test file in the appropriate location.
Show context details
Shell Result:
Shell script executed:
Find existing test files in the models directory to understand the structure
fd -e ts . packages/xrpl/test/models/ | head -10
Look for existing common test files
fd "common" packages/xrpl/test/
Output:
packages/xrpl/test/models/AMMBid.test.ts
packages/xrpl/test/models/AMMCreate.test.ts
packages/xrpl/test/models/AMMDelete.test.ts
packages/xrpl/test/models/AMMDeposit.test.ts
packages/xrpl/test/models/AMMVote.test.ts
packages/xrpl/test/models/AMMWithdraw.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/test/models/DIDDelete.test.ts
I need to understand the import patterns and test structure by examining existing test files in the models directory.
Show context details
Shell Result:
Shell script executed:
Check the content of an existing test file to understand the patterns
ls packages/xrpl/test/models/*.test.ts | head -1 | xargs cat | head -20
Look for import statements in existing test files
rg "import.*ValidationError" packages/xrpl/test/models/ -A 1 -B 1
Output:
import { assert } from 'chai'
import { validate, ValidationError } from '../../src'
import { validateAMMBid } from '../../src/models/transactions/AMMBid'
/**
- AMMBid Transaction Verification Testing.
- Providing runtime verification testing for each specific transaction type.
*/
describe('AMMBid', function () {
let bid
beforeEach(function () {
bid = {
TransactionType: 'AMMBid',
Account: 'rWYkbWkCeg8dP6rXALnjgZSjjLyih5NXm',
Asset: {
currency: 'XRP',
},
packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts-
packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/MPTokenIssuanceDestroy.test.ts-
packages/xrpl/test/models/ticketCreate.test.ts-
packages/xrpl/test/models/ticketCreate.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/ticketCreate.test.ts-import { validateTicketCreate } from '../../src/models/transactions/ticketCreate'
packages/xrpl/test/models/signerListSet.test.ts-
packages/xrpl/test/models/signerListSet.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/signerListSet.test.ts-import { validateSignerListSet } from '../../src/models/transactions/signerListSet'
packages/xrpl/test/models/setRegularKey.test.ts-
packages/xrpl/test/models/setRegularKey.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/setRegularKey.test.ts-import { validateSetRegularKey } from '../../src/models/transactions/setRegularKey'
packages/xrpl/test/models/paymentChannelFund.test.ts-
packages/xrpl/test/models/paymentChannelFund.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/paymentChannelFund.test.ts-import { validatePaymentChannelFund } from '../../src/models/transactions/paymentChannelFund'
packages/xrpl/test/models/paymentChannelCreate.test.ts-
packages/xrpl/test/models/paymentChannelCreate.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/paymentChannelCreate.test.ts-import { validatePaymentChannelCreate } from '../../src/models/transactions/paymentChannelCreate'
packages/xrpl/test/models/trustSet.test.ts-
packages/xrpl/test/models/trustSet.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/trustSet.test.ts-import { validateTrustSet } from '../../src/models/transactions/trustSet'
packages/xrpl/test/models/paymentChannelClaim.test.ts-
packages/xrpl/test/models/paymentChannelClaim.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/paymentChannelClaim.test.ts-import { validatePaymentChannelClaim } from '../../src/models/transactions/paymentChannelClaim'
packages/xrpl/test/models/payment.test.ts-
packages/xrpl/test/models/payment.test.ts:import { validate, PaymentFlags, ValidationError } from '../../src'
packages/xrpl/test/models/payment.test.ts-import { validatePayment } from '../../src/models/transactions/payment'
packages/xrpl/test/models/oracleSet.test.ts-
packages/xrpl/test/models/oracleSet.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/oracleSet.test.ts-import { validateOracleSet } from '../../src/models/transactions/oracleSet'
packages/xrpl/test/models/offerCreate.test.ts-
packages/xrpl/test/models/offerCreate.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/offerCreate.test.ts-import { validateOfferCreate } from '../../src/models/transactions/offerCreate'
packages/xrpl/test/models/oracleDelete.test.ts-
packages/xrpl/test/models/oracleDelete.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/oracleDelete.test.ts-import { validateOracleDelete } from '../../src/models/transactions/oracleDelete'
packages/xrpl/test/models/offerCancel.test.ts-
packages/xrpl/test/models/offerCancel.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/offerCancel.test.ts-import { validateOfferCancel } from '../../src/models/transactions/offerCancel'
packages/xrpl/test/models/escrowFinish.test.ts-
packages/xrpl/test/models/escrowFinish.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/escrowFinish.test.ts-import { validateEscrowFinish } from '../../src/models/transactions/escrowFinish'
packages/xrpl/test/models/escrowCancel.test.ts-
packages/xrpl/test/models/escrowCancel.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/escrowCancel.test.ts-import { validateEscrowCancel } from '../../src/models/transactions/escrowCancel'
packages/xrpl/test/models/escrowCreate.test.ts-
packages/xrpl/test/models/escrowCreate.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/escrowCreate.test.ts-import { validateEscrowCreate } from '../../src/models/transactions/escrowCreate'
packages/xrpl/test/models/depositPreauth.test.ts-
packages/xrpl/test/models/depositPreauth.test.ts:import { AuthorizeCredential, validate, ValidationError } from '../../src'
packages/xrpl/test/models/depositPreauth.test.ts-import { validateDepositPreauth } from '../../src/models/transactions/depositPreauth'
packages/xrpl/test/models/checkCash.test.ts-
packages/xrpl/test/models/checkCash.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/checkCash.test.ts-import { validateCheckCash } from '../../src/models/transactions/checkCash'
packages/xrpl/test/models/checkCancel.test.ts-
packages/xrpl/test/models/checkCancel.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/checkCancel.test.ts-import { validateCheckCancel } from '../../src/models/transactions/checkCancel'
packages/xrpl/test/models/baseTransaction.test.ts-
packages/xrpl/test/models/baseTransaction.test.ts:import { ValidationError } from '../../src'
packages/xrpl/test/models/baseTransaction.test.ts-import { validateBaseTransaction } from '../../src/models/transactions/common'
packages/xrpl/test/models/accountSet.test.ts-
packages/xrpl/test/models/accountSet.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/accountSet.test.ts-import { validateAccountSet } from '../../src/models/transactions/accountSet'
packages/xrpl/test/models/clawback.test.ts-
packages/xrpl/test/models/clawback.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/clawback.test.ts-
packages/xrpl/test/models/accountDelete.test.ts-
packages/xrpl/test/models/accountDelete.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/accountDelete.test.ts-import { validateAccountDelete } from '../../src/models/transactions/accountDelete'
packages/xrpl/test/models/XChainModifyBridge.test.ts-
packages/xrpl/test/models/XChainModifyBridge.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainModifyBridge.test.ts-import { validateXChainModifyBridge } from '../../src/models/transactions/XChainModifyBridge'
packages/xrpl/test/models/XChainCreateClaimID.test.ts-
packages/xrpl/test/models/XChainCreateClaimID.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainCreateClaimID.test.ts-import { validateXChainCreateClaimID } from '../../src/models/transactions/XChainCreateClaimID'
packages/xrpl/test/models/XChainCommit.test.ts-
packages/xrpl/test/models/XChainCommit.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainCommit.test.ts-import { validateXChainCommit } from '../../src/models/transactions/XChainCommit'
packages/xrpl/test/models/XChainClaim.test.ts-
packages/xrpl/test/models/XChainClaim.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainClaim.test.ts-import { validateXChainClaim } from '../../src/models/transactions/XChainClaim'
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts-
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainAddClaimAttestation.test.ts-import { validateXChainAddClaimAttestation } from '../../src/models/transactions/XChainAddClaimAttestation'
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts-
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainAddAccountCreateAttestation.test.ts-import { validateXChainAddAccountCreateAttestation } from '../../src/models/transactions/XChainAddAccountCreateAttestation'
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts-
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainAccountCreateCommit.test.ts-import { validateXChainAccountCreateCommit } from '../../src/models/transactions/XChainAccountCreateCommit'
packages/xrpl/test/models/NFTokenCreateOffer.test.ts-
packages/xrpl/test/models/NFTokenCreateOffer.test.ts:import { validate, ValidationError, NFTokenCreateOfferFlags } from '../../src'
packages/xrpl/test/models/NFTokenCreateOffer.test.ts-
packages/xrpl/test/models/checkCreate.test.ts-
packages/xrpl/test/models/checkCreate.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/checkCreate.test.ts-import { validateCheckCreate } from '../../src/models/transactions/checkCreate'
packages/xrpl/test/models/NFTokenCancelOffer.test.ts-
packages/xrpl/test/models/NFTokenCancelOffer.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/NFTokenCancelOffer.test.ts-
packages/xrpl/test/models/XChainCreateBridge.test.ts-
packages/xrpl/test/models/XChainCreateBridge.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/XChainCreateBridge.test.ts-import { validateXChainCreateBridge } from '../../src/models/transactions/XChainCreateBridge'
packages/xrpl/test/models/NFTokenAcceptOffer.test.ts-
packages/xrpl/test/models/NFTokenAcceptOffer.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/NFTokenAcceptOffer.test.ts-
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts-
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts:import { validate, ValidationError, MPTokenIssuanceSetFlags } from '../../src'
packages/xrpl/test/models/MPTokenIssuanceSet.test.ts-
packages/xrpl/test/models/MPTokenAuthorize.test.ts-
packages/xrpl/test/models/MPTokenAuthorize.test.ts:import { validate, ValidationError, MPTokenAuthorizeFlags } from '../../src'
packages/xrpl/test/models/MPTokenAuthorize.test.ts-
packages/xrpl/test/models/CredentialAccept.test.ts-
packages/xrpl/test/models/CredentialAccept.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/CredentialAccept.test.ts-import { validateCredentialAccept } from '../../src/models/transactions/CredentialAccept'
packages/xrpl/test/models/DIDSet.test.ts-
packages/xrpl/test/models/DIDSet.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/DIDSet.test.ts-import { validateDIDSet } from '../../src/models/transactions/DIDSet'
packages/xrpl/test/models/AMMWithdraw.test.ts-
packages/xrpl/test/models/AMMWithdraw.test.ts:import { AMMWithdrawFlags, validate, ValidationError } from '../../src'
packages/xrpl/test/models/AMMWithdraw.test.ts-import { validateAMMWithdraw } from '../../src/models/transactions/AMMWithdraw'
packages/xrpl/test/models/AMMVote.test.ts-
packages/xrpl/test/models/AMMVote.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/AMMVote.test.ts-import { validateAMMVote } from '../../src/models/transactions/AMMVote'
packages/xrpl/test/models/NFTokenBurn.test.ts-
packages/xrpl/test/models/NFTokenBurn.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/NFTokenBurn.test.ts-
packages/xrpl/test/models/AMMDelete.test.ts-
packages/xrpl/test/models/AMMDelete.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/AMMDelete.test.ts-import { validateAMMDelete } from '../../src/models/transactions/AMMDelete'
packages/xrpl/test/models/CredentialDelete.test.ts-
packages/xrpl/test/models/CredentialDelete.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/CredentialDelete.test.ts-import { validateCredentialDelete } from '../../src/models/transactions/CredentialDelete'
packages/xrpl/test/models/AMMDeposit.test.ts-
packages/xrpl/test/models/AMMDeposit.test.ts:import { AMMDepositFlags, validate, ValidationError } from '../../src'
packages/xrpl/test/models/AMMDeposit.test.ts-import { validateAMMDeposit } from '../../src/models/transactions/AMMDeposit'
packages/xrpl/test/models/AMMBid.test.ts-
packages/xrpl/test/models/AMMBid.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/AMMBid.test.ts-import { validateAMMBid } from '../../src/models/transactions/AMMBid'
packages/xrpl/test/models/CredentialCreate.test.ts-
packages/xrpl/test/models/CredentialCreate.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/CredentialCreate.test.ts-import { validateCredentialCreate } from '../../src/models/transactions/CredentialCreate'
packages/xrpl/test/models/AMMCreate.test.ts-
packages/xrpl/test/models/AMMCreate.test.ts:import { validate, ValidationError } from '../../src'
packages/xrpl/test/models/AMMCreate.test.ts-import { validateAMMCreate } from '../../src/models/transactions/AMMCreate'
I need to understand the types and error handling patterns used in the areAmountsEqual function to create appropriate test cases.
Show context details
Shell Result:
Shell script executed:
Find the ValidationError import path
rg "ValidationError" packages/xrpl/src/errors.ts -A 2 -B 2
Check the structure of Amount types
rg "IssuedCurrencyAmount|MPTAmount" packages/xrpl/src/models/common/ -A 2 -B 2
Output:
- @category Errors
*/
class ValidationError extends XrplError {}
/**
TimeoutError,
ResponseFormatError,
ValidationError,
NotFoundError,
XRPLFaucetError,
packages/xrpl/src/models/common/index.ts-export type Currency = IssuedCurrency | XRP
packages/xrpl/src/models/common/index.ts-
packages/xrpl/src/models/common/index.ts:export interface IssuedCurrencyAmount extends IssuedCurrency {
packages/xrpl/src/models/common/index.ts- value: string
packages/xrpl/src/models/common/index.ts-}
packages/xrpl/src/models/common/index.ts-
packages/xrpl/src/models/common/index.ts:export interface MPTAmount {
packages/xrpl/src/models/common/index.ts- mpt_issuance_id: string
packages/xrpl/src/models/common/index.ts- value: string
packages/xrpl/src/models/common/index.ts-}
packages/xrpl/src/models/common/index.ts-
packages/xrpl/src/models/common/index.ts:export type Amount = IssuedCurrencyAmount | string
packages/xrpl/src/models/common/index.ts-
packages/xrpl/src/models/common/index.ts-export interface Balance {
I need to examine existing test patterns for error handling and assertion styles in the XRPL test suite.
Show context details
Shell Result:
Shell script executed:
Look for existing test patterns with assert.throws
rg "assert.throws" packages/xrpl/test/models/ -A 2 -B 2 | head -10
Check for chai import patterns
rg "import.*chai" packages/xrpl/test/models/ -A 1 -B 1 | head -5
Output:
packages/xrpl/test/models/trustSet.test.ts- it('throws when LimitAmount is missing', function () {
packages/xrpl/test/models/trustSet.test.ts- delete trustSet.LimitAmount
packages/xrpl/test/models/trustSet.test.ts: assert.throws(
packages/xrpl/test/models/trustSet.test.ts- () => validateTrustSet(trustSet),
packages/xrpl/test/models/trustSet.test.ts- ValidationError,
packages/xrpl/test/models/trustSet.test.ts- 'TrustSet: missing field LimitAmount',
packages/xrpl/test/models/trustSet.test.ts- )
packages/xrpl/test/models/trustSet.test.ts: assert.throws(
packages/xrpl/test/models/trustSet.test.ts- () => validate(trustSet),
packages/xrpl/test/models/trustSet.test.ts- ValidationError,
packages/xrpl/test/models/MPTokenAuthorize.test.ts:import { assert } from 'chai'
packages/xrpl/test/models/MPTokenAuthorize.test.ts-
packages/xrpl/test/models/escrowCreate.test.ts:import { assert } from 'chai'
packages/xrpl/test/models/escrowCreate.test.ts-
I need to understand the BigNumber usage and test patterns for numeric comparisons in the codebase.
Show context details
Shell Result:
Shell script executed:
Look for BigNumber test patterns
rg "BigNumber" packages/xrpl/test/models/ -A 2 -B 2 | head -5
Check how amount comparisons are tested elsewhere
rg "amount.*equal" packages/xrpl/test/models/ -A 2 -B 2 | head -5
Output:
- ✅ Create PR with these edits
- 📌 Create commit in current branch
- 📋 Get copyable edits
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!
|
@ckeshava Thank you for taking the time to review my PR 👍 The issue arises from how JavaScript compares objects. In JavaScript, two object literals are considered equal only if they reference the exact same object in memory. Here’s an example to illustrate: To reproduce this behavior in your test, you can update it as follows: The problem occurs here when comparing amounts like this: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/xrpl/test/client/autofill.test.ts (1)
102-119: Test implementation looks good, consider expanding coverage.The test cases effectively validate the core functionality for object-based amounts. Consider adding tests for:
- Different currencies between
DeliverMaxandAmount- Different issuers between
DeliverMaxandAmount- Mixed string and object amount representations
Also applies to: 129-143
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/xrpl/test/client/autofill.test.ts(3 hunks)
🔇 Additional comments (2)
packages/xrpl/test/client/autofill.test.ts (2)
9-9: LGTM!The addition of
IssuedCurrencyAmountimport is necessary for type checking in the new test cases.
102-102: Consider using a more descriptive test name.Based on the previous review suggestion, consider using a more descriptive test name that clearly indicates what is being tested.
- it('Validate Payment transaction API v2: Payment Transaction: identical DeliverMax and Amount fields using amount objects', async function () { + it('Validate Payment transaction API v2: Payment Transaction: identical DeliverMax and Amount fields using amount objects', async function () {
DeliverMax and Amount correctly DeliverMax and Amount
…l-same-amounts-validation
packages/xrpl/HISTORY.md
Outdated
|
|
||
| ### Changed | ||
| * Deprecated `setTransactionFlagsToNumber`. Start using convertTxFlagsToNumber instead | ||
| * `autofill` function in client not validating amounts correctly |
There was a problem hiding this comment.
nit: would describe this under "Fixed" instead
There was a problem hiding this comment.
Updated as suggested, thanks! 👍
| const isAmount1Invalid = !isAmount(amount1) | ||
| if (isAmount1Invalid || !isAmount(amount2)) { |
There was a problem hiding this comment.
nit: for cleanliness, I'd prefer if both were created as variables or both were checked inline during the condition
| `Amount: invalid field. Expected Amount but received ${JSON.stringify( | ||
| isAmount1Invalid ? amount1 : amount2, |
There was a problem hiding this comment.
for consistency with the other error messaging in the library, we could just leave this one as something along the lines of "invalid Amount", or slightly more descriptive instead of dynamic
There was a problem hiding this comment.
Updated error to 'Invalid amount'—let me know if that works
|
Definitely shouldn't use lodash after we worked hard to remove it (it's a gigantic dependency and adds a ton of bloat to the library) |
| /** | ||
| * Check if two amounts are equal. | ||
| * | ||
| * @param amount1 - The first amount to compare. | ||
| * @param amount2 - The second amount to compare. | ||
| * @returns Whether the two amounts are equal. | ||
| * @throws When the amounts are not valid. | ||
| */ | ||
| export function areAmountsEqual(amount1: unknown, amount2: unknown): boolean { | ||
| const isAmount1Invalid = !isAmount(amount1) | ||
| if (isAmount1Invalid || !isAmount(amount2)) { | ||
| throw new ValidationError( | ||
| `Amount: invalid field. Expected Amount but received ${JSON.stringify( | ||
| isAmount1Invalid ? amount1 : amount2, | ||
| )}`, | ||
| ) | ||
| } | ||
|
10000
|
||
| if (isString(amount1) && isString(amount2)) { | ||
| return new BigNumber(amount1).eq(amount2) | ||
| } | ||
|
|
||
| if (isIssuedCurrency(amount1) && isIssuedCurrency(amount2)) { | ||
| return ( | ||
| amount1.currency === amount2.currency && | ||
| amount1.issuer === amount2.issuer && | ||
| new BigNumber(amount1.value).eq(amount2.value) | ||
| ) | ||
| } | ||
|
|
||
| if (isMPTAmount(amount1) && isMPTAmount(amount2)) { | ||
| return ( | ||
| amount1.mpt_issuance_id === amount2.mpt_issuance_id && | ||
| new BigNumber(amount1.value).eq(amount2.value) | ||
| ) | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Hello! Can you add unit tests to the areAmountsEqual method?
a35bdc3 to
420c7f6
Compare
Autofill function in Client not validating
DeliverMaxandAmountcorrectlyFixes an issue where the autofill function throws an error when passing amounts as objects.
Context of Change
In JavaScript, objects are compared by reference, not by their property values. Here's an example to illustrate this behavior:
Because objects are compared by reference, even identical objects like a and b are considered different. This was causing incorrect validation in the autofill function when dealing with amounts represented as objects.
Type of Change
Did you update HISTORY.md?
Test Plan
I have added tests to verify that the bug is resolved.