-
Notifications
You must be signed in to change notification settings - Fork 101
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
Kusama People Chain #217
Kusama People Chain #217
Conversation
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.
Did not go in-depth reviewing people code, but compared it against the Rococo deployed code. Integration to Kusama looks good.
integration-tests/emulated/tests/people/people-kusama/src/lib.rs
Outdated
Show resolved
Hide resolved
pub const PARA_ID: u32 = 1004; | ||
pub const ED: Balance = people_kusama_runtime::ExistentialDeposit::get(); | ||
|
||
pub fn genesis() -> Storage { |
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.
Can the genesis storage not be produced by the runtime?
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'm not sure, @NachoPal ?
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.
In general it should be possible to do: RuntimeGenesis::default().build()
to populate externality storage with the genesis values.
But as i see we have this copy&pasted in all runtimes, so maybe can keep this for a future follow-up to clean them all.
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.
There's some refactoring and improvement from @michalkucharczyk in polkadot-sdk on genesis config, maybe he has an opinion on how this can be done best with minimal copy+pasta.
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 am not very familiar with this part of code. But if I understand correctly this function is later used for some integration tests. So it is not strictly runtime function, just a testing helper?
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 it comes to duplication: If the set of customized fields in genesis config is the same for every runtime/test then we could use single json patch and use GenesisBuilder runtime API to build the storage. But this approach would also require that serialization format is exactly the same for every runtime (e.g. SessionKeys).
However json-patch approach may slow down execution of the tests (this concern was raised by @NachoPal if I recall correctly) - as we need to execute a runtime call to get the state.
// Straight up local `AccountId32` origins just alias directly to `AccountId`. | ||
AccountId32Aliases<RelayNetwork, AccountId>, | ||
// Here/local root location to `AccountId`. | ||
HashedDescription<AccountId, DescribeTerminus>, |
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.
We can also include DescribeAllTerminal
. I do not see a use case now, but this will let us to derive an account for pallets or plurality locations
// ./target/production/polkadot | ||
// benchmark | ||
// pallet | ||
// --chain=rococo-dev |
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.
Are these regenerated before release? Or should they be run with bench bot in this PR?
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.
Do we have bench bot in the Fellowship? I assumed they get regenerated on release (cc @bkontur )?
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 bench bot for fellows (yet?) #50 (comment).
Yes, for sure on release.
I am working on that here: #223, but if you need/want to check before, I could do it just for this branch also, just this pallet and/or the full people chain?
Just let me know
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.
All good, wasn't sure how it worked in the fellowship repo
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 am working on that here: #223, but if you need/want to check before, I could do it just for this branch also, just this pallet and/or the full people chain?
For me there's no need to run in order to merge this, but maybe others disagree. For sure it needs to be part of release though, so perhaps we want to do a run on this branch just to ensure there are no hiccups?
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.
Did only quick scan through. Approving to unblock the merge.
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.
Some minor comments but lgtm.
/merge |
Enabled Available commands
For more information see the documentation |
Head branch was pushed to by a user without write access
/merge |
Enabled Available commands
For more information see the documentation |
6d93705
into
polkadot-fellows:main
pub struct ParentOrParentsPlurality; | ||
impl Contains<Location> for ParentOrParentsPlurality { | ||
fn contains(location: &Location) -> bool { | ||
matches!(location.unpack(), (1, []) | (1, [Plurality { .. }])) | ||
} | ||
} |
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.
just one late nit: this is everywhere replaced with parachains_common::xcm_config::ParentRelayOrSiblingParachains
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.
fixed here: #245
I wanted to regenerate weights for people chain, but I realized that it was not added to the |
@@ -14,5 +14,9 @@ | |||
{ | |||
"name": "bridge-hub-polkadot", | |||
"package": "bridge-hub-polkadot-integration-tests" | |||
}, |
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 this needs to be added to the runtimes-matrix.json
- for running tests/check-migrations...
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.
fixed here: #245
See https://forum.polkadot.network/t/people-chain-launch-and-identity-migration-plan/5930 for launch process.
A branch with runtimes for steps 2 and 3 of the launch process is here: joepetrowski#1