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

introduce new field in struct and update genesis file used explicitly in DEV #1248

Merged
merged 12 commits into from
May 24, 2024

Conversation

yuriechan
Copy link
Contributor

@yuriechan yuriechan commented May 21, 2024

Pull Request Summary

  • Allow disabling safe guard in DEV

Check list

  • added or updated unit tests
  • updated Astar official documentation
  • added OnRuntimeUpgrade hook for precompile revert code registration
  • added benchmarks & weights for any modified runtime logics.

To Reviewers

  • I wasn't entirely sure if this current logic change requires adding OnRuntimeUpgrade hook - If you think it is needed, please do let me know in the review comments.
  • No benchmarks were added as this only adds a if condition check within the genesis config - If you think it is needed, please do let me know in the review comments.

Fixes #1188

@yuriechan yuriechan marked this pull request as ready for review May 22, 2024 13:42
@Dinonard Dinonard added the runtime This PR/Issue is related to the topic “runtime”. label May 23, 2024
@@ -478,6 +479,7 @@ pub mod pallet {
pub tier_thresholds: Vec<TierThreshold>,
pub slots_per_tier: Vec<u16>,
pub _config: PhantomData<T>,
pub disable_safeguard: bool,
Copy link
Member

Choose a reason for hiding this comment

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

If the default value of Safeguard changes, does this approach still work?
E.g. Safeguard is set to false by default, and user sets disable_safeguard to false. What's the result?

Perhaps it's better to set the storage value directly.

Nitpick comment - PhantomData should be at the bottom of the fields declaration.

@ermalkaleci
Copy link
Contributor

This is a storage value. Why don't you use chopsticks or sudo to override storage. What is the use case of this?

@yuriechan yuriechan requested a review from Dinonard May 24, 2024 03:28
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

LGTM, just a comment about tests.

@@ -2962,3 +2962,135 @@ fn safeguard_on_by_default() {
assert!(Safeguard::<Test>::get());
});
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

It's ok to test all scenarios if you wish, of course, but the setup code bloats 99% of the added test code. Maybe optimize that, and reuse storage for creating multiple test externalities.

@Dinonard
Copy link
Member

@ermalkaleci it's related to this issue: #1188

@yuriechan yuriechan requested a review from Dinonard May 24, 2024 06:43
@Dinonard
Copy link
Member

@yuriechan please make sure to test cargo test --features try-runtime,runtime-benchmarks to ensure no tests are broken.

@yuriechan
Copy link
Contributor Author

@yuriechan please make sure to test cargo test --features try-runtime,runtime-benchmarks to ensure no tests are broken.

I ran the command locally, and it seems that all test cases are passing.

@Dinonard Dinonard merged commit 7a788a3 into AstarNetwork:master May 24, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dApp staking v3 - allow force call by default in dev chains
3 participants