Skip to content

Commit

Permalink
Add optional includeEmptyNeurons to listNeurons (#667)
Browse files Browse the repository at this point in the history
# Motivation

When neurons are disbursed or merged they don't stop existing but become
empty neurons.
Those neurons are not displayed in NNS dapp but are always included in
the response and filtered out on the client.
This unnecessarily wasteful so an optional parameter is being added
which can be used to stop including empty neurons.

The functionality is not yet implemented but the parameter is added
already so that we can start passing it.

# Changes

1. Ran `scripts/import-candid ../../ic`
2. Ran `scripts/compile-idl-js`
3. Reverted all canisters except the governance canisters.
4. Added optional `includeEmptyNeurons` parameter to `listNeurons` and
pass it on to the backend. Note that the backend treats absent as
`true`.

# Tests

1. Unit tests added and updated.
2. Tested manually with NNS dapp. Functionality itself could not be
tested because it's not implemented yet but passing the parameter
doesn't break anything.

# Todos

- [x] Add entry to changelog (if necessary).

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
  • Loading branch information
dskloetd and github-actions[bot] authored Jun 26, 2024
1 parent 63ae83b commit a737a1d
Show file tree
Hide file tree
Showing 14 changed files with 86 additions and 39 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Features

- Add support for `wasm_memory_limit` in the canister settings.
- Add optional `includeEmptyNeurons` parameter to `listNeurons`.

## Fix

Expand Down
68 changes: 35 additions & 33 deletions packages/nns/README.md

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions packages/nns/candid/governance.certified.idl.js
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,7 @@ export const idlFactory = ({ IDL }) => {
});
const ListNeurons = IDL.Record({
'neuron_ids' : IDL.Vec(IDL.Nat64),
'include_empty_neurons_readable_by_caller' : IDL.Opt(IDL.Bool),
'include_neurons_readable_by_caller' : IDL.Bool,
});
const ListNeuronsResponse = IDL.Record({
Expand Down
1 change: 1 addition & 0 deletions packages/nns/candid/governance.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ export interface ListKnownNeuronsResponse {
}
export interface ListNeurons {
neuron_ids: BigUint64Array | bigint[];
include_empty_neurons_readable_by_caller: [] | [boolean];
include_neurons_readable_by_caller: boolean;
}
export interface ListNeuronsResponse {
Expand Down
3 changes: 2 additions & 1 deletion packages/nns/candid/governance.did
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Generated from IC repo commit e3fca54 (2024-06-19 tags: release-2024-06-19_23-01-base) 'rs/nns/governance/canister/governance.did' by import-candid
// Generated from IC repo commit f16b6a7376 (2024-06-24) 'rs/nns/governance/canister/governance.did' by import-candid
type AccountIdentifier = record { hash : blob };
type Action = variant {
RegisterKnownNeuron : KnownNeuron;
Expand Down Expand Up @@ -264,6 +264,7 @@ type LedgerParameters = record {
type ListKnownNeuronsResponse = record { known_neurons : vec KnownNeuron };
type ListNeurons = record {
neuron_ids : vec nat64;
include_empty_neurons_readable_by_caller : opt bool;
include_neurons_readable_by_caller : bool;
};
type ListNeuronsResponse = record {
Expand Down
1 change: 1 addition & 0 deletions packages/nns/candid/governance.idl.js
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,7 @@ export const idlFactory = ({ IDL }) => {
});
const ListNeurons = IDL.Record({
'neuron_ids' : IDL.Vec(IDL.Nat64),
'include_empty_neurons_readable_by_caller' : IDL.Opt(IDL.Bool),
'include_neurons_readable_by_caller' : IDL.Bool,
});
const ListNeuronsResponse = IDL.Record({
Expand Down
1 change: 1 addition & 0 deletions packages/nns/candid/governance_test.certified.idl.js
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,7 @@ export const idlFactory = ({ IDL }) => {
});
const ListNeurons = IDL.Record({
'neuron_ids' : IDL.Vec(IDL.Nat64),
'include_empty_neurons_readable_by_caller' : IDL.Opt(IDL.Bool),
'include_neurons_readable_by_caller' : IDL.Bool,
});
const ListNeuronsResponse = IDL.Record({
Expand Down
1 change: 1 addition & 0 deletions packages/nns/candid/governance_test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ export interface ListKnownNeuronsResponse {
}
export interface ListNeurons {
neuron_ids: BigUint64Array | bigint[];
include_empty_neurons_readable_by_caller: [] | [boolean];
include_neurons_readable_by_caller: boolean;
}
export interface ListNeuronsResponse {
Expand Down
3 changes: 2 additions & 1 deletion packages/nns/candid/governance_test.did
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Generated from IC repo commit e3fca54 (2024-06-19 tags: release-2024-06-19_23-01-base) 'rs/nns/governance/canister/governance_test.did' by import-candid
// Generated from IC repo commit f16b6a7376 (2024-06-24) 'rs/nns/governance/canister/governance_test.did' by import-candid
type AccountIdentifier = record { hash : blob };
type Action = variant {
RegisterKnownNeuron : KnownNeuron;
Expand Down Expand Up @@ -264,6 +264,7 @@ type LedgerParameters = record {
type ListKnownNeuronsResponse = record { known_neurons : vec KnownNeuron };
type ListNeurons = record {
neuron_ids : vec nat64;
include_empty_neurons_readable_by_caller : opt bool;
include_neurons_readable_by_caller : bool;
};
type ListNeuronsResponse = record {
Expand Down
1 change: 1 addition & 0 deletions packages/nns/candid/governance_test.idl.js
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,7 @@ export const idlFactory = ({ IDL }) => {
});
const ListNeurons = IDL.Record({
'neuron_ids' : IDL.Vec(IDL.Nat64),
'include_empty_neurons_readable_by_caller' : IDL.Opt(IDL.Bool),
'include_neurons_readable_by_caller' : IDL.Bool,
});
const ListNeuronsResponse = IDL.Record({
Expand Down
9 changes: 8 additions & 1 deletion packages/nns/src/canisters/governance/request.converters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -976,9 +976,16 @@ const fromClaimOrRefreshBy = (by: By): RawBy => {
}
};

export const fromListNeurons = (neuronIds?: NeuronId[]): RawListNeurons => ({
export const fromListNeurons = ({
neuronIds,
includeEmptyNeurons,
}: {
neuronIds?: NeuronId[];
includeEmptyNeurons?: boolean;
}): RawListNeurons => ({
neuron_ids: BigUint64Array.from(neuronIds ?? []),
include_neurons_readable_by_caller: neuronIds ? false : true,
include_empty_neurons_readable_by_caller: toNullable(includeEmptyNeurons),
});

export const fromManageNeuron = ({
Expand Down
28 changes: 27 additions & 1 deletion packages/nns/src/governance.canister.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,33 @@ describe("GovernanceCanister", () => {
const neurons = await governance.listNeurons({
certified: true,
});
expect(service.list_neurons).toBeCalled();
expect(service.list_neurons).toBeCalledWith({
neuron_ids: new BigUint64Array(),
include_neurons_readable_by_caller: true,
include_empty_neurons_readable_by_caller: [],
});
expect(service.list_neurons).toBeCalledTimes(1);
expect(neurons.length).toBe(1);
});

it("list user neurons excluding empty neurons", async () => {
const service = mock<ActorSubclass<GovernanceService>>();
service.list_neurons.mockResolvedValue(mockListNeuronsResponse);

const governance = GovernanceCanister.create({
certifiedServiceOverride: service,
serviceOverride: service,
});
const neurons = await governance.listNeurons({
certified: true,
includeEmptyNeurons: false,
});
expect(service.list_neurons).toBeCalledWith({
neuron_ids: new BigUint64Array(),
include_neurons_readable_by_caller: true,
include_empty_neurons_readable_by_caller: [false],
});
expect(service.list_neurons).toBeCalledTimes(1);
expect(neurons.length).toBe(1);
});
});
Expand Down
5 changes: 4 additions & 1 deletion packages/nns/src/governance.canister.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,18 @@ export class GovernanceCanister {
* If `certified` is true, the request is fetched as an update call, otherwise
* it is fetched using a query call.
*
* The backend treats `includeEmptyNeurons` as true if absent.
*/
public listNeurons = async ({
certified = true,
neuronIds,
includeEmptyNeurons,
}: {
certified: boolean;
neuronIds?: NeuronId[];
includeEmptyNeurons?: boolean;
}): Promise<NeuronInfo[]> => {
const rawRequest = fromListNeurons(neuronIds);
const rawRequest = fromListNeurons({ neuronIds, includeEmptyNeurons });
const raw_response =
await this.getGovernanceService(certified).list_neurons(rawRequest);
return toArrayOfNeuronInfo({
Expand Down
2 changes: 1 addition & 1 deletion packages/nns/src/governance_test.canister.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export class GovernanceTestCanister {
*/
async updateNeuron(neuron: Neuron) {
assertNonNullish(neuron.id);
const rawListNeuronsRequest = fromListNeurons([neuron.id]);
const rawListNeuronsRequest = fromListNeurons({ neuronIds: [neuron.id] });
const rawListNeuronsResponse = await this.certifiedService.list_neurons(
rawListNeuronsRequest,
);
Expand Down

0 comments on commit a737a1d

Please sign in to comment.