Skip to content

Commit 6dc952e

Browse files
jasonz-dfinityjasonz-dfinity
committed
Refactor AddOrRemoveNodeProvider
1 parent d89af62 commit 6dc952e

File tree

5 files changed

+533
-285
lines changed

5 files changed

+533
-285
lines changed

‎rs/nns/governance/src/governance.rs‎

Lines changed: 5 additions & 152 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,7 @@ use crate::{
4444
RewardNodeProvider, RewardNodeProviders, SettleNeuronsFundParticipationRequest,
4545
SettleNeuronsFundParticipationResponse, StopOrStartCanister, Tally, Topic,
4646
UpdateCanisterSettings, UpdateNodeProvider, Vote, VotingPowerEconomics,
47-
WaitForQuietState,
48-
add_or_remove_node_provider::Change,
49-
archived_monthly_node_provider_rewards,
47+
WaitForQuietState, archived_monthly_node_provider_rewards,
5048
create_service_nervous_system::LedgerParameters,
5149
get_neurons_fund_audit_info_response,
5250
governance::{
@@ -168,7 +166,6 @@ pub mod tla_macros;
168166
#[cfg(feature = "tla")]
169167
pub mod tla;
170168

171-
use crate::pb::v1::AddOrRemoveNodeProvider;
172169
use crate::reward::distribution::RewardsDistribution;
173170
use crate::storage::with_voting_state_machines_mut;
174171
#[cfg(feature = "tla")]
@@ -4128,80 +4125,9 @@ impl Governance {
41284125
let result = self.approve_genesis_kyc(&proposal.principals);
41294126
self.set_proposal_execution_status(pid, result);
41304127
}
4131-
ValidProposalAction::AddOrRemoveNodeProvider(ref proposal) => {
4132-
if let Some(change) = &proposal.change {
4133-
match change {
4134-
Change::ToAdd(node_provider) => {
4135-
if node_provider.id.is_none() {
4136-
self.set_proposal_execution_status(
4137-
pid,
4138-
Err(GovernanceError::new_with_message(
4139-
ErrorType::PreconditionFailed,
4140-
"Node providers must have a principal id.",
4141-
)),
4142-
);
4143-
return;
4144-
}
4145-
4146-
// Check if the node provider already exists
4147-
if self
4148-
.heap_data
4149-
.node_providers
4150-
.iter()
4151-
.any(|np| np.id == node_provider.id)
4152-
{
4153-
self.set_proposal_execution_status(
4154-
pid,
4155-
Err(GovernanceError::new_with_message(
4156-
ErrorType::PreconditionFailed,
4157-
"A node provider with the same principal already exists.",
4158-
)),
4159-
);
4160-
return;
4161-
}
4162-
self.heap_data.node_providers.push(node_provider.clone());
4163-
self.set_proposal_execution_status(pid, Ok(()));
4164-
}
4165-
Change::ToRemove(node_provider) => {
4166-
if node_provider.id.is_none() {
4167-
self.set_proposal_execution_status(
4168-
pid,
4169-
Err(GovernanceError::new_with_message(
4170-
ErrorType::PreconditionFailed,
4171-
"Node providers must have a principal id.",
4172-
)),
4173-
);
4174-
return;
4175-
}
4176-
4177-
if let Some(pos) = self
4178-
.heap_data
4179-
.node_providers
4180-
.iter()
4181-
.position(|np| np.id == node_provider.id)
4182-
{
4183-
self.heap_data.node_providers.remove(pos);
4184-
self.set_proposal_execution_status(pid, Ok(()));
4185-
} else {
4186-
self.set_proposal_execution_status(
4187-
pid,
4188-
Err(GovernanceError::new_with_message(
4189-
ErrorType::NotFound,
4190-
"Can't find a NodeProvider with the same principal id.",
4191-
)),
4192-
);
4193-
}
4194-
}
4195-
}
4196-
} else {
4197-
self.set_proposal_execution_status(
4198-
pid,
4199-
Err(GovernanceError::new_with_message(
4200-
ErrorType::PreconditionFailed,
4201-
"The proposal didn't contain a change.",
4202-
)),
4203-
);
4204-
}
4128+
ValidProposalAction::AddOrRemoveNodeProvider(proposal) => {
4129+
let result = proposal.execute(&mut self.heap_data.node_providers);
4130+
self.set_proposal_execution_status(pid, result);
42054131
}
42064132
ValidProposalAction::RewardNodeProvider(ref reward) => {
42074133
self.reward_node_provider(pid, reward).await;
@@ -4805,7 +4731,7 @@ impl Governance {
48054731
}
48064732

48074733
ValidProposalAction::AddOrRemoveNodeProvider(add_or_remove_node_provider) => {
4808-
self.validate_add_or_remove_node_provider(add_or_remove_node_provider)
4734+
add_or_remove_node_provider.validate(&self.heap_data.node_providers)
48094735
}
48104736
ValidProposalAction::ApproveGenesisKyc(_)
48114737
| ValidProposalAction::RewardNodeProvider(_)
@@ -4916,79 +4842,6 @@ impl Governance {
49164842
Ok(())
49174843
}
49184844

4919-
fn validate_add_or_remove_node_provider(
4920-
&self,
4921-
add_or_remove_node_provider: &AddOrRemoveNodeProvider,
4922-
) -> Result<(), GovernanceError> {
4923-
match &add_or_remove_node_provider.change {
4924-
None => Err(GovernanceError::new_with_message(
4925-
ErrorType::InvalidProposal,
4926-
"AddOrRemoveNodeProvider proposal must have a change field",
4927-
)),
4928-
Some(Change::ToAdd(node_provider)) => {
4929-
let Some(np_id) = node_provider.id else {
4930-
return Err(GovernanceError::new_with_message(
4931-
ErrorType::InvalidProposal,
4932-
"AddOrRemoveNodeProvider proposal must have a node provider id",
4933-
));
4934-
};
4935-
// Validate that np does not exist
4936-
if self
4937-
.heap_data
4938-
.node_providers
4939-
.iter()
4940-
.any(|np| np.id.as_ref() == Some(&np_id))
4941-
{
4942-
return Err(GovernanceError::new_with_message(
4943-
ErrorType::InvalidProposal,
4944-
format!(
4945-
"AddOrRemoveNodeProvider cannot add already existing Node Provider: {np_id}"
4946-
),
4947-
));
4948-
}
4949-
4950-
if let Some(ref account_identifier) = node_provider.reward_account {
4951-
validate_account_identifier(account_identifier).map_err(|e| {
4952-
GovernanceError::new_with_message(
4953-
ErrorType::InvalidProposal,
4954-
format!("The account_identifier field is invalid: {e}"),
4955-
)
4956-
})?;
4957-
}
4958-
4959-
// Validate that np does not exist
4960-
// validate the account_identifier
4961-
Ok(())
4962-
}
4963-
Some(Change::ToRemove(node_provider)) => {
4964-
let Some(np_id) = node_provider.id else {
4965-
return Err(GovernanceError::new_with_message(
4966-
ErrorType::InvalidProposal,
4967-
"AddOrRemoveNodeProvider proposal must have a node provider id",
4968-
));
4969-
};
4970-
4971-
// Validate that np exists
4972-
if !self
4973-
.heap_data
4974-
.node_providers
4975-
.iter()
4976-
.any(|np| np.id.as_ref() == Some(&np_id))
4977-
{
4978-
return Err(GovernanceError::new_with_message(
4979-
ErrorType::InvalidProposal,
4980-
format!(
4981-
"AddOrRemoveNodeProvider ToRemove must target an existing Node Provider \
4982-
but targeted {np_id}"
4983-
),
4984-
));
4985-
}
4986-
4987-
Ok(())
4988-
}
4989-
}
4990-
}
4991-
49924845
fn validate_add_or_remove_data_centers_payload(payload: &[u8]) -> Result<(), String> {
49934846
let decoded_payload = match Decode!([decoder_config()]; payload, AddOrRemoveDataCentersProposalPayload)
49944847
{

‎rs/nns/governance/src/governance/tests/mod.rs‎

Lines changed: 0 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -1647,130 +1647,6 @@ fn test_compute_ballots_for_standard_proposal() {
16471647
);
16481648
}
16491649

1650-
#[test]
1651-
fn test_validate_add_or_remove_node_provider() {
1652-
let node_provider_id = PrincipalId::new_user_test_id(1);
1653-
let existing_node_provider = api::NodeProvider {
1654-
id: Some(node_provider_id),
1655-
reward_account: None,
1656-
};
1657-
1658-
let governance = Governance::new(
1659-
api::Governance {
1660-
node_providers: vec![existing_node_provider.clone()],
1661-
..Default::default()
1662-
},
1663-
Arc::new(MockEnvironment::new(vec![], 100)),
1664-
Arc::new(StubIcpLedger {}),
1665-
Arc::new(StubCMC {}),
1666-
Box::new(MockRandomness::new()),
1667-
);
1668-
1669-
let existing_node_provider = NodeProvider::from(existing_node_provider);
1670-
1671-
// Test case 1: No change field
1672-
let add_or_remove_no_change = AddOrRemoveNodeProvider { change: None };
1673-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_no_change);
1674-
assert!(result.is_err());
1675-
1676-
// Test case 2: ToAdd with new node provider (should succeed)
1677-
let new_node_provider_id = PrincipalId::new_user_test_id(2);
1678-
let valid_account = AccountIdentifier::new(new_node_provider_id, None);
1679-
1680-
let new_node_provider = NodeProvider {
1681-
id: Some(new_node_provider_id),
1682-
reward_account: Some(valid_account.into_proto_with_checksum()),
1683-
};
1684-
let add_or_remove_add_new = AddOrRemoveNodeProvider {
1685-
change: Some(Change::ToAdd(new_node_provider)),
1686-
};
1687-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_add_new);
1688-
assert!(
1689-
result.is_ok(),
1690-
"Expected to succeed, but got error: {result:?}"
1691-
);
1692-
1693-
// Test case 3: ToAdd with existing node provider (should fail)
1694-
let add_or_remove_add_existing = AddOrRemoveNodeProvider {
1695-
change: Some(Change::ToAdd(existing_node_provider.clone())),
1696-
};
1697-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_add_existing);
1698-
assert!(result.is_err());
1699-
1700-
// Test case 4: ToAdd with invalid account identifier (should fail)
1701-
let node_provider_with_invalid_account = NodeProvider {
1702-
id: Some(PrincipalId::new_user_test_id(3)),
1703-
reward_account: Some(icp_ledger::protobuf::AccountIdentifier {
1704-
hash: vec![1, 2, 3], // Invalid length
1705-
}),
1706-
};
1707-
let add_or_remove_invalid_account = AddOrRemoveNodeProvider {
1708-
change: Some(Change::ToAdd(node_provider_with_invalid_account)),
1709-
};
1710-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_invalid_account);
1711-
assert!(result.is_err());
1712-
1713-
// Test case 5: ToAdd with 28-byte length (should fail)
1714-
let node_provider_with_invalid_account = NodeProvider {
1715-
id: Some(PrincipalId::new_user_test_id(3)),
1716-
reward_account: Some(icp_ledger::protobuf::AccountIdentifier {
1717-
hash: vec![1; 28], // 32-byte required, but only 28 bytes provided
1718-
}),
1719-
};
1720-
let add_or_remove_invalid_account = AddOrRemoveNodeProvider {
1721-
change: Some(Change::ToAdd(node_provider_with_invalid_account)),
1722-
};
1723-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_invalid_account);
1724-
assert!(result.is_err());
1725-
1726-
// Test case 6: ToRemove with existing node provider (should succeed)
1727-
let add_or_remove_remove_existing = AddOrRemoveNodeProvider {
1728-
change: Some(Change::ToRemove(existing_node_provider)),
1729-
};
1730-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_remove_existing);
1731-
assert!(result.is_ok());
1732-
1733-
// Test case 7: ToRemove with non-existing node provider (should fail)
1734-
let non_existing_node_provider = NodeProvider {
1735-
id: Some(PrincipalId::new_user_test_id(999)),
1736-
reward_account: None,
1737-
};
1738-
let add_or_remove_remove_non_existing = AddOrRemoveNodeProvider {
1739-
change: Some(Change::ToRemove(non_existing_node_provider)),
1740-
};
1741-
let result =
1742-
governance.validate_add_or_remove_node_provider(&add_or_remove_remove_non_existing);
1743-
assert!(result.is_err());
1744-
1745-
// Test Case 8: ToAdd with no NodeProvider ID (should fail)
1746-
let node_provider_without_id = NodeProvider {
1747-
id: None,
1748-
reward_account: Some(valid_account.into_proto_with_checksum()),
1749-
};
1750-
let add_or_remove_no_id = AddOrRemoveNodeProvider {
1751-
change: Some(Change::ToAdd(node_provider_without_id)),
1752-
};
1753-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_no_id);
1754-
assert!(
1755-
result.is_err(),
1756-
"Expected to fail, but got success: {result:?}"
1757-
);
1758-
1759-
// Test Case 9: ToRemove with no NodeProvider ID (should fail)
1760-
let node_provider_without_id = NodeProvider {
1761-
id: None,
1762-
reward_account: None,
1763-
};
1764-
let add_or_remove_no_id = AddOrRemoveNodeProvider {
1765-
change: Some(Change::ToRemove(node_provider_without_id)),
1766-
};
1767-
let result = governance.validate_add_or_remove_node_provider(&add_or_remove_no_id);
1768-
assert!(
1769-
result.is_err(),
1770-
"Expected to fail, but got success: {result:?}"
1771-
);
1772-
}
1773-
17741650
#[test]
17751651
fn test_record_known_neuron_abstentions() {
17761652
record_known_neuron_abstentions(

0 commit comments

Comments
 (0)