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

Migrate frame-system to pallet attribute macro #7898

Merged
57 commits merged into from
Jan 20, 2021
Merged

Migrate frame-system to pallet attribute macro #7898

57 commits merged into from
Jan 20, 2021

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Jan 14, 2021

Starting on #7882.

Converts the System pallet to the new pallet attribute macro introduced in #6877.

Following roughly the upgrade guidelines here: https://crates.parity.io/frame_support/attr.pallet.html#upgrade-guidelines.

For reviewing I would suggest going through commit by commit.

⚠️ Breaking Change ⚠️

From https://crates.parity.io/frame_support/attr.pallet.html#checking-upgrade-guidelines

storages now use PalletInfo for module_prefix instead of the one given to decl_storage: Thus any use of this pallet in construct_runtime! should be careful to update name in order not to break storage or to upgrade storage (moreover for instantiable pallet). If pallet is published, make sure to warn about this breaking change.

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 14, 2021
@ascjones ascjones requested a review from gui1117 January 14, 2021 15:15
@ascjones ascjones marked this pull request as ready for review January 14, 2021 15:15
frame/system/src/lib.rs Outdated Show resolved Hide resolved
frame/system/src/lib.rs Show resolved Hide resolved
frame/system/src/lib.rs Show resolved Hide resolved
@ascjones
Copy link
Contributor Author

Might need to double check that I've merged #7363 properly, it required some manual merging because of all the reordering in this PR

@ascjones
Copy link
Contributor Author

ascjones commented Jan 18, 2021

Metadata diff using https://github.com/ascjones/subsee, against master at the time 8aa9281.

TLDR: BlockLength and Version added as constants.

diff --git a/master-8aa92811-full.json b/migrated-full.json
index f0eb814..8f89e7b 100644
--- a/master-8aa92811-full.json
+++ b/migrated-full.json
@@ -620,7 +620,7 @@
             {
               "name": "NewAccount",
               "arguments": [
-                "AccountId"
+                "T::AccountId"
               ],
               "documentation": [
                 " A new \\[account\\] was created."
@@ -629,7 +629,7 @@
             {
               "name": "KilledAccount",
               "arguments": [
-                "AccountId"
+                "T::AccountId"
               ],
               "documentation": [
                 " An \\[account\\] was reaped."
@@ -637,44 +637,6 @@
             }
           ],
           "constants": [
-            {
-              "name": "BlockHashCount",
-              "ty": "T::BlockNumber",
-              "value": [
-                96,
-                9,
-                0,
-                0
-              ],
-              "documentation": [
-                " The maximum number of blocks to allow in mortal eras."
-              ]
-            },
-            {
-              "name": "DbWeight",
-              "ty": "RuntimeDbWeight",
-              "value": [
-                64,
-                120,
-                125,
-                1,
-                0,
-                0,
-                0,
-                0,
-                0,
-                225,
-                245,
-                5,
-                0,
-                0,
-                0,
-                0
-              ],
-              "documentation": [
-                " The weight of runtime database operations the runtime can invoke."
-              ]
-            },
             {
               "name": "BlockWeights",
               "ty": "limits::BlockWeights",
@@ -778,7 +740,256 @@
                 0
               ],
               "documentation": [
-                " The weight configuration (limits & base values) for each class of extrinsics and block."
+                " Block & extrinsics weights: base values and limits."
+              ]
+            },
+            {
+              "name": "BlockLength",
+              "ty": "limits::BlockLength",
+              "value": [
+                0,
+                0,
+                60,
+                0,
+                0,
+                0,
+                80,
+                0,
+                0,
+                0,
+                80,
+                0
+              ],
+              "documentation": [
+                " The maximum length of a block (in bytes)."
+              ]
+            },
+            {
+              "name": "BlockHashCount",
+              "ty": "T::BlockNumber",
+              "value": [
+                96,
+                9,
+                0,
+                0
+              ],
+              "documentation": [
+                " Maximum number of block number to block hash mappings to keep (oldest pruned first)."
+              ]
+            },
+            {
+              "name": "DbWeight",
+              "ty": "RuntimeDbWeight",
+              "value": [
+                64,
+                120,
+                125,
+                1,
+                0,
+                0,
+                0,
+                0,
+                0,
+                225,
+                245,
+                5,
+                0,
+                0,
+                0,
+                0
+              ],
+              "documentation": [
+                " The weight of runtime database operations the runtime can invoke."
+              ]
+            },
+            {
+              "name": "Version",
+              "ty": "RuntimeVersion",
+              "value": [
+                16,
+                110,
+                111,
+                100,
+                101,
+                56,
+                115,
+                117,
+                98,
+                115,
+                116,
+                114,
+                97,
+                116,
+                101,
+                45,
+                110,
+                111,
+                100,
+                101,
+                10,
+                0,
+                0,
+                0,
+                5,
+                1,
+                0,
+                0,
+                0,
+                0,
+                0,
+                0,
+                48,
+                223,
+                106,
+                203,
+                104,
+                153,
+                7,
+                96,
+                155,
+                3,
+                0,
+                0,
+                0,
+                55,
+                227,
+                151,
+                252,
+                124,
+                145,
+                245,
+                228,
+                1,
+                0,
+                0,
+                0,
+                64,
+                254,
+                58,
+                212,
+                1,
+                248,
+                149,
+                154,
+                4,
+                0,
+                0,
+                0,
+                210,
+                188,
+                152,
+                151,
+                238,
+                208,
+                143,
+                21,
+                2,
+                0,
+                0,
+                0,
+                247,
+                139,
+                39,
+                139,
+                229,
+                63,
+                69,
+                76,
+                2,
+                0,
+                0,
+                0,
+                237,
+                153,
+                197,
+                172,
+                178,
+                94,
+                237,
+                245,
+                2,
+                0,
+                0,
+                0,
+                203,
+                202,
+                37,
+                227,
+                159,
+                20,
+                35,
+                135,
+                2,
+                0,
+                0,
+                0,
+                104,
+                122,
+                212,
+                74,
+                211,
+                127,
+                3,
+                194,
+                1,
+                0,
+                0,
+                0,
+                188,
+                157,
+                137,
+                144,
+                79,
+                91,
+                146,
+                63,
+                1,
+                0,
+                0,
+                0,
+                104,
+                182,
+                107,
+                161,
+                34,
+                201,
+                63,
+                167,
+                1,
+                0,
+                0,
+                0,
+                55,
+                200,
+                187,
+                19,
+                80,
+                169,
+                162,
+                168,
+                1,
+                0,
+                0,
+                0,
+                171,
+                60,
+                5,
+                114,
+                41,
+                31,
+                235,
+                139,
+                1,
+                0,
+                0,
+                0,
+                2,
+                0,
+                0,
+                0
+              ],
+              "documentation": [
+                " Get the chain's current version."
               ]
             },
             {

@gui1117
Copy link
Contributor

gui1117 commented Jan 20, 2021

For the metadata changes such as:

              "arguments": [
-                "AccountId"
+                "T::AccountId"
               ],

We can actually solve by using metadata attribute in event but I guess both are valid metadata of the type.

Copy link
Contributor

@gui1117 gui1117 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.
Any merge of master must take care that system hasn't been changed.

Also you probably need to merge master again to fix the CI

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

LGTM

@gui1117
Copy link
Contributor

gui1117 commented Jan 20, 2021

bot merge

@ghost
Copy link

ghost commented Jan 20, 2021

Trying merge.

@ghost ghost merged commit 4868630 into master Jan 20, 2021
@ghost ghost deleted the aj-migrate-system branch January 20, 2021 10:48
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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.

6 participants