Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Allow two Parachains to swap #4772

Merged
merged 26 commits into from
Feb 12, 2022
Merged

Allow two Parachains to swap #4772

merged 26 commits into from
Feb 12, 2022

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Jan 23, 2022 •
edited
Loading

This PR polishes the swap function available in the paras_registrar.

Specifically, we add the ability for two parachains to swap their slots, in addition to allowing a parachain and a parathread to swap.

To better support this functionality, this PR changes the way that crowdloan accounts are generated. Rather than use the ParaId which would be clunky to swap, we use the fund_index which is unique per crowdloan, and would require no underlying swap.

A migration was included to adopt this change in the runtime.

Additional tests were added to verify this functionality, and it will been enabled in Kusama and Polkadot.

paritytech/polkadot-sdk#877

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jan 23, 2022
@shawntabrizi
Copy link
Member Author

Looks like the best thing to do here is to migrate some of the crowdloan code to not depend on the ParaId as a unique identifier, but actually the fund_index, which is globally unique, and can be transferred to different parachains without being changed.

@shawntabrizi shawntabrizi added B7-runtimenoteworthy D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. C1-low PR touches the given topic and has a low impact on builders. labels Feb 1, 2022
@shawntabrizi shawntabrizi mentioned this pull request Feb 1, 2022
Merged
shawntabrizi
shawntabrizi commented Feb 1, 2022
View reviewed changes
runtime/common/src/paras_registrar.rs
@@ -271,31 +274,52 @@ pub mod pallet {
pub fn swap(origin: OriginFor<T>, id: ParaId, other: ParaId) -> DispatchResult {
Self::ensure_root_para_or_owner(origin, id)?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will basically need to be called by Root unless we write specific logic around the owner of the same parachain id being able to do a swap even when the parachain is "locked".

shawntabrizi
shawntabrizi commented Feb 1, 2022
View reviewed changes
runtime/common/src/crowdloan/mod.rs
trie_index: TrieIndex,
pub last_period: LeasePeriod,
/// Unique index used to represent this fund.
pub fund_index: FundIndex,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should not require a migration since the encoding is exactly the same as before.

shawntabrizi
shawntabrizi commented Feb 1, 2022
View reviewed changes
runtime/common/src/crowdloan/mod.rs
#[pallet::getter(fn next_trie_index)]
pub(super) type NextTrieIndex<T> = StorageValue<_, u32, ValueQuery>;
#[pallet::getter(fn next_fund_index)]
pub(super) type NextFundIndex<T> = StorageValue<_, u32, ValueQuery>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be covered by the migration I have written.

@emostov
Copy link
Contributor

emostov commented Feb 8, 2022 •
edited
Loading

Sans the irregularities with the try-runtime logging we discussed offline, this looks good to me

EDIT: logging irregularities seem to be a non-issue, seems to have just been a human error.

This was referenced Feb 8, 2022
Merged
Closed
@wischli
Copy link

wischli commented Feb 9, 2022

Thanks for this alteration, LGTM.

This was referenced Feb 9, 2022
Closed
Open
rphmeier
rphmeier reviewed Feb 10, 2022
View reviewed changes
runtime/common/src/paras_registrar.rs
@@ -564,6 +576,15 @@ impl<T: Config> Pallet<T> {

Ok((ParaGenesisArgs { genesis_head, validation_code, parachain }, deposit))
}

/// Swap a parachain and parathread, which involves scheduling an appropriate lifecycle update.
fn do_thread_and_chain_swap(to_downgrade: ParaId, to_upgrade: ParaId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right

rphmeier
rphmeier approved these changes Feb 10, 2022
View reviewed changes
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look over the tests very deeply but the swapping and migration logic looks OK.

@shawntabrizi
Copy link
Member Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 79757b9 into master Feb 12, 2022
@paritytech-processbot paritytech-processbot bot deleted the shawntabrizi-complete-swap branch February 12, 2022 17:16
ordian added a commit that referenced this pull request Feb 22, 2022
@ordian
Merge branch 'master' into ao-approval-distribution-fix
b611ab8 
* master: (27 commits)
  Bump `tokio` to 1.17.0 (#4965)
  bump transaction_version (#4956)
  Bump tracing-subscriber from 0.3.8 to 0.3.9 (#4954)
  staking miner: reuse ws conn for remote-ext (#4849)
  Revert "collator-protocol: short-term fixes for connectivity (#4640)" (#4914)
  Bump tracing from 0.1.30 to 0.1.31 (#4941)
  Better spam slots handling (#4845)
  Bump proc-macro-crate from 1.1.0 to 1.1.2 (#4936)
  corrected paras code validation event comments (#4932)
  Companion for refactor election score #10834 (#4927)
  Companion CI: Make sure to pass the update crates properly (#4928)
  update digest to v0.10.2 (#4907)
  Companion for #10832 (#4918)
  Companion for `Remove u32_trait` (#4920)
  Bump serde_json from 1.0.78 to 1.0.79 (#4916)
  Bump rand from 0.8.4 to 0.8.5 (#4917)
  Remove stale migrations post 9.16 release (#4848)
  Add proxy type for Kappa Sigma Mu (#4851)
  Baseline weights for `force_apply_min_commission` (#4896)
  Allow two Parachains to swap (#4772)
  ...
@shawntabrizi shawntabrizi added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Mar 3, 2022
This was referenced Mar 4, 2022
Closed
Closed
@chevdor chevdor mentioned this pull request Mar 18, 2022
Closed
13 tasks
@jacogr jacogr mentioned this pull request Mar 19, 2022
Closed
@librelois librelois mentioned this pull request Mar 21, 2022
Closed
@shawntabrizi shawntabrizi mentioned this pull request Mar 22, 2022
Merged
@Dengjianping Dengjianping mentioned this pull request Apr 19, 2022
Merged
10 tasks
@DrW3RK DrW3RK mentioned this pull request May 5, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma left review comments

@emostov emostov emostov approved these changes

@rphmeier rphmeier rphmeier approved these changes

@gavofyork gavofyork Awaiting requested review from gavofyork

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shawntabrizi @emostov @joao-paulo-parity @wischli @kianenigma @rphmeier