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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions rs/nervous_system/integration_tests/src/pocket_ic_helpers.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use candid::{Decode, Encode, Nat, Principal};
use canister_test::Wasm;
use futures::stream;
use futures::StreamExt;
use futures::{stream, StreamExt};
use ic_base_types::{CanisterId, PrincipalId, SubnetId};
use ic_ledger_core::Tokens;
use ic_nervous_system_agent::pocketic_impl::{PocketIcAgent, PocketIcCallError};
use ic_nervous_system_agent::sns::Sns;
use ic_nervous_system_agent::CallCanisters;
use ic_nervous_system_agent::{
pocketic_impl::{PocketIcAgent, PocketIcCallError},
sns::Sns,
CallCanisters,
};
use ic_nervous_system_common::{E8, ONE_DAY_SECONDS};
use ic_nervous_system_common_test_keys::{TEST_NEURON_1_ID, TEST_NEURON_1_OWNER_PRINCIPAL};
use ic_nns_common::pb::v1::{NeuronId, ProposalId};
Expand Down Expand Up @@ -60,17 +61,15 @@ use icrc_ledger_types::icrc1::{
account::Account,
transfer::{TransferArg, TransferError},
};
use itertools::EitherOrBoth;
use itertools::Itertools;
use itertools::{EitherOrBoth, Itertools};
use maplit::btreemap;
use pocket_ic::{
management_canister::CanisterSettings, nonblocking::PocketIc, ErrorCode, PocketIcBuilder,
RejectResponse,
};
use prost::Message;
use rust_decimal::prelude::ToPrimitive;
use std::ops::Range;
use std::{collections::BTreeMap, fmt::Write, time::Duration};
use std::{collections::BTreeMap, fmt::Write, ops::Range, time::Duration};

pub const STARTING_CYCLES_PER_CANISTER: u128 = 2_000_000_000_000_000;

Expand Down Expand Up @@ -833,6 +832,8 @@ pub mod nns {
include_neurons_readable_by_caller: true,
include_empty_neurons_readable_by_caller: None,
include_public_neurons_in_full_neurons: None,
page_number: None,
page_size: None
})
.unwrap(),
)
Expand Down Expand Up @@ -1366,8 +1367,9 @@ pub mod sns {
use assert_matches::assert_matches;
use ic_crypto_sha2::Sha256;
use ic_nervous_system_agent::sns::governance::{GovernanceCanister, SubmitProposalError};
use ic_sns_governance::governance::UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS;
use ic_sns_governance::pb::v1::get_neuron_response;
use ic_sns_governance::{
governance::UPGRADE_STEPS_INTERVAL_REFRESH_BACKOFF_SECONDS, pb::v1::get_neuron_response,
};
use pocket_ic::ErrorCode;
use sns_pb::UpgradeSnsControlledCanister;

Expand Down
27 changes: 27 additions & 0 deletions rs/nns/governance/api/src/ic_nns_governance.pb.v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3320,6 +3320,20 @@ pub struct ListProposalInfoResponse {
/// of neurons listed in `neuron_ids` and, if `caller_neurons` is true,
/// the list of neuron IDs of neurons for which the caller is the
/// controller or one of the hot keys.
///
/// Paging is available if the result set is larger than `MAX_LIST_NEURONS_RESULTS`,
/// which is currently 500 neurons. If you are unsure of the number of results in a set,
/// you can use the `total_pages_available` field in the response to determine how many
/// additional pages need to be queried. It will be based on your `page_size` parameter.
/// When paging through results, it is good to keep in mind that newly inserted neurons
/// could be missed if they are inserted between calls to pages, and this could result in missing
/// a neuron in the combined responses.
///
/// If a user provides neuron_ids that do not exist in the request, there is no guarantee that
/// each page will contain the exactly the page size, even if it is not the final request. This is
/// because neurons are retrieved by their neuron_id, and no additional checks are made on the
/// validity of the neuron_ids provided by the user before deciding which sets of neuron_ids
/// will be returned in the current page.
#[derive(candid::CandidType, candid::Deserialize, serde::Serialize, comparable::Comparable)]
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
Expand Down Expand Up @@ -3350,6 +3364,15 @@ pub struct ListNeurons {
/// existing (unmigrated) callers.
#[prost(bool, optional, tag = "4")]
pub include_public_neurons_in_full_neurons: Option<bool>,
/// If this is set, we return the batch of neurons at a given page, using the `page_size` to
/// determine how many neurons are returned in each page.
#[prost(uint64, optional, tag = "5")]
pub page_number: Option<u64>,
/// 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.
max-dfinity marked this conversation as resolved.
Show resolved Hide resolved
/// If not set, this defaults to MAX_LIST_NEURONS_RESULTS.
#[prost(uint64, optional, tag = "6")]
pub page_size: Option<u64>,
}
/// A response to a `ListNeurons` request.
///
Expand All @@ -3368,6 +3391,10 @@ pub struct ListNeuronsResponse {
/// `ManageNeuron` topic).
#[prost(message, repeated, tag = "2")]
pub full_neurons: Vec<Neuron>,
/// This is returned to tell the caller how many pages of results are available to query.
/// If there are fewer than the page_size neurons, this will equal 1.
#[prost(uint64, optional, tag = "3")]
pub total_pages_available: Option<u64>,
}
/// A response to "ListKnownNeurons"
#[derive(candid::CandidType, candid::Deserialize, serde::Serialize, comparable::Comparable)]
Expand Down
34 changes: 17 additions & 17 deletions rs/nns/governance/canbench/canbench_results.yml
Original file line number Diff line number Diff line change
@@ -1,55 +1,55 @@
benches:
add_neuron_active_maximum:
total:
instructions: 42752805
instructions: 42749561
heap_increase: 1
stable_memory_increase: 0
scopes: {}
add_neuron_active_typical:
total:
instructions: 2170667
instructions: 2170531
heap_increase: 0
stable_memory_increase: 0
scopes: {}
add_neuron_inactive_maximum:
total:
instructions: 112624643
instructions: 112621399
heap_increase: 1
stable_memory_increase: 0
scopes: {}
add_neuron_inactive_typical:
total:
instructions: 8497304
instructions: 8497168
heap_increase: 0
stable_memory_increase: 0
scopes: {}
cascading_vote_all_heap:
total:
instructions: 35002536
instructions: 34977262
heap_increase: 0
stable_memory_increase: 128
scopes: {}
cascading_vote_heap_neurons_stable_index:
total:
instructions: 61137575
instructions: 61112301
heap_increase: 0
stable_memory_increase: 128
scopes: {}
cascading_vote_stable_everything:
total:
instructions: 188621982
instructions: 188615092
heap_increase: 0
stable_memory_increase: 128
scopes: {}
cascading_vote_stable_neurons_with_heap_index:
total:
instructions: 162353547
instructions: 162346657
heap_increase: 0
stable_memory_increase: 128
scopes: {}
centralized_following_all_stable:
total:
instructions: 78268135
instructions: 78266673
heap_increase: 0
stable_memory_increase: 128
scopes: {}
Expand All @@ -61,7 +61,7 @@ benches:
scopes: {}
draw_maturity_from_neurons_fund_heap:
total:
instructions: 7598030
instructions: 7584430
heap_increase: 0
stable_memory_increase: 0
scopes: {}
Expand All @@ -85,7 +85,7 @@ benches:
scopes: {}
list_neurons_heap:
total:
instructions: 4763239
instructions: 4802543
heap_increase: 9
stable_memory_increase: 0
scopes: {}
Expand All @@ -103,13 +103,13 @@ benches:
scopes: {}
list_neurons_stable:
total:
instructions: 113374022
instructions: 113411672
heap_increase: 5
stable_memory_increase: 0
scopes: {}
list_proposals:
total:
instructions: 168095717
instructions: 168095740
heap_increase: 0
stable_memory_increase: 0
scopes: {}
Expand All @@ -127,13 +127,13 @@ benches:
scopes: {}
neuron_data_validation_heap:
total:
instructions: 406864991
instructions: 406848584
heap_increase: 0
stable_memory_increase: 0
scopes: {}
neuron_data_validation_stable:
total:
instructions: 362661286
instructions: 362648972
heap_increase: 0
stable_memory_increase: 0
scopes: {}
Expand All @@ -151,13 +151,13 @@ benches:
scopes: {}
range_neurons_performance:
total:
instructions: 56448740
instructions: 56447340
heap_increase: 0
stable_memory_increase: 0
scopes: {}
single_vote_all_stable:
total:
instructions: 2805838
instructions: 2803168
heap_increase: 0
stable_memory_increase: 128
scopes: {}
Expand Down
5 changes: 4 additions & 1 deletion rs/nns/governance/canister/canister.rs
Original file line number Diff line number Diff line change
Expand Up @@ -888,7 +888,10 @@ fn get_latest_reward_event() -> RewardEvent {
#[query]
fn get_neuron_ids() -> Vec<NeuronId> {
debug_log("get_neuron_ids");
let votable = governance().get_neuron_ids_by_principal(&caller());
let votable = governance()
.get_neuron_ids_by_principal(&caller())
.into_iter()
.collect();

governance()
.get_managed_neuron_ids_for(votable)
Expand Down
3 changes: 3 additions & 0 deletions rs/nns/governance/canister/governance.did
Original file line number Diff line number Diff line change
Expand Up @@ -439,11 +439,14 @@ type ListNeurons = record {
neuron_ids : vec nat64;
include_empty_neurons_readable_by_caller : opt bool;
include_neurons_readable_by_caller : bool;
page_number: opt nat64;
page_size: opt nat64;
};

type ListNeuronsResponse = record {
neuron_infos : vec record { nat64; NeuronInfo };
full_neurons : vec Neuron;
total_pages_available: opt nat64;
};

type ListNodeProviderRewardsRequest = record {
Expand Down
3 changes: 3 additions & 0 deletions rs/nns/governance/canister/governance_test.did
Original file line number Diff line number Diff line change
Expand Up @@ -438,11 +438,14 @@ type ListNeurons = record {
neuron_ids : vec nat64;
include_empty_neurons_readable_by_caller : opt bool;
include_neurons_readable_by_caller : bool;
page_number: opt nat64;
page_size: opt nat64;
};

type ListNeuronsResponse = record {
neuron_infos : vec record { nat64; NeuronInfo };
full_neurons : vec Neuron;
total_pages_available: opt nat64;
};

type ListNodeProviderRewardsRequest = record {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2635,6 +2635,20 @@ message ListProposalInfoResponse {
// of neurons listed in `neuron_ids` and, if `caller_neurons` is true,
// the list of neuron IDs of neurons for which the caller is the
// controller or one of the hot keys.
//
// Paging is available if the result set is larger than `MAX_LIST_NEURONS_RESULTS`,
// which is currently 500 neurons. If you are unsure of the number of results in a set,
// you can use the `total_pages_available` field in the response to determine how many
// additional pages need to be queried. It will be based on your `page_size` parameter.
// When paging through results, it is good to keep in mind that newly inserted neurons
// could be missed if they are inserted between calls to pages, and this could result in missing
// a neuron in the combined responses.
//
// If a user provides neuron_ids that do not exist in the request, there is no guarantee that
// each page will contain the exactly the page size, even if it is not the final request. This is
// because neurons are retrieved by their neuron_id, and no additional checks are made on the
// validity of the neuron_ids provided by the user before deciding which sets of neuron_ids
// will be returned in the current page.
message ListNeurons {
option (ic_base_types.pb.v1.tui_signed_message) = true;
// The neurons to get information about. The "requested list"
Expand All @@ -2659,6 +2673,13 @@ message ListNeurons {
// since this feature was added later, it is opt in to avoid confusing
// existing (unmigrated) callers.
optional bool include_public_neurons_in_full_neurons = 4;
// If this is set, we return the batch of neurons at a given page, using the `page_size` to
// determine how many neurons are returned in each page.
optional uint64 page_number = 5;
// 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.
// If not set, this defaults to MAX_LIST_NEURONS_RESULTS.
optional uint64 page_size = 6;
}

// A response to a `ListNeurons` request.
Expand All @@ -2673,6 +2694,9 @@ message ListNeuronsResponse {
// hot key, or controller or hot key of some followee on the
// `ManageNeuron` topic).
repeated Neuron full_neurons = 2;
// This is returned to tell the caller how many pages of results are available to query.
// If there are fewer than the page_size neurons, this will equal 1.
optional fixed64 total_pages_available = 3;
}

// A response to "ListKnownNeurons"
Expand Down
27 changes: 27 additions & 0 deletions rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3932,6 +3932,20 @@ pub struct ListProposalInfoResponse {
/// of neurons listed in `neuron_ids` and, if `caller_neurons` is true,
/// the list of neuron IDs of neurons for which the caller is the
/// controller or one of the hot keys.
///
/// Paging is available if the result set is larger than `MAX_LIST_NEURONS_RESULTS`,
/// which is currently 500 neurons. If you are unsure of the number of results in a set,
/// you can use the `total_pages_available` field in the response to determine how many
/// additional pages need to be queried. It will be based on your `page_size` parameter.
/// When paging through results, it is good to keep in mind that newly inserted neurons
/// could be missed if they are inserted between calls to pages, and this could result in missing
/// a neuron in the combined responses.
///
/// If a user provides neuron_ids that do not exist in the request, there is no guarantee that
/// each page will contain the exactly the page size, even if it is not the final request. This is
/// because neurons are retrieved by their neuron_id, and no additional checks are made on the
/// validity of the neuron_ids provided by the user before deciding which sets of neuron_ids
/// will be returned in the current page.
#[derive(
candid::CandidType,
candid::Deserialize,
Expand Down Expand Up @@ -3968,6 +3982,15 @@ pub struct ListNeurons {
/// existing (unmigrated) callers.
#[prost(bool, optional, tag = "4")]
pub include_public_neurons_in_full_neurons: ::core::option::Option<bool>,
/// If this is set, we return the batch of neurons at a given page, using the `page_size` to
/// determine how many neurons are returned in each page.
#[prost(uint64, optional, tag = "5")]
pub page_number: ::core::option::Option<u64>,
/// 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.
/// If not set, this defaults to MAX_LIST_NEURONS_RESULTS.
#[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!

}
/// A response to a `ListNeurons` request.
///
Expand All @@ -3992,6 +4015,10 @@ pub struct ListNeuronsResponse {
/// `ManageNeuron` topic).
#[prost(message, repeated, tag = "2")]
pub full_neurons: ::prost::alloc::vec::Vec<Neuron>,
/// This is returned to tell the caller how many pages of results are available to query.
/// If there are fewer than the page_size neurons, this will equal 1.
#[prost(fixed64, optional, tag = "3")]
pub total_pages_available: ::core::option::Option<u64>,
}
/// A response to "ListKnownNeurons"
#[derive(
Expand Down
Loading
Loading