Skip to content
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

Implement pallet-data-preservers #341

Merged
merged 13 commits into from
Dec 13, 2023
Merged

Conversation

tmpolaczyk
Copy link
Contributor

@tmpolaczyk tmpolaczyk commented Nov 27, 2023

Implement V0 of pallet-data-preservers, will not support data providers or payments yet. The container chain manager (or root) is expected to set the list of bootnodes manually.

Also, change mark_valid_for_collating extrinisic, now it will fail if the container chain does not have any bootnodes.

Add migration
Add genesis data to pallet data preservers
Copy link
Contributor

github-actions bot commented Nov 29, 2023

Coverage Report

(master)

@@                        Coverage Diff                        @@
##           master   tomasz-pallet-data-preservers      +/-   ##
=================================================================
+ Coverage   73.76%                          73.95%   +0.19%     
+ Files          89                              91       +2     
+ Lines       20241                           20482     +241     
=================================================================
+ Hits        14929                           15146     +217     
+ Misses       5312                            5336      +24     
Files Changed Coverage
/node/src/chain_spec.rs 90.40% (+0.43%) 🔼
/pallets/registrar/src/lib.rs 92.18% (-0.50%) 🔽
/runtime/dancebox/src/lib.rs 79.73% (+0.09%) 🔼
/runtime/dancebox/src/migrations.rs 86.70% (+1.68%) 🔼
/runtime/dancebox/tests/common/mod.rs 99.73% (+0.01%) 🔼
/runtime/dancebox/tests/integration_test.rs 99.69% (+0.01%) 🔼

Coverage generated Mon Dec 11 11:39:38 UTC 2023

.iter()
.map(|(para_id, _genesis_data, boot_nodes)| (*para_id, boot_nodes.clone()))
.collect();
let para_ids: Vec<_> = para_ids
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this to filter those not having bootnodes? if so please right a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's because I removed the bootnodes from the registrar genesis config, now bootnodes are part of the data preservers genesis.

para_id: ParaId,
boot_nodes: BoundedVec<BoundedVec<u8, T::MaxBootNodeUrlLen>, T::MaxBootNodes>,
) -> DispatchResult {
T::SetBootNodesOrigin::ensure_origin(origin, &para_id)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good :)

Comment on lines +147 to +148

impl<T: Config> Pallet<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably need a trait here that is able to access bootnodes for a paraId

@@ -767,6 +772,105 @@ impl pallet_services_payment::Config for Runtime {
type WeightInfo = pallet_services_payment::weights::SubstrateWeight<Runtime>;
}

pub struct DanceboxContainerChainManagerOrRootOrigin<T, RootOrigin> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good, but I need to implement try_origin instead of ensure_origin for the trait, let's see if I can adapt it somehow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tmpolaczyk check 160e00c, revert it if you find it less clean, I do find it cleaner than what we had


fn check_valid_for_collating(para_id: ParaId) -> DispatchResult {
// To be able to call mark_valid_for_collating, a container chain must have bootnodes
if DataPreservers::boot_nodes(para_id).len() > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably add this as a trait that is implemented for the data-preservers palet, and you pass this as config in the registrar pallet. That way you can return an error of the pallet itself

@tmpolaczyk tmpolaczyk marked this pull request as ready for review December 4, 2023 17:14
@tmpolaczyk tmpolaczyk changed the title WIP pallet-data-preservers Implement pallet-data-preservers Dec 4, 2023
@tmpolaczyk tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes labels Dec 4, 2023
Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

LGTM! if benchmarks are still to run please do

Copy link
Contributor

@fgamundi fgamundi left a comment

Choose a reason for hiding this comment

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

LGTM

@girazoki girazoki merged commit 4082061 into master Dec 13, 2023
26 checks passed
@girazoki girazoki deleted the tomasz-pallet-data-preservers branch December 13, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants