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

[FRAME] Enum UpgradeCheckSelect should be a bit flag #2015

Open
ggwpez opened this issue Oct 24, 2023 · 2 comments
Open

[FRAME] Enum UpgradeCheckSelect should be a bit flag #2015

ggwpez opened this issue Oct 24, 2023 · 2 comments
Assignees
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@ggwpez
Copy link
Member

ggwpez commented Oct 24, 2023

This enum could also get a new variant for the new DecodeState check. Then it becomes impractical to not use a bit flag:

pub enum UpgradeCheckSelect {
	None,
	All,
	PreAndPost,
	TryState,
}

(Easy issue good for newcomers.)

@ggwpez ggwpez added T1-FRAME This PR/Issue is related to core FRAME, the framework. C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. labels Oct 24, 2023
@ggwpez ggwpez added D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I4-refactor Code needs refactoring. labels Feb 5, 2024
@chungquantin
Copy link
Contributor

@ggwpez I have added this PR to resolve the issue. However, I think this PR will need a further consideration: #3719

However, I have a question: What is the purpose of implementing TryDecodeEntireStorage on UpgradeCheckSelect? As my understanding, the trait is used for Decode the entire data under the given storage type (from TryDecodeEntireStorage doc). Hence, I simply decode and accumulate all enum field values using information provided. If you could help me to provide more context, it would be clearer!

@ggwpez
Copy link
Member Author

ggwpez commented Mar 17, 2024

Thanks for picking this up @chungquantin!

However, I have a question: What is the purpose of implementing TryDecodeEntireStorage on UpgradeCheckSelect?

This is not what i meant - TryDecodeEntireStorage should not be implemented on that, but instead used to select whether that check is enabled in this function:

pub fn try_runtime_upgrade(checks: UpgradeCheckSelect) -> Result<Weight, TryRuntimeError> {

You see that it uses checks.any() here instead of something like checks.should_check_state():

We want to use a bitflag for UpgradeCheckSelect here to selectively enable/disable specific checks, instead of an all-or-nothing approach (how it currently is).

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. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. 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

2 participants