Autofill function in Client not validating DeliverMax and Amount correctly
#2857
Conversation
…en both passed as objects
WalkthroughThis pull request introduces several enhancements to the XRPL JavaScript library, focusing on improving transaction handling and validation. The changes include adding new utility functions like 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.
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?
| @@ -98,6 +99,25 @@ describe('client.autofill', function () { | |||
| assert.strictEqual('DeliverMax' in txResult, false) | |||
| }) | |||
|
|
|||
| it('Validate Payment transaction API v2: Payment Transaction: differing DeliverMax and Amount fields using amount objects', async function () { | |||
There was a problem hiding this comment.
| it('Validate Payment transaction API v2: Payment Transaction: differing 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 () { |
|
@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 |
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.