Skip to content

Conversation

dpkjnr
Copy link
Contributor

@dpkjnr dpkjnr commented May 15, 2025

TICKET: WIN-5390

@dpkjnr dpkjnr requested a review from Copilot May 15, 2025 03:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures that multiple XRP multisigners are ordered correctly by their numeric address value and adds a unit test to verify this behavior.

  • Introduces addressToBigNumber helper and sorts Signers array in signWithPrivateKey
  • Adds a new test case to confirm the numeric sorting of signer entries

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
modules/sdk-coin-xrp/src/ripple.ts Added BigNumber import, addressToBigNumber helper, and updated Signers insertion to include sorting
modules/sdk-coin-xrp/test/unit/xrp.ts Imported SIGNER_* constants and added a test case for signer sorting by numeric address value
Comments suppressed due to low confidence (4)

modules/sdk-coin-xrp/src/ripple.ts:21

  • [nitpick] The name addressToBigNumber is descriptive but verbose. Consider renaming to something shorter like addressToBN or parseAddressBN to improve readability.
function addressToBigNumber(address: string): BigNumber {

modules/sdk-coin-xrp/test/unit/xrp.ts:231

  • [nitpick] The test currently signs in the input order; consider adding a test where the signers array is initially shuffled to explicitly verify that the sorting logic correctly reorders them in numeric ascending order.
const signers = [SIGNER_USER, SIGNER_BACKUP, SIGNER_BITGO];

modules/sdk-coin-xrp/test/unit/xrp.ts:238

  • The ripple namespace isn't imported in this test file, so calling ripple.signWithPrivateKey will throw. Add a proper import, e.g. import * as ripple from '../../src/ripple'; or use the module export that exposes signWithPrivateKey.
let signedTx = ripple.signWithPrivateKey(unsignedTxHex, signers[0].prv, {

modules/sdk-coin-xrp/test/unit/xrp.ts:263

  • [nitpick] This helper uses BigInt to parse addresses while production code uses BigNumber. Consider using the same numeric type (e.g. BigNumber) in tests to align with the implementation and avoid confusion.
const addressToBigNumber = (address) => {

Comment on lines +75 to +79
tx.Signers.sort((a, b) => {
const addressBN1 = addressToBigNumber(a.Signer.Account);
const addressBN2 = addressToBigNumber(b.Signer.Account);
return addressBN1.comparedTo(addressBN2);
});
Copy link
Preview

Copilot AI May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The comparator decodes and converts each address to a BigNumber on every comparison. For larger signer arrays this could be optimized by computing each numeric value once before sorting and reusing them.

Suggested change
tx.Signers.sort((a, b) => {
const addressBN1 = addressToBigNumber(a.Signer.Account);
const addressBN2 = addressToBigNumber(b.Signer.Account);
return addressBN1.comparedTo(addressBN2);
});
// Precompute BigNumber values for each signer address
tx.Signers = tx.Signers.map(signer => ({
...signer,
_addressBN: addressToBigNumber(signer.Signer.Account),
}));
// Sort the Signers array using the precomputed BigNumber values
tx.Signers.sort((a, b) => a._addressBN.comparedTo(b._addressBN));
// Remove the temporary _addressBN property after sorting
tx.Signers = tx.Signers.map(({ _addressBN, ...signer }) => signer);

Copilot uses AI. Check for mistakes.

@dpkjnr dpkjnr force-pushed the fix/WIN-5390-xrp-signers-sorting branch from 0208b49 to d3fc28f Compare May 15, 2025 03:39
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale label Jun 30, 2025
@dpkjnr dpkjnr closed this Jul 3, 2025
@veetragjain veetragjain reopened this Jul 29, 2025
@veetragjain veetragjain force-pushed the fix/WIN-5390-xrp-signers-sorting branch from d3fc28f to eef8b0d Compare July 29, 2025 07:11
@veetragjain veetragjain marked this pull request as ready for review July 29, 2025 08:06
@veetragjain veetragjain requested a review from a team as a code owner July 29, 2025 08:06
@veetragjain veetragjain merged commit b550b7a into master Jul 29, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants