-
Notifications
You must be signed in to change notification settings - Fork 22
feat: upgrade to Cosmos SDK v0.50.x #141
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
base: main
Are you sure you want to change the base?
Conversation
for _, m := range app.mm.Modules { | ||
if moduleWithName, ok := m.(module.HasName); ok { | ||
moduleName := moduleWithName.Name() | ||
if appModule, ok := moduleWithName.(appmodule.AppModule); ok { | ||
modules[moduleName] = appModule | ||
} | ||
} | ||
} |
Check warning
Code scanning / CodeQL
Iteration over map Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my first round of review for this PR, I left a couple of comments.
Awesome work Julien!
replace ( | ||
cosmossdk.io/x/feegrant => cosmossdk.io/x/feegrant v0.1.1 | ||
github.com/cosmos/cosmos-sdk => github.com/cosmos/cosmos-sdk v0.50.13-0.20250409215204-371eb7e0de11 | ||
github.com/cosmos/ibc-go/v10 => github.com/cosmos/ibc-go/v10 v10.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it required ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because they tagged ibc v10 with a Cosmos SDK v0.53 dependency... meaning go get bumps the SDK to v0.50.
Additionally they haven't tagged a new version of v0.50 while the latest client/v2 requires it (waiting for a v0.50.14 f.e.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR will have to wait for v0.50.14 to be released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, i think your suggestion of directly using our fork is best tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we'll have to wait for the release of v3 to merge this PR.
looks like most of the issues is due to wrong bech32 conversion. i'll add the address codec when it is needed instead of using Stringer |
Looking at the test failures, one is about command line execution for the gov module:
The underlying command is What I don't understand is why this field is empty when Any idea? |
Actually they are still populated in the basic manager: Line 220 in b20bdbb
The reason may indeed be because they are empty in the app module itself. And from v0.50, the commands are enriched using autocli, which uses the AppModule. |
Thanks, your last commit fixed that one. I'm looking at the others. |
Add missing PreBlocker declaration in baseapp
More gas required
An error in the genesis file was causing the feemarket price change test to fail, because the set of the max block gas was ignored. The default value of -1 was causing a max block gas being equal to maxUint64 in the feemarket module, making the target block gas also very high and so the required amount of gas to see an increase of the gas price was not possible. The error was caused by the usage of cometbft/libs/json to marshal the genesis in the e2e tests, bc since 0.50, the genesis file is read using the legacy encoding/json, and those libs does not treat integer the same way (cometbft/libs/json marshals integers into string). So the fix is to replace cometbft/libs/json with the legacy encoding/json in the e2e setup. Also had to increase the max block gas in the e2e tests from 50,000,000 to 60,000,000 because the previous max was reached in the `testFeemarketGasPriceChange` test.
All tests are passing now, lint fixes are addressed in #151 |
return true // break the iteration | ||
} | ||
validator, err := g.stakingKeeper.GetValidator(ctx, validatorAddr) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I'm looking at this version of the diff, maybe we should handle the case when err
is of type stakingtypes.ErrNoValidatorFound
and not panic-ing in that case, just skip.
keepers *keepers.AppKeepers, | ||
) upgradetypes.UpgradeHandler { | ||
return func(ctx context.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { | ||
return vm, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we supposed to call RunMigrations
here to trigger imported module migrations if any?
Bump Atom One to Cosmos SDK v0.50.x.
Some tests still need to be fixed. However, this is ready for a preliminary review.
Relevant documentation: https://github.com/cosmos/cosmos-sdk/blob/v0.50.13/UPGRADING.md