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

upgrade to cosmos sdk v0.45 #782

Merged
merged 5 commits into from Jan 28, 2022
Merged

upgrade to cosmos sdk v0.45 #782

merged 5 commits into from Jan 28, 2022

Conversation

sunnya97
Copy link
Collaborator

@sunnya97 sunnya97 commented Jan 24, 2022

Closes: #676

Update go.mod once osmosis-labs/cosmos-sdk#59 is approved

Description


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2022

Codecov Report

Merging #782 (e19d617) into main (6e7d32b) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #782      +/-   ##
==========================================
+ Coverage   19.96%   19.98%   +0.02%     
==========================================
  Files         189      189              
  Lines       24542    24547       +5     
==========================================
+ Hits         4899     4905       +6     
+ Misses      18788    18787       -1     
  Partials      855      855              
Impacted Files Coverage Δ
x/incentives/module.go 54.54% <0.00%> (+2.27%) ⬆️
x/txfees/keeper/feedecorator.go 75.00% <0.00%> (+2.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e7d32b...e19d617. Read the comment docs.

go.mod Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@ go 1.17
require (
github.com/cosmos/cosmos-sdk v0.44.5
github.com/cosmos/go-bip39 v1.0.0
github.com/cosmos/iavl v0.17.2
github.com/cosmos/iavl v0.17.3
Copy link
Member

Choose a reason for hiding this comment

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

The patch fixed an edge case in an incorrect manner, that puts another blocking mutex contention in every query. I guess we can just fix later in our later improvements, since its not a big impact until #604 anyway.

Comment on lines +370 to +378
capabilitytypes.ModuleName, minttypes.ModuleName,
poolincentivestypes.ModuleName,
distrtypes.ModuleName, slashingtypes.ModuleName,
evidencetypes.ModuleName, stakingtypes.ModuleName, ibchost.ModuleName, ibctransfertypes.ModuleName,
authtypes.ModuleName, banktypes.ModuleName, govtypes.ModuleName, crisistypes.ModuleName, genutiltypes.ModuleName,
authz.ModuleName,
paramstypes.ModuleName, vestingtypes.ModuleName,
gammtypes.ModuleName, incentivestypes.ModuleName, lockuptypes.ModuleName, claimtypes.ModuleName,
poolincentivestypes.ModuleName, superfluidtypes.ModuleName, bech32ibctypes.ModuleName, txfeestypes.ModuleName,
Copy link
Member

Choose a reason for hiding this comment

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

Was it a bug before that we didn't have everything listed, or did not listing them here mean their begin block could occur in any order after everything ordered was done?

Ditto question for end blocker

Copy link
Contributor

Choose a reason for hiding this comment

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

This was a new requirement with 0.45 to avoid things being unlisted.

Before you only listed the ones being run. Now you must list all or it will panic on startup

Copy link
Member

Choose a reason for hiding this comment

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

oh cool, thanks for the explanation! Didn't know that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, idk how I feel about this. Seems to just make it more messy imo...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this whole explicit singly ordered list seems like an unscalable and error prone method.

I hope with the new app wiring work, theres a way for modules to declare what they need to come after, and then app.go linearizes the init genesis' itself. I should ask whats the plan there

app/app.go Outdated Show resolved Hide resolved
@@ -388,7 +402,9 @@ func NewOsmosisApp(
ibchost.ModuleName,
gammtypes.ModuleName,
txfeestypes.ModuleName,
genutiltypes.ModuleName, evidencetypes.ModuleName, ibctransfertypes.ModuleName,
genutiltypes.ModuleName, evidencetypes.ModuleName, authz.ModuleName,
paramstypes.ModuleName, upgradetypes.ModuleName, vestingtypes.ModuleName,
Copy link
Member

Choose a reason for hiding this comment

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

Are these three all new, or were there bugs before with them not being present?

(The three being paramstypes.ModuleName, upgradetypes.ModuleName, vestingtypes.ModuleName)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as #782 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

one of the things that I did in the annotation branch is "flatten" the *.ModuleName to make it easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@faddat wdym by flatten?

@faddat
Copy link
Member

faddat commented Jan 27, 2022

Hi!

When I started building Craft Economy a few weeks ago, I explored the latest commits from the cosmos-sdk.

I've been working with some newer devs on it, and ended up doing a pretty detailed annotation of app.go

I just did the same for Osmosis and I'm gonna send it as a PR to the sunny/sdk-v0.45 branch

:)

@ValarDragon ValarDragon merged commit 32d5953 into main Jan 28, 2022
@ValarDragon ValarDragon deleted the sunny/sdk-v0.45 branch January 28, 2022 05:33
@github-actions github-actions bot mentioned this pull request Jan 15, 2024
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Update our SDK dependency to SDK v0.45.x
5 participants