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

chore: polkadot 0.9.42 migration #540

Merged
merged 128 commits into from
Oct 11, 2023
Merged

Conversation

Ad96el
Copy link
Member

@Ad96el Ad96el commented Jul 20, 2023

Fixed #2799

Follow-up PR for the migration.

@Ad96el Ad96el force-pushed the chore/polkadot-0.9.42_migration branch from a597e10 to 8b0e6ff Compare July 20, 2023 14:03
@Ad96el Ad96el requested review from ntn-x2 and weichweich and removed request for ntn-x2 July 20, 2023 14:28
@Ad96el Ad96el force-pushed the chore/polkadot-0.9.42_migration branch from 53a6e7d to 58b6845 Compare July 21, 2023 08:21
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

I see why you did what you did for benchmarking, but I think using actual values from the migration extrinsic will lead to both more accurate results and easier weight computation.

pallets/attestation/src/attestations.rs Outdated Show resolved Hide resolved
pallets/attestation/src/lib.rs Outdated Show resolved Hide resolved
pallets/attestation/src/migrations.rs Outdated Show resolved Hide resolved
pallets/attestation/src/migrations.rs Outdated Show resolved Hide resolved
pallets/attestation/src/migrations.rs Outdated Show resolved Hide resolved
runtimes/peregrine/src/lib.rs Outdated Show resolved Hide resolved
pallets/parachain-staking/src/migrations.rs Outdated Show resolved Hide resolved
pallets/parachain-staking/src/migrations.rs Outdated Show resolved Hide resolved
pallets/parachain-staking/src/migrations.rs Outdated Show resolved Hide resolved
pallets/pallet-migration/src/benchmarking.rs Outdated Show resolved Hide resolved
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

Very nice approach! I love the lazy migration. I mostly left style comments. One question that bugged me was why are why double hashing keys that are already hashed?

support/src/traits.rs Outdated Show resolved Hide resolved
pallets/pallet-migration/src/lib.rs Outdated Show resolved Hide resolved
pallets/pallet-migration/src/lib.rs Show resolved Hide resolved
pallets/pallet-migration/src/lib.rs Show resolved Hide resolved
pallets/pallet-migration/src/mock.rs Outdated Show resolved Hide resolved
pallets/pallet-migration/src/test.rs Outdated Show resolved Hide resolved
pallets/pallet-migration/src/test.rs Outdated Show resolved Hide resolved
pallets/attestation/src/mock.rs Outdated Show resolved Hide resolved
pallets/did/src/did_details.rs Outdated Show resolved Hide resolved
pub(crate) BoundedVec<u8, T::MaxNameLength>,
PhantomData<(T, T::MinNameLength)>,
);
pub struct AsciiWeb3Name<T: Config>(pub BoundedVec<u8, T::MaxNameLength>, PhantomData<(T, T::MinNameLength)>);
Copy link
Member

Choose a reason for hiding this comment

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

I would rather expose a into_inner() function on the struct than making the bounded vec leak to other crates.

pallets/attestation/src/mock.rs Outdated Show resolved Hide resolved
pallets/did/src/did_details.rs Outdated Show resolved Hide resolved
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

Minor nitpick, but otherwise LGTM, by checking what has changed since my last review.

@@ -29,25 +27,28 @@ mod mock;
#[cfg(test)]
mod test;

pub use crate::{default_weights::WeightInfo, pallet::*};
Copy link
Member

@ntn-x2 ntn-x2 Oct 10, 2023

Choose a reason for hiding this comment

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

I think this should only be imported inside the pallet module.

@Ad96el Ad96el enabled auto-merge (squash) October 10, 2023 11:35
@Ad96el Ad96el force-pushed the chore/polkadot-0.9.42_migration branch from ed20e23 to a375f3f Compare October 10, 2023 11:50
@Ad96el Ad96el disabled auto-merge October 11, 2023 08:24
@Ad96el Ad96el merged commit ef007e6 into develop Oct 11, 2023
@Ad96el Ad96el deleted the chore/polkadot-0.9.42_migration branch October 11, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants