Make pallet-bags-list instantiable #10957
Conversation
frame/bags-list/src/mock.rs
Outdated
| { | ||
| System: frame_system::{Pallet, Call, Storage, Event<T>, Config}, | ||
| BagsList: bags_list::{Pallet, Call, Storage, Event<T>}, | ||
| BagsList: bags_list::{Pallet, Call, Storage, Event<T, I>}, |
There was a problem hiding this comment.
Here I would add two copies of this bags list pallet, and then do a test which modifies the pallet in different ways and shows they both hold state correctly.
Generally, any PR should have tests which show what the PR does.
There was a problem hiding this comment.
we can have a proper and realistic test for this after #10944, in the meantime I can't think of any imaginative way tbh. I think as long as this PR does not alter any logic, it is fine to go in without further tests. We don't want to test "does pallet instances way the work we expect" here.
There was a problem hiding this comment.
(unless if there are ways that we can make a pallet instantiable incorrectly. My assumption was that if it compiles, it works).
shawntabrizi
added
B0-silent
frame/bags-list/src/list/mod.rs
Outdated
| impl<T: Config<I>, I: 'static> Bag<T, I> { | ||
| #[cfg(test)] | ||
| pub(crate) fn new( | ||
| phantom: PhantomData<I>, |
There was a problem hiding this comment.
really think this is not the pattern of how to do this: just use Default::default? or anything else?
There was a problem hiding this comment.
Yeah, phantom should not need to be passed in as a argument
kianenigma
left a comment
kianenigma
left a comment
There was a problem hiding this comment.
few minor notes, but overall looks good to me! Thanks for your contribution
emostov
left a comment
emostov
left a comment
There was a problem hiding this comment.
Mostly there - see comments. Tag me once all the comments are addressed. Thanks!
|
Also, if you include you polkadot or kusama address in a comment we can tip you :) |
|
/tip small |
|
A small tip was successfully submitted for Doordashcon (12zsKEDVcHpKEWb99iFt3xrTCQQXZMu477nJQsTBBrof5k2h on polkadot). |
|
included in #10997 |
|
Thanks for the Tip :) |
4 participants
This PR attempts to solve #10943
polkadot address: 12zsKEDVcHpKEWb99iFt3xrTCQQXZMu477nJQsTBBrof5k2h