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

Implement #[pallet::composite_enum] #13722

Merged
merged 15 commits into from
Apr 4, 2023

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Mar 27, 2023

Partial paritytech/polkadot-sdk#236.

This PR adds a couple of new pallet parts: FreezeReason, HoldReason, LockId and SlashReason. The corresponding aggregate enums RuntimeFreezeReason, RuntimeHoldReason, RuntimeLockId and RuntimeSlashReason are also generated by construct_runtime. This is primarily used in pallets that wish to provide a reason for freezing funds, holding funds, locking funds and/or slashing funds in them.

#[pallet::composite_enum] is also added as an attribute to put on top of an enum declared in a pallet module. It now only accepts FreezeReason, HoldReason, LockId and SlashReason as identifiers for the enum; any other identifier gets rejected during pallet attribute parsing. If there are no #[derive] attributes marked for the enum, the code would then automatically add derives for the Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Decode, Encode, MaxEncodedLen, RuntimeDebug, TypeInfo traits.

@KiChjang KiChjang added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 27, 2023
@KiChjang KiChjang force-pushed the kckyeung/pallet-hold-reason branch from 127e0bb to dc5050b Compare March 27, 2023 15:21
Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

Would be cool to have a UI test ensuring that it complains if you try to provide an argument to #[hold_reason(..)] since none are supported

Copy link
Contributor

@xlc xlc left a comment

Choose a reason for hiding this comment

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

how generalize can we make this be?
what happen if we need a freeze reason? withdraw reason?
a Task for the omni scheduler?

@KiChjang
Copy link
Contributor Author

I could add slash reason/reserve reason/lock ID within this PR, but it's best to check and see whether this approach is good first.

And as Basti has mentioned in this comment, it makes sense to have separate attributes for different things.

@kianenigma
Copy link
Contributor

I could add slash reason/reserve reason/lock ID within this PR, but it's best to check and see whether this approach is good first.

If we want to add those now, it would require us to pretty much copy paste the existing diff with new names. Could it be that under the hood we have a general system for a pallet to define "composite_parts", but statically only allow it to be hold_reason for now?

As for Basti's comment, If the main reason is that typos are hard to detect, I think the can manually check it by having construct_runtime name the composite enums that are acceptable. Then, if any pallet is producing a composite enum that is not in the list, there will be a warning and it will be ignored.

I won't strongly push back on this either as the broad currency refactor is greatly more important, but I want to bring it up one more time before heading to review and merge.

@kianenigma kianenigma added B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. and removed B0-silent Changes should not be mentioned in any release notes labels Mar 28, 2023
@xlc
Copy link
Contributor

xlc commented Mar 28, 2023

Yeah this is a very useful pattern and I want to make sure this feature not only available to core pallets. For example, ORML may want to define some LockReason enum. We cannot do it without modify some low level FRAME code. It will be great if this macro can be implemented in such way that users can define custom enum type.

Something like #[pallet::aggregated(HoldReason)]

@KiChjang KiChjang changed the title Implement #[pallet::hold_reason] Implement #[pallet::composite_enum] Mar 30, 2023
@KiChjang
Copy link
Contributor Author

KiChjang commented Mar 30, 2023

With the latest commit, the code now supports an attribute called #[pallet::composite_enum], and will check whether the name of the enum is either of FreezeReason, HoldReason, LockId or SlashReason. I haven't still supported arbitrary identifiers yet, because of a few unresolved questions:

  1. As mentioned previously, hunting down for spelling mistakes is tough, and the current setup doesn't really attempt to solve it. Putting the name of the enum on the attribute doesn't really solve anything; in fact, it simply just moves the problem to the attribute.
  2. The way construct_runtime knows which enum to expand is by creating new pallet parts, each for the 4 new different aggregate enums. If we support arbitrary identifiers, we'll need to figure out an easy way to create new pallet parts that are named after the new identifiers, OR completely refrain from using pallet parts to know which aggregate enums to expand and think of a new way to gather that info from pallets.
  3. Reporting metadata information for custom aggregate enums. We can't exactly create new fields for the pallet metadata, but I guess we can create a hashmap for it. It's not 100% trivial but I think this is not a big challenge as compared with the other issues mentioned.

However, even with the problems listed above, the basic infrastructure of supporting arbitrary aggregate enum identifiers are in place, and it should take less effort to parse and expand them when we figure out the best way to resolve the aforementioned issues.

Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

re-reviewed for composite stuff, 👍🏻

// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
Copy link
Contributor

@sam0x17 sam0x17 Mar 31, 2023

Choose a reason for hiding this comment

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

This is great for our purposes. I wonder, though, how easy would it be for an outside user to add a custom composite for, say, their own parachain? Would they have to patch this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They'd actually have to do a little more stuff:

  1. Modify the parser in construct_runtime so that it knows how to scan for the existence of a new aggregate enum via a new pallet part.
  2. Instruct construct_runtime to check for the new pallet part and expanding it into the aggregate enum, complete with conversion functions as well.
  3. Scan for all pallets during the expansion mentioned in 2 for any other pallets that contain the new pallet part, and aggregate all of them into an aggregated enum.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Is there a way to reduce code duplication?
Maybe use a decl macro?

@KiChjang
Copy link
Contributor Author

KiChjang commented Apr 4, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 057f2af into master Apr 4, 2023
@paritytech-processbot paritytech-processbot bot deleted the kckyeung/pallet-hold-reason branch April 4, 2023 12:28
@KiChjang KiChjang changed the title Implement #[pallet::composite_enum] Implement #[pallet::composable_enum] Apr 5, 2023
@KiChjang KiChjang changed the title Implement #[pallet::composable_enum] Implement #[pallet::composite_enum] Apr 5, 2023
gpestana pushed a commit that referenced this pull request Apr 23, 2023
* Implement #[pallet::hold_reason]

* Appease clippy

* cargo fmt

* Update test expectations

* Update test expectations

* Support composite_enum attribute instead

* Update test expectations

* Change hold_reason to composite_enum

* Add UI test for unsupported identifier when using composite_enum

* Fix comment

* Add documentation for pallet::composable_enum

* More docs

* cargo fmt
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/april-updates-for-substrate-and-polkadot-devs/2764/1

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Implement #[pallet::hold_reason]

* Appease clippy

* cargo fmt

* Update test expectations

* Update test expectations

* Support composite_enum attribute instead

* Update test expectations

* Change hold_reason to composite_enum

* Add UI test for unsupported identifier when using composite_enum

* Fix comment

* Add documentation for pallet::composable_enum

* More docs

* cargo fmt
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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants