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

Fix updateNeuron not to change the neuron subaccount #663

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

dskloetd
Copy link
Collaborator

Motivation

Currently when you use updateNeuron on the GovernanceTestCanister for testing, there is no way not to change the neuron subaccount in the process while probably you would never want to change the neuron subaccount.
This is because when calling update_neuron the full neuron, including account (which is the neuron subaccount) must be specified but nns-js never returns the neuron subaccount. It only returns the accountIdentifier which is derived from the subaccount in a one-way fashion.

The current implementation of using the accountIdentifier as the neuron account is wrong and results in the neuron account changing each time updateNeuron is used.

So in order not to change the subaccount when calling update_neuron (and without changing the API of nns-js) we must first fetch the neuron to know its subaccount.

Changes

  1. Change toRawNeuron (which is only used in updateNeuron) to take an account parameter to use for the raw neuron.
  2. Change updateNeuron to first fetch the existing neuron in order to use its account to pass to toRawNeuron.
  3. Verify that the account of the fetched neuron matches the accountIdentifier of the passed neuron to make sure people don't try to change the accountIdentifier while it's not supported.

Tests

  1. Unit tests added
  2. Tested manually by adding maturity in NNS dapp and seeing that the neuron account doesn't change.

Todos

  • Add entry to changelog (if necessary).

@dskloetd dskloetd requested a review from a team as a code owner June 19, 2024 09:54
@dskloetd dskloetd enabled auto-merge (squash) June 19, 2024 09:54
Copy link
Contributor

size-limit report 📦

Path Size
@dfinity/ckbtc 7.91 KB (0%)
@dfinity/cketh 3.49 KB (0%)
@dfinity/cmc 1.29 KB (0%)
@dfinity/ledger-icrc 3.88 KB (0%)
@dfinity/ledger-icp 15.22 KB (0%)
@dfinity/nns 34.76 KB (+0.41% 🔺)
@dfinity/nns-proto 140.98 KB (0%)
@dfinity/sns 15.71 KB (0%)
@dfinity/utils 4.47 KB (0%)
@dfinity/ic-management 2.78 KB (0%)

Copy link
Contributor

@mstrasinskis mstrasinskis left a comment

Choose a reason for hiding this comment

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

Thanks!

@dskloetd dskloetd merged commit beb85b5 into main Jun 19, 2024
11 checks passed
@dskloetd dskloetd deleted the kloet/update-neuron branch June 19, 2024 13:28
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.

None yet

2 participants