-
Notifications
You must be signed in to change notification settings - Fork 112
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
feat: cosmos v0.50 upgrade #3357
Conversation
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 198 files out of 282 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3357 +/- ##
===========================================
- Coverage 63.03% 63.01% -0.02%
===========================================
Files 436 436
Lines 30660 30683 +23
===========================================
+ Hits 19325 19336 +11
- Misses 10525 10535 +10
- Partials 810 812 +2
|
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.
I was initially expected sdk.Context
to be replaced by context.Context
everywhere but this is finally not needed?
it seems to be replaced in cosmos-sdk modules, but when code interacts with it (eg. in hooks) it can unwrap sdk.Ctx from context.Context we can also refactor our modules i guess, but seems out of scope |
Ok, let's keep it less complex |
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.
Nothing looks too crazy in this PR. I think we should move to get it merged as fast as possible to avoid conflicts.
We do need to get the ethermint integration tests running before the next release.
Agree on this |
Description
Upgrades cosmos to v50.
Ethermint side: zeta-chain/ethermint#179
Closes:
Note: mockery is broken for authority module as it can't mock inline interface for sdk.Msg
Also need to check if changes in simulation tests are working.
Some parts like server/start.go should be reviewed in more details to ensure something is not missing.
How Has This Been Tested?