pallet-collective | port to frame v2 #8151
Conversation
|
Hello @thiolliere , I hope Frame v2 migration is done, Unit test & Benchmarking are passed. Refactored the Unit test, similar to file structure used in pallet-balance. Please kindly share your suggestions & comments. |
gui1117
added
A0-please_review
|
related #7882 It will need a polkadot companion or a migration because the pallet prefix used by storage is changing (in general)
|
frame/collective/src/tests.rs
Outdated
| #![cfg(test)] | ||
|
|
||
| #[macro_export] | ||
| macro_rules! decl_tests { |
There was a problem hiding this comment.
we don't need to wrap test in a macro.
pallet-balance do that in order to run test on 2 different configuration, but here we have only one, thus it is not needed.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
just to be sure, only trivial change were made in the test isn't it ?
There was a problem hiding this comment.
at some point I would prefer not to do the refactor in this PR, so it is easier to review
There was a problem hiding this comment.
I assumed even pallet file structure needs to be refactored, sorry for my bad assumption 👍 ,
Shall I move the test back to lib.rs ? May be as suggested, will take file restructure part of another PR
| let events = System::<T>::events(); | ||
| let system_event: <T as frame_system::Config>::Event = generic_event.into(); | ||
| // compare to the last event record | ||
| let EventRecord { event, .. } = &events[events.len() - 1]; | ||
| assert_eq!(event, &system_event); | ||
| } | ||
|
|
||
| benchmarks_instance! { |
There was a problem hiding this comment.
why did you change this to benchmark ?
NOTE: I think pallet-balance should make use of benchmarks_instance.
There was a problem hiding this comment.
ok it is not supported, waiting on #8190
| /// Old name generated by `decl_event`. | ||
| // TODO :: Have to re-check is "Note message" required ? | ||
| // #[deprecated(note = "use `Event` instead")] |
There was a problem hiding this comment.
better to make it deprecated IMO
| ensure!( | ||
| !<ProposalOf<T, I>>::contains_key(proposal_hash), | ||
| <Error<T, I>>::DuplicateProposal | ||
| ); |
There was a problem hiding this comment.
these refactor should be done in another PR
There was a problem hiding this comment.
actually this refactor is fine to me, sorry for having been too nitpicking
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
…m/paritytech/substrate into shamb0-br1-collective-port-frame-v2 gui-benchmark-pallet-instantiable updates
Plz kindly ignore this,
Changes moved to #8193
#8193
pallet-collective | port to frame v2
related #7882
From https://crates.parity.io/frame_support/attr.pallet.html#checking-upgrade-guidelines
So users of the collective pallet must be careful about the name they used in construct_runtime!. Hence the runtime-migration label, which might not be needed depending on the configuration of the collective pallet.