Skip to content

fix(sui): fix duplicate operations at page boundary in AlpacaApi.listOperations#15064

Open
jnicoulaud-ledger wants to merge 6 commits intodevelopfrom
fix-sui-pagination
Open

fix(sui): fix duplicate operations at page boundary in AlpacaApi.listOperations#15064
jnicoulaud-ledger wants to merge 6 commits intodevelopfrom
fix-sui-pagination

Conversation

@jnicoulaud-ledger
Copy link
Contributor

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • SUI, Alpaca API only

📝 Description

Pagination returns same operation in different pages.

❓ Context

  • JIRA or GitHub link:

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@vercel
Copy link

vercel bot commented Mar 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
react-ui-storybook Building Building Preview, Comment Mar 5, 2026 11:52am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
ledger-live-github-bot Ignored Ignored Preview Mar 5, 2026 11:52am
native-ui-storybook Ignored Ignored Preview Mar 5, 2026 11:52am

Request Review

@live-github-bot live-github-bot bot added coin-modules-api Has changes in the coin-framework/coin-modules APIs coin-modules labels Mar 5, 2026
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 5, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

jprudent
jprudent previously approved these changes Mar 5, 2026
@jnicoulaud-ledger jnicoulaud-ledger marked this pull request as ready for review March 5, 2026 11:16
@jnicoulaud-ledger jnicoulaud-ledger requested review from a team as code owners March 5, 2026 11:16
Copilot AI review requested due to automatic review settings March 5, 2026 11:16
@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2026

⚠️ E2E tests are required

Changes detected require e2e testing before merge (even before asking for any review).

🖥️ Desktop

-> Run Desktop E2E

  • Select "Run workflow"
  • Branch: fix-sui-pagination
  • Device: nanoSP or stax

📱 Mobile

-> Run Mobile E2E

  • Select "Run workflow"
  • Branch: fix-sui-pagination
  • Device: nanoX

Affected coins modules: sui

Copy link
Contributor

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

Fixes Sui AlpacaApi.listOperations pagination so the same operation isn’t returned on multiple pages (notably at page boundaries when merging IN/OUT streams).

Changes:

  • Reworks getListOperations pagination by merging IN/OUT transactions, de-duping by digest, sorting by timestamp, filtering by a new cursor boundary, and emitting a new cursor format.
  • Replaces the old dedupOperations approach with new cursor parsing/serialization and adds extensive unit tests for cursor/boundary scenarios.
  • Adds an integration test asserting that fully paginated asc results equal the reverse of fully paginated desc results.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
libs/coin-modules/coin-sui/src/network/sdk.ts Implements new merged pagination logic and introduces a timestamp+digest cursor format.
libs/coin-modules/coin-sui/src/network/sdk.test.ts Adds unit tests for cursor parsing and page-boundary/non-duplication behavior; removes old dedupOperations tests.
libs/coin-modules/coin-sui/src/api/index.integ.test.ts Adds a live integration test comparing full asc vs reversed desc pagination results.
.changeset/few-olives-agree.md Declares a release bump for @ledgerhq/coin-sui for this pagination change.

Comment on lines +785 to +789
if (op.digest === parsedCursor.digest) return false;
if (parsedCursor.timestamp === undefined) return true;

const ts = Number(op.timestampMs ?? 0);
return order === "asc" ? ts > parsedCursor.timestamp : ts < parsedCursor.timestamp;
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The boundary filter drops all operations with timestampMs === parsedCursor.timestamp (except it also drops the boundary digest explicitly). If multiple distinct operations share the boundary timestamp, they will never be returned on subsequent pages. To avoid missing ops, filter using a total ordering consistent with the sort and cursor (e.g., for asc: keep (ts > cursor.ts) OR (ts === cursor.ts AND digest > cursor.digest); similarly for desc).

Suggested change
if (op.digest === parsedCursor.digest) return false;
if (parsedCursor.timestamp === undefined) return true;
const ts = Number(op.timestampMs ?? 0);
return order === "asc" ? ts > parsedCursor.timestamp : ts < parsedCursor.timestamp;
// never return the boundary operation itself
if (op.digest === parsedCursor.digest) return false;
const boundaryTs = parsedCursor.timestamp;
const boundaryDigest = parsedCursor.digest;
// if we don't have full boundary information, fall back to previous behavior
if (boundaryTs === undefined || !boundaryDigest) return true;
const ts = Number(op.timestampMs ?? 0);
if (order === "asc") {
// keep ops strictly after the cursor in (timestamp, digest) ordering
return ts > boundaryTs || (ts === boundaryTs && op.digest > boundaryDigest);
}
// order === "desc": keep ops strictly after the cursor in reverse (timestamp, digest) ordering
return ts < boundaryTs || (ts === boundaryTs && op.digest < boundaryDigest);

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment on lines +804 to +808
if (op.digest === parsedCursor.digest) return false;
if (parsedCursor.timestamp === undefined) return true;

const ts = Number(op.timestampMs ?? 0);
return order === "asc" ? ts > parsedCursor.timestamp : ts < parsedCursor.timestamp;
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The cursor filtering only compares timestamps (ts > parsedCursor.timestamp / <) and removes the exact digest match. If multiple transactions share the same timestampMs (common for txs in the same checkpoint), this will drop all ops at that timestamp on subsequent pages (except possibly the first page), causing missing operations. The filtering should be consistent with the full sort order (timestamp + digest tie-breaker), e.g. when timestamps are equal include only digests that come after/before the cursor digest depending on order.

Suggested change
if (op.digest === parsedCursor.digest) return false;
if (parsedCursor.timestamp === undefined) return true;
const ts = Number(op.timestampMs ?? 0);
return order === "asc" ? ts > parsedCursor.timestamp : ts < parsedCursor.timestamp;
// never include the operation exactly at the cursor
if (op.digest === parsedCursor.digest) return false;
const cursorTs = parsedCursor.timestamp;
if (cursorTs === undefined) return true;
const ts = Number(op.timestampMs ?? 0);
if (ts === cursorTs && parsedCursor.digest) {
// when timestamps are equal, use digest as tie-breaker to match sort order
return order === "asc"
? op.digest > parsedCursor.digest
: op.digest < parsedCursor.digest;
}
return order === "asc" ? ts > cursorTs : ts < cursorTs;

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 5, 2026 11:44
Copy link
Contributor

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

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

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 5, 2026 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coin-modules coin-modules-api Has changes in the coin-framework/coin-modules APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants