Skip to content

feat: add notes pagination - #336

Open
Patrick-Ehimen wants to merge 8 commits into
NethermindEth:mainfrom
Patrick-Ehimen:feat/notes-pagination
Open

feat: add notes pagination#336
Patrick-Ehimen wants to merge 8 commits into
NethermindEth:mainfrom
Patrick-Ehimen:feat/notes-pagination

Conversation

@Patrick-Ehimen

@Patrick-Ehimen Patrick-Ehimen commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Closes #299

Summary

  • Adds OFFSET/LIMIT support to list_user_notes (SQL layer, 20 entries per page)
  • Adds count_user_notes() to support total page count calculation
  • Exposes getUserNotes(address, offset, limit) and getUserNotesCount(address) via WASM bindings
  • Adds Prev/Next pagination controls and page indicator to the Advanced tab notes inventory
  • Adds client-side pagination (10/page) to the Selective Disclosure note picker - all notes are kept in memory so selections persist across pages

Note

The disclosure page fetches up to 200 notes in a single call (pre-existing limit) and paginates client-side. This is a reasonable tradeoff since the note picker requires cross-page selection state (up to 4 notes).

@maksimryndin

Copy link
Copy Markdown
Contributor

hey @Patrick-Ehimen! ✋ Let's merge to the latest and fix the CI?

@v0-e

v0-e commented Jul 8, 2026

Copy link
Copy Markdown
Member

Thank you @Patrick-Ehimen 👍

On the Rust side looks good.
On the UI side, seems that if one has more 20+ available notes, the "Spent" notes do not appear (nor the Next/Prev buttons):
image
Seems that filtering is happening on that page only.

# Conflicts:
#	app/crates/platforms/web/src/client/mod.rs
#	app/js/disclosure.js
#	app/js/ui/notes-table.js
Comment thread sdk/state/src/storage.rs Outdated
/// `spent` filters to spent-only (`Some(true)`), unspent-only
/// (`Some(false)`), or all notes (`None`) — applied in SQL so `offset`/
/// `limit` page over the filtered set, matching [`Self::count_user_notes`].
pub fn list_user_notes(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

probably it makes sense to have a single method to return the notes page and the total count (because for example in some databases which can be used later) the both queries can be combined into one with window functions?

Here in sqlite we can use a wrapping transaction to ensure we look at the same db state (as between the 2 queries there is a gap when new notes arrive)?

Comment thread sdk/state/src/storage.rs

/// All notes for `address` in `pool_contract_id` (newest first), spent and
/// unspent.
pub fn list_pool_user_notes(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this can be removed with the changes introduced, right?

StorageWorkerRequest::UserNotes(address, limit) => {
log::trace!("[{WORKER_NAME}] list user notes for the account {address}");
let list = with_storage!(s => s.list_user_notes(&address, limit)?)?;
StorageWorkerRequest::UserNotes {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's combine a page and a total count?

Comment thread sdk/web/src/protocol.rs
UserKeys(Address),
AspSecret(Address),
UserNotes(Address, u32),
UserNotes {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's combine in a single request?

@maksimryndin

Copy link
Copy Markdown
Contributor

hey @Patrick-Ehimen ! ✋ thanks a lot! some comments to address

Addresses review from maksimryndin on NethermindEth#336: list_user_notes and
count_user_notes ran as two separate queries (and two separate worker
round trips over WASM), which could observe different DB snapshots if
a note landed between them. Merge them into list_user_notes_page,
wrapped in a single read transaction, and collapse the worker
protocol/JS facade down to one request per page fetch.
Reconcile the paginated user notes API with upstream's new Account SDK:
- Move `list_user_notes_page` (offset/limit/spent → UserNotesPage) onto the
  Storage trait and Account SDK so pagination flows through the wallet
  session, not the top-level client.
- Update the WASM `Account::userNotes` to take
  `{ offset?, limit?, spent? }` and return `{ notes, total }`.
- Point `disclosure.js` and `notes-table.js` at `client().account().userNotes(...)`.
maksimryndin
maksimryndin previously approved these changes Jul 23, 2026

@maksimryndin maksimryndin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! thanks a lot @Patrick-Ehimen !

asking for the review of the team :)

@Patrick-Ehimen

Copy link
Copy Markdown
Contributor Author

@maksimryndin Heads up on the failing push-bootnode / Build and publish Docker image check, it's not caused by anything in this PR.
The reusable workflow at .github/workflows/bootnode-push-jfrog-dev.yml calls jfrog/setup-jfrog-cli with OIDC auth, and that step fails on any PR from a fork with:

Failed to fetch OpenID Connect JSON Web Token: Unable to get ACTIONS_ID_TOKEN_REQUEST_URL env variable

GitHub doesn't expose id-token: write to reusable workflows triggered by fork PRs, so the JFrog OIDC login can't succeed regardless of the code changes. The same failure will hit any fork PR that touches tools/bootnode/**, sdk/types/**, or deployments/testnet/deployments.json.

If you'd like to silence it for fork PRs, guarding the job with something like:
jobs: push-bootnode: if: github.event.pull_request.head.repo.full_name == github.repository uses: NethermindEth/github-workflows/.github/workflows/docker-build-push-jfrog.yaml@...

would skip it on forks while still running on internal branches and on push to main. Happy to open a separate infra PR for that if it's useful, but wanted to flag it here since it's blocking the checks on this one.

@maksimryndin

Copy link
Copy Markdown
Contributor

@maksimryndin Heads up on the failing push-bootnode / Build and publish Docker image check, it's not caused by anything in this PR. The reusable workflow at .github/workflows/bootnode-push-jfrog-dev.yml calls jfrog/setup-jfrog-cli with OIDC auth, and that step fails on any PR from a fork with:

Failed to fetch OpenID Connect JSON Web Token: Unable to get ACTIONS_ID_TOKEN_REQUEST_URL env variable

GitHub doesn't expose id-token: write to reusable workflows triggered by fork PRs, so the JFrog OIDC login can't succeed regardless of the code changes. The same failure will hit any fork PR that touches tools/bootnode/**, sdk/types/**, or deployments/testnet/deployments.json.

If you'd like to silence it for fork PRs, guarding the job with something like: jobs: push-bootnode: if: github.event.pull_request.head.repo.full_name == github.repository uses: NethermindEth/github-workflows/.github/workflows/docker-build-push-jfrog.yaml@...

would skip it on forks while still running on internal branches and on push to main. Happy to open a separate infra PR for that if it's useful, but wanted to flag it here since it's blocking the checks on this one.

cc @v0-e

@v0-e

v0-e commented Jul 23, 2026

Copy link
Copy Markdown
Member

Thank you @Patrick-Ehimen, yeah if you can, we can have just push to main like https://github.com/NethermindEth/stellar-private-payments/blob/main/.github/workflows/deployment.yml 👍
Either way, it will not be a blocking job for this PR

On this PR, I didn't test with real notes, but I'm getting similar issues to #336 (comment) with 20 > notes in the two deployed pools.

@Fantoni0

Fantoni0 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Hey @Patrick-Ehimen ! Thanks so much for your contributions 🙌 The repo has advanced a bit since the last time you tried this, and you shouldn't be getting any errors now. Would you be willing to try again? Or should I tackle this one?

@Patrick-Ehimen

Copy link
Copy Markdown
Contributor Author

@maksimryndin i will look into it asap

@Patrick-Ehimen

Copy link
Copy Markdown
Contributor Author

@maksimryndin i will get on it ASAP

Resolve conflicts from upstream's tracing/privacy refactor:

- sdk/web/src/workers/storage.rs: keep the paginated UserNotes arm
  (offset/limit/spent -> list_user_notes_page), adopt upstream's
  tracing::trace! and wrap the account address in Sensitive(..) so it
  is redacted in logs. Union the type imports (Sensitive + UserNotesPage).
- sdk/web/src/client/account.rs: keep UserNotesOptions, drop
  DeriveAspUserLeafOptions (removed upstream; deriveAspUserLeaf now
  takes no arguments).
- sdk/web/js/types/index.d.ts: same, keep UserNotesOptions and drop the
  DeriveAspUserLeafOptions declaration.

@v0-e v0-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same issues in the UI when we have more than 20 notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add notes pagination

4 participants