Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT pagination for nns governance list_neurons #833

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yhabib
Copy link
Contributor

@yhabib yhabib commented Jan 30, 2025

Motivation

As present here and changed here, the NNS governance canister will implement pagination for the list_neurons endpoint.

Pagination will only activate if the response contains more than 500 neurons. Currently, most of our cases involve requesting non-empty neurons, and we do not have users with such a high number of neurons, so for those users nothing has changed. However, the nns-dapp has a use case, Reporting, where we also request empty neurons. In this case, we have a few hundred users with more than 500 neurons.

Some [insights]((https://dfinity.slack.com/archives/C01R76CSJ4R/p1738328033654549?thread_ts=1738243719.968339&cid=C01R76CSJ4R) from the Boundary Node.

Expect usage patterns:

  • Non-empty neurons:
    • All users with fewer than 500 neurons → 1 page
  • Non-empty and empty neurons, such as Reporting feature:
    • Most users with fewer than 500 neurons → 1 page
    • Some users with approximately 5,000 neurons → 10 pages
    • Few users with approximately 40,000 neurons → 80 pages

Changes

  • Ran ./scripts/import-candid ../ic from ic-js root folder
  • Ran ./scripts/compile-idl-js from ic-js root folder
  • Reverted all canisters except the governance canisters.
  • Recursive logic

Tests

  • todo

Todos

  • Add entry to changelog (if necessary).

Copy link
Contributor

github-actions bot commented Jan 30, 2025

size-limit report 📦

Path Size
@dfinity/ckbtc 8.12 KB (0%)
@dfinity/cketh 3.68 KB (0%)
@dfinity/cmc 1.41 KB (0%)
@dfinity/ledger-icrc 4.29 KB (0%)
@dfinity/ledger-icp 15.41 KB (0%)
@dfinity/nns 37.48 KB (+0.65% 🔺)
@dfinity/nns-proto 140.98 KB (0%)
@dfinity/sns 17.19 KB (0%)
@dfinity/utils 4.71 KB (0%)
@dfinity/zod-schemas 626 B (0%)
@dfinity/ic-management 3.22 KB (0%)

@yhabib
Copy link
Contributor Author

yhabib commented Jan 30, 2025

Hi @dskloetd @mstrasinskis @peterpeterparker @lmuntaner

I'm pinging you all as you've contributed to the code related to the area of this change, and I would like your opinion on how to proceed.

As mentioned in the description, the governance canister will introduce pagination to the list_neurons endpoint. I see several ways to move forward:

  1. Update the existing endpoint to make recursive calls, abstracting the pagination from the clients.
  2. Update the existing endpoint and expose the pagination logic to the client, allowing them to manage it. This would be a breaking change, as it would significantly alter the response signature.
  3. Create a new function that abstracts the recursion from the client.
  4. Create a new function that exposes the recursion to the client.

I prefer option 1, but I would like to know your thoughts.

Note: the PR is a basic draft. I wanted to get a feeling of how it would look such a feature in ic-js.

@lmuntaner
Copy link
Contributor

Why would option 3 be a breaking change? Aren't the new fields optional?

I can see a client wanted to take care of the pagination, but probably most won't want to deal with it.

IMHO, I would prefer a function with no pagination and another function with pagination. It makes sense that the current function offers no pagination. The current behavior was always to return all the neurons, so there is no change there, right?

@dskloetd
Copy link
Collaborator

I think option 1 is fine for now. As long as we are aware that the issue is not solved for hardware wallets.

@mstrasinskis
Copy link
Contributor

Because of the large page size, I doubt that exposing pagination will provide much benefit to users. Option 1 sounds reasonable to me.

@yhabib
Copy link
Contributor Author

yhabib commented Jan 31, 2025

@lmuntaner answering your questions

Why would option 3 be a breaking change? Aren't the new fields optional?

The response signature will change if we want to include pagination information.

IMHO, I would prefer a function with no pagination and another function with pagination. It makes sense that the current function offers no pagination. The current behavior was always to return all the neurons, so there is no change there, right?

The current function without pagination will work until the first user exceeds 500 neurons. At that point, the result will be incorrect, as it will only return the first 500 neurons, leaving the consumer unaware of the limitation. Therefore, everyone should eventually migrate to the new function.

@lmuntaner
Copy link
Contributor

The response signature will change if we want to include pagination information.

That will be managed by agent-js, right? the client of ic-js doesn't know about this. Would it break something for the ic-js client?

leaving the consumer unaware of the limitation

I agree. I meant that the current clients of list_neurons expect to receive all neurons after such a call. If you make recursive calls because now the backend requires pagination is consistent with the behavior of the method as of today.

@dskloetd
Copy link
Collaborator

The current listNeurons function does not return the response from the agent directly. It returns NeuronInfo[]:
https://github.com/dfinity/ic-js/pull/833/files#diff-f4f0835b56118edf98f5b690eb524de8bd8738c8d742c69c3c2cd516d55e9857R157

@lmuntaner
Copy link
Contributor

The current listNeurons function does not return the response from the agent directly. It returns NeuronInfo[]: https://github.com/dfinity/ic-js/pull/833/files#diff-f4f0835b56118edf98f5b690eb524de8bd8738c8d742c69c3c2cd516d55e9857R157

Ah, I see. And we can't add the page of those results now. Got it. Thanks! I forgot.

@yhabib yhabib force-pushed the yhabib/list_neurons/pagination branch from 5877968 to 316a936 Compare February 4, 2025 08:30
@yhabib yhabib force-pushed the yhabib/list_neurons/pagination branch from 316a936 to c8378b8 Compare February 4, 2025 08:30
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.

4 participants