Merge pull request #9478 from BitGo/mukesh/cshld-1452-sdk-xrp-fix-partial-payment-cross-currency-verify

fix(sdk-coin-xrp): partial-payment verify and explain fallthrough (CSHLD-1452)

Author: mukeshsp

modules/sdk-coin-xrp/src/lib/constants.ts

@@ -26,3 +26,11 @@ export const VALID_ACCOUNT_SET_FLAGS = [
 export const USER_KEY_SETTING_FLAG = 65536;
 export const MASTER_KEY_DEACTIVATION_FLAG = 1048576;
 export const REQUIRE_DESTINATION_TAG_FLAG = 131072;
+
+// https://xrpl.org/payment.html#payment-flags
+// tfPartialPayment allows a Payment to deliver less than the Amount field. The actual
+// delivered value is in the transaction metadata (meta.delivered_amount), NOT in the signed
+// blob. NOTE: 0x00020000 is numerically identical to REQUIRE_DESTINATION_TAG_FLAG above —
+// they are different flag spaces (Payment tx flag vs AccountRoot ledger flag) and must not
+// be reused interchangeably.
+export const TF_PARTIAL_PAYMENT = 0x00020000;

modules/sdk-coin-xrp/src/lib/iface.ts

@@ -15,9 +15,25 @@ import {
   Signer,
   SignerEntry,
   SignerListSet,
+  TransactionMetadata,
   TrustSet,
 } from 'xrpl';
 
+/**
+ * XRP transaction types supported by this SDK.
+ *
+ * The string values mirror XRPL's `TransactionType` field names exactly and are part of the
+ * public SDK surface — downstream consumers (e.g. bitgo-microservices `xrpToken.ts`) compare
+ * transaction types against these members with `===`/`!==`. Do NOT change the string values
+ * (e.g. switch to numeric enum values) or rename members; that would silently break string
+ * comparisons across the SDK boundary with no compile error.
+ *
+ * AMM and other newer XRPL transaction types are intentionally absent — they are rejected by
+ * `Transaction.fromRawTransaction()` via the reverse-enum lookup. To add support for a new
+ * type (e.g. an AMM family member), add it here with its exact XRPL name AND extend the
+ * switch statements in `lib/transaction.ts` (`toJson`, `explainTransaction`,
+ * `fromRawTransaction`) and the coin-level `xrp.ts:explainTransaction`.
+ */
 export enum XrpTransactionType {
   AccountDelete = 'AccountDelete',
   AccountSet = 'AccountSet',
@@ -55,6 +71,13 @@ export interface ExplainTransactionOptions {
   halfSigned?: {
     txHex: string; // txHex is poorly named here; it is just a wrapped JSON object
   };
+  /**
+   * Optional XRP transaction metadata. When present and the Payment `tfPartialPayment` flag
+   * is set, the explained `outputAmount` is taken from `meta.delivered_amount` (the actual
+   * delivered value) instead of the requested `Amount` field. Without metadata, a partial
+   * payment's delivered amount is unknown and `partialPayment: true` is surfaced instead.
+   */
+  meta?: TransactionMetadata;
 }
 
 export interface VerifyAddressOptions extends BaseVerifyAddressOptions {
@@ -105,7 +128,18 @@ export type TransactionExplanation =
   | AccountSetTransactionExplanation
   | TrustSetTransactionExplanation
   | SignerListSetTransactionExplanation
-  | MPTokenAuthorizeTransactionExplanation;
+  | MPTokenAuthorizeTransactionExplanation
+  | PaymentTransactionExplanation;
+
+export interface PaymentTransactionExplanation extends BaseTransactionExplanation {
+  /**
+   * True when the Payment `tfPartialPayment` flag is set, meaning the delivered amount may
+   * be less than the requested `Amount`. When `meta.delivered_amount` was provided to
+   * explainTransaction, `outputAmount`/`outputs[].amount` reflect the delivered value and
+   * this flag is still set so consumers can distinguish partial from full delivery.
+   */
+  partialPayment?: boolean;
+}
 
 export interface AccountSetTransactionExplanation extends BaseTransactionExplanation {
   accountSet: {

modules/sdk-coin-xrp/src/lib/transaction.ts

@@ -16,6 +16,7 @@ import BigNumber from 'bignumber.js';
 import { MPTokenAuthorize, Signer } from 'xrpl';
 import {
   AccountSetTransactionExplanation,
+  PaymentTransactionExplanation,
   SignerListSetTransactionExplanation,
   TransactionExplanation,
   TxData,
@@ -249,7 +250,7 @@ export class Transaction extends BaseTransaction {
     };
   }
 
-  private explainPaymentTransaction(): BaseTransactionExplanation {
+  private explainPaymentTransaction(): PaymentTransactionExplanation {
     const tx = this._xrpTransaction as xrpl.Payment;
     const address = utils.normalizeAddress({ address: tx.Destination, destinationTag: tx.DestinationTag });
     let amount: string | number;
@@ -261,8 +262,22 @@ export class Transaction extends BaseTransaction {
       amount = (tx.Amount as xrpl.IssuedCurrencyAmount).value;
     }
 
+    // The lib-level explainer only has the decoded signed blob, not transaction metadata, so
+    // it cannot resolve meta.delivered_amount. When tfPartialPayment is set, `amount` is the
+    // *requested* amount, not what was actually delivered — surface that via the flag so
+    // consumers don't treat outputAmount as the settled value.
+    const partialPayment = utils.isPartialPayment(tx.Flags as number);
+
     return {
-      displayOrder: ['id', 'outputAmount', 'changeAmount', 'outputs', 'changeOutputs', 'fee'],
+      displayOrder: [
+        'id',
+        'outputAmount',
+        'changeAmount',
+        'outputs',
+        'changeOutputs',
+        'fee',
+        ...(partialPayment ? ['partialPayment'] : []),
+      ],
       id: this._id as string,
       changeOutputs: [],
       outputAmount: amount,
@@ -277,6 +292,7 @@ export class Transaction extends BaseTransaction {
         fee: tx.Fee as string,
         feeRate: undefined,
       },
+      ...(partialPayment ? { partialPayment: true } : {}),
     };
   }
 
@@ -441,6 +457,12 @@ export class Transaction extends BaseTransaction {
       } else {
         value = (Amount as xrpl.IssuedCurrencyAmount).value;
       }
+      // NOTE: `value` reflects the requested `Amount`, not the delivered amount. If the
+      // tfPartialPayment flag is set the actual delivered value lives in metadata
+      // (meta.delivered_amount), which is not available on the decoded signed blob here.
+      // This path is used for build/sign flows (BitGo never builds partial payments); for
+      // display/verification of external partial payments use explainTransaction, which
+      // honors meta.delivered_amount and surfaces partialPayment: true.
       this.inputs.push({ address: Account, value, coin });
       this.outputs.push({
         address: utils.normalizeAddress({ address: Destination, destinationTag: DestinationTag }),

modules/sdk-coin-xrp/src/lib/utils.ts

@@ -11,7 +11,7 @@ import * as rippleKeypairs from 'ripple-keypairs';
 import * as url from 'url';
 import * as xrpl from 'xrpl';
 import { Amount, IssuedCurrencyAmount, isMPTAmount, MPTAmount } from 'xrpl';
-import { VALID_ACCOUNT_SET_FLAGS } from './constants';
+import { TF_PARTIAL_PAYMENT, VALID_ACCOUNT_SET_FLAGS } from './constants';
 import { Address, SignerDetails } from './iface';
 import { KeyPair as XrpKeyPair } from './keyPair';
 import assert from 'assert';
@@ -234,6 +234,40 @@ class Utils implements BaseUtils {
     return isMPTAmount(amount);
   }
 
+  /**
+   * Returns true when the Payment `tfPartialPayment` flag is set on a transaction's Flags
+   * field. When set, the delivered amount is in `meta.delivered_amount`, not `tx.Amount`.
+   */
+  public isPartialPayment(flags: number | undefined): boolean {
+    return (Number(flags) & TF_PARTIAL_PAYMENT) !== 0;
+  }
+
+  /**
+   * Extracts the numeric/string delivered amount value from XRP transaction metadata.
+   * `meta.delivered_amount` is an `Amount | 'unavailable'`:
+   *   - string → XRP drops (or the literal 'unavailable' for pre-2014 txs)
+   *   - IssuedCurrencyAmount → `{ currency, issuer, value }`
+   * Returns undefined when metadata is missing, the field is 'unavailable', or the shape
+   * is unexpected — callers must fall back to `tx.Amount` in that case and surface the
+   * partial-payment flag so consumers know the value is requested, not delivered.
+   */
+  public getDeliveredAmountValue(meta?: xrpl.TransactionMetadata): string | undefined {
+    const delivered = meta?.delivered_amount;
+    if (delivered === undefined || delivered === 'unavailable') {
+      return undefined;
+    }
+    if (typeof delivered === 'string') {
+      return delivered;
+    }
+    if (this.isIssuedCurrencyAmount(delivered)) {
+      return delivered.value;
+    }
+    if (this.isMPTAmount(delivered)) {
+      return delivered.value;
+    }
+    return undefined;
+  }
+
   /**
    * Get the associated XRP Currency details from token name. Throws an error if token is unsupported
    * @param {string} tokenName - The token name

modules/sdk-coin-xrp/src/xrp.ts

@@ -297,28 +297,94 @@ export class Xrp extends BaseCoin {
         mptIssuanceId: transaction.MPTokenIssuanceID,
         ...(transaction.MPTHolder !== undefined && { mptHolder: transaction.MPTHolder }),
       };
+    } else if (transaction.TransactionType === 'AccountDelete') {
+      // AccountDelete sweeps the full account balance (minus fee) to Destination; the exact
+      // amount is unknown at build time, so we record '0' as a placeholder (matches the
+      // Transaction-class explainer in lib/transaction.ts). Without this branch the method
+      // previously fell through to the Payment shape and returned undefined outputAmount/amount,
+      // since AccountDelete carries no Amount field.
+      const address =
+        transaction.Destination + (transaction.DestinationTag >= 0 ? '?dt=' + transaction.DestinationTag : '');
+      return {
+        displayOrder: ['id', 'outputAmount', 'changeAmount', 'outputs', 'changeOutputs', 'fee'],
+        id: id,
+        changeOutputs: [],
+        outputAmount: '0',
+        changeAmount: 0,
+        outputs: [
+          {
+            address,
+            amount: '0',
+          },
+        ],
+        fee: {
+          fee: transaction.Fee,
+          feeRate: undefined,
+          size: txHex.length / 2,
+        },
+      };
+    } else if (transaction.TransactionType === 'SignerListSet') {
+      return {
+        displayOrder: ['id', 'outputAmount', 'changeAmount', 'outputs', 'changeOutputs', 'fee', 'signerListSet'],
+        id: id,
+        changeOutputs: [],
+        outputAmount: 0,
+        changeAmount: 0,
+        outputs: [],
+        fee: {
+          fee: transaction.Fee,
+          feeRate: undefined,
+          size: txHex.length / 2,
+        },
+        signerListSet: {
+          signerQuorum: transaction.SignerQuorum,
+          signerEntries: transaction.SignerEntries,
+        },
+      };
+    } else if (transaction.TransactionType === 'Payment') {
+      const address =
+        transaction.Destination + (transaction.DestinationTag >= 0 ? '?dt=' + transaction.DestinationTag : '');
+      // When tfPartialPayment is set, tx.Amount is the *requested* amount, not the delivered
+      // one. Prefer meta.delivered_amount when metadata was supplied; otherwise keep Amount
+      // and surface partialPayment: true so consumers know the value is not the settlement.
+      const partialPayment = utils.isPartialPayment(transaction.Flags as number);
+      const deliveredValue = partialPayment ? utils.getDeliveredAmountValue(params.meta) : undefined;
+      const outputAmount = deliveredValue !== undefined ? deliveredValue : transaction.Amount;
+      return {
+        displayOrder: [
+          'id',
+          'outputAmount',
+          'changeAmount',
+          'outputs',
+          'changeOutputs',
+          'fee',
+          ...(partialPayment ? ['partialPayment'] : []),
+        ],
+        id: id,
+        changeOutputs: [],
+        outputAmount: outputAmount,
+        changeAmount: 0,
+        outputs: [
+          {
+            address,
+            amount: outputAmount,
+          },
+        ],
+        fee: {
+          fee: transaction.Fee,
+          feeRate: undefined,
+          size: txHex.length / 2,
+        },
+        ...(partialPayment ? { partialPayment: true } : {}),
+      };
     }
 
-    const address =
-      transaction.Destination + (transaction.DestinationTag >= 0 ? '?dt=' + transaction.DestinationTag : '');
-    return {
-      displayOrder: ['id', 'outputAmount', 'changeAmount', 'outputs', 'changeOutputs', 'fee'],
-      id: id,
-      changeOutputs: [],
-      outputAmount: transaction.Amount,
-      changeAmount: 0,
-      outputs: [
-        {
-          address,
-          amount: transaction.Amount,
-        },
-      ],
-      fee: {
-        fee: transaction.Fee,
-        feeRate: undefined,
-        size: txHex.length / 2,
-      },
-    };
+    // No silent fallthrough: every other TransactionType (AMM*, Offer*, Escrow*, NFToken*,
+    // Check*, pseudo-tx, etc.) is unsupported by this explainer. Throwing here mirrors the
+    // safe switch in lib/transaction.ts:196-211 and prevents callers (verifyTransaction,
+    // recover) from receiving a Payment-shaped object with undefined fields for types that
+    // have no Amount/Destination.
+    throw new Error(`Unsupported XRP transaction type: ${transaction.TransactionType}`);
   }
 
   getTransactionTypeRawTxHex(txHex: string): XrpTransactionType | undefined {
@@ -474,20 +540,41 @@ export class Xrp extends BaseCoin {
     const output = [...explanation.outputs, ...explanation.changeOutputs][0];
     const expectedOutput = txParams.recipients && txParams.recipients[0];
 
+    // A Payment carrying the tfPartialPayment flag may deliver less than its Amount field.
+    // BitGo never builds partial payments, so such a prebuild cannot match a send intent —
+    // reject it rather than risk verifying a transaction that under-delivers.
+    if ('partialPayment' in explanation && explanation.partialPayment === true) {
+      throw new Error('Partial payment (tfPartialPayment) is not permitted for verified send transactions');
+    }
+
+    // XRP Payment amounts arrive in two shapes:
+    //  - string (XRP drops, base units) — recipient amount is also base units, compare directly.
+    //  - object (IssuedCurrencyAmount / MPTAmount) — `value` is in display units, while the
+    //    recipient amount from txParams is in base units. Convert the display value via the
+    //    coin's base factor (getBaseFactor() returns 10^decimalPlaces and is overridden by
+    //    XrpToken to use the *token's* decimals, not the base coin's). Previously the object
+    //    case skipped the comparison entirely, leaving every cross-currency / token transfer
+    //    unverified.
+    const toBaseUnits = (amount: any): string => {
+      if (amount === undefined || amount === null) {
+        return '';
+      }
+      if (typeof amount === 'object' && 'value' in amount) {
+        return new BigNumber(amount.value).times(this.getBaseFactor()).toFixed();
+      }
+      return new BigNumber(amount).toFixed();
+    };
+
     const comparator = (recipient1, recipient2) => {
       if (utils.getAddressDetails(recipient1.address).address !== utils.getAddressDetails(recipient2.address).address) {
         return false;
       }
-      const amount1 = new BigNumber(recipient1.amount);
-      const amount2 = new BigNumber(recipient2.amount);
+      const amount1 = new BigNumber(toBaseUnits(recipient1.amount));
+      const amount2 = new BigNumber(toBaseUnits(recipient2.amount));
       return amount1.toFixed() === amount2.toFixed();
     };
 
-    if (
-      (txParams.type === undefined || txParams.type === 'payment') &&
-      typeof output.amount !== 'object' &&
-      !comparator(output, expectedOutput)
-    ) {
+    if ((txParams.type === undefined || txParams.type === 'payment') && !comparator(output, expectedOutput)) {
       throw new Error('transaction prebuild does not match expected output');
     }