feat(sns): Add periodic task for caching the upgrade steps #1788
+676
−36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anchpop
force-pushed
the
@anchpop/NNS1-3376
branch
4 times, most recently
from
c041226
to
2921040
Compare
aterga
reviewed
Oct 2, 2024
| service : (Governance) -> { | ||
| add_maturity : (AddMaturityRequest) -> (AddMaturityResponse); | ||
| trigger_refresh_cached_upgrade_steps : (TriggerRefreshCachedUpgradeStepsRequest) -> (TriggerRefreshCachedUpgradeStepsResponse); |
There was a problem hiding this comment.
Suggested change
| trigger_refresh_cached_upgrade_steps : (TriggerRefreshCachedUpgradeStepsRequest) -> (TriggerRefreshCachedUpgradeStepsResponse); | |
| refresh_cached_upgrade_steps : (RefreshCachedUpgradeStepsRequest) -> (RefreshCachedUpgradeStepsResponse); |
aterga
reviewed
Oct 2, 2024
rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
aterga
reviewed
Oct 2, 2024
rs/sns/governance/proto/ic_sns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
Co-authored-by: Arshavir Ter-Gabrielyan <[email protected]>
Co-authored-by: Arshavir Ter-Gabrielyan <[email protected]>
anchpop
force-pushed
the
@anchpop/NNS1-3376
branch
from
7adf905
to
0817379
Compare
aterga
reviewed
Oct 3, 2024
| /// 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.
I still think this could be called just refresh_upgrade_steps, but leave it up to you to make the call.
aterga
reviewed
Oct 3, 2024
| @@ -719,8 +719,15 @@ type WaitForQuietState = record { | |||
| current_deadline_timestamp_seconds : nat64; | |||
| }; | |||
|
|
|||
| type RefreshCachedUpgradeStepsRequest = record {}; | |||
There was a problem hiding this comment.
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?
aterga
reviewed
Oct 3, 2024
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.
should_refresh_cached_upgrade_steps is no longer async
aterga
reviewed
Oct 3, 2024
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.
If this really should be impossible, then let's make this err-level log
Would this be expected to happen in testing?
aterga
reviewed
Oct 3, 2024
| #[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.
I don't get why we need this type in SNS Governance. Isn't this an endpoint of SNS-W?
aterga
reviewed
Oct 3, 2024
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.
Wasn't this lifted into a constant in a previous commit?
anchpop
force-pushed
the
@anchpop/NNS1-3376
branch
from
94a13e5
to
1a0dd5a
Compare
anchpop
force-pushed
the
@anchpop/NNS1-3376
branch
from
1a0dd5a
to
1ae9f98
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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