From abdb192c697690f503843c1fdba4c69db552ea10 Mon Sep 17 00:00:00 2001 From: achowdhry-ripple Date: Thu, 16 Jan 2025 11:59:44 -0500 Subject: [PATCH] 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 * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/src/models/utils/flags.ts Co-authored-by: Omar Khan * Update packages/xrpl/test/models/utils.test.ts Co-authored-by: Omar Khan * rename a test per pr comment * lint fixes * Update packages/xrpl/test/models/utils.test.ts Co-authored-by: Omar Khan --------- Co-authored-by: Omar Khan --- packages/xrpl/HISTORY.md | 8 +- packages/xrpl/src/client/index.ts | 4 +- packages/xrpl/src/models/index.ts | 3 +- .../src/models/transactions/transaction.ts | 4 +- packages/xrpl/src/models/utils/flags.ts | 94 +++++--- packages/xrpl/test/models/utils.test.ts | 227 +++++++++++++----- 6 files changed, 242 insertions(+), 98 deletions(-) diff --git a/packages/xrpl/HISTORY.md b/packages/xrpl/HISTORY.md index b834707f69..db958dab05 100644 --- 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) diff --git a/packages/xrpl/src/client/index.ts b/packages/xrpl/src/client/index.ts index 722b169f2a..0afaa29b72 100644 --- 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 { const tx = { ...transaction } setValidAddresses(tx) - setTransactionFlagsToNumber(tx) + tx.Flags = convertTxFlagsToNumber(tx) const promises: Array> = [] if (tx.NetworkID == null) { diff --git a/packages/xrpl/src/models/index.ts b/packages/xrpl/src/models/index.ts index 5c98faf82e..56212a9bc3 100644 --- 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' diff --git a/packages/xrpl/src/models/transactions/transaction.ts b/packages/xrpl/src/models/transactions/transaction.ts index 44840187bb..8c02b823d5 100644 --- 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): 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) diff --git a/packages/xrpl/src/models/utils/flags.ts b/packages/xrpl/src/models/utils/flags.ts index 0d04b0ecad..c2a88662a7 100644 --- 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 + // 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.', + ) + + if (tx.Flags) { + // eslint-disable-next-line no-param-reassign -- intended param reassign in setter, retain old functionality for compatibility + 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 (!tx.Flags) { + return 0 } if (typeof tx.Flags === 'number') { - return + return tx.Flags } - tx.Flags = txToFlag[tx.TransactionType] - ? convertFlagsToNumber(tx.Flags, txToFlag[tx.TransactionType]) - : 0 -} + 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)}`, + ) + } -// 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) + 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 } diff --git a/packages/xrpl/test/models/utils.test.ts b/packages/xrpl/test/models/utils.test.ts index c4313610b7..88d9286a0a 100644 --- 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) + }) + }) })