fix: remove tx serialization check (#2293)

* remove tx serialization check

* clean up

* update changelog

* bring back some of the logic to check txs

* move memo logic to `validate`

* move other checks to `validate`

* fix tests

* update cspell

.vscode/settings.json

@@ -1,11 +1,15 @@
 {
     "editor.tabSize": 2,
     "cSpell.words": [
-        "Multisigned",
-        "Setf",
         "hostid",
+        "keypair",
+        "keypairs",
+        "multisign",
+        "multisigned",
+        "multisigning",
         "preauthorization",
-        "secp256k1"
+        "secp256k1",
+        "Setf"
     ],
     "[javascript]": {
         "editor.defaultFormatter": "esbenp.prettier-vscode",

packages/xrpl/HISTORY.md

@@ -9,6 +9,7 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr
 ### Fixed
 * Fixed `ServerState.transitions` typing, it is now a string instead of a number. (Only used in return from `server_state` request)
 * Added `destination_amount` to `PathOption` which is returned as part of a `path_find` request
+* Removed the `decode(encode(tx)) == tx` check from the wallet signing process
 
 ### Removed
 * RPCs and utils related to the old sidechain design

packages/xrpl/src/Wallet/index.ts

@@ -1,8 +1,6 @@
-/* eslint-disable max-lines -- There are lots of equivalent constructors which make sense to have here. */
 import BigNumber from 'bignumber.js'
 import { fromSeed } from 'bip32'
 import { mnemonicToSeedSync, validateMnemonic } from 'bip39'
-import isEqual from 'lodash/isEqual'
 import omitBy from 'lodash/omitBy'
 import {
   classicAddressToXAddress,
@@ -25,11 +23,8 @@ import {
 } from 'ripple-keypairs'
 
 import ECDSA from '../ECDSA'
-import { ValidationError, XrplError } from '../errors'
-import { IssuedCurrencyAmount } from '../models/common'
+import { ValidationError } from '../errors'
 import { Transaction, validate } from '../models/transactions'
-import { isIssuedCurrency } from '../models/transactions/common'
-import { isHex } from '../models/utils'
 import { ensureClassicAddress } from '../sugar/utils'
 import { hashSignedTx } from '../utils/hashes/hashLedger'
 
@@ -368,7 +363,6 @@ class Wallet {
     }
 
     const serialized = encode(txToSignAndEncode)
-    this.checkTxSerialization(serialized, tx)
     return {
       tx_blob: serialized,
       hash: hashSignedTx(serialized),
@@ -401,124 +395,6 @@ class Wallet {
   public getXAddress(tag: number | false = false, isTestnet = false): string {
     return classicAddressToXAddress(this.classicAddress, tag, isTestnet)
   }
-
-  /**
-   *  Decode a serialized transaction, remove the fields that are added during the signing process,
-   *  and verify that it matches the transaction prior to signing. This gives the user a sanity check
-   *  to ensure that what they try to encode matches the message that will be recieved by rippled.
-   *
-   * @param serialized - A signed and serialized transaction.
-   * @param tx - The transaction prior to signing.
-   * @throws A ValidationError if the transaction does not have a TxnSignature/Signers property, or if
-   * the serialized Transaction desn't match the original transaction.
-   * @throws XrplError if the transaction includes an issued currency which is equivalent to XRP ignoring case.
-   */
-  // eslint-disable-next-line class-methods-use-this, max-lines-per-function -- Helper for organization purposes
-  private checkTxSerialization(serialized: string, tx: Transaction): void {
-    // Decode the serialized transaction:
-    const decoded = decode(serialized)
-    const txCopy = { ...tx }
-
-    /*
-     * And ensure it is equal to the original tx, except:
-     * - It must have a TxnSignature or Signers (multisign).
-     */
-    if (!decoded.TxnSignature && !decoded.Signers) {
-      throw new ValidationError(
-        'Serialized transaction must have a TxnSignature or Signers property',
-      )
-    }
-    // - We know that the original tx did not have TxnSignature, so we should delete it:
-    delete decoded.TxnSignature
-    // - We know that the original tx did not have Signers, so if it exists, we should delete it:
-    delete decoded.Signers
-
-    /*
-     * - If SigningPubKey was not in the original tx, then we should delete it.
-     *   But if it was in the original tx, then we should ensure that it has not been changed.
-     */
-    if (!tx.SigningPubKey) {
-      delete decoded.SigningPubKey
-    }
-
-    /*
-     * - Memos have exclusively hex data which should ignore case.
-     *   Since decode goes to upper case, we set all tx memos to be uppercase for the comparison.
-     */
-    txCopy.Memos?.map((memo) => {
-      const memoCopy = { ...memo }
-      if (memo.Memo.MemoData) {
-        if (!isHex(memo.Memo.MemoData)) {
-          throw new ValidationError('MemoData field must be a hex value')
-        }
-        memoCopy.Memo.MemoData = memo.Memo.MemoData.toUpperCase()
-      }
-
-      if (memo.Memo.MemoType) {
-        if (!isHex(memo.Memo.MemoType)) {
-          throw new ValidationError('MemoType field must be a hex value')
-        }
-        memoCopy.Memo.MemoType = memo.Memo.MemoType.toUpperCase()
-      }
-
-      if (memo.Memo.MemoFormat) {
-        if (!isHex(memo.Memo.MemoFormat)) {
-          throw new ValidationError('MemoFormat field must be a hex value')
-        }
-        memoCopy.Memo.MemoFormat = memo.Memo.MemoFormat.toUpperCase()
-      }
-
-      return memo
-    })
-
-    if (txCopy.TransactionType === 'NFTokenMint' && txCopy.URI) {
-      txCopy.URI = txCopy.URI.toUpperCase()
-    }
-
-    /* eslint-disable @typescript-eslint/consistent-type-assertions -- We check at runtime that this is safe */
-    Object.keys(txCopy).forEach((key) => {
-      const standard_currency_code_len = 3
-      if (txCopy[key] && isIssuedCurrency(txCopy[key])) {
-        const decodedAmount = decoded[key] as unknown as IssuedCurrencyAmount
-        const decodedCurrency = decodedAmount.currency
-        const txCurrency = (txCopy[key] as IssuedCurrencyAmount).currency
-
-        if (
-          txCurrency.length === standard_currency_code_len &&
-          txCurrency.toUpperCase() === 'XRP'
-        ) {
-          throw new XrplError(
-            `Trying to sign an issued currency with a similar standard code to XRP (received '${txCurrency}'). XRP is not an issued currency.`,
-          )
-        }
-
-        // Standardize the format of currency codes to the 40 byte hex string for comparison
-        const amount = txCopy[key] as IssuedCurrencyAmount
-        if (amount.currency.length !== decodedCurrency.length) {
-          /* eslint-disable-next-line max-depth -- Easier to read with two if-statements */
-          if (decodedCurrency.length === standard_currency_code_len) {
-            decodedAmount.currency = isoToHex(decodedCurrency)
-          } else {
-            /* eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -- We need to update txCopy directly */
-            txCopy[key].currency = isoToHex(txCopy[key].currency)
-          }
-        }
-      }
-    })
-    /* eslint-enable @typescript-eslint/consistent-type-assertions -- Done with dynamic checking */
-
-    if (!isEqual(decoded, txCopy)) {
-      const data = {
-        decoded,
-        tx,
-      }
-      const error = new ValidationError(
-        'Serialized transaction does not match original txJSON. See error.data',
-        data,
-      )
-      throw error
-    }
-  }
 }
 
 /**
@@ -567,20 +443,4 @@ function removeTrailingZeros(tx: Transaction): void {
   }
 }
 
-/**
- * Convert an ISO code to a hex string representation
- *
- * @param iso - A 3 letter standard currency code
- */
-/* eslint-disable @typescript-eslint/no-magic-numbers -- Magic numbers are from rippleds of currency code encoding */
-function isoToHex(iso: string): string {
-  const bytes = Buffer.alloc(20)
-  if (iso !== 'XRP') {
-    const isoBytes = iso.split('').map((chr) => chr.charCodeAt(0))
-    bytes.set(isoBytes, 12)
-  }
-  return bytes.toString('hex').toUpperCase()
-}
-/* eslint-enable @typescript-eslint/no-magic-numbers -- Only needed in this function */
-
 export default Wallet

packages/xrpl/src/models/transactions/transaction.ts

@@ -2,6 +2,8 @@
 /* eslint-disable max-lines-per-function -- need to work with a lot of Tx verifications */
 
 import { ValidationError } from '../../errors'
+import { IssuedCurrencyAmount, Memo } from '../common'
+import { isHex } from '../utils'
 import { setTransactionFlagsToNumber } from '../utils/flags'
 
 import { AccountDelete, validateAccountDelete } from './accountDelete'
@@ -9,6 +11,7 @@ import { AccountSet, validateAccountSet } from './accountSet'
 import { CheckCancel, validateCheckCancel } from './checkCancel'
 import { CheckCash, validateCheckCash } from './checkCash'
 import { CheckCreate, validateCheckCreate } from './checkCreate'
+import { isIssuedCurrency } from './common'
 import { DepositPreauth, validateDepositPreauth } from './depositPreauth'
 import { EscrowCancel, validateEscrowCancel } from './escrowCancel'
 import { EscrowCreate, validateEscrowCreate } from './escrowCreate'
@@ -101,6 +104,56 @@ export function validate(transaction: Record<string, unknown>): void {
   if (typeof tx.TransactionType !== 'string') {
     throw new ValidationError("Object's `TransactionType` is not a string")
   }
+
+  /*
+   * - Memos have exclusively hex data.
+   */
+  if (tx.Memos != null && typeof tx.Memos !== 'object') {
+    throw new ValidationError('Memo must be array')
+  }
+  if (tx.Memos != null) {
+    // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- needed here
+    ;(tx.Memos as Array<Memo | null>).forEach((memo) => {
+      if (memo?.Memo == null) {
+        throw new ValidationError('Memo data must be in a `Memo` field')
+      }
+      if (memo.Memo.MemoData) {
+        if (!isHex(memo.Memo.MemoData)) {
+          throw new ValidationError('MemoData field must be a hex value')
+        }
+      }
+
+      if (memo.Memo.MemoType) {
+        if (!isHex(memo.Memo.MemoType)) {
+          throw new ValidationError('MemoType field must be a hex value')
+        }
+      }
+
+      if (memo.Memo.MemoFormat) {
+        if (!isHex(memo.Memo.MemoFormat)) {
+          throw new ValidationError('MemoFormat field must be a hex value')
+        }
+      }
+    })
+  }
+
+  Object.keys(tx).forEach((key) => {
+    const standard_currency_code_len = 3
+    if (tx[key] && isIssuedCurrency(tx[key])) {
+      // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- needed
+      const txCurrency = (tx[key] as IssuedCurrencyAmount).currency
+
+      if (
+        txCurrency.length === standard_currency_code_len &&
+        txCurrency.toUpperCase() === 'XRP'
+      ) {
+        throw new ValidationError(
+          `Cannot have an issued currency with a similar standard code to XRP (received '${txCurrency}'). XRP is not an issued currency.`,
+        )
+      }
+    }
+  })
+
   // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- okay here
   setTransactionFlagsToNumber(tx as unknown as Transaction)
   switch (tx.TransactionType) {

packages/xrpl/test/models/trustSet.test.ts

@@ -16,7 +16,7 @@ describe('TrustSet', function () {
       TransactionType: 'TrustSet',
       Account: 'rUn84CUYbNjRoTQ6mSW7BVJPSVJNLb1QLo',
       LimitAmount: {
-        currency: 'XRP',
+        currency: 'USD',
         issuer: 'rcXY84C4g14iFp6taFXjjQGVeHqSCh9RX',
         value: '4329.23',
       },

packages/xrpl/test/wallet/index.test.ts

@@ -763,7 +763,7 @@ describe('Wallet', function () {
       }
       assert.throws(() => {
         wallet.sign(payment)
-      }, /^Trying to sign an issued currency with a similar standard code to XRP \(received 'xrp'\)\. XRP is not an issued currency\./u)
+      }, /^Cannot have an issued currency with a similar standard code to XRP \(received 'xrp'\)\. XRP is not an issued currency\./u)
     })
 
     it('sign does NOT throw when a payment contains an issued currency like xrp in hex string format', async function () {