fix Autofill validation of DeliverMax and Amount
#2857
Conversation
…en both passed as objects
WalkthroughThis pull request introduces updates to the 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: 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) | ||
| } | ||
|
|
||
| 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
|
@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 () {
mvadari
changed the title
DeliverMax and Amount correctly DeliverMax and Amount
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.