Make Pallet ModuleId and LockIdentifier Configurable #5695

emostov · 2020-04-18T16:18:44Z

Closes #5149

This PR aims to switch MODULE_ID from being hardcoded into pallets over to being specified in runtime configuration.

polkadot companion: paritytech/polkadot#1007

Below are the pallets changed and notes:

treasury
elections
- no changes in runtime because elections does not appear to be incorporated currently.
elections-phragrem
- please review the change to the test I made around line 1137. I changed it from assert_eq!(lock.id, MODULE_ID); to assert_eq!(lock.id, *b"phrelect"); because MODULE_ID is no longer defined. However, I am not sure if the right value should be dynamic.
evm
- no changes in runtime because evm does not appear to be incorporated currently.
Society

Before you submitting, please check that:

You added a brief description of the PR, e.g.:
- What does it do?
- What important points reviewers should know?
- Is there something left for follow-up PRs?
You labeled the PR with appropriate labels if you have permissions to do so.
You mentioned a related issue if this PR related to it, e.g. Fixes #228 or Related #1337.
You asked any particular reviewers to review. If you aren't sure, start with GH suggestions.
Your PR adheres the style guide
- In particular, mind the maximal line length.
- There is no commented code checked in unless necessary.
- Any panickers have a proof or removed.
You bumped the runtime version if there are breaking changes in the runtime.
You updated any rustdocs which may have changed
Has the PR altered the external API or interfaces used by Polkadot? Do you have the corresponding Polkadot PR ready?

After you've read this notice feel free to remove it.
Thank you!

✄ -----------------------------------------------------------------------------

parity-cla-bot · 2020-04-18T16:18:47Z

It looks like @emostov signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

shawntabrizi · 2020-04-18T20:11:32Z

Looks about right, can you please check spacing issues with the lines you have introduced (we use tabs), and there seems to be compilation error, maybe just needs to merge master.

Sync with main report

emostov · 2020-04-18T21:46:05Z

I merged in upstream/master and it looks like there still is a compilation error in some of the CI jobs. I did a release build and later ran all the non-ignored tests locally. Both seemed to finish without an issue. Not sure what else to try/look at regarding the compilation error.

Planning on having an accompanying Polkadot PR up within the next 24hrs as long as I can find a quick resolution to the CI errors.

…able-module-id merge master

frame/elections-phragmen/src/lib.rs

shawntabrizi

Looks good to me!

bkchr

Looks good, just 2 nitpicks.

bkchr · 2020-04-21T09:18:30Z

frame/elections-phragmen/src/lib.rs

@@ -1124,7 +1130,7 @@ mod tests {

 	fn has_lock(who: &u64) -> u64 {
 		let lock = Balances::locks(who)[0].clone();
-		assert_eq!(lock.id, MODULE_ID);
+		assert_eq!(lock.id, *b"phrelect");


Suggested change

assert_eq!(lock.id, *b"phrelect");

assert_eq!(lock.id, ElectionsPhragmenModuleId::get());

bkchr · 2020-04-21T09:24:31Z

frame/elections/src/lib.rs

@@ -147,6 +145,8 @@ type ApprovalFlag = u32;
 const APPROVAL_FLAG_LEN: usize = 32;

 pub trait Trait: frame_system::Trait {
+	type ModuleId: Get<LockIdentifier>;


Added a doc comment /// Identifier for the elections-phragmen pallet's lock and also an equivalent doc comment for elections.

… into zeke/fix/configurable-module-id Merge in origin branch

frame/elections/src/lib.rs

frame/evm/src/lib.rs

Co-Authored-By: Amar Singh <[email protected]>

bkchr · 2020-04-22T13:57:09Z

Please merge master to fix the conflict.

…emostov/substrate into zeke/fix/configurable-module-id merge origin

emostov · 2020-04-22T15:24:57Z

Before this gets merged, should I take any steps to ensure that the polkadot companion PR can be merged in quick succession?

bkchr · 2020-04-22T19:00:18Z

Before this gets merged, should I take any steps to ensure that the polkadot companion PR can be merged in quick succession?

Can we as push to your branch?

emostov · 2020-04-22T19:37:14Z

Before this gets merged, should I take any steps to ensure that the polkadot companion PR can be merged in quick succession?

Can we as push to your branch?

I think you are asking if you can push to my branch? If so, the answer is: yes. Let me know if I need to do anything on my end to make that happen.

* add module ids to kusama runtime * update kusam with polkadot runtimes to have moduleids configured * trivial * define module id for treasury in crowdfund.rs * crodfund builds without issue * remove commented out code * switch crowdfund to configurable moduleid * test-runtime passing * trivial syntax * add module id to mock * Update `Cargo.lock` Co-authored-by: zeke <[email protected]>

* transition treasury to configurable moduleids * make election module id configurable * convert runtime and pallet to accept module id config elections-phragmen * add ModuleId to evm pallet * change society pallet to configurable module id * delete commented out module_id * delete commented out code and merge in upstream master * try and convert 4 whitespace to tab * fix remaining space to tab conversions * trivial cleaning * delete comment from elections-phragrems tests * trivial * Update frame/elections-phragmen/src/lib.rs * add docs for elections and elections phragmen * make has_lock test get moduleid dynamically * Apply suggestions from code review Co-Authored-By: Amar Singh <[email protected]> * make sure get is imported to evm Co-authored-by: Shawn Tabrizi <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Amar Singh <[email protected]> Co-authored-by: Benjamin Kampmann <[email protected]>

transition treasury to configurable moduleids

793bd77

paritytech-ci added the B2-breaksapi label Apr 18, 2020

emostov added 5 commits April 18, 2020 09:31

make election module id configurable

60a6b61

convert runtime and pallet to accept module id config elections-phragmen

e64568b

add ModuleId to evm pallet

4eadc31

change society pallet to configurable module id

1fe0ba7

delete commented out module_id

6072ee4

shawntabrizi changed the title ~~Active draft for issue #5149~~ Make Pallet ModuleId and LockIdentifier Configurable Apr 18, 2020

emostov added 6 commits April 18, 2020 13:27

Merge remote-tracking branch 'upstream/master'

11a4c33

Sync with main report

delete commented out code and merge in upstream master

1475410

try and convert 4 whitespace to tab

ae2a161

fix remaining space to tab conversions

ef510f3

trivial cleaning

bde301e

delete comment from elections-phragrems tests

21540f2

emostov mentioned this pull request Apr 19, 2020

Companion PR to #5695 paritytech/polkadot#1007

Closed

Merge remote-tracking branch 'upstream/master' into zeke/fix/configur…

2870d5d

…able-module-id merge master

emostov marked this pull request as ready for review April 20, 2020 14:44

emostov requested review from kianenigma and sorpaas as code owners April 20, 2020 14:44

trivial

db56244

shawntabrizi reviewed Apr 20, 2020

View reviewed changes

frame/elections-phragmen/src/lib.rs Outdated Show resolved Hide resolved

shawntabrizi approved these changes Apr 20, 2020

View reviewed changes

Update frame/elections-phragmen/src/lib.rs

b3d81a6

bkchr suggested changes Apr 21, 2020

View reviewed changes

shawntabrizi mentioned this pull request Apr 21, 2020

Bounties #5715

Merged

5 tasks

emostov added 2 commits April 21, 2020 09:36

add docs for elections and elections phragmen

09ab880

make has_lock test get moduleid dynamically

9348d9f

Merge remote-tracking branch 'origin/zeke/fix/configurable-module-id'…

81ef6a5

… into zeke/fix/configurable-module-id Merge in origin branch

shawntabrizi requested a review from bkchr April 21, 2020 18:06

4meta5 reviewed Apr 21, 2020

View reviewed changes

frame/elections/src/lib.rs Outdated Show resolved Hide resolved

4meta5 reviewed Apr 21, 2020

View reviewed changes

frame/evm/src/lib.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

15e5660

Co-Authored-By: Amar Singh <[email protected]>

bkchr approved these changes Apr 21, 2020

View reviewed changes

shawntabrizi added the A8-mergeoncegreen label Apr 22, 2020

gnunicorn added the A3-needsresolving label Apr 22, 2020

Merge branch 'master' into zeke/fix/configurable-module-id

067e5df

gnunicorn removed the A3-needsresolving label Apr 22, 2020

emostov added 2 commits April 22, 2020 08:11

merge upstream/master

a78c0c0

Merge branch 'zeke/fix/configurable-module-id' of https://github.com/…

83649e9

…emostov/substrate into zeke/fix/configurable-module-id merge origin

make sure get is imported to evm

530d490

sjeohp-zz mentioned this pull request Apr 23, 2020

Polkadot/substrate lock-step merges paritytech/parity-processbot#157

Closed

Merge branch 'master' into zeke/fix/configurable-module-id

42a1ff1

bkchr merged commit 5038c3a into paritytech:master Apr 24, 2020

emostov deleted the zeke/fix/configurable-module-id branch May 13, 2020 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make Pallet ModuleId and LockIdentifier Configurable #5695

Make Pallet ModuleId and LockIdentifier Configurable #5695

Uh oh!

emostov commented Apr 18, 2020 •

edited

Loading

Uh oh!

parity-cla-bot commented Apr 18, 2020

Uh oh!

shawntabrizi commented Apr 18, 2020

Uh oh!

emostov commented Apr 18, 2020

Uh oh!

Uh oh!

shawntabrizi left a comment

Uh oh!

bkchr left a comment

Uh oh!

bkchr Apr 21, 2020

Uh oh!

bkchr Apr 21, 2020

Uh oh!

emostov Apr 21, 2020

Uh oh!

Uh oh!

Uh oh!

bkchr commented Apr 22, 2020

Uh oh!

emostov commented Apr 22, 2020

Uh oh!

bkchr commented Apr 22, 2020

Uh oh!

emostov commented Apr 22, 2020 •

edited

Loading

Uh oh!

Uh oh!

	assert_eq!(lock.id, *b"phrelect");
	assert_eq!(lock.id, ElectionsPhragmenModuleId::get());

Make Pallet ModuleId and LockIdentifier Configurable #5695

Make Pallet ModuleId and LockIdentifier Configurable #5695

Uh oh!

Conversation

emostov commented Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Apr 18, 2020

Uh oh!

shawntabrizi commented Apr 18, 2020

Uh oh!

emostov commented Apr 18, 2020

Uh oh!

Uh oh!

shawntabrizi left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

emostov Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkchr commented Apr 22, 2020

Uh oh!

emostov commented Apr 22, 2020

Uh oh!

bkchr commented Apr 22, 2020

Uh oh!

emostov commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

emostov commented Apr 18, 2020 •

edited

Loading

emostov commented Apr 22, 2020 •

edited

Loading