Protection check for stakingV4 configs #4983
Conversation
mariusmihaic
changed the title
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## feat/staking-v4 #4983 +/- ##
==================================================
Coverage ? 70.90%
==================================================
Files ? 662
Lines ? 86491
Branches ? 0
==================================================
Hits ? 61323
Misses ? 20622
Partials ? 4546 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
|
||
| var log = logger.GetOrCreate("configChecker") | ||
|
|
||
| func SanityCheckEnableEpochsStakingV4(cfg *Configs) error { |
There was a problem hiding this comment.
add comment to public function
There was a problem hiding this comment.
Right, added it
config/configChecker.go
Outdated
| stakingV4StepsInExpectedOrder := (enableEpochsCfg.StakingV4Step1EnableEpoch == enableEpochsCfg.StakingV4Step2EnableEpoch-1) && | ||
| (enableEpochsCfg.StakingV4Step2EnableEpoch == enableEpochsCfg.StakingV4Step3EnableEpoch-1) | ||
| if !stakingV4StepsInExpectedOrder { | ||
| log.Warn("staking v4 enable epoch steps should be in cardinal order " + |
There was a problem hiding this comment.
Please return error. Why do you want something like this for playground ? You can have scripts which change to code here in case of playground, it is used in other places.
There was a problem hiding this comment.
yes let's return the error here as we won't be testing something other than will eventually be on the mainnet. Use fmt.Errorf here and on the other 3 locations to easily pinpoint the error and have a quick fix on the configs
There was a problem hiding this comment.
I was not sure if to consider these "soft errors" or not, so I've decided to let them as they are and fix it after review if necessary.
Thanks for suggestion, I've returned errors for all cases
config/configChecker.go
Outdated
|
|
||
| maxNodesConfigAdaptedForStakingV4 = true | ||
| if idx == 0 { | ||
| log.Warn(fmt.Sprintf("found config change in MaxNodesChangeEnableEpoch for StakingV4Step3EnableEpoch = %d, ", enableEpochsCfg.StakingV4Step3EnableEpoch) + |
There was a problem hiding this comment.
agree: return error & remove else
There was a problem hiding this comment.
Sure, thanks for suggestion, applied changes
config/configChecker.go
Outdated
|
|
||
| func checkMaxNodesChangedCorrectly(prevMaxNodesChange MaxNodesChangeConfig, currMaxNodesChange MaxNodesChangeConfig, numOfShards uint32) error { | ||
| if prevMaxNodesChange.NodesToShufflePerShard != currMaxNodesChange.NodesToShufflePerShard { | ||
| log.Warn("previous MaxNodesChangeEnableEpoch.NodesToShufflePerShard != MaxNodesChangeEnableEpoch.NodesToShufflePerShard" + |
There was a problem hiding this comment.
agree: return error.
There was a problem hiding this comment.
Sure, thanks for suggestion, applied changes
| log.Warn(fmt.Sprintf("found config change in MaxNodesChangeEnableEpoch for StakingV4Step3EnableEpoch = %d, ", enableEpochsCfg.StakingV4Step3EnableEpoch) + | ||
| "but no previous config change entry in MaxNodesChangeEnableEpoch, DO NOT use this config in production") | ||
| return fmt.Errorf("found config change in MaxNodesChangeEnableEpoch for StakingV4Step3EnableEpoch = %d, but %w ", | ||
| enableEpochsCfg.StakingV4Step3EnableEpoch, errNoMaxNodesConfigBeforeStakingV4) | ||
| } else { |
There was a problem hiding this comment.
could have dropped the else branch improving the code readability.
| @@ -2,6 +2,12 @@ package config | |||
|
|
|||
| import "errors" | |||
|
|
|||
| var errStakingV4StepsNotInOrder = errors.New("staking v4 enable epochs are not in ascending order; expected StakingV4Step1EnableEpoch < StakingV4Step2EnableEpoch < StakingV4Step3EnableEpoch") | |||
| var errStakingV4StepsNotInOrder = errors.New("staking v4 enable epoch steps should be in cardinal order(e.g.: StakingV4Step1EnableEpoch = 2, StakingV4Step2EnableEpoch = 3, StakingV4Step3EnableEpoch = 4)") | |||
There was a problem hiding this comment.
👍 good and comprehensive errors.
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?