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 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr
* parseTransactionFlags as a utility function in the xrpl package to streamline transactions flags-to-map conversion
* Added new MPT transaction definitions (XLS-33)
* New `MPTAmount` type support for `Payment` and `Clawback` transactions
* New util `areAmountsEqual` to check if 2 amounts are strictly equal

### Fixed
* `TransactionStream` model supports APIv2
* `TransactionStream` model includes `close_time_iso` field
* `Ledger` model includes `close_time_iso` field
* `autofill` function in client not validating amounts correctly

## 4.0.0 (2024-07-15)

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
32 changes: 32 additions & 0 deletions packages/xrpl/src/models/transactions/common.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import BigNumber from 'bignumber.js'
import { isValidClassicAddress, isValidXAddress } from 'ripple-address-codec'
import { TRANSACTION_TYPES } from 'ripple-binary-codec'

Expand Down Expand Up @@ -168,6 +169,37 @@ 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 (isRecord(amount1) && isRecord(amount2)) {
return Object.entries(amount1).every(
([key, value]) => amount2[key] === value,
)
}

return false
}
Copy link

@coderabbitai coderabbitai bot Dec 19, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance amount comparison logic for better type safety and edge cases

The areAmountsEqual function has a few areas that could be improved:

  1. The object comparison is shallow and may miss nested differences
  2. Type narrowing is not leveraged after isRecord check
  3. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be a reasonable suggestion. Can it be incorporated into the PR?

Copy link

Choose a reason for hiding this comment

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

@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!

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link

Choose a reason for hiding this comment

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

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!


/**
* 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: differing DeliverMax and Amount fields using amount objects', async function () {
JordiParraCrespo marked this conversation as resolved.
Show resolved Hide resolved
// @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