-
Notifications
You must be signed in to change notification settings - Fork 47
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
Conversation
a597e10
to
8b0e6ff
Compare
53a6e7d
to
58b6845
Compare
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 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.
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.
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?
pub(crate) BoundedVec<u8, T::MaxNameLength>, | ||
PhantomData<(T, T::MinNameLength)>, | ||
); | ||
pub struct AsciiWeb3Name<T: Config>(pub BoundedVec<u8, T::MaxNameLength>, PhantomData<(T, T::MinNameLength)>); |
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 would rather expose a into_inner()
function on the struct than making the bounded vec leak to other crates.
…ocol/kilt-node into chore/polkadot-0.9.42_migration
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.
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::*}; |
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 this should only be imported inside the pallet module.
ed20e23
to
a375f3f
Compare
Fixed #2799
Follow-up PR for the migration.