Fix: parseTransactionFlags unintentionally modifies transaction

packages/xrpl/HISTORY.md

@@ -3,6 +3,9 @@
 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
+* Fix for parseTransactionFlags to no longer modify a passed in transaction when returning a parsed mapping
+* Deprecate setTransactionFlagsToNumber. Should use convertTxFlagsToNumber instead
+* Add convertTxFlagsToNumber as a utility function in xrpl to return the number conversion of a transaction's flags
 
 ### Added
 * parseTransactionFlags as a utility function in the xrpl package to streamline transactions flags-to-map conversion

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) {

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'

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 { NFTokenCreateOfferFlags } from '../transactions/NFTokenCreateOffer'
 import { NFTokenMintFlags } from '../transactions/NFTokenMint'
 import { OfferCreateFlags } from '../transactions/offerCreate'
@@ -60,34 +58,50 @@ const 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 tx.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
-  }
+  // 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 tx.Flags from the caller.',
+  )
+
+  // eslint-disable-next-line no-param-reassign -- intended param reassign in setter
+  tx.Flags = convertTxFlagsToNumber(tx)
+}
+
+/**
+ * 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 (typeof tx.Flags === 'number') {
-    return
+    return tx.Flags
   }
 
-  tx.Flags = txToFlag[tx.TransactionType]
-    ? convertFlagsToNumber(tx.Flags, txToFlag[tx.TransactionType])
-    : 0
-}
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- safe member access
+  const flagEnum = txToFlag[tx.TransactionType]
 
-// 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)
+  if (flagEnum && tx.Flags) {
+    return Object.keys(tx.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 tx.Flags?.[flag] ? resultFlags | flagEnum[flag] : resultFlags
+    }, 0)
+  }
+  return 0
 }
 
 /**
@@ -97,22 +111,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
+  const transactionTypeFlags = txToFlag[tx.TransactionType]
+  Object.values(transactionTypeFlags).forEach((flag) => {
+    if (
+      typeof flag === 'string' &&
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- safe member access
+      isFlagEnabled(flags, transactionTypeFlags[flag])
+    ) {
+      booleanFlagMap[flag] = true
     }
   })
 
-  return flagsMap
+  return booleanFlagMap
 }

packages/xrpl/test/models/utils.test.ts

@@ -15,7 +15,7 @@ import {
 import { AccountRootFlags } from '../../src/models/ledger'
 import { isFlagEnabled } from '../../src/models/utils'
 import {
-  setTransactionFlagsToNumber,
+  convertTxFlagsToNumber,
   parseAccountRootFlags,
   parseTransactionFlags,
 } from '../../src/models/utils/flags'
@@ -71,8 +71,8 @@ describe('Models Utils', function () {
       const { tfPassive, tfFillOrKill } = OfferCreateFlags
       const expected: number = tfPassive | tfFillOrKill
 
-      setTransactionFlagsToNumber(tx)
-      assert.strictEqual(tx.Flags, expected)
+      const flagNum = convertTxFlagsToNumber(tx)
+      assert.strictEqual(flagNum, expected)
     })
 
     it('sets PaymentChannelClaimFlags to its numeric value', function () {
@@ -90,8 +90,8 @@ describe('Models Utils', function () {
       const { tfRenew } = PaymentChannelClaimFlags
       const expected: number = tfRenew
 
-      setTransactionFlagsToNumber(tx)
-      assert.strictEqual(tx.Flags, expected)
+      const flagNum = convertTxFlagsToNumber(tx)
+      assert.strictEqual(flagNum, expected)
     })
 
     it('sets PaymentTransactionFlags to its numeric value', function () {
@@ -110,8 +110,8 @@ describe('Models Utils', function () {
       const { tfPartialPayment, tfLimitQuality } = PaymentFlags
       const expected: number = tfPartialPayment | tfLimitQuality
 
-      setTransactionFlagsToNumber(tx)
-      assert.strictEqual(tx.Flags, expected)
+      const flagNum = convertTxFlagsToNumber(tx)
+      assert.strictEqual(flagNum, expected)
     })
 
     it('sets TrustSetFlags to its numeric value', function () {
@@ -137,8 +137,8 @@ describe('Models Utils', function () {
       const { tfSetfAuth, tfClearNoRipple, tfClearFreeze } = TrustSetFlags
       const expected: number = tfSetfAuth | tfClearNoRipple | tfClearFreeze
 
-      setTransactionFlagsToNumber(tx)
-      assert.strictEqual(tx.Flags, expected)
+      const flagNum = convertTxFlagsToNumber(tx)
+      assert.strictEqual(flagNum, expected)
     })
 
     it('sets other transaction types flags to its numeric value', function () {
@@ -148,8 +148,8 @@ describe('Models Utils', function () {
         Flags: {},
       }
 
-      setTransactionFlagsToNumber(tx)
-      assert.strictEqual(tx.Flags, 0)
+      const flagNum = convertTxFlagsToNumber(tx)
+      assert.strictEqual(flagNum, 0)
     })
 
     // eslint-disable-next-line complexity -- Simpler to list them all out at once.