-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: main
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
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:
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. |
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? |
I think option 1 is fine for now. As long as we are aware that the issue is not solved for hardware wallets. |
Because of the large page size, I doubt that exposing pagination will provide much benefit to users. Option 1 sounds reasonable to me. |
@lmuntaner answering your questions
The response signature will change if we want to include pagination information.
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. |
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?
I agree. I meant that the current clients of |
The current |
Ah, I see. And we can't add the page of those results now. Got it. Thanks! I forgot. |
5877968
to
316a936
Compare
316a936
to
c8378b8
Compare
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:
Changes
./scripts/import-candid ../ic
from ic-js root folder./scripts/compile-idl-js
from ic-js root folderTests
Todos