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

feat(nns): Add pagination to list_neurons API #3358

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

max-dfinity
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the feat label Jan 7, 2025
@max-dfinity max-dfinity force-pushed the msum/list-neurons-pagination branch 5 times, most recently from a654324 to 2d22844 Compare January 8, 2025 00:32
@max-dfinity max-dfinity marked this pull request as ready for review January 8, 2025 00:44
@max-dfinity max-dfinity requested review from a team as code owners January 8, 2025 00:44
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).

To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:

  1. Done.

  2. No canister behavior changes.

@max-dfinity max-dfinity force-pushed the msum/list-neurons-pagination branch from 6804e43 to 4fb4103 Compare January 8, 2025 00:48
Copy link
Member

@mbjorkqvist mbjorkqvist left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, @max-dfinity!

If a client requests multiple pages of list neurons, I suppose it is possible that the number of neurons matching the query may change in the meantime. Is this something that should be taken into account, or is the current "best effort" approach deemed good enough? I could imagine that if a new neuron is added during the requests, it would have a high ID and end up at the end of the list (in the last request)? I tried to look for more details of the approach, but neither this PR, nor the related JIRA ticket has any description, and the design linked in the epic also doesn't seem to cover this work.

Copy link
Member

@mbjorkqvist mbjorkqvist left a comment

Choose a reason for hiding this comment

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

Thanks a lot for adjusting the API to take the Rosetta cold storage wallet use case into account, @max-dfinity! From what I can tell this latest approach would solve the issue with potentially having to go back to cold storage for each subsequent page. I left some comments mainly related to documentation.

rs/nns/integration_tests/src/governance_neurons.rs Outdated Show resolved Hide resolved
rs/nns/governance/unreleased_changelog.md Outdated Show resolved Hide resolved
/// If this is set, we use the page limit provided to determine how large pages will be.
/// This cannot be greater than MAX_LIST_NEURONS_RESULTS, which is set to 500.
#[prost(uint64, optional, tag = "6")]
pub page_size: ::core::option::Option<u64>,
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, this is the only place in the code where there is documentation on how to use the list_neurons API? I could imagine that the common use case is to pass in None for both page_number and page_size in the initial call, and if the number of neurons returned in the response is smaller than total_neurons_found, call list_neurons again with page_number: Some(1) and page_size: None, and continue with increasing values of page_number until the total number of neurons received (sum of all neurons in all responses) matches total_neurons_found, but also only as long as total_neurons_found doesn't change (increase) between responses. What about if someone passes in a page_size that is larger than MAX_LIST_NEURONS_RESULTS, in the hope of being able to avoid pagination? IIUC, they would, from the number of neurons returned in the response, and the value of total_neurons_found, have to figure out that the page_size they specified is too large? All this to say that while this design change certainly helps for the cold storage wallet case (thanks for taking this into account!), there do seem to be some edge cases that could benefit from more documentation/examples.

The documentation in the portal should also be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbjorkqvist Do the doc changes and design changes address your concern?

Instead of total_neurons_found, it is now returning 'total_pages_available', which avoids specifying whether or not each neuron they put in the list actually exists. It avoids some edge cases there. I also explained how that works in the documentation. Let me know if you think it can be improved more, and also thank you for the thoughtful feedback!

@max-dfinity max-dfinity force-pushed the msum/list-neurons-pagination branch from 869baaa to f298f9b Compare January 17, 2025 21:37
@max-dfinity max-dfinity force-pushed the msum/list-neurons-pagination branch from 4341ad4 to dbba667 Compare January 17, 2025 22:57
@max-dfinity max-dfinity force-pushed the msum/list-neurons-pagination branch from f91cc4c to c899296 Compare January 22, 2025 00:12
@max-dfinity max-dfinity force-pushed the msum/list-neurons-pagination branch from 2cb212d to eac51a6 Compare January 22, 2025 02:00
rs/nns/governance/src/governance.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants