-
Notifications
You must be signed in to change notification settings - Fork 79
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
Altair Runtime Upgrade 5 + Polkadot upgrade to v0.9.16 #611
Conversation
pallets/loans/Cargo.toml
Outdated
frame-benchmarking = { git = "https://github.com/paritytech/substrate", default-features = false , optional = true , branch = "polkadot-v0.9.16" } | ||
pallet-balances = { git = "https://github.com/paritytech/substrate", default-features = false , optional = true, branch = "polkadot-v0.9.16" } | ||
pallet-uniques = { git = "https://github.com/paritytech/substrate", default-features = false , optional = true, branch = "polkadot-v0.9.16" } | ||
orml-tokens = { git = "https://github.com/open-web3-stack/open-runtime-module-library", default-features = false, optional = true, branch = "new_reanchor" } |
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.
Haven't released yet v0.9.16 on ORML but PR on this branch is open and active, should be published soon.
>; | ||
|
||
// Migration for scheduler pallet to move from a plain Call to a CallOrHash. | ||
pub struct SchedulerMigrationV3; |
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.
Not sure if it is needed for this stateless runtime, but should not harm.
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 unless/until we have some sort of persistent development chain running somewhere, this isn't needed.
If it makes sense to add/remove the migrations here at the same time as elsewhere let's go ahead and do it, though, just to keep things consistent.
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.
Looks good overall. I think we need to update some of our commands given that the --parachain-id
option is not expected at a different location. I can look a this later this weekend or on Monday morning
pallets/loans/Cargo.toml
Outdated
frame-benchmarking = { git = "https://github.com/paritytech/substrate", default-features = false , optional = true , branch = "polkadot-v0.9.16" } | ||
pallet-balances = { git = "https://github.com/paritytech/substrate", default-features = false , optional = true, branch = "polkadot-v0.9.16" } | ||
pallet-uniques = { git = "https://github.com/paritytech/substrate", default-features = false , optional = true, branch = "polkadot-v0.9.16" } | ||
orml-tokens = { git = "https://github.com/open-web3-stack/open-runtime-module-library", default-features = false, optional = true, branch = "master" } |
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.
tmp - until v0.9.16 is released
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.
LGTM! Thanks for handling this so fast @mikiquantum!!
Just wanna clarify why we change the runtime version bumb
- from:
100(x)
->100(x+1)
- to:
1(x)00
->1(x+1)00
@@ -47,7 +47,6 @@ use sp_std::vec::Vec; | |||
pub trait Reward { | |||
/// The account from the parachain, that the claimer provided in her/his transaction. | |||
type ParachainAccountId: Debug | |||
+ Default |
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.
Nice!
@@ -89,6 +90,7 @@ pub mod pallet { | |||
// method. | |||
#[pallet::pallet] | |||
#[pallet::generate_store(pub (super) trait Store)] | |||
#[pallet::without_storage_info] |
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.
What is this for?
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.
Looks related to paritytech/substrate#10662.
Sounds like this attribute is a bit of a hack and we should work on being able to remove it? But that can be a future upgrade, the less we change when we're in a hurry the better.
Would be nice to ticket this, though.
@@ -63,6 +63,7 @@ impl frame_system::Config for Test { | |||
type SystemWeightInfo = (); | |||
type SS58Prefix = (); | |||
type OnSetCode = (); | |||
type MaxConsumers = frame_support::traits::ConstU32<16>; |
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.
Why ist this needed? What did Parity do? ^^
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.
pub struct AuthorGiven; | ||
|
||
impl FindAuthor<u64> for AuthorGiven { | ||
fn find_author<'a, I>(_digests: I) -> Option<u64> | ||
where | ||
I: 'a + IntoIterator<Item = (ConsensusEngineId, &'a [u8])>, | ||
{ | ||
Some(100) | ||
} | ||
} | ||
|
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.
Does anybody know what was the issue with that? Only saw in the commits of the latest version, that they changed the import logic of the node.
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.
Storage of the block author author()
is now an OptionQuery
instead of a ValueQuery
over the AccountId, which the trait now it doesn't support a Default value. Is either set or not. Hence this cascading list of changes.
runtime/altair/src/lib.rs
Outdated
@@ -72,10 +70,14 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { | |||
spec_name: create_runtime_str!("altair"), | |||
impl_name: create_runtime_str!("altair"), | |||
authoring_version: 1, | |||
spec_version: 1008, | |||
spec_version: 1900, |
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.
Why the logical change here? What does it represent or is it a typo?
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.
This matches how parity does their versioning - polkadot X.Y turns into runtime XY00. I'm not sure we need to do this, and I'm personally happy to just keep incrementing a release number without trying to tie it to any more formal versioning structure.
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.
Since we are tied to updating the node as well and that follows a semver X.Y.Z. One reason I thought about using the same as parity is bc if we dont, then our x.y.z for 1008, would be 1.0.8, which puts us in version 1 already, which prob we are not ready for yet. Alternatively we can use 0.10.08 as the equivalent of 1008, I can change that too
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.
Like the later more, but I am happy with both.
runtime/centrifuge/src/lib.rs
Outdated
@@ -65,10 +65,14 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { | |||
spec_name: create_runtime_str!("centrifuge"), | |||
impl_name: create_runtime_str!("centrifuge"), | |||
authoring_version: 1, | |||
spec_version: 1000, | |||
spec_version: 1100, |
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.
Also new versions we use here?
CentrifugeChainSpec::from_genesis( | ||
"Centrifuge Dev", | ||
"centrifuge_dev", | ||
ChainType::Live, |
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.
Why live here?
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.
My thought was that when developing locally you should use $runtime-local and when deploying as dev environment we can use the $runtime-dev and with a defined set of authorities. The reason being that ChainType::Local might not behave as expected in deployed environments.
type WeightInfo = pallet_utility::weights::SubstrateWeight<Self>; | ||
} | ||
|
||
parameter_types! { | ||
pub MaximumSchedulerWeight: Weight = Perbill::from_percent(80) * MaximumBlockWeight::get(); | ||
pub const MaxScheduledPerBlock: u32 = 50; | ||
// Retry a scheduled item every 10 blocks (1 minute) until the preimage exists. | ||
pub const NoPreimagePostponement: Option<u32> = Some(10); |
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.
Comment is wrong - 10 blocks is 2 minutes for us
@@ -870,7 +865,8 @@ construct_runtime!( | |||
Identity: pallet_identity::{Pallet, Call, Storage, Event<T>} = 67, | |||
Vesting: pallet_vesting::{Pallet, Call, Storage, Event<T>, Config<T>} = 68, | |||
Treasury: pallet_treasury::{Pallet, Call, Storage, Config, Event<T>} = 69, | |||
// Uniques: pallet_uniques::{Pallet, Call, Storage, Event<T>} = 70, | |||
Preimage: pallet_preimage::{Pallet, Call, Storage, Event<T>} = 70, | |||
Uniques: pallet_uniques::{Pallet, Call, Storage, Event<T>} = 72, |
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.
No pallet 71?
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.
It is defined a bit above, and seems that for some reasons there is an order dependency.
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.
@mustermeiszer I just compiled successfully changing the order moving down pallet 71 to its ordered place, do you why you added it up there before?
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.
When you start the chain the storage will not be present and we will not be to inject candidates from the collator-selection pallet. As mentioned in the comment.
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.
Okay, then leaving it up there
Nothing jumps out to me that @mustermeiszer didn't already comment on |
This PR aims to: