-
Notifications
You must be signed in to change notification settings - Fork 316
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
feat(sns): Add periodic task for caching the upgrade steps #1788
base: master
Are you sure you want to change the base?
Conversation
c041226
to
2921040
Compare
service : (Governance) -> { | ||
add_maturity : (AddMaturityRequest) -> (AddMaturityResponse); | ||
trigger_refresh_cached_upgrade_steps : (TriggerRefreshCachedUpgradeStepsRequest) -> (TriggerRefreshCachedUpgradeStepsResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trigger_refresh_cached_upgrade_steps : (TriggerRefreshCachedUpgradeStepsRequest) -> (TriggerRefreshCachedUpgradeStepsResponse); | |
refresh_cached_upgrade_steps : (RefreshCachedUpgradeStepsRequest) -> (RefreshCachedUpgradeStepsResponse); |
rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
7adf905
to
0817379
Compare
/// Triggers a refresh of the cached upgrade steps for testing | ||
#[cfg(feature = "test")] | ||
#[export_name = "canister_update refresh_cached_upgrade_steps"] | ||
fn refresh_cached_upgrade_steps() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this could be called just refresh_upgrade_steps
, but leave it up to you to make the call.
@@ -719,8 +719,15 @@ type WaitForQuietState = record { | |||
current_deadline_timestamp_seconds : nat64; | |||
}; | |||
|
|||
type RefreshCachedUpgradeStepsRequest = record {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be useful if in testing we could use this function to specify what the SNS-W should give, which would simplify integration tests.
Currently, this just triggers an event that would otherwise be triggered by heartbeat, right?
rs/sns/governance/src/governance.rs
Outdated
@@ -4648,17 +4651,16 @@ impl Governance { | |||
let now = self.env.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should_refresh_cached_upgrade_steps
is no longer async
rs/sns/governance/src/governance.rs
Outdated
@@ -4667,7 +4669,7 @@ impl Governance { | |||
let (current_version, sns_governance_canister_id) = { | |||
let now = self.env.now(); | |||
let Some(current_version) = self.proto.deployed_version.clone() else { | |||
// TODO: add logs | |||
log!(INFO, "Deployed_version not set - should be impossible"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this really should be impossible, then let's make this err-level log
Would this be expected to happen in testing?
#[derive(Clone, PartialEq, candid::CandidType, candid::Deserialize)] | ||
pub(crate) struct ListUpgradeStepsRequest { | ||
#[derive(Clone, PartialEq, candid::CandidType, candid::Deserialize, Debug)] | ||
pub struct ListUpgradeStepsRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why we need this type in SNS Governance. Isn't this an endpoint of SNS-W?
rs/sns/governance/src/governance.rs
Outdated
let now = self.env.now(); | ||
|
||
if let Some(ref cached_upgrade_steps) = self.proto.cached_upgrade_steps { | ||
const UPDATE_INTERVAL: u64 = 5 * 60; // 5 minutes // TODO: move somewhere else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't this lifted into a constant in a previous commit?
1a0dd5a
to
1ae9f98
Compare
Co-authored-by: Arshavir Ter-Gabrielyan <arshavir.ter.gabrielyan@dfinity.org>
2314bdb
to
8db693c
Compare
rs/sns/governance/src/governance.rs
Outdated
@@ -4632,6 +4633,82 @@ impl Governance { | |||
self.maybe_move_staked_maturity(); | |||
|
|||
self.maybe_gc(); | |||
|
|||
if self.should_refresh_cached_upgrade_steps().await { | |||
// TODO: first, update deployed_version if an upgrade triggered by target_version is in progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make a ticket for this?
This PR implements part of periodic task type A, which fetches the remaining upgrade steps from SNS-W.
This PR does not implement the part of periodic task type A that refreshes deployed_version when an upgrade triggered by a change to target_version is in progress.
This PR also implements a SnsGov.get_upgrade_journal endpoint, which will be used in the future to get all the info needed to audit SNS Upgrades, but for now will just be used to see the cached_upgrade_steps
Remaining work: Add a pocketic test