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

Migrate pallet-proxy to pallet attribute macro #8365

Merged
merged 7 commits into from
Mar 17, 2021

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Mar 16, 2021

Part of #7882.

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

Upgrade guidelines used.

⚠️ Breaking Change ⚠️

From checking upgrade guidelines

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

So users of the Proxy 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 Proxy pallet.

Notes

There are some changes to the docs in metadata for the constants. The docs in the metadata for constants are now more complete.

Part of #7882.

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

[Upgrade guidelines used](https://substrate.dev/rustdocs/v3.0.0/frame_support/attr.pallet.html#upgrade-guidelines).

## ⚠️ Breaking Change  ⚠️ 

From [checking upgrade guidelines](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`: use of this pallet in `construct_runtime!` needs careful updating of the name in order to not break storage or to upgrade storage (moreover for instantiable pallet). If pallet is published, make sure to warn about this breaking change.

So users of the `Assets` 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 `Assets` pallet.

### Notes

There are some changes to the docs in metadata for the constants. The docs in the metadata for constants are now more complete.
@emostov emostov self-assigned this Mar 16, 2021
@emostov emostov 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. labels Mar 16, 2021
@emostov emostov requested a review from dvdplm March 16, 2021 03:16
@emostov
Copy link
Contributor Author

emostov commented Mar 16, 2021

Metadata diff

$ diff -c post-proxy-meta.json pre-proxy-meta.json
*** post-proxy-meta.json        2021-03-15 19:52:29.000000000 -0700
--- pre-proxy-meta.json 2021-03-14 19:17:22.000000000 -0700
***************
*** 395,401 ****
          }
        ],
        "documentation": [
!         " Dispatch the given `call` from an account that the sender is authorized for through",
          " `add_proxy`.",
          "",
          " Removes any corresponding announcement(s).",
--- 395,401 ----
          }
        ],
        "documentation": [
!         " Dispatch the given `call` from an account that the sender is authorised for through",
          " `add_proxy`.",
          "",
          " Removes any corresponding announcement(s).",
***************
*** 473,482 ****
          0
        ],
        "documentation": [
!         " The base amount of currency needed to reserve for creating a proxy.",
!         "",
!         " This is held for an additional storage item whose value size is",
!         " `sizeof(Balance)` bytes and whose key size is `sizeof(AccountId)` bytes."
        ]
      },
      {
--- 473,479 ----
          0
        ],
        "documentation": [
!         " The base amount of currency needed to reserve for creating a proxy."
        ]
      },
      {
***************
*** 501,510 ****
          0
        ],
        "documentation": [
!         " The amount of currency needed per proxy added.",
!         "",
!         " This is held for adding 32 bytes plus an instance of `ProxyType` more into a pre-existing",
!         " storage value."
        ]
      },
      {
--- 498,504 ----
          0
        ],
        "documentation": [
!         " The amount of currency needed per proxy added."
        ]
      },
      {
***************
*** 528,534 ****
          0
        ],
        "documentation": [
!         " The maximum amount of time-delayed announcements that are allowed to be pending."
        ]
      },
      {
--- 522,528 ----
          0
        ],
        "documentation": [
!         " `MaxPending` metadata shadow."
        ]
      },
      {
***************
*** 553,561 ****
          0
        ],
        "documentation": [
!         " The base amount of currency needed to reserve for creating an announcement.",
!         "",
!         " This is held when a new storage item holding a `Balance` is created (typically 16 bytes)."
        ]
      },
      {
--- 547,553 ----
          0
        ],
        "documentation": [
!         " `AnnouncementDepositBase` metadata shadow."
        ]
      },
      {
***************
*** 580,589 ****
          0
        ],
        "documentation": [
!         " The amount of currency needed per announcement made.",
!         "",
!         " This is held for adding an `AccountId`, `Hash` and `BlockNumber` (typically 68 bytes)",
!         " into a pre-existing storage value."
        ]
      }
    ],
--- 572,578 ----
          0
        ],
        "documentation": [
!         " `AnnouncementDepositFactor` metadata shadow."
        ]
      }
    ],

frame/proxy/src/lib.rs Outdated Show resolved Hide resolved
frame/proxy/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, modulo doc nitpicks.

Comment on lines 132 to 133
/// This is held for adding 32 bytes plus an instance of `ProxyType` more into a pre-existing
/// storage value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this doc string says; can you rephrase it a bit you think?

Copy link
Member

Choose a reason for hiding this comment

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

it is saying that we should modify ProxyDepositFactor to take into account 32 bytes of data + proxytype.encode().len() more bytes of data.

It is a hint for the person configuring this pallet to pick a reasonable number for a deposit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawntabrizi do you think it is worth updating the comment or should it already be clear enough to the target audience?

Heres a sample of how it could be updated, but not sure it really adds much.

/// This is held for adding 32 bytes plus an instance of `ProxyType` more into a pre-existing
/// storage value. Thus, when configuring `ProxyDepositFactor` one should take into account 
/// `32 + proxytype.encode().len()` bytes of data.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that looks like a reasonable addition :)

Couldn't hurt i think.

/// The type of hash used for hashing the call.
type CallHasher: Hash;

/// The base amount of currency needed to reserve for creating an announcement.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The base amount of currency needed to reserve for creating an announcement.
/// The base amount of currency needed in reserve for creating an announcement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if you think I am wrong but I think the current version makes more sense. When you make an announcement you will need "to" reserve at least this amount (so it would move from free => reserve balance). But the balance is not already "in" reserve prior to making the announcement.

Copy link
Member

@shawntabrizi shawntabrizi Mar 16, 2021

Choose a reason for hiding this comment

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

yes i agree w/ @emostov

}

/// Dispatch the given `call` from an account that the sender is authorised for through
/// Dispatch the given `call` from an account that the sender is authorized for through
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a preference for british spelling. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL

Copy link
Member

Choose a reason for hiding this comment

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

def no, we should stick with US spelling where possible, as this is standard for Computer Science.

Any UK spelling should ideally be corrected.

Copy link
Member

Choose a reason for hiding this comment

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

see #4940

emostov and others added 4 commits March 16, 2021 10:48
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@gui1117 gui1117 added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Mar 17, 2021
@gui1117 gui1117 merged commit cb033a4 into master Mar 17, 2021
@gui1117 gui1117 deleted the zeke-proxy-to-pallet-macro branch March 17, 2021 08:52
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
* Migrate pallet-proxy to pallet attribute macro

Part of paritytech#7882.

Converts the `Proxy` pallet to the new pallet attribute macro introduced in paritytech#6877.

[Upgrade guidelines used](https://substrate.dev/rustdocs/v3.0.0/frame_support/attr.pallet.html#upgrade-guidelines).

## ⚠️ Breaking Change  ⚠️ 

From [checking upgrade guidelines](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`: use of this pallet in `construct_runtime!` needs careful updating of the name in order to not break storage or to upgrade storage (moreover for instantiable pallet). If pallet is published, make sure to warn about this breaking change.

So users of the `Assets` 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 `Assets` pallet.

### Notes

There are some changes to the docs in metadata for the constants. The docs in the metadata for constants are now more complete.
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants