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 sdk to 0.52 #206

Merged
merged 7 commits into from
Jan 9, 2025
Merged

deps: upgrade sdk to 0.52 #206

merged 7 commits into from
Jan 9, 2025

Conversation

gjermundgaraba
Copy link
Contributor

@gjermundgaraba gjermundgaraba commented Jan 8, 2025

Description

Needed for #201 and #202

closes: #207


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.

  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Wrote unit and integration tests.
  • Added relevant natspec and godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@gjermundgaraba gjermundgaraba changed the title dips: upgrade sdk to 0.52 deps: upgrade sdk to 0.52 Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.97%. Comparing base (febaabb) to head (40c4ef0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #206   +/-   ##
=======================================
  Coverage   96.97%   96.97%           
=======================================
  Files          11       11           
  Lines         562      562           
=======================================
  Hits          545      545           
  Misses         17       17           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -19,7 +19,7 @@ func IbcGoChainSpec(name, chainId string) *interchaintest.ChainSpec {
Images: []ibc.DockerImage{
{
Repository: "ghcr.io/cosmos/ibc-go-wasm-simd", // FOR LOCAL IMAGE USE: Docker Image Name
Version: "feat-ibc-eureka", // FOR LOCAL IMAGE USE: Docker Image Tag
Version: "gjermund-fix-wasm-dockerfile", // FOR LOCAL IMAGE USE: Docker Image Tag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be changed back after cosmos/ibc-go#7830 is merged

@gjermundgaraba gjermundgaraba marked this pull request as ready for review January 9, 2025 01:14
@gjermundgaraba gjermundgaraba requested a review from srdtrk as a code owner January 9, 2025 01:14
Copy link
Member

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

lgtm. Left a couple comments that can be addressed later. Surprised that the rust packages don't need updates.

Comment on lines +102 to +111
// in order to ensure that the underlying account is created, we need to perform an operation on its behalf.
// in this case we just send 1 token to itself so the account gets created.
err := chain.SendFunds(ctx, cosmosUser.KeyName(), ibc.WalletAmount{
Address: cosmosUser.FormattedAddress(),
Denom: chain.Config().Denom,
Amount: sdkmath.NewInt(1),
})

s.Require().NoError(err)
s.Require().NoError(testutil.WaitForBlocks(ctx, 2, chain))
Copy link
Member

Choose a reason for hiding this comment

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

GetAndFund test users doesn't do this? Also I thought this requirement was gonna be removed in SDK v0.52

e2e/interchaintestv8/e2esuite/utils.go Outdated Show resolved Hide resolved
e2e/interchaintestv8/e2esuite/utils.go Outdated Show resolved Hide resolved
Comment on lines -60 to -64
sdk.GetConfig().SetBech32PrefixForAccount(chain.Config().Bech32Prefix, chain.Config().Bech32Prefix+sdk.PrefixPublic)
sdk.GetConfig().SetBech32PrefixForValidator(
chain.Config().Bech32Prefix+sdk.PrefixValidator+sdk.PrefixOperator,
chain.Config().Bech32Prefix+sdk.PrefixValidator+sdk.PrefixOperator+sdk.PrefixPublic,
)
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should check that this function works with chains using other prefixes (like osmo instead of cosmos)

})

s.Require().NoError(err)
s.Require().NoError(testutil.WaitForBlocks(ctx, 2, chain))
Copy link
Member

Choose a reason for hiding this comment

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

We should also check if we really need the WaitForBlocks since this function is ran on a loop in setup. (Shouldn't be too bad tho)

@srdtrk srdtrk merged commit f639c91 into main Jan 9, 2025
60 checks passed
@srdtrk srdtrk deleted the gjermund/sdk-upgrade branch January 9, 2025 06:21
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.

Update to use SDK 0.52
2 participants