chore: move v2 consts to v2 directory #3040
Conversation
ninabarbakadze
requested review from
evan-forbes,
rootulp and
cmwaters
as code owners
|
Warning Rate Limit Exceeded@ninabarbakadze has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe codebase has undergone a refactoring process that affects how the global minimum gas price is handled. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
app/ante/fee_checker.go
Outdated
| @@ -29,7 +30,7 @@ func CheckTxFeeWithGlobalMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, in | |||
| // global minimum fee only applies to app versions greater than one | |||
| if ctx.BlockHeader().Version.App > v1.Version { | |||
| // convert the global minimum gas price to a big.Int | |||
| globalMinGasPrice, err := sdk.NewDecFromStr(fmt.Sprintf("%f", appconsts.GlobalMinGasPrice)) | |||
| globalMinGasPrice, err := sdk.NewDecFromStr(fmt.Sprintf("%f", v2.GlobalMinGasPrice)) | |||
There was a problem hiding this comment.
[not blocking][question] if the constant is now app version specific, should we define a getter like this in versioned_consts.go:
func GlobalMinGasPrice(version uint64) (float64, error) {
switch version {
case v2.Version:
return v2.GlobalMinGasPrice, nil
default:
return 0, fmt.Errorf("global min gas price not defined for version %d", version)
}
}and then use here
// global minimum fee only applies to app versions greater than one
if ctx.BlockHeader().Version.App > v1.Version {
gmgp, err := appconsts.GlobalMinGasPrice(ctx.BlockHeader().Version.App)
if err != nil {
return nil, 0, errors.Wrap(err, "invalid GlobalMinGasPrice")
}
// convert the global minimum gas price to a big.Int
globalMinGasPrice, err := sdk.NewDecFromStr(fmt.Sprintf("%f", gmgp))
if err != nil {
return nil, 0, errors.Wrap(err, "invalid GlobalMinGasPrice")
}There was a problem hiding this comment.
Feel free to rename variables, add godocs, etc. Mostly for illustrative purposes ^
There was a problem hiding this comment.
thanks for the review and suggestions! I completely missed the versioned_consts.go file 🤦🏻♀️ updated now
There was a problem hiding this comment.
No problemo. I wonder if the appconsts in v1 and v2 should actually be un-exported so that consumers are forced to use the getters rather than hard-coding a particular versioned constant.
I know Callum found a few instances where code was relying on a default or a particular versioned constant rather than querying for a versioned constant based on app version. Can discuss more outside this PR if maintainers think we should pursue this idea.
There was a problem hiding this comment.
yes, I think making them unexported makes sense. we should probably go through the app and replace all hardcoded values with getters+constants based on app version as you suggested. cc @cmwaters @evan-forbes
| // GlobalMinGasPrice is used in the processProposal to ensure | ||
| // that all transactions have a gas price greater than or equal to this value. | ||
| func GlobalMinGasPrice(version uint64) (float64, error) { | ||
| switch version { | ||
| case v2.Version: | ||
| return v2.GlobalMinGasPrice, nil | ||
| default: | ||
| return 0, fmt.Errorf("global min gas price not defined for version %d", version) | ||
| } | ||
| } |
There was a problem hiding this comment.
[optional] add tests in versioned_consts_test.go. The unit test may look crazy simple for now but I expect it may be more helpful down the line when we introduce new app versions.
There was a problem hiding this comment.
sanaz already approved the changes but I'm almost done and will push it in another pr
Co-authored-by: Rootul P <[email protected]>
Overview
moving GlobalMinGasPrice constant under v2
Fixes - #3035