Merge pull request #8834 from LedgerHQ/fix_pagination_issues

[Coin-Module] Fix XRP pagination issue

libs/coin-modules/coin-xrp/package.json

@@ -134,6 +134,7 @@
     "lint": "eslint ./src --no-error-on-unmatched-pattern --ext .ts,.tsx --cache",
     "lint:fix": "pnpm lint --fix",
     "test": "jest",
+    "test-watch": "jest --watch",
     "test-integ": "DOTENV_CONFIG_PATH=.env.integ.test jest --config=jest.integ.config.js",
     "typecheck": "tsc --noEmit",
     "unimported": "unimported"

libs/coin-modules/coin-xrp/src/api/index.ts

@@ -45,7 +45,12 @@ async function operations(
   address: string,
   { limit, start }: Pagination,
 ): Promise<[Operation[], number]> {
-  const [ops, index] = await listOperations(address, { limit, startAt: start ?? 0 });
+  const options: {
+    limit?: number;
+    minHeight?: number;
+  } = { limit: limit };
+  if (start) options.minHeight = start;
+  const [ops, index] = await listOperations(address, options);
   return [
     ops.map(op => {
       const { simpleType, blockHash, blockTime, blockHeight, ...rest } = op;

libs/coin-modules/coin-xrp/src/bridge/synchronization.ts

@@ -63,7 +63,7 @@ async function filterOperations(
   address: string,
   blockHeight: number,
 ): Promise<Operation[]> {
-  const [operations, _] = await listOperations(address, { startAt: blockHeight });
+  const [operations, _] = await listOperations(address, { minHeight: blockHeight });
 
   return operations.map(
     op =>

libs/coin-modules/coin-xrp/src/logic/listOperations.test.ts

@@ -1,9 +1,12 @@
+import { assert } from "console";
 import { listOperations } from "./listOperations";
 import { RIPPLE_EPOCH } from "./utils";
 
+const maxHeight = 2;
+const minHeight = 1;
 const mockGetServerInfos = jest.fn().mockResolvedValue({
   info: {
-    complete_ledgers: "1-2",
+    complete_ledgers: `${minHeight}-${maxHeight}`,
   },
 });
 const mockGetTransactions = jest.fn();
@@ -18,6 +21,94 @@ describe("listOperations", () => {
     mockGetTransactions.mockClear();
   });
 
+  it("when there are no transactions then the result is empty", async () => {
+    // Given
+    mockGetTransactions.mockResolvedValue([]);
+    // When
+    const [results, token] = await listOperations("any address", { minHeight: 0 });
+    // Then
+    expect(mockGetServerInfos).toHaveBeenCalledTimes(1);
+    expect(mockGetTransactions).toHaveBeenCalledTimes(1);
+    expect(results).toEqual([]);
+    expect(token).toEqual(minHeight);
+  });
+
+  it("when there are no transactions and a limit then the result is empty", async () => {
+    // Given
+    mockGetTransactions.mockResolvedValue([]);
+    // When
+    const [results, token] = await listOperations("any address", { minHeight: 0, limit: 1 });
+    // Then
+    expect(mockGetServerInfos).toHaveBeenCalledTimes(1);
+    expect(mockGetTransactions).toHaveBeenCalledTimes(1);
+    expect(results).toEqual([]);
+    expect(token).toEqual(minHeight);
+  });
+
+  const paymentTx = {
+    ledger_hash: "HASH_VALUE_BLOCK",
+    hash: "HASH_VALUE",
+    close_time_iso: "2000-01-01T00:00:01Z",
+    meta: { delivered_amount: "100" },
+    tx_json: {
+      TransactionType: "Payment",
+      Fee: "1",
+      ledger_index: 2,
+      date: 1000,
+      Account: "sender",
+      Destination: "dest",
+      Sequence: 1,
+    },
+  };
+  const otherTx = { ...paymentTx, tx_json: { ...paymentTx.tx_json, TransactionType: "Other" } };
+
+  it("should only list operations of type payment", async () => {
+    // Given
+    const lastTransaction = paymentTx;
+    mockGetTransactions.mockResolvedValueOnce([paymentTx, otherTx, lastTransaction]);
+    mockGetTransactions.mockResolvedValue([]); // subsequent calls
+
+    // When
+    const [results, token] = await listOperations("any address", { minHeight: 0, limit: 3 });
+
+    // Then
+    expect(mockGetServerInfos).toHaveBeenCalledTimes(1);
+    // it's called twice because first call yields only 2 transactions, and 3 are asked
+    expect(mockGetTransactions).toHaveBeenCalledTimes(2);
+    expect(results.length).toEqual(2);
+    expect(token).toEqual(lastTransaction.tx_json.ledger_index - 1);
+  });
+
+  it("should make enough calls so that the limit requested is satified", async () => {
+    const lastTransaction = otherTx;
+    const txs = [paymentTx, paymentTx, otherTx, otherTx, otherTx, otherTx, otherTx, otherTx];
+    assert(txs.length === 8);
+    mockGetTransactions.mockResolvedValue(txs);
+
+    const [results, token] = await listOperations("any address", { minHeight: 0, limit: 8 });
+
+    expect(mockGetServerInfos).toHaveBeenCalledTimes(1);
+    // it's called 4 times because each call yields only 2 transactions, and 8 are asked
+    expect(mockGetTransactions).toHaveBeenCalledTimes(4);
+    expect(results.length).toEqual(8);
+    expect(token).toEqual(lastTransaction.tx_json.ledger_index - 1);
+  });
+
+  it("should make enough calls, even if there is not enough txs to satisfy the limit", async () => {
+    mockGetTransactions.mockResolvedValueOnce([otherTx, otherTx, otherTx, otherTx]);
+    mockGetTransactions.mockResolvedValueOnce([paymentTx, paymentTx]);
+    mockGetTransactions.mockResolvedValue([]); // subsequent calls
+    const lastTransaction = paymentTx;
+
+    const [results, token] = await listOperations("any address", { minHeight: 0, limit: 4 });
+
+    expect(mockGetServerInfos).toHaveBeenCalledTimes(1);
+    // it's called 2 times because the second call is a shortage of txs
+    expect(mockGetTransactions).toHaveBeenCalledTimes(2);
+    expect(results.length).toEqual(2);
+    expect(token).toEqual(lastTransaction.tx_json.ledger_index - 1);
+  });
+
   it.each([
     {
       address: "WHATEVER_ADDRESS",
@@ -34,7 +125,7 @@ describe("listOperations", () => {
   ])(
     "should return the list of operations associated to an account",
     async ({ address, opSender, opDestination, expectedType }) => {
-      // Givem
+      // Given
       const deliveredAmount = 100;
       const fee = 10;
       mockGetTransactions.mockResolvedValue([
@@ -95,7 +186,7 @@ describe("listOperations", () => {
       ]);
 
       // When
-      const [results, _] = await listOperations(address, { startAt: 0 });
+      const [results, _] = await listOperations(address, { minHeight: 0 });
 
       // Then
       expect(mockGetServerInfos).toHaveBeenCalledTimes(1);

libs/coin-modules/coin-xrp/src/logic/listOperations.ts

@@ -13,26 +13,28 @@ export async function listOperations(
   address: string,
   {
     limit,
-    mostRecentIndex,
-    startAt,
+    maxHeight,
+    minHeight,
   }: {
     limit?: number;
-    mostRecentIndex?: number | undefined;
-    startAt?: number;
+    maxHeight?: number | undefined; // used for pagination
+    minHeight?: number; // used to retrieve operations from a specific block height until top most
   },
 ): Promise<[XrpOperation[], number]> {
   const serverInfo = await getServerInfos();
   const ledgers = serverInfo.info.complete_ledgers.split("-");
   const minLedgerVersion = Number(ledgers[0]);
   const maxLedgerVersion = Number(ledgers[1]);
 
-  let options: {
+  type Options = {
     ledger_index_min?: number;
     ledger_index_max?: number;
     limit?: number;
     tx_type?: string;
-  } = {
-    ledger_index_max: mostRecentIndex ?? maxLedgerVersion,
+  };
+
+  let options: Options = {
+    ledger_index_max: maxHeight ?? maxLedgerVersion,
     tx_type: "Payment",
   };
 
@@ -42,38 +44,55 @@ export async function listOperations(
       limit,
     };
   }
-  if (startAt) {
+  if (minHeight) {
     options = {
       ...options,
       // if there is no ops, it might be after a clear and we prefer to pull from the oldest possible history
-      ledger_index_min: Math.max(startAt, minLedgerVersion),
+      ledger_index_min: Math.max(minHeight, minLedgerVersion),
     };
   }
 
-  // We need to filter out the transactions that are not "Payment" type because the filter on "tx_type" of the node RPC
-  // is not working as expected. It returns all the transactions. We need to call the node RPC multiple times to get the
-  // desired number of transactions by the limiter.
-  let transactions: XrplOperation[] = [];
-  let needToStop = true;
-  do {
-    const newTransactions = await getTransactions(address, options);
-    const newPaymentsTxs = newTransactions.filter(tx => tx.tx_json.TransactionType === "Payment");
-    if (options.limit) {
-      needToStop = newTransactions.length < options.limit;
-      options.ledger_index_max = newTransactions.slice(-1)[0].tx_json.ledger_index - 1;
+  async function getPaymentTransactions(
+    address: string,
+    options: Options,
+  ): Promise<[boolean, Options, XrplOperation[]]> {
+    const txs = await getTransactions(address, options);
+    // Filter out the transactions that are not "Payment" type because the filter on "tx_type" of the node RPC is not working as expected.
+    const paymentTxs = txs.filter(tx => tx.tx_json.TransactionType === "Payment");
+    const shortage = (options.limit && txs.length < options.limit) || false;
+    const lastTransaction = txs.slice(-1)[0];
+    const nextOptions = { ...options };
+    if (lastTransaction) {
+      nextOptions.ledger_index_max = lastTransaction.tx_json.ledger_index - 1;
+      if (nextOptions.limit) nextOptions.limit -= paymentTxs.length;
     }
-    transactions = transactions.concat(newPaymentsTxs);
-  } while (
-    options.limit &&
-    !needToStop &&
-    transactions.length < options.limit &&
-    (options.limit -= transactions.length)
-  );
-
-  return [
-    transactions.map(convertToCoreOperation(address)),
-    transactions.slice(-1)[0].tx_json.ledger_index - 1, // Returns the next index to start from for pagination
-  ];
+    return [shortage, nextOptions, paymentTxs];
+  }
+
+  // TODO BUG: given the number of txs belonging to the SAME block > limit
+  //           when user loop over pages using the provided token
+  //           then user misses some txs that doesn't fit the page size limit
+  //           because the "next token" is a block height (solution is to use an opaque token instead)
+  let [txShortage, nextOptions, transactions] = await getPaymentTransactions(address, options);
+  const isEnough = () => txShortage || (limit && transactions.length >= limit);
+  // We need to call the node RPC multiple times to get the desired number of transactions by the limiter.
+  while (nextOptions.limit && !isEnough()) {
+    const [newTxShortage, newNextOptions, newTransactions] = await getPaymentTransactions(
+      address,
+      nextOptions,
+    );
+    txShortage = newTxShortage;
+    nextOptions = newNextOptions;
+    transactions = transactions.concat(newTransactions);
+  }
+
+  const lastTransaction = transactions.slice(-1)[0];
+  // the next index to start the pagination from
+  const nextIndex = lastTransaction
+    ? Math.max(lastTransaction.tx_json.ledger_index - 1, minLedgerVersion)
+    : minLedgerVersion;
+
+  return [transactions.map(convertToCoreOperation(address)), nextIndex];
 }
 
 const convertToCoreOperation =

libs/coin-modules/coin-xrp/src/network/index.ts

@@ -72,10 +72,12 @@ export const getTransactions = async (
 ): Promise<XrplOperation[]> => {
   const result = await rpcCall<AccountTxResponse>("account_tx", {
     account: address,
+    // newest first
+    // note that order within the results is not guaranteed (see documentation of account_tx)
+    forward: false,
     ...options,
     api_version: 2,
   });
-
   return result.transactions;
 };
 
@@ -91,7 +93,7 @@ export async function getLedgerIndex(): Promise<number> {
 
 async function rpcCall<T extends object>(
   method: string,
-  params: Record<string, string | number>,
+  params: Record<string, string | number | boolean> = {},
 ): Promise<T> {
   const {
     data: { result },