Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Autofill validation of DeliverMax and Amount #2857

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr

## Unreleased Changes

### Added
* New util `areAmountsEqual` to check if 2 amounts are strictly equal

### Fixed
* `autofill` function in client not validating amounts correctly

## 4.1.0 (2024-12-23)

### Added
Expand Down
3 changes: 2 additions & 1 deletion packages/xrpl/src/client/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import type {
OnEventToListenerMap,
} from '../models/methods/subscribe'
import type { SubmittableTransaction } from '../models/transactions'
import { areAmountsEqual } from '../models/transactions/common'
import { setTransactionFlagsToNumber } from '../models/utils/flags'
import {
ensureClassicAddress,
Expand Down Expand Up @@ -699,7 +700,7 @@ class Client extends EventEmitter<EventTypes> {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment -- ignore type-assertions on the DeliverMax property
// @ts-expect-error -- DeliverMax property exists only at the RPC level, not at the protocol level
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- This is a valid null check for Amount
if (tx.Amount != null && tx.Amount !== tx.DeliverMax) {
if (tx.Amount != null && !areAmountsEqual(tx.Amount, tx.DeliverMax)) {
return Promise.reject(
new ValidationError(
'PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided',
Expand Down
41 changes: 41 additions & 0 deletions packages/xrpl/src/models/transactions/common.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable max-lines -- common utility file */
import { HEX_REGEX } from '@xrplf/isomorphic/utils'
import BigNumber from 'bignumber.js'
import { isValidClassicAddress, isValidXAddress } from 'ripple-address-codec'
import { TRANSACTION_TYPES } from 'ripple-binary-codec'

Expand Down Expand Up @@ -191,6 +192,46 @@ export function isAmount(amount: unknown): amount is Amount {
)
}

/**
* 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
}
Comment on lines +195 to +233
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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


/**
* Verify the form and type of an XChainBridge at runtime.
*
Expand Down
36 changes: 36 additions & 0 deletions packages/xrpl/test/client/autofill.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
EscrowFinish,
Payment,
Transaction,
IssuedCurrencyAmount,
} from '../../src'
import { ValidationError } from '../../src/errors'
import rippled from '../fixtures/rippled'
Expand Down Expand Up @@ -98,6 +99,25 @@ describe('client.autofill', function () {
assert.strictEqual('DeliverMax' in txResult, false)
})

it('Validate Payment transaction API v2: Payment Transaction: identical DeliverMax and Amount fields using amount objects', async function () {
// @ts-expect-error -- DeliverMax is a non-protocol, RPC level field in Payment transactions
paymentTx.DeliverMax = {
currency: 'USD',
value: AMOUNT,
issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X',
}
paymentTx.Amount = {
currency: 'USD',
value: AMOUNT,
issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X',
}

const txResult = await testContext.client.autofill(paymentTx)

assert.strictEqual((txResult.Amount as IssuedCurrencyAmount).value, AMOUNT)
assert.strictEqual('DeliverMax' in txResult, false)
})

it('Validate Payment transaction API v2: Payment Transaction: differing DeliverMax and Amount fields', async function () {
// @ts-expect-error -- DeliverMax is a non-protocol, RPC level field in Payment transactions
paymentTx.DeliverMax = '6789'
Expand All @@ -106,6 +126,22 @@ describe('client.autofill', function () {
await assertRejects(testContext.client.autofill(paymentTx), ValidationError)
})

it('Validate Payment transaction API v2: Payment Transaction: differing DeliverMax and Amount fields using objects', async function () {
// @ts-expect-error -- DeliverMax is a non-protocol, RPC level field in Payment transactions
paymentTx.DeliverMax = {
currency: 'USD',
value: '31415',
issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X',
}
paymentTx.Amount = {
currency: 'USD',
value: '27182',
issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X',
}

await assertRejects(testContext.client.autofill(paymentTx), ValidationError)
})

it('should not autofill if fields are present', async function () {
const tx: Transaction = {
TransactionType: 'DepositPreauth',
Expand Down
Loading