Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make composite_enum generalized #190

Open
xlc opened this issue Apr 4, 2023 · 5 comments
Open

Make composite_enum generalized #190

xlc opened this issue Apr 4, 2023 · 5 comments
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D2-substantial Can be fixed by an experienced coder with a working knowledge of the codebase. I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@xlc
Copy link
Contributor

xlc commented Apr 4, 2023

Followup of paritytech/substrate#13722

Right now it now only accepts FreezeReason, HoldReason, LockId and SlashReason as identifiers for the enum.

Should generalize the implementation to make it possible to extend identifier types.

@bkchr
Copy link
Member

bkchr commented Apr 5, 2023

IMO calling it composite_enum and then matching on the name was mistake. There should be an attribute for every type we want to combine in the runtime. Random identifiers are not a great idea, because people otherwise will do simple spelling mistakes and debug for hours why their pallet enum isn't part of the composite in the runtime. However, we could create some kind of composite_enum macro that can generate a custom attribute that will then composite in the runtime.

@gilescope
Copy link
Contributor

composite_enum without something like explicit pallet_index (i.e. explicit discriminators) seems like a footgun to me.

@bkchr
Copy link
Member

bkchr commented Apr 18, 2023

Not sure what you are talking about, on the runtime side they reuse the pallet index like all the other things.

@ggwpez
Copy link
Member

ggwpez commented Apr 20, 2023

IMO calling it composite_enum and then matching on the name was mistake. There should be an attribute for every type we want to combine in the runtime. Random identifiers are not a great idea, because people otherwise will do simple spelling mistakes and debug for hours why their pallet enum isn't part of the composite in the runtime.

Do you mean the current master is problematic? We do have an UI test that checks for typo reports:
https://github.com/paritytech/substrate/blob/057f2afd686dceb48c08d8a31f35b338bb5992d3/frame/support/test/tests/pallet_ui/composite_enum_unsupported_identifier.stderr#L1

@bkchr
Copy link
Member

bkchr commented Apr 20, 2023

Do you mean the current master is problematic?

Yes. Yeah, the UI test is fine and a good idea. However, discoverability of this is hard IMO. We also have for all the other things separate macros.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed J0-enhancement labels Aug 25, 2023
@ggwpez ggwpez added D2-substantial Can be fixed by an experienced coder with a working knowledge of the codebase. I4-refactor Code needs refactoring. C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. and removed I5-enhancement An additional feature request. labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D2-substantial Can be fixed by an experienced coder with a working knowledge of the codebase. I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

No branches or pull requests

6 participants