Fix: parseTransactionFlags unintentionally modifies transaction (#2825)

* remove setTransactions and any reference to modifying a transaction parameter * remove use of setter in autofill * changelog and test fixes * add back deprecated function with warnings * add new helper function to exported * update readme with deprecated * remove references to deprecated setter * fix changelog syntax * revert to test old functions * lint * lint for deprecation * remove unsafe assertions * separate null check * clean up tests * remove outdated logic * fix history md * fix tests after merge conflicts * Update packages/xrpl/HISTORY.md Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * Update packages/xrpl/test/models/utils.test.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> * rename a test per pr comment * lint fixes * Update packages/xrpl/test/models/utils.test.ts Co-authored-by: Omar Khan <khancodegt@gmail.com> --------- Co-authored-by: Omar Khan <khancodegt@gmail.com>
2026-06-03 08:46:40 +00:00 · 2025-01-16 11:59:44 -05:00
parent 84943ae0b6
commit abdb192c69
6 changed files with 244 additions and 100 deletions
--- a/packages/xrpl/HISTORY.md
+++ b/packages/xrpl/HISTORY.md
@@ -2,7 +2,13 @@

 Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xrpl-announce) for release announcements. We recommend that xrpl.js (ripple-lib) users stay up-to-date with the latest stable release.

-## Unreleased Changes
+## Unreleased
+
+### Added
+* Adds utility function `convertTxFlagsToNumber`
+
+### Changed
+* Deprecated `setTransactionFlagsToNumber`. Start using convertTxFlagsToNumber instead

 ## 4.1.0 (2024-12-23)

--- a/packages/xrpl/src/client/index.ts
+++ b/packages/xrpl/src/client/index.ts
@@ -47,7 +47,7 @@ import type {
  OnEventToListenerMap,
 } from '../models/methods/subscribe'
 import type { SubmittableTransaction } from '../models/transactions'
-import { setTransactionFlagsToNumber } from '../models/utils/flags'
+import { convertTxFlagsToNumber } from '../models/utils/flags'
 import {
  ensureClassicAddress,
  submitRequest,
@@ -665,7 +665,7 @@ class Client extends EventEmitter<EventTypes> {
    const tx = { ...transaction }

    setValidAddresses(tx)
-    setTransactionFlagsToNumber(tx)
+    tx.Flags = convertTxFlagsToNumber(tx)

    const promises: Array<Promise<void>> = []
    if (tx.NetworkID == null) {
--- a/packages/xrpl/src/models/index.ts
+++ b/packages/xrpl/src/models/index.ts
@@ -8,8 +8,9 @@
 */
 export * as LedgerEntry from './ledger'
 export {
-  setTransactionFlagsToNumber,
  parseAccountRootFlags,
+  setTransactionFlagsToNumber,
+  convertTxFlagsToNumber,
  parseTransactionFlags,
 } from './utils/flags'
 export * from './methods'
--- a/packages/xrpl/src/models/transactions/transaction.ts
+++ b/packages/xrpl/src/models/transactions/transaction.ts
@@ -4,7 +4,7 @@
 import { ValidationError } from '../../errors'
 import { IssuedCurrencyAmount, Memo } from '../common'
 import { isHex } from '../utils'
-import { setTransactionFlagsToNumber } from '../utils/flags'
+import { convertTxFlagsToNumber } from '../utils/flags'

 import { AccountDelete, validateAccountDelete } from './accountDelete'
 import { AccountSet, validateAccountSet } from './accountSet'
@@ -255,7 +255,7 @@ export function validate(transaction: Record<string, unknown>): void {
  })

  // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- okay here
-  setTransactionFlagsToNumber(tx as unknown as Transaction)
+  tx.Flags = convertTxFlagsToNumber(tx as unknown as Transaction)
  switch (tx.TransactionType) {
    case 'AMMBid':
      validateAMMBid(tx)
--- a/packages/xrpl/src/models/utils/flags.ts
+++ b/packages/xrpl/src/models/utils/flags.ts
@@ -1,4 +1,3 @@
-/* eslint-disable no-param-reassign -- param reassign is safe */
 /* eslint-disable no-bitwise -- flags require bitwise operations */
 import { ValidationError } from '../../errors'
 import {
@@ -8,7 +7,6 @@ import {
 import { AccountSetTfFlags } from '../transactions/accountSet'
 import { AMMDepositFlags } from '../transactions/AMMDeposit'
 import { AMMWithdrawFlags } from '../transactions/AMMWithdraw'
-import { GlobalFlags } from '../transactions/common'
 import { MPTokenAuthorizeFlags } from '../transactions/MPTokenAuthorize'
 import { MPTokenIssuanceCreateFlags } from '../transactions/MPTokenIssuanceCreate'
 import { MPTokenIssuanceSetFlags } from '../transactions/MPTokenIssuanceSet'
@@ -63,37 +61,61 @@ const txToFlag = {
  XChainModifyBridge: XChainModifyBridgeFlags,
 }

+function isTxToFlagKey(
+  transactionType: string,
+): transactionType is keyof typeof txToFlag {
+  return transactionType in txToFlag
+}
+
 /**
 * Sets a transaction's flags to its numeric representation.
 *
+ * @deprecated
+ * This utility function is deprecated.
+ * Use convertTxFlagsToNumber() instead and use the returned value to modify the Transaction.Flags from the caller.
+ *
 * @param tx - A transaction to set its flags to its numeric representation.
 */
 export function setTransactionFlagsToNumber(tx: Transaction): void {
-  if (tx.Flags == null) {
-    tx.Flags = 0
-    return
-  }
-  if (typeof tx.Flags === 'number') {
-    return
-  }
+  // eslint-disable-next-line no-console -- intended deprecation warning
+  console.warn(
+    'This function is deprecated. Use convertTxFlagsToNumber() instead and use the returned value to modify the Transaction.Flags from the caller.',
+  )

-  tx.Flags = txToFlag[tx.TransactionType]
-    ? convertFlagsToNumber(tx.Flags, txToFlag[tx.TransactionType])
-    : 0
+  if (tx.Flags) {
+    // eslint-disable-next-line no-param-reassign -- intended param reassign in setter, retain old functionality for compatibility
+    tx.Flags = convertTxFlagsToNumber(tx)
+  }
 }

-// eslint-disable-next-line @typescript-eslint/no-explicit-any -- added ValidationError check for flagEnum
-function convertFlagsToNumber(flags: GlobalFlags, flagEnum: any): number {
-  return Object.keys(flags).reduce((resultFlags, flag) => {
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- safe member access
-    if (flagEnum[flag] == null) {
-      throw new ValidationError(
-        `flag ${flag} doesn't exist in flagEnum: ${JSON.stringify(flagEnum)}`,
-      )
-    }
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- safe member access
-    return flags[flag] ? resultFlags | flagEnum[flag] : resultFlags
-  }, 0)
+/**
+ * Returns a Transaction's Flags as its numeric representation.
+ *
+ * @param tx - A Transaction to parse Flags for
+ * @returns A numerical representation of a Transaction's Flags
+ */
+export function convertTxFlagsToNumber(tx: Transaction): number {
+  if (!tx.Flags) {
+    return 0
+  }
+  if (typeof tx.Flags === 'number') {
+    return tx.Flags
+  }
+
+  if (isTxToFlagKey(tx.TransactionType)) {
+    const flagEnum = txToFlag[tx.TransactionType]
+    return Object.keys(tx.Flags).reduce((resultFlags, flag) => {
+      if (flagEnum[flag] == null) {
+        throw new ValidationError(
+          `Invalid flag ${flag}. Valid flags are ${JSON.stringify(flagEnum)}`,
+        )
+      }
+
+      return tx.Flags?.[flag] ? resultFlags | flagEnum[flag] : resultFlags
+    }, 0)
+  }
+
+  return 0
 }

 /**
@@ -103,22 +125,24 @@ function convertFlagsToNumber(flags: GlobalFlags, flagEnum: any): number {
 * @returns A map with all flags as booleans.
 */
 export function parseTransactionFlags(tx: Transaction): object {
-  setTransactionFlagsToNumber(tx)
-  if (typeof tx.Flags !== 'number' || !tx.Flags || tx.Flags === 0) {
+  const flags = convertTxFlagsToNumber(tx)
+  if (flags === 0) {
    return {}
  }

-  const flags = tx.Flags
-  const flagsMap = {}
+  const booleanFlagMap = {}

-  // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- safe member access
-  const flagEnum = txToFlag[tx.TransactionType]
-  Object.values(flagEnum).forEach((flag) => {
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- safe member access
-    if (typeof flag === 'string' && isFlagEnabled(flags, flagEnum[flag])) {
-      flagsMap[flag] = true
-    }
-  })
+  if (isTxToFlagKey(tx.TransactionType)) {
+    const transactionTypeFlags = txToFlag[tx.TransactionType]
+    Object.values(transactionTypeFlags).forEach((flag) => {
+      if (
+        typeof flag === 'string' &&
+        isFlagEnabled(flags, transactionTypeFlags[flag])
+      ) {
+        booleanFlagMap[flag] = true
+      }
+    })
+  }

-  return flagsMap
+  return booleanFlagMap
 }
--- a/packages/xrpl/test/models/utils.test.ts
+++ b/packages/xrpl/test/models/utils.test.ts
@@ -1,3 +1,4 @@
+/* eslint-disable import/no-deprecated -- using deprecated setTransactionFlagsToNumbers to ensure no breaking changes */
 /* eslint-disable no-bitwise -- flags require bitwise operations */
 import { assert } from 'chai'

@@ -16,6 +17,7 @@ import { AccountRootFlags } from '../../src/models/ledger'
 import { isFlagEnabled } from '../../src/models/utils'
 import {
  setTransactionFlagsToNumber,
+  convertTxFlagsToNumber,
  parseAccountRootFlags,
  parseTransactionFlags,
 } from '../../src/models/utils/flags'
@@ -46,6 +48,65 @@ describe('Models Utils', function () {
    })
  })

+  describe('parseAccountRootFlags', function () {
+    // eslint-disable-next-line complexity -- Simpler to list them all out at once.
+    it('all flags enabled', function () {
+      const accountRootFlags =
+        AccountRootFlags.lsfDefaultRipple |
+        AccountRootFlags.lsfDepositAuth |
+        AccountRootFlags.lsfDisableMaster |
+        AccountRootFlags.lsfDisallowXRP |
+        AccountRootFlags.lsfGlobalFreeze |
+        AccountRootFlags.lsfNoFreeze |
+        AccountRootFlags.lsfPasswordSpent |
+        AccountRootFlags.lsfRequireAuth |
+        AccountRootFlags.lsfRequireDestTag |
+        AccountRootFlags.lsfDisallowIncomingNFTokenOffer |
+        AccountRootFlags.lsfDisallowIncomingCheck |
+        AccountRootFlags.lsfDisallowIncomingPayChan |
+        AccountRootFlags.lsfDisallowIncomingTrustline |
+        AccountRootFlags.lsfAllowTrustLineClawback
+
+      const parsed = parseAccountRootFlags(accountRootFlags)
+
+      assert.isTrue(
+        parsed.lsfDefaultRipple &&
+          parsed.lsfDepositAuth &&
+          parsed.lsfDisableMaster &&
+          parsed.lsfDisallowXRP &&
+          parsed.lsfGlobalFreeze &&
+          parsed.lsfNoFreeze &&
+          parsed.lsfPasswordSpent &&
+          parsed.lsfRequireAuth &&
+          parsed.lsfRequireDestTag &&
+          parsed.lsfDisallowIncomingNFTokenOffer &&
+          parsed.lsfDisallowIncomingCheck &&
+          parsed.lsfDisallowIncomingPayChan &&
+          parsed.lsfDisallowIncomingTrustline &&
+          parsed.lsfAllowTrustLineClawback,
+      )
+    })
+
+    it('no flags set', function () {
+      const parsed = parseAccountRootFlags(0)
+
+      assert.isUndefined(parsed.lsfDefaultRipple)
+      assert.isUndefined(parsed.lsfDepositAuth)
+      assert.isUndefined(parsed.lsfDisableMaster)
+      assert.isUndefined(parsed.lsfDisallowXRP)
+      assert.isUndefined(parsed.lsfGlobalFreeze)
+      assert.isUndefined(parsed.lsfNoFreeze)
+      assert.isUndefined(parsed.lsfPasswordSpent)
+      assert.isUndefined(parsed.lsfRequireAuth)
+      assert.isUndefined(parsed.lsfRequireDestTag)
+      assert.isUndefined(parsed.lsfDisallowIncomingNFTokenOffer)
+      assert.isUndefined(parsed.lsfDisallowIncomingCheck)
+      assert.isUndefined(parsed.lsfDisallowIncomingPayChan)
+      assert.isUndefined(parsed.lsfDisallowIncomingTrustline)
+      assert.isUndefined(parsed.lsfAllowTrustLineClawback)
+    })
+  })
+
  describe('setTransactionFlagsToNumber', function () {
    it('sets OfferCreateFlags to its numeric value', function () {
      const tx: OfferCreate = {
@@ -151,64 +212,9 @@ describe('Models Utils', function () {
      setTransactionFlagsToNumber(tx)
      assert.strictEqual(tx.Flags, 0)
    })
+  })

-    // eslint-disable-next-line complexity -- Simpler to list them all out at once.
-    it('parseAccountRootFlags all enabled', function () {
-      const accountRootFlags =
-        AccountRootFlags.lsfDefaultRipple |
-        AccountRootFlags.lsfDepositAuth |
-        AccountRootFlags.lsfDisableMaster |
-        AccountRootFlags.lsfDisallowXRP |
-        AccountRootFlags.lsfGlobalFreeze |
-        AccountRootFlags.lsfNoFreeze |
-        AccountRootFlags.lsfPasswordSpent |
-        AccountRootFlags.lsfRequireAuth |
-        AccountRootFlags.lsfRequireDestTag |
-        AccountRootFlags.lsfDisallowIncomingNFTokenOffer |
-        AccountRootFlags.lsfDisallowIncomingCheck |
-        AccountRootFlags.lsfDisallowIncomingPayChan |
-        AccountRootFlags.lsfDisallowIncomingTrustline |
-        AccountRootFlags.lsfAllowTrustLineClawback
-
-      const parsed = parseAccountRootFlags(accountRootFlags)
-
-      assert.isTrue(
-        parsed.lsfDefaultRipple &&
-          parsed.lsfDepositAuth &&
-          parsed.lsfDisableMaster &&
-          parsed.lsfDisallowXRP &&
-          parsed.lsfGlobalFreeze &&
-          parsed.lsfNoFreeze &&
-          parsed.lsfPasswordSpent &&
-          parsed.lsfRequireAuth &&
-          parsed.lsfRequireDestTag &&
-          parsed.lsfDisallowIncomingNFTokenOffer &&
-          parsed.lsfDisallowIncomingCheck &&
-          parsed.lsfDisallowIncomingPayChan &&
-          parsed.lsfDisallowIncomingTrustline &&
-          parsed.lsfAllowTrustLineClawback,
-      )
-    })
-
-    it('parseAccountFlags all false', function () {
-      const parsed = parseAccountRootFlags(0)
-
-      assert.isUndefined(parsed.lsfDefaultRipple)
-      assert.isUndefined(parsed.lsfDepositAuth)
-      assert.isUndefined(parsed.lsfDisableMaster)
-      assert.isUndefined(parsed.lsfDisallowXRP)
-      assert.isUndefined(parsed.lsfGlobalFreeze)
-      assert.isUndefined(parsed.lsfNoFreeze)
-      assert.isUndefined(parsed.lsfPasswordSpent)
-      assert.isUndefined(parsed.lsfRequireAuth)
-      assert.isUndefined(parsed.lsfRequireDestTag)
-      assert.isUndefined(parsed.lsfDisallowIncomingNFTokenOffer)
-      assert.isUndefined(parsed.lsfDisallowIncomingCheck)
-      assert.isUndefined(parsed.lsfDisallowIncomingPayChan)
-      assert.isUndefined(parsed.lsfDisallowIncomingTrustline)
-      assert.isUndefined(parsed.lsfAllowTrustLineClawback)
-    })
-
+  describe('parseTransactionFlags', function () {
    it('parseTransactionFlags all enabled', function () {
      const tx: PaymentChannelClaim = {
        Account: 'r...',
@@ -264,4 +270,111 @@ describe('Models Utils', function () {
      assert.notStrictEqual(flagsMap, expected)
    })
  })
+
+  describe('convertTxFlagsToNumber', function () {
+    it('converts OfferCreateFlags to its numeric value', function () {
+      const tx: OfferCreate = {
+        Account: 'r3rhWeE31Jt5sWmi4QiGLMZnY3ENgqw96W',
+        Fee: '10',
+        TakerGets: {
+          currency: 'DSH',
+          issuer: 'rcXY84C4g14iFp6taFXjjQGVeHqSCh9RX',
+          value: '43.11584856965009',
+        },
+        TakerPays: '12928290425',
+        TransactionType: 'OfferCreate',
+        TxnSignature:
+          '3045022100D874CDDD6BB24ED66E83B1D3574D3ECAC753A78F26DB7EBA89EAB8E7D72B95F802207C8CCD6CEA64E4AE2014E59EE9654E02CA8F03FE7FCE0539E958EAE182234D91',
+        Flags: {
+          tfPassive: true,
+          tfImmediateOrCancel: false,
+          tfFillOrKill: true,
+          tfSell: false,
+        },
+      }
+
+      const { tfPassive, tfFillOrKill } = OfferCreateFlags
+      const expected: number = tfPassive | tfFillOrKill
+
+      const result = convertTxFlagsToNumber(tx)
+      assert.strictEqual(result, expected)
+    })
+
+    it('converts PaymentChannelClaimFlags to its numeric value', function () {
+      const tx: PaymentChannelClaim = {
+        Account: 'r...',
+        TransactionType: 'PaymentChannelClaim',
+        Channel:
+          'C1AE6DDDEEC05CF2978C0BAD6FE302948E9533691DC749DCDD3B9E5992CA6198',
+        Flags: {
+          tfRenew: true,
+          tfClose: false,
+        },
+      }
+
+      const { tfRenew } = PaymentChannelClaimFlags
+      const expected: number = tfRenew
+
+      const result = convertTxFlagsToNumber(tx)
+      assert.strictEqual(result, expected)
+    })
+
+    it('converts PaymentTransactionFlags to its numeric value', function () {
+      const tx: Payment = {
+        TransactionType: 'Payment',
+        Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
+        Amount: '1234',
+        Destination: 'rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy',
+        Flags: {
+          tfNoRippleDirect: false,
+          tfPartialPayment: true,
+          tfLimitQuality: true,
+        },
+      }
+
+      const { tfPartialPayment, tfLimitQuality } = PaymentFlags
+      const expected: number = tfPartialPayment | tfLimitQuality
+
+      const result = convertTxFlagsToNumber(tx)
+      assert.strictEqual(result, expected)
+    })
+
+    it('converts TrustSetFlags to its numeric value', function () {
+      const tx: TrustSet = {
+        TransactionType: 'TrustSet',
+        Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
+        LimitAmount: {
+          currency: 'XRP',
+          issuer: 'rcXY84C4g14iFp6taFXjjQGVeHqSCh9RX',
+          value: '4329.23',
+        },
+        QualityIn: 1234,
+        QualityOut: 4321,
+        Flags: {
+          tfSetfAuth: true,
+          tfSetNoRipple: false,
+          tfClearNoRipple: true,
+          tfSetFreeze: false,
+          tfClearFreeze: true,
+        },
+      }
+
+      const { tfSetfAuth, tfClearNoRipple, tfClearFreeze } = TrustSetFlags
+      const expected: number = tfSetfAuth | tfClearNoRipple | tfClearFreeze
+
+      const result = convertTxFlagsToNumber(tx)
+      assert.strictEqual(result, expected)
+    })
+
+    it('converts other transaction types flags to its numeric value', function () {
+      const tx: DepositPreauth = {
+        TransactionType: 'DepositPreauth',
+        Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
+        Flags: {},
+      }
+
+      const result = convertTxFlagsToNumber(tx)
+      assert.strictEqual(result, 0)
+    })
+  })
 })