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

deps: upgrade of SDK to 0.46 and tendermint to 0.35 #1205

Merged

Conversation

crodriguezvega
Copy link
Contributor

Description

Upgrade of SDK to 0.46-alpha3 and Tendermint to 0.35.

This PR is based on the excellent work done by @faddat on his PR. This PR is basically a subset of all the changes included in #949 (i.e. it adds only the changes strictly necessary for the upgrade of SDK and Tendermint) to narrow the scope and make the review easier.

TODO:

  • Add changelog entry.
  • Add migration docs.

closes: #909 #539


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@fedekunze
Copy link
Contributor

Will there not be a v0.45 compatible version?

@crodriguezvega
Copy link
Contributor Author

crodriguezvega commented Mar 30, 2022

Will there not be a v0.45 compatible version?

You mean SDK 0.45 with Tendermint 0.35? We could look into it...

@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2022

Codecov Report

Merging #1205 (44a057b) into carlos/upgrade-sdk-0.46-tendermint-0.35 (f4a5dcb) will decrease coverage by 0.34%.
The diff coverage is 81.11%.

Impacted file tree graph

@@                             Coverage Diff                             @@
##           carlos/upgrade-sdk-0.46-tendermint-0.35    #1205      +/-   ##
===========================================================================
- Coverage                                    79.82%   79.47%   -0.35%     
===========================================================================
  Files                                          151      151              
  Lines                                        10907    10945      +38     
===========================================================================
- Hits                                          8706     8699       -7     
- Misses                                        1776     1819      +43     
- Partials                                       425      427       +2     
Impacted Files Coverage Δ
...27-interchain-accounts/controller/keeper/keeper.go 96.46% <ø> (ø)
.../apps/27-interchain-accounts/host/keeper/keeper.go 83.49% <ø> (ø)
modules/apps/transfer/keeper/keeper.go 94.36% <ø> (ø)
modules/core/02-client/proposal_handler.go 77.77% <ø> (ø)
modules/core/02-client/types/codec.go 77.77% <ø> (ø)
modules/core/02-client/types/proposal.go 86.48% <ø> (ø)
modules/core/04-channel/keeper/keeper.go 84.41% <ø> (ø)
modules/core/keeper/keeper.go 85.71% <ø> (ø)
...s/apps/27-interchain-accounts/host/keeper/relay.go 75.00% <70.00%> (-3.05%) ⬇️
modules/core/keeper/sdk_middleware.go 81.33% <81.33%> (ø)
... and 13 more

testing/sdk_test.go Show resolved Hide resolved
go.mod Outdated
@@ -4,119 +4,142 @@ module github.com/cosmos/ibc-go/v3

replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1

replace github.com/zondax/hid => github.com/zondax/hid v0.9.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add this to make the branch build on my Mac running Big Sur. I will take it off before merging.

testing/utils.go Outdated Show resolved Hide resolved
@crodriguezvega
Copy link
Contributor Author

Will there not be a v0.45 compatible version?

Just did a quick test of trying to upgrade to Tendermint 0.35 with SDK 0.45.1 and I get errors like this:

github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts imports
        github.com/cosmos/cosmos-sdk/types imports
        github.com/tendermint/tendermint/rpc/core/types: module github.com/tendermint/tendermint@latest found (v0.35.2, replaced by github.com/tendermint/tendermint@v0.35.2), but does not contain package github.com/tendermint/tendermint/rpc/core/types

So not sure if it will be possible...

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Nice start! Left several suggestions after briefly diving through a lot of the changes. I'm wondering if we should split up this update. For example, the IBC ante handler changes are technically not required to update to v0.46 (simply remove them), then we can have a followup pr which properly fixes the code. The old structure of the ante handler code doesn't make sense to just copy over given the new API

modules/core/02-client/proposal_handler.go Show resolved Hide resolved
testing/simapp/app.go Outdated Show resolved Hide resolved
modules/light-clients/07-tendermint/types/update_test.go Outdated Show resolved Hide resolved
modules/core/middleware/middleware.go Outdated Show resolved Hide resolved
modules/core/middleware/middleware.go Outdated Show resolved Hide resolved
modules/core/middleware/middleware.go Outdated Show resolved Hide resolved
modules/core/middleware/middleware.go Outdated Show resolved Hide resolved
modules/core/middleware/middleware.go Outdated Show resolved Hide resolved
@zavgorodnii
Copy link

zavgorodnii commented Apr 4, 2022

hey! testing/simapp/params/amino.go imports "github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx", which is not present in cosmos-sdk v0.46.0-alpha3.0.20220325134903-a69764f9f01b. is this intentional?

@amaury1093
Copy link

amaury1093 commented Apr 4, 2022

"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"

This has been moved to "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx".

Edit: also, beta1 has been released, which should be more API stable.

@zavgorodnii
Copy link

"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"

This has been moved to "github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"

right! just saw a reference to the old path here:

"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"

@faddat
Copy link
Contributor

faddat commented Apr 15, 2022

hey, I'm really happy to see the progress here and also to be getting help on discerning exactly which changes were needed for v46 compatiblity.

For our upgrade, the primary authors were @catShaark and @nguyenvuong1122000.

In the coming week, I'll be updating the testnet code for https://github.com/notional-labs/craft to the beta1 cosmos-sdk (or possibly release/v0.46.x) and we should have some more input on the work here.

@crodriguezvega
Copy link
Contributor Author

hey! testing/simapp/params/amino.go imports "github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx", which is not present in cosmos-sdk v0.46.0-alpha3.0.20220325134903-a69764f9f01b. is this intentional?

Thanks for pointing this out! I just pushed a fix.

colin-axner and others added 2 commits April 25, 2022 15:11
…SDK v0.46 (#1288)

* refactor: simplify ibc redundancy check used as sdk middleware

Instead of having the function checkRedundancy check if the call is on CheckTx or SimulateTx, only call the function on CheckTx or SimulateTx

* add godoc
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Looking good! I still need to rereview the sdk middleware code and I am still concerned about updating the ABCIResponsesResultHash, implementing the function ourself defeats the purpose of the test added to indicate when the expected downstream behaviour changes

I would prefer updates to newer release (ie beta3) to be done in a separate pr since this one already has many changes. Ideally we merge this as soon as the ABCIResponsesResultHash is resolved

testing/mock/privval_test.go Outdated Show resolved Hide resolved
testing/mock/privval_test.go Outdated Show resolved Hide resolved
testing/mock/privval_test.go Outdated Show resolved Hide resolved
testing/mock/privval_test.go Outdated Show resolved Hide resolved
testing/mock/privval_test.go Outdated Show resolved Hide resolved
testing/simapp/test_helpers.go Outdated Show resolved Hide resolved
@@ -390,7 +392,7 @@ func writeFile(name string, dir string, contents []byte) error {
return err
}

err = tmos.WriteFile(file, contents, 0644)
err = ioutil.WriteFile(file, contents, 0644)
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: running this branch locally deletes a local genesis file which causes other branches (without the SDK update) cmd_test.go tests to break. I think this line is the culprit. I guess there isn't much to do other than just get this merged asap :)

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

utACK pending the merge of #1349 (approval only applies to merging into the 0.46 feature branch, not main)

go.mod Outdated Show resolved Hide resolved
okwme and others added 11 commits May 15, 2022 16:28
…age responses (#1349)

* begin refactoring ack_test

* fix test due to delayed block execution

* refactor: switch ics27 response creation to use new SDK version, update tests

* fix import
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
@crodriguezvega crodriguezvega merged commit c4ce2b9 into carlos/upgrade-sdk-0.46-tendermint-0.35 May 17, 2022
@crodriguezvega crodriguezvega deleted the carlos/upgrade-pr-branch branch May 17, 2022 19:00
@crodriguezvega
Copy link
Contributor Author

Another PR will be opened when there's an SDK 0.46 RC pre-release.

@colin-axner colin-axner mentioned this pull request May 19, 2022
9 tasks
@faddat faddat mentioned this pull request Jul 20, 2022
9 tasks
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request Nov 6, 2023
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

### Summary by CodeRabbit

- **Documentation**: Updated the file structure for better organization.
Introduced a new specification file "Block Validity" for enhanced
clarity on block and header validation in our blockchain system.
- **New Feature**: Added detailed descriptions and requirements for
verifying blocks and headers, including basic validation and
verification against the previous block. This will improve the
reliability and security of our blockchain system.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Connor O'Hara <connor@switchboard.xyz>
Co-authored-by: nashqueue <99758629+nashqueue@users.noreply.github.com>
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.

10 participants