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

feat: Cosmos SDK v0.46.0 #914

Merged
merged 32 commits into from
Aug 10, 2022
Merged

feat: Cosmos SDK v0.46.0 #914

merged 32 commits into from
Aug 10, 2022

Conversation

aljo242
Copy link
Contributor

@aljo242 aljo242 commented Jul 27, 2022

Updates the Cosmos SDK to v0.46.0. Please review this PR after the other onces I have submitted are completed.

This PR builds off of those, and will be much more manageable to review if the other PRs are already merged.

This PR introduces temporary replace clauses in our go.mod for the following packages:

  • github.com/cosmos/ibc-go/v5 => github.com/aljo242/ibc-go/v5 v5.1.0
  • github.com/tendermint/fundraising => github.com/aljo242/fundraising v0.1.2-0.20220726203752-c39abf16b02d

Once the respective repos are updated, we can remove these lines. I have an open PR for x/fundraising and ibc-go is near completion.

Part of #909

Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Include tests
  • Respect code style and lint
  • Update documentation (*.md) (if needed)

@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #914 (bfdf277) into develop (e8a2f9c) will decrease coverage by 0.00%.
The diff coverage is 8.16%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #914      +/-   ##
===========================================
- Coverage    10.56%   10.55%   -0.01%     
===========================================
  Files          327      327              
  Lines        75191    75202      +11     
===========================================
- Hits          7944     7941       -3     
- Misses       67059    67072      +13     
- Partials       188      189       +1     
Impacted Files Coverage Δ
pkg/types/consensus_state.go 100.00% <ø> (ø)
pkg/types/monitoring.pb.go 0.79% <ø> (ø)
pkg/types/validator_set.go 92.30% <ø> (ø)
x/campaign/types/events.pb.go 0.55% <ø> (ø)
x/campaign/types/mainnet_account.pb.go 0.75% <ø> (ø)
x/campaign/types/params.pb.go 0.88% <ø> (ø)
x/campaign/types/query.pb.go 0.66% <ø> (ø)
x/claim/keeper/mission_delegation_hooks.go 0.00% <0.00%> (ø)
x/claim/types/claim_record.pb.go 0.83% <ø> (ø)
x/claim/types/mission.pb.go 1.01% <ø> (ø)
... and 35 more

@lumtis
Copy link
Contributor

lumtis commented Jul 27, 2022

What difference with #906?

Is it with the final imports for IBC and fundraising?

@aljo242
Copy link
Contributor Author

aljo242 commented Jul 27, 2022

What difference with #906?

That PR included all of the changes that I made in the previous PRs to prepare for v0.46.0. It would have been a mess to review. I have closed #906 now.

This PR also fixes tests that were broken by the upgrade.

Is it with the final imports for IBC and fundraising?

These are the final imports for a temporary solution. My update to fundraising works, but I'm just waiting for them to approve. We will still need to make a PR later that removes the replace lines in the go.mod once the dependencies are fixed.

Copy link
Contributor

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Great work

I put some initial comments. I need to do some experimentation and check further the IBC part

cmd/genaccounts.go Outdated Show resolved Hide resolved
x/claim/keeper/mission_delegation_hooks.go Outdated Show resolved Hide resolved
x/mint/keeper/integration_test.go Outdated Show resolved Hide resolved
@lumtis
Copy link
Contributor

lumtis commented Jul 28, 2022

Supporting the proto changes will be part of ignite/cli#2156

@aljo242
Copy link
Contributor Author

aljo242 commented Jul 28, 2022

Supporting the proto changes will be part of ignite/cli#2156

Sounds good. If that's the only outstanding issue, are we good to try to get this approved?

@lumtis
Copy link
Contributor

lumtis commented Jul 28, 2022

Supporting the proto changes will be part of ignite/cli#2156

Sounds good. If that's the only outstanding issue, are we good to try to get this approved?

I think this is better if we can merge it once we can run it with ignite c serve and use ignite c build.

If we need the new version for spn dependency in ignite, maybe we can temporarily update spn dep in ignite to this branch?

@aljo242
Copy link
Contributor Author

aljo242 commented Jul 28, 2022

Supporting the proto changes will be part of ignite/cli#2156

Sounds good. If that's the only outstanding issue, are we good to try to get this approved?

I think this is better if we can merge it once we can run it with ignite c serve and use ignite c build.

If we need the new version for spn dependency in ignite, maybe we can temporarily update spn dep in ignite to this branch?

I think it would be better the other way around. If this upgrade fully works, then the only remaining issue is with ignite. So it makes more sense to upgrade spn, then fix ignite after.

After all, we have now modified our dependencies so that:

  • ignite imports spn
  • spn no longer imports ignite

@lumtis
Copy link
Contributor

lumtis commented Jul 28, 2022

What does it change if we do go get github.com/tendermint/spn@6bff6689d24b74fd3d6c2786db475e5250708ef9 on Ignite?

This will remove the circular dep, then we just need to update again spn dep on Ignite once ignite/cli#2673 is finished.
I can't test everything on the changes without using Ignite

@aljo242
Copy link
Contributor Author

aljo242 commented Jul 28, 2022

What does it change if we do go get github.com/tendermint/spn@6bff6689d24b74fd3d6c2786db475e5250708ef9 on Ignite?

This will remove the circular dep, then we just need to update again spn dep on Ignite once ignite/cli#2673 is finished. I can't test everything on the changes without using Ignite

Good point - I was thinking this would introduce a fork in the go.mod but my branch is on here. I'll go ahead and do that.

@ilgooz
Copy link
Member

ilgooz commented Jul 29, 2022

There could be one problem with that because that commit hash wouldn't be guaranteed to be kept forever on Github.

The reason for this, once we merge this PR, all the commits here will be squashed into a single commit and the commit that has mentioned will not have a reference on develop PR and also in the base branch of this PR since the branch will be deleted after merging.

I suggest merging this PR first and using the commit hash added to the develop branch instead.

@ilgooz
Copy link
Member

ilgooz commented Jul 29, 2022

But if we plan to change the commit hash again after we finish work then np.

@aljo242
Copy link
Contributor Author

aljo242 commented Jul 29, 2022

Ah - I see what you mean. I just need some approvals and we can merge

@lumtis
Copy link
Contributor

lumtis commented Jul 29, 2022

There could be one problem with that because that commit hash wouldn't be guaranteed to be kept forever on Github.

The reason for this, once we merge this PR, all the commits here will be squashed into a single commit and the commit that has mentioned will not have a reference on develop PR and also in the base branch of this PR since the branch will be deleted after merging.

I suggest merging this PR first and using the commit hash added to the develop branch instead.

It will be temporary, the time the serve command works CLI side for 0.46 we don't need to wait for the full 0.46 PR to be merged CLI side

@lumtis
Copy link
Contributor

lumtis commented Jul 29, 2022

And as soon as this PR is merged we can change it

@ilgooz
Copy link
Member

ilgooz commented Jul 29, 2022

In that case there should be no problem. 👍

Alex Johnson and others added 2 commits July 31, 2022 22:31
* add .clang-format

* setup buf

* add scripts and update Makefile

* make proto-format

* make proto-gen
@aljo242 aljo242 self-assigned this Jul 31, 2022
* cosmos proto Int

* remaining cosmos scalar annotations

* proto format
@lumtis lumtis linked an issue Aug 9, 2022 that may be closed by this pull request
5 tasks
Copy link
Contributor

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Tested all the features

image

@lumtis lumtis merged commit 9366849 into develop Aug 10, 2022
@lumtis lumtis deleted the feat/v0.46.0 branch August 10, 2022 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare for Cosmos SDK v0.46.x
4 participants