Skip to content

Commit

Permalink
refactor(nns): Eliminate ExecutedCreateServiceNervousSystemProposal (…
Browse files Browse the repository at this point in the history 
…#1366)

This PR refactors the code in NNS Governance responsible for executing
`CreateServiceNervousSystem` proposals s.t. it no longer requires the
internal type `ExecutedCreateServiceNervousSystemProposal`. This type
was intended to be internal but has become public for CLI tools to
validate instances of this proposal type, which adds an undesirable
crate-level dependency between, e.g., `//rs/nns/governance` and
`//rs/sns/init`.
  • Loading branch information
@aterga
aterga authored Sep 5, 2024
1 parent e1f0db7 commit 6d16531
Show file tree
Hide file tree
Showing 5 changed files with 181 additions and 170 deletions.
134 changes: 86 additions & 48 deletions rs/nns/governance/src/governance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,7 @@ use crate::{
StopOrStartCanister, Tally, Topic, UpdateCanisterSettings, UpdateNodeProvider, Visibility,
Vote, WaitForQuietState, XdrConversionRate as XdrConversionRatePb,
},
proposals::{
call_canister::CallCanister,
create_service_nervous_system::ExecutedCreateServiceNervousSystemProposal,
},
proposals::call_canister::CallCanister,
};
use async_trait::async_trait;
use candid::{Decode, Encode};
Expand Down Expand Up @@ -4663,6 +4660,17 @@ impl Governance {
}
}

async fn create_service_nervous_system(
&mut self,
proposal_id: u64,
create_service_nervous_system: &CreateServiceNervousSystem,
) {
let result = self
.do_create_service_nervous_system(proposal_id, create_service_nervous_system)
.await;
self.set_proposal_execution_status(proposal_id, result);
}

async fn do_create_service_nervous_system(
&mut self,
proposal_id: u64,
Expand Down Expand Up @@ -4696,61 +4704,97 @@ impl Governance {
(NeuronsFundSnapshot::empty(), None)
};

let executed_create_service_nervous_system_proposal =
ExecutedCreateServiceNervousSystemProposal {
current_timestamp_seconds,
create_service_nervous_system: create_service_nervous_system.clone(),
proposal_id: proposal_id.id,
random_swap_start_time: self.randomly_pick_swap_start(),
neurons_fund_participation_constraints,
};
let random_swap_start_time = self.randomly_pick_swap_start();
let create_service_nervous_system = create_service_nervous_system.clone();

self.execute_create_service_nervous_system_proposal(
executed_create_service_nervous_system_proposal,
create_service_nervous_system,
neurons_fund_participation_constraints,
current_timestamp_seconds,
proposal_id,
random_swap_start_time,
initial_neurons_fund_participation_snapshot,
)
.await
}

async fn create_service_nervous_system(
&mut self,
proposal_id: u64,
create_service_nervous_system: &CreateServiceNervousSystem,
) {
let result = self
.do_create_service_nervous_system(proposal_id, create_service_nervous_system)
.await;
self.set_proposal_execution_status(proposal_id, result);
// This function is public as it is used in various tests, also outside this crate.
fn make_sns_init_payload(
create_service_nervous_system: CreateServiceNervousSystem,
neurons_fund_participation_constraints: Option<NeuronsFundParticipationConstraints>,
current_timestamp_seconds: u64,
proposal_id: ProposalId,
random_swap_start_time: GlobalTimeOfDay,
) -> Result<SnsInitPayload, String> {
let (swap_start_timestamp_seconds, swap_due_timestamp_seconds) = {
let start_time = create_service_nervous_system
.swap_parameters
.as_ref()
.and_then(|swap_parameters| swap_parameters.start_time);

let duration = create_service_nervous_system
.swap_parameters
.as_ref()
.and_then(|swap_parameters| swap_parameters.duration);

CreateServiceNervousSystem::swap_start_and_due_timestamps(
start_time.unwrap_or(random_swap_start_time),
duration.unwrap_or_default(),
current_timestamp_seconds,
)
}?;

let sns_init_payload = SnsInitPayload::try_from(create_service_nervous_system)?;

Ok(SnsInitPayload {
neurons_fund_participation_constraints,
nns_proposal_id: Some(proposal_id.id),
swap_start_timestamp_seconds: Some(swap_start_timestamp_seconds),
swap_due_timestamp_seconds: Some(swap_due_timestamp_seconds),
..sns_init_payload
})
}

async fn execute_create_service_nervous_system_proposal(
&mut self,
executed_create_service_nervous_system_proposal: ExecutedCreateServiceNervousSystemProposal,
create_service_nervous_system: CreateServiceNervousSystem,
neurons_fund_participation_constraints: Option<NeuronsFundParticipationConstraints>,
current_timestamp_seconds: u64,
proposal_id: ProposalId,
random_swap_start_time: GlobalTimeOfDay,
initial_neurons_fund_participation_snapshot: NeuronsFundSnapshot,
) -> Result<(), GovernanceError> {
let is_start_time_unspecified = executed_create_service_nervous_system_proposal
.create_service_nervous_system
let is_start_time_unspecified = create_service_nervous_system
.swap_parameters
.as_ref()
.map(|swap_parameters| swap_parameters.start_time.is_none())
.unwrap_or(false);

// Step 1: Convert proposal into main request object.
let sns_init_payload =
match SnsInitPayload::try_from(executed_create_service_nervous_system_proposal.clone())
{
Ok(ok) => ok,
Err(err) => {
return Err(GovernanceError::new_with_message(
ErrorType::InvalidProposal,
format!(
"Failed to convert ExecutedCreateServiceNervousSystemProposal \
to SnsInitPayload: {}",
err,
),
))
}
};
// Step 1.1: Convert proposal into SnsInitPayload.
let sns_init_payload = Self::make_sns_init_payload(
create_service_nervous_system,
neurons_fund_participation_constraints,
current_timestamp_seconds,
proposal_id,
random_swap_start_time,
)
.map_err(|err| {
GovernanceError::new_with_message(
ErrorType::InvalidProposal,
format!(
"Failed to convert CreateServiceNervousSystem proposal to SnsInitPayload: {}",
err,
),
)
})?;

// Step 1.2: Validate the SnsInitPayload.
sns_init_payload.validate_post_execution().map_err(|err| {
GovernanceError::new_with_message(
ErrorType::InvalidProposal,
format!("Failed to validate SnsInitPayload: {}", err),
)
})?;

// If the test configuration is active (implying we are not running in
// production), and the start time has not been specified,
Expand All @@ -4770,9 +4814,7 @@ impl Governance {
println!(
"{}The swap's start time for proposal {:?} is unspecified, \
so a random time of {:?} will be used.",
LOG_PREFIX,
executed_create_service_nervous_system_proposal.proposal_id,
executed_create_service_nervous_system_proposal.random_swap_start_time
LOG_PREFIX, proposal_id, random_swap_start_time,
);
}

Expand All @@ -4784,10 +4826,6 @@ impl Governance {

// Step 3: React to response from deploy_new_sns (Ok or Err).

let proposal_id = ProposalId {
id: executed_create_service_nervous_system_proposal.proposal_id,
};

// Step 3.1: If the call was not successful, issue refunds (and then, return).
if let Err(ref mut err) = &mut deploy_new_sns_response {
let refund_result = self.refund_maturity_to_neurons_fund(
Expand Down
52 changes: 39 additions & 13 deletions rs/nns/governance/src/governance/tests/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ mod convert_from_create_service_nervous_system_to_sns_init_payload_tests {
}

#[cfg(feature = "test")]
mod convert_from_executed_create_service_nervous_system_proposal_to_sns_init_payload_tests_with_test_feature {
mod convert_create_service_nervous_system_proposal_to_sns_init_payload_tests_with_test_feature {
use super::*;
use ic_nervous_system_proto::pb::v1 as pb;
use ic_sns_init::pb::v1::sns_init_payload;
Expand Down Expand Up @@ -547,23 +547,49 @@ mod convert_from_executed_create_service_nervous_system_proposal_to_sns_init_pay
let current_timestamp_seconds = 13_245;
let proposal_id = 1000;

let executed_create_service_nervous_system_proposal =
ExecutedCreateServiceNervousSystemProposal {
current_timestamp_seconds,
create_service_nervous_system: CREATE_SERVICE_NERVOUS_SYSTEM_WITH_MATCHED_FUNDING
.clone(),
proposal_id,
random_swap_start_time: GlobalTimeOfDay {
// Step 2: Call the code under test.
let converted = {
let create_service_nervous_system =
CREATE_SERVICE_NERVOUS_SYSTEM_WITH_MATCHED_FUNDING.clone();
// The computation for swap_start_timestamp_seconds and swap_due_timestamp_seconds below
// is inlined from `Governance::make_sns_init_payload`.
let (swap_start_timestamp_seconds, swap_due_timestamp_seconds) = {
let random_swap_start_time = GlobalTimeOfDay {
seconds_after_utc_midnight: Some(0),
},
};

let start_time = create_service_nervous_system
.swap_parameters
.as_ref()
.and_then(|swap_parameters| swap_parameters.start_time);

let duration = create_service_nervous_system
.swap_parameters
.as_ref()
.and_then(|swap_parameters| swap_parameters.duration);

CreateServiceNervousSystem::swap_start_and_due_timestamps(
start_time.unwrap_or(random_swap_start_time),
duration.unwrap_or_default(),
current_timestamp_seconds,
)
.expect("Cannot compute swap_start_timestamp_seconds, swap_due_timestamp_seconds.")
};

let sns_init_payload = SnsInitPayload::try_from(create_service_nervous_system).unwrap();

SnsInitPayload {
neurons_fund_participation_constraints: Some(
NEURONS_FUND_PARTICIPATION_CONSTRAINTS.clone(),
),
};
nns_proposal_id: Some(proposal_id),
swap_start_timestamp_seconds: Some(swap_start_timestamp_seconds),
swap_due_timestamp_seconds: Some(swap_due_timestamp_seconds),
..sns_init_payload
}
};

// Step 2: Call the code under test.
let converted =
SnsInitPayload::try_from(executed_create_service_nervous_system_proposal).unwrap();
converted.validate_post_execution().unwrap();

// Step 3: Inspect the result.

Expand Down
62 changes: 1 addition & 61 deletions rs/nns/governance/src/proposals/create_service_nervous_system.rs
View file
Original file line number Diff line number Diff line change
@@ -1,66 +1,6 @@
use crate::pb::v1::{create_service_nervous_system, CreateServiceNervousSystem};
use ic_nervous_system_proto::pb::v1::{Duration, GlobalTimeOfDay};
use ic_sns_init::pb::v1::{self as sns_init_pb, sns_init_payload, SnsInitPayload};
use ic_sns_swap::pb::v1::NeuronsFundParticipationConstraints;

#[derive(Clone, Debug)]
pub struct ExecutedCreateServiceNervousSystemProposal {
pub current_timestamp_seconds: u64,
pub create_service_nervous_system: CreateServiceNervousSystem,
pub proposal_id: u64,
pub random_swap_start_time: GlobalTimeOfDay,
/// Information about the Neurons' Fund participation needed by the Swap canister.
pub neurons_fund_participation_constraints: Option<NeuronsFundParticipationConstraints>,
}

impl TryFrom<ExecutedCreateServiceNervousSystemProposal> for SnsInitPayload {
type Error = String;

fn try_from(src: ExecutedCreateServiceNervousSystemProposal) -> Result<Self, Self::Error> {
let mut defects = vec![];

let current_timestamp_seconds = src.current_timestamp_seconds;
let nns_proposal_id = Some(src.proposal_id);
let neurons_fund_participation_constraints = src.neurons_fund_participation_constraints;
let start_time = src
.create_service_nervous_system
.swap_parameters
.as_ref()
.and_then(|swap_parameters| swap_parameters.start_time);
let duration = src
.create_service_nervous_system
.swap_parameters
.as_ref()
.and_then(|swap_parameters| swap_parameters.duration);

let (swap_start_timestamp_seconds, swap_due_timestamp_seconds) =
match CreateServiceNervousSystem::swap_start_and_due_timestamps(
start_time.unwrap_or(src.random_swap_start_time),
duration.unwrap_or_default(),
current_timestamp_seconds,
) {
Ok((swap_start_timestamp_seconds, swap_due_timestamp_seconds)) => (
Some(swap_start_timestamp_seconds),
Some(swap_due_timestamp_seconds),
),
Err(err) => {
defects.push(err);
(None, None)
}
};

let mut result = SnsInitPayload::try_from(src.create_service_nervous_system)?;

result.nns_proposal_id = nns_proposal_id;
result.swap_start_timestamp_seconds = swap_start_timestamp_seconds;
result.swap_due_timestamp_seconds = swap_due_timestamp_seconds;
result.neurons_fund_participation_constraints = neurons_fund_participation_constraints;

result.validate_post_execution()?;

Ok(result)
}
}

impl CreateServiceNervousSystem {
pub fn sns_token_e8s(&self) -> Option<u64> {
Expand Down Expand Up @@ -366,7 +306,7 @@ impl TryFrom<CreateServiceNervousSystem> for SnsInitPayload {
neurons_fund_participation,

// These are not known from only the CreateServiceNervousSystem
// proposal. See TryFrom<ExecutedCreateServiceNervousSystemProposal>
// proposal. See `Governance::make_sns_init_payload`.
nns_proposal_id: None,
swap_start_timestamp_seconds: None,
swap_due_timestamp_seconds: None,
Expand Down
Loading

0 comments on commit 6d16531

Please sign in to comment.