From 42464b84deeea5765ef17d268a2e34fe3d3d7359 Mon Sep 17 00:00:00 2001
From: Jackson Mills <jmills@ripple.com>
Date: Wed, 4 Aug 2021 12:52:55 -0700
Subject: [PATCH] refactor combine logic for clarity (#1486)

refactor combine logic for clarity by using functional styles and breaking down the logic into digestible pieces
---
 src/transaction/combine.ts | 59 ++++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/src/transaction/combine.ts b/src/transaction/combine.ts
index 3191824d..20fb8d36 100644
--- a/src/transaction/combine.ts
+++ b/src/transaction/combine.ts
@@ -1,43 +1,66 @@
 import * as _ from 'lodash'
 import binary from 'ripple-binary-codec'
-import * as utils from './utils'
 import BigNumber from 'bignumber.js'
+import {ValidationError} from '../common/errors'
 import {decodeAccountID} from 'ripple-address-codec'
 import {validate} from '../common'
 import {computeBinaryTransactionHash} from '../common/hashes'
+import {JsonObject} from 'ripple-binary-codec/dist/types/serialized-type'
+
+/**
+ * The transactions should all be equal except for the 'Signers' field. 
+ */
+ function validateTransactionEquivalence(transactions: Array<JsonObject>) {
+  const exampleTransaction = JSON.stringify({...transactions[0], Signers: null})
+  if (transactions.slice(1).some(tx => JSON.stringify({...tx, Signers: null}) !== exampleTransaction)) {
+    throw new ValidationError('txJSON is not the same for all signedTransactions')
+  }
+}
 
 function addressToBigNumber(address) {
   const hex = Buffer.from(decodeAccountID(address)).toString('hex')
   return new BigNumber(hex, 16)
 }
 
+/**
+ * If presented in binary form, the Signers array must be sorted based on 
+ * the numeric value of the signer addresses, with the lowest value first. 
+ * (If submitted as JSON, the submit_multisigned method handles this automatically.)
+ * https://xrpl.org/multi-signing.html
+ */
 function compareSigners(a, b) {
   return addressToBigNumber(a.Signer.Account).comparedTo(
     addressToBigNumber(b.Signer.Account)
   )
 }
 
+function getTransactionWithAllSigners(transactions: Array<JsonObject>): JsonObject {
+  // Signers must be sorted - see compareSigners for more details
+  const sortedSigners = _.flatMap(transactions, tx => tx.Signers)
+    .filter(signer => signer)
+    .sort(compareSigners)
+
+  return {...transactions[0], Signers: sortedSigners}
+}
+
+/**
+ * 
+ * @param signedTransactions A collection of the same transaction signed by different signers. The only difference
+ * between the elements of signedTransactions should be the Signers field.
+ * @returns An object with the combined transaction (now having a sorted list of all signers) which is encoded, along
+ * with a transaction id based on the combined transaction.
+ */
 function combine(signedTransactions: Array<string>): object {
   validate.combine({signedTransactions})
 
-  // TODO: signedTransactions is an array of strings in the documentation, but
-  // tests and this code handle it as an array of objects. Fix!
-  const txs: any[] = signedTransactions.map(binary.decode)
-  const tx = _.omit(txs[0], 'Signers')
-  if (!txs.every((_tx) => _.isEqual(tx, _.omit(_tx, 'Signers')))) {
-    throw new utils.common.errors.ValidationError(
-      'txJSON is not the same for all signedTransactions'
-    )
+  const transactions: JsonObject[] = signedTransactions.map(binary.decode);
+  validateTransactionEquivalence(transactions)
+
+  const signedTransaction = binary.encode(getTransactionWithAllSigners(transactions))
+  return {
+    signedTransaction: signedTransaction, 
+    id: computeBinaryTransactionHash(signedTransaction)
   }
-  const unsortedSigners = txs.reduce(
-    (accumulator, _tx) => accumulator.concat(_tx.Signers || []),
-    []
-  )
-  const signers = unsortedSigners.sort(compareSigners)
-  const signedTx = Object.assign({}, tx, {Signers: signers})
-  const signedTransaction = binary.encode(signedTx)
-  const id = computeBinaryTransactionHash(signedTransaction)
-  return {signedTransaction, id}
 }
 
 export default combine