refactor(nns): consolidate deterministic subaccount validation in NeuronStore#9186
Conversation
Add NeuronStore::validate_deterministic_subaccount() which checks that a caller-supplied subaccount is not already in use, replacing the repeated has_neuron_with_subaccount + GovernanceError pattern in split_neuron, spawn_neuron, and disburse_to_neuron. has_neuron_with_subaccount is now private to NeuronStore since all external callers use either new_neuron_subaccount (random) or validate_deterministic_subaccount (deterministic).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
- Rename validate_deterministic_subaccount to ensure_subaccount_available - Remove unnecessary shadowing in disburse_to_neuron - Add unit tests for ensure_subaccount_available success and collision paths
There was a problem hiding this comment.
Pull request overview
This PR consolidates deterministic (caller-supplied) neuron subaccount collision validation into NeuronStore, removing duplicated “compute → check collision → precondition-failed” logic across multiple Governance operations.
Changes:
- Introduces
NeuronStore::ensure_subaccount_available()and a dedicatedNeuronStoreError::SubaccountAlreadyExists. - Replaces inline deterministic subaccount collision checks in
split_neuron,spawn_neuron, anddisburse_to_neuronwith the new helper. - Makes
has_neuron_with_subaccountprivate and adds unit tests for the new helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rs/nns/governance/src/neuron_store.rs | Adds the consolidated deterministic subaccount availability check, error variant/display, and error mapping to GovernanceError. |
| rs/nns/governance/src/governance.rs | Switches deterministic subaccount paths in split/spawn/disburse-to-neuron to use ensure_subaccount_available. |
| rs/nns/governance/src/neuron_store/neuron_store_tests.rs | Adds unit tests covering success and collision error paths for ensure_subaccount_available. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…-generation' into jason/refactor-deterministic-subaccount-validation # Conflicts: # rs/nns/governance/src/neuron_store/neuron_store_tests.rs
jasonz-dfinity
dismissed
github-actions[bot]’s stale review
- No canister behavior changes -- pure internal refactor consolidating an existing collision check.
- Existing APIs behave as before.
- No data migration needed.
- No security-sensitive changes -- refactor preserves the existing collision check logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NeuronStoreError::SubaccountAlreadyExists { subaccount } => { | ||
| write!( | ||
| f, | ||
| "There is already a neuron with subaccount {subaccount:?}." |
Add underscore separators to integer literal suffixes (5 occurrences) to satisfy clippy::unseparated_literal_suffix, and switch the SubaccountAlreadyExists Display impl to use Subaccount's hex Display instead of Debug for a more readable user-facing error.
Why
The "compute deterministic subaccount → check collision → return PreconditionFailed" pattern was duplicated across
split_neuron,spawn_neuron, anddisburse_to_neuronwith identical error handling.What
NeuronStore::ensure_subaccount_available()which checks that a caller-supplied subaccount is not already in usehas_neuron_with_subaccountprivate since all external callers now use eithernew_neuron_subaccount(random) orensure_subaccount_available(deterministic)PR Chain
Testing
ensure_subaccount_available: success path and collision error path