Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Allow pallet in construct_runtime to have fixed index #6969

Merged
37 commits merged into from
Sep 22, 2020

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Aug 27, 2020

Fix #6855
companion: paritytech/polkadot#1692

BREAKING CHANGE:

the encoding of Event, Call and Origin is changed:
Each variant of these enums are now encoded at the index of the pallet, the index of the pallet is defined similarly to rust fieldless
enum:

construct_runtime!(
...
module1: .. { .. }, // index is 0
module2: .. { .. } = 3, // index is 3
module3: .. { .. }, // index is 4
module4: .. { .. } = 1, // index is 1
module5: .. { .. }, // index is 2
)

This breaking change result in the bump of metadata to version 12 which now provides for each module its index.

Note: Origin system variant is also encoded using this index, contrary to previously where it was fixed at index 0.

Note: to help migration, user should make use of indices in order not to break Call, and make use of scheduler::migrate_origin to update the usage of OriginCaller.

Context

This PR makes pallet having a fixed index. This avoid breaking call, event or origin when pallet definition changes. And ease adding/removing pallets.
So each pallet is now defined a fixed index either implicitly or using = $index syntax similarly to rust fieldless enum.

Syntax:

(syntax is as followed as suggested by Gav but I can do as suggested by Basti in issue, it's only a matter of 10 lines of code)

construct_runtime!(
...
{
   System: system::{Call, Event, Origin}, // index is 0
   Pallet1: pallet1::{Event, Origin} = 20,
   Pallet2: pallet2::{Call, Origin}, // index is 21
   Pallet3: pallet3::{Event, Origin}, // index is 22
   Pallet4: pallet4::{} = 10,
   Pallet5: pallet5::{Call, Origin}, index is 11
}
);

The index must be a integer literal strictly less than 256, it is not allowed to be an expression as parity-scale-codec doesn't allow expression.

Metadata change

So the metadata has now this new information with a new field in module_metadata: index: u8. this index must be used when decoding call variant for corresponding module.

cc @jacogr

TODO

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 27, 2020
@gui1117 gui1117 requested a review from gavofyork August 27, 2020 16:22
@jacogr
Copy link
Contributor

jacogr commented Aug 27, 2020

Just make sure to bump the metadata version to v12 (have not looked yet if it is done), without it there is going to be absolute chaos in the ecosystem, JS, Python, ...

... EDIT: And it is bumped, so yes, can be done by all.

@jacogr
Copy link
Contributor

jacogr commented Aug 27, 2020

cc @emielvanderhoek cc @arjanz

@gui1117 gui1117 added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 1, 2020
@gui1117 gui1117 changed the title Implement pallet with fixed index in construct_runtime Allow pallet in construct_runtime to have fixed index Sep 1, 2020
@gui1117 gui1117 marked this pull request as ready for review September 1, 2020 13:08
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I like the = Something syntax for setting the index! :)

frame/metadata/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/origin.rs Outdated Show resolved Hide resolved
frame/support/src/origin.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/construct_runtime/mod.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/construct_runtime/mod.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/construct_runtime/mod.rs Outdated Show resolved Hide resolved
gui1117 and others added 2 commits September 7, 2020 16:33
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@shawntabrizi
Copy link
Member

shawntabrizi commented Sep 7, 2020

I have just seen this PR and looked over it very quickly, but I think it would be a fair requirement that to use the = index syntax, we should require the user specifies the index for all pallets, rather than letting logic determine the magic number selected for each one.

I think it would avoid future footguns, and adds little overhead. So either they all have it, or none have it.

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 7, 2020

ok it seems we agree on breaking call and origin (and event but it is not important), then we can just force = index; everywhere.

At the same time we can change encoding of origin: origin: OriginCaller::System variant will have the given index.

(for me the only reason to keep this implicit syntax was to not have a breaking change for the encoding of Call and Origin)

@gui1117 gui1117 added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 7, 2020
@shawntabrizi
Copy link
Member

@thiolliere I would also include a breaking change that if a user does not include = index format, that the enums generated are always equal length, even if the pallet does not include Call or Event or whatever... prevent future breaking things.

@gui1117 gui1117 removed the A0-please_review Pull request needs code review. label Sep 8, 2020
@apopiak
Copy link
Contributor

apopiak commented Sep 8, 2020

@thiolliere I would also include a breaking change that if a user does not include = index format, that the enums generated are always equal length, even if the pallet does not include Call or Event or whatever... prevent future breaking things.

+1 for having consistent indices over Call, Event, etc.

@gui1117
Copy link
Contributor Author

gui1117 commented Sep 8, 2020

So I updated PR description:

  • I allowed implicit index, because it can be handy for tests.
  • all event/call/origin are now using this index, so it is consisten and future proof

@gui1117 gui1117 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 8, 2020
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

LGTM, some language nitpicks

gui1117 and others added 2 commits September 21, 2020 11:37
Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks good to me (only very high level review of macro changes)

fn find_part(&self, name: &str) -> Option<&ModulePart> {
self.module_parts.iter().find(|part| part.name() == name)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

given assumption that System is always there, maybe you can maybe simplify a thing or two by fn get_system() -> Option<&ModulePart> or fn is_system() -> bool but just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, for now system is only queried one time and filtered out only one time too, but if I would refactor such functions looks more readable.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Brief view looks good!

gui1117 and others added 5 commits September 21, 2020 14:14
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
This reverts commit f2de8f2.
@gui1117
Copy link
Contributor Author

gui1117 commented Sep 22, 2020

bot merge

@ghost
Copy link

ghost commented Sep 22, 2020

Waiting for commit status.

@apopiak
Copy link
Contributor

apopiak commented Nov 10, 2020

Does not include a runtime migration itself, but runtimes including this will need to migrate.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make outer call/origin non breaking change when one pallet removes one call/origin
9 participants