sns-testing: SNS lifecycle #2
Merged
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
rvem
force-pushed
the
rvem/sns-testing-sns-cycle
branch
from
fe1bd22 to
65bea14
Compare
rvem
force-pushed
the
rvem/sns-testing-sns-cycle
branch
from
ae0ca5d to
fc71de3
Compare
rvem
force-pushed
the
rvem/sns-testing-sns-cycle
branch
3 times, most recently
from
704dbac to
4899f0c
Compare
rvem
commented
Jan 31, 2025
sras
reviewed
Jan 31, 2025
| }, | ||
| swap::{await_swap_lifecycle, smoke_test_participate_and_finalize}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Seems like nesting is a bit too much here in these imports.
sras
reviewed
Jan 31, 2025
| /// 5. `install_index_canister` - a flag indicating whether the Index canister for ICP Ledger should be installed. | ||
| /// 6. `install_cmc` - a flag indicating whether the Cycles minting canister should be installed. | ||
| /// 4. `custom_initial_registry_mutations` are custom mutations for the inital Registry. These | ||
| /// should mutations should comply with Registry invariants, otherwise this function will fail. |
There was a problem hiding this comment.
it seems that "should" is redundant
There was a problem hiding this comment.
FWIW, this typo was there before my changes, but I'll fix it
sras
reviewed
Jan 31, 2025
| custom_initial_registry_mutations: Option<Vec<RegistryAtomicMutateRequest>>, | ||
| neurons_fund_hotkeys: Vec<PrincipalId>, | ||
| install_index_canister: bool, | ||
| install_cmc: bool, | ||
| ) -> Vec<PrincipalId> { | ||
| assert!( | ||
| !(with_mainnet_nns_canister_versions && with_test_nns_governance_canister), | ||
| "Cannot mainnet canister version together wit NNS governance test canister version" |
There was a problem hiding this comment.
"Cannot mainnet canister version together wit NNS governance test canister version"
Did you mean "Cannot use mainnet" ?
wit
Typo?
sras
reviewed
Jan 31, 2025
| let (sns, proposal_id) = | ||
| propose_to_deploy_sns_and_wait(pocket_ic, create_service_nervous_system, sns_proposal_id) | ||
| .await; | ||
| await_swap_lifecycle(pocket_ic, sns.swap.canister_id, Lifecycle::Open) |
There was a problem hiding this comment.
The issue seem to require this
sns-testing scenario should close the swap using a sufficient amount of test participants with a sufficient amount of ICPs.
Do we know that it works like this in this await_swap_lifecycle? Or is it done somewhere else?
There was a problem hiding this comment.
Or is it done somewhere else?
The swap is handled in smoke_test_participate_and_finalize slightly below. This helper considers required parameters in order to correctly close the swap
rvem
force-pushed
the
serokell/sns-testing
branch
from
4214bb7 to
018ddc3
Compare
rvem
force-pushed
the
rvem/sns-testing-sns-cycle
branch
from
e263103 to
3763b44
Compare
sras
reviewed
Feb 5, 2025
Comment on lines
+16
to
+26
| match x { | ||
| None => (), | ||
| Some(x) => { | ||
| match x.greeting { | ||
| None => (), | ||
| Some(g) => { | ||
| STR.with(|s| *s.borrow_mut() = g); | ||
| } | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
May be using if let is better here..
1) Add a simple test canister that will be decentralized as a part of newly created SNS 2) Add helpers to install this canister and run it as the new SNS 3) Complete the SNS swap to finalize SNS adoption
Add helpers to upgrade the test canister. Currently, the helper will reuse the same WASM module, but it's possible to provide the new state value as a 'post_upgrade' hook argument. Test scenario as well as CLI now perform canister upgrade. The former also allows to see the SNS voting procedure in the NNS dapp web UI.
Problem: Currently, helpers from 'pocket_ic_helpers' module use 'advance_time' pocket-ic method to artificially advance time in order to wait for certain events to happen. However, using 'advance_time' in live mode breaks agent certificates checking since pocket-ic time becomes larger than the real time. Solution: 1) When pocket-ic is running in live mode, run 'std::thread::sleep()' instead of 'advance_time()'. 2) Avoid waiting two days for SNS swap to start in live mode. 3) Allow to use the test version of NNS Governance canister which is capable of starting SNS swap immediately after SNS proposal adoption.
Problem: We want to be able to run pocket-ic in live mode. However, this is not viable with non-test version of NNS Governance canister because it postpones the SNS swap start. Solution: Use test version of NNS Governance canister. Ensure that swap start_time is 'None' to start the swap immediately after SNS creation proposal adoption.
rvem
force-pushed
the
rvem/sns-testing-sns-cycle
branch
from
529a00b to
9d4a80e
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.
No description provided.