From d3fc28f0236679295009a38cc2430707271820f1 Mon Sep 17 00:00:00 2001
From: Deepak Jangid <deepakjangid@bitgo.com>
Date: Thu, 15 May 2025 01:25:24 +0530
Subject: [PATCH] fix(sdk-coin-xrp): update XRP signers sorting to use numeric
 comparison

Ticket: WIN-5390
---
 modules/sdk-coin-xrp/src/ripple.ts    | 27 ++++++++++++--
 modules/sdk-coin-xrp/test/unit/xrp.ts | 53 +++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/modules/sdk-coin-xrp/src/ripple.ts b/modules/sdk-coin-xrp/src/ripple.ts
index 5a651ef02d..22e87aca35 100644
--- a/modules/sdk-coin-xrp/src/ripple.ts
+++ b/modules/sdk-coin-xrp/src/ripple.ts
@@ -7,9 +7,22 @@
 import * as rippleKeypairs from 'ripple-keypairs';
 import * as xrpl from 'xrpl';
 import { ECPair } from '@bitgo/secp256k1';
+import BigNumber from 'bignumber.js';
 
 import * as binary from 'ripple-binary-codec';
 
+/**
+ * Convert an XRP address to a BigNumber for numeric comparison.
+ * This is needed for proper sorting of signers as required by the XRP protocol.
+ *
+ * @param address - The XRP address to convert
+ * @returns BigNumber representation of the address
+ */
+function addressToBigNumber(address: string): BigNumber {
+  const hex = Buffer.from(xrpl.decodeAccountID(address)).toString('hex');
+  return new BigNumber(hex, 16);
+}
+
 function computeSignature(tx, privateKey, signAs) {
   const signingData = signAs ? binary.encodeForMultisigning(tx, signAs) : binary.encodeForSigning(tx);
   return rippleKeypairs.sign(signingData, privateKey);
@@ -53,9 +66,17 @@ const signWithPrivateKey = function (txHex, privateKey, options) {
       TxnSignature: computeSignature(tx, privateKey, options.signAs),
     };
     // Ordering of private key signing matters, or the Ripple fullnode will throw an 'Unsorted Signers array' error.
-    // Additional signers must be added to the front of the signers array list.
-    if (tx.TxnSignature || tx.Signers) {
-      tx.Signers.unshift({ Signer: signer });
+    // XRP requires Signers array to be sorted based on numeric value of signer addresses (lowest first)
+    if (tx.Signers) {
+      // Add the current signer
+      tx.Signers.push({ Signer: signer });
+
+      // Sort the Signers array by numeric value of Account (address) to ensure proper ordering
+      tx.Signers.sort((a, b) => {
+        const addressBN1 = addressToBigNumber(a.Signer.Account);
+        const addressBN2 = addressToBigNumber(b.Signer.Account);
+        return addressBN1.comparedTo(addressBN2);
+      });
     } else {
       tx.Signers = [{ Signer: signer }];
     }
diff --git a/modules/sdk-coin-xrp/test/unit/xrp.ts b/modules/sdk-coin-xrp/test/unit/xrp.ts
index 8b45187cfd..c8b9616e9e 100644
--- a/modules/sdk-coin-xrp/test/unit/xrp.ts
+++ b/modules/sdk-coin-xrp/test/unit/xrp.ts
@@ -10,6 +10,7 @@ import assert from 'assert';
 import * as rippleBinaryCodec from 'ripple-binary-codec';
 import sinon from 'sinon';
 import * as testData from '../resources/xrp';
+import { SIGNER_USER, SIGNER_BACKUP, SIGNER_BITGO } from '../resources/xrp';
 import * as _ from 'lodash';
 import { XrpToken } from '../../src';
 import * as xrpl from 'xrpl';
@@ -224,6 +225,58 @@ describe('XRP:', function () {
     coSignedHexTransaction.id.should.equal(coSignedJsonTransaction.id);
   });
 
+  it('should correctly sort signers by numeric value of addresses', function () {
+    // Use the test signers from the test resources
+    // These are known valid key pairs where the private key corresponds to the address
+    const signers = [SIGNER_USER, SIGNER_BACKUP, SIGNER_BITGO];
+
+    // Unsigned transaction
+    const unsignedTxHex =
+      '120000228000000024000000072E00000000201B0018D07161400000000003DE2968400000000000002D8114726D0D8A26568D5D9680AC80577C912236717191831449EE221CCACC4DD2BF8862B22B0960A84FC771D9';
+
+    // Sign with first signer
+    let signedTx = ripple.signWithPrivateKey(unsignedTxHex, signers[0].prv, {
+      signAs: signers[0].address,
+    });
+
+    // Sign with second signer
+    signedTx = ripple.signWithPrivateKey(signedTx.signedTransaction, signers[1].prv, {
+      signAs: signers[1].address,
+    });
+
+    // Sign with third signer
+    signedTx = ripple.signWithPrivateKey(signedTx.signedTransaction, signers[2].prv, {
+      signAs: signers[2].address,
+    });
+
+    // Decode the signed transaction
+    const decodedTx = rippleBinaryCodec.decode(signedTx.signedTransaction);
+
+    // Verify that the Signers array exists and has the correct length
+    assert(Array.isArray(decodedTx.Signers));
+    (decodedTx.Signers as Array<any>).length.should.equal(3);
+
+    // Extract the addresses from the Signers array
+    const signerAddresses = (decodedTx.Signers as Array<any>).map((signer) => signer.Signer.Account);
+
+    // Convert addresses to BigNumber for numeric comparison
+    const addressToBigNumber = (address) => {
+      const hex = Buffer.from(xrpl.decodeAccountID(address)).toString('hex');
+      return BigInt('0x' + hex);
+    };
+
+    // Convert the addresses to BigNumber values
+    const signerValues = signerAddresses.map(addressToBigNumber);
+
+    // Verify that the Signers array is sorted in ascending order by numeric value
+    for (let i = 0; i < signerValues.length - 1; i++) {
+      assert(
+        signerValues[i] < signerValues[i + 1],
+        `Signers not properly sorted: ${signerValues[i]} should be less than ${signerValues[i + 1]}`
+      );
+    }
+  });
+
   it('Should be unable to explain bogus XRP transaction', async function () {
     await basecoin
       .explainTransaction({ txHex: 'abcdefgH' })