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: upgrade interchain stack to ibc5 #8791

Merged
merged 15 commits into from
Feb 6, 2024
Merged

feat: upgrade interchain stack to ibc5 #8791

merged 15 commits into from
Feb 6, 2024

Conversation

JimLarson
Copy link
Contributor

@JimLarson JimLarson commented Jan 23, 2024

refs: #8224
closes: #8719
closes: #8783

Description

Upgrade ibc-go to v5.3.2 (from v4.5.1) , with associated upgrades of cosmos-sdk (0.45.16 to 0.46.16) and cometbft (0.34.27 to 0.34.30).

Security Considerations

Careful code review is required.

Scaling Considerations

Performance testing is required.

Documentation Considerations

N/A

Testing Considerations

More testing notes will be added.

Upgrade Considerations

No major upgrade-time work foreseen.

@JimLarson JimLarson added enhancement New feature or request agoric-cosmos force:integration Force integration tests to run on PR labels Jan 23, 2024
@JimLarson JimLarson self-assigned this Jan 23, 2024
@JimLarson JimLarson changed the title 8224 ibc5 feat: upgrade interchain stack to ibc5 Jan 23, 2024
@JimLarson
Copy link
Contributor Author

Made issue #8803 for some post-upgrade changes to remove use of deprecated APIs. We don't need to do them now, but we should do them before a migration where they go away.

@JimLarson
Copy link
Contributor Author

Reviewers: please check the cosmos-sdk and ibc-go migration guides as part of your review.

@JimLarson
Copy link
Contributor Author

The remaining failing tests are expected failures. Protobuf / breakage because of protobuf compilation target changes, and golangci-lint because of use of deprecated APIs (see #8803 for later cleanup).

@JimLarson
Copy link
Contributor Author

The cosmos-sdk module tag will be bumped to the final version once agoric-labs/cosmos-sdk#347 lands.

@JimLarson JimLarson marked this pull request as ready for review January 31, 2024 22:27
@JimLarson
Copy link
Contributor Author

Adding @mhofman as an optional extra reviewer.

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Everything looks kosher! At least, I didn't find anything suspicious that a few minutes of search didn't clarify for me.

// replace broken goleveldb.
github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7

// use cometft
Copy link
Member

Choose a reason for hiding this comment

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

Just to prove I'm actually paying attention:

Suggested change
// use cometft
// use cometbft

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. :-)

@JimLarson JimLarson added the automerge:rebase Automatically rebase updates, then merge label Feb 6, 2024
@mergify mergify bot merged commit 64cb69f into master Feb 6, 2024
72 of 74 checks passed
@mergify mergify bot deleted the 8224-ibc5 branch February 6, 2024 09:31
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -560,6 +567,7 @@ func NewAgoricApp(
appCodec, keys[icahosttypes.StoreKey],
app.GetSubspace(icahosttypes.SubModuleName),
app.IBCKeeper.ChannelKeeper,
app.IBCKeeper.ChannelKeeper,
Copy link
Member

Choose a reason for hiding this comment

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

Why the duplicated keeper here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a new ICS4Wrapper parameter, for which the channel keeper is the correct default.

github.com/sasha-s/go-deadlock v0.3.1 // indirect
github.com/spf13/afero v1.9.2 // indirect
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/subosito/gotenv v1.4.1 // indirect
github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirect
github.com/syndtr/goleveldb v1.0.1-0.20220721030215-126854af5e6d // indirect
Copy link
Member

Choose a reason for hiding this comment

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

Have we figured out what causes this one to be upgraded if cosmos-sdk didn't bump it?

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 think it's some sub-dependency and/or the preference of selecting the latest compatible release. Cosmos-sdk has to override this version with a replace as well.


// For testing against a local cosmos-sdk or tendermint
// replace github.com/cosmos/cosmos-sdk => ../../../forks/cosmos-sdk
// replace github.com/tendermint/tendermint => ../../../forks/tendermint
// github.com/cosmos/cosmos-sdk => ../../../forks/cosmos-sdk
Copy link
Member

Choose a reason for hiding this comment

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

Why did the replace disappear here?

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 changed it to a block construct, starting on line 166, to better match gaia.

mhofman pushed a commit that referenced this pull request Feb 7, 2024
feat: upgrade interchain stack to ibc5
mhofman pushed a commit that referenced this pull request Feb 7, 2024
feat: upgrade interchain stack to ibc5
mhofman pushed a commit that referenced this pull request Feb 18, 2024
feat: upgrade interchain stack to ibc5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agoric-cosmos automerge:rebase Automatically rebase updates, then merge enhancement New feature or request force:integration Force integration tests to run on PR proto:expect-breakage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

agd reported incompatible with Ledger cosmos app MsgReturnGrants returns the wrong Type
3 participants