-
Notifications
You must be signed in to change notification settings - Fork 617
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
chore: upgrade wasmd to v0.27.0.rc3-osmo and ibc-go to v3 #1527
Conversation
Codecov Report
@@ Coverage Diff @@
## v8.x #1527 +/- ##
=======================================
Coverage 20.66% 20.66%
=======================================
Files 204 204
Lines 25991 25991
=======================================
Hits 5372 5372
Misses 19637 19637
Partials 982 982 Continue to review full report at Codecov.
|
error on e2e test is as follows:
|
My guess is I'm going to have to re-release the v8 docker image since we are changing the wasm version. Will check it out in depth tomorrow |
@czarcas7ic I think you are right. The error is referring to the parameter to wasmd that was removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on fighting through all the dependency / fork issues! We should note these difficulties so we can make more realistic release timelines in the future.
FROM golang:1.18.2-alpine3.15 as build | ||
|
||
RUN set -eux; apk add --no-cache ca-certificates build-base; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my learning, why was
RUN set -eux; apk add --no-cache ca-certificates build-base;
and
LINK_STATICALLY=true
added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LINK_STATICALLY=true
is needed to pick up certain build flags needed for statically linking cosm wasm dependencies
RUN set -eux; apk add --no-cache ca-certificates build-base;
is needed because we changes the base image and need to install some build tools for building cosm wasm dependencies.
This was adapted from the latest Dockerfile in wasmd repository: https://github.com/CosmWasm/wasmd/blob/2b0b1677df42daffa9817cf2f4e266f4e40d69b8/Dockerfile#L9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK (I can't speak to the infra changes, but everything else looks good to me).
// DefaultMaxWasmCodeSize limit max bytes read to prevent gzip bombs | ||
// It is 1200 KB in x/wasm, update it later via governance if really needed | ||
MaxWasmCodeSize: wasmtypes.DefaultMaxWasmCodeSize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cref CosmWasm/wasmd#809 if anyones curious
@@ -274,15 +274,16 @@ func (app *OsmosisApp) InitNormalKeepers( | |||
// Create Transfer Keepers | |||
transferKeeper := ibctransferkeeper.NewKeeper( | |||
appCodec, keys[ibctransfertypes.StoreKey], app.GetSubspace(ibctransfertypes.ModuleName), | |||
app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper, | |||
app.IBCKeeper.ChannelKeeper, app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.AccountKeeper, app.BankKeeper, app.ScopedTransferKeeper, | ||
) | ||
app.TransferKeeper = &transferKeeper | ||
app.transferModule = transfer.NewAppModule(*app.TransferKeeper) | ||
transferIBCModule := transfer.NewIBCModule(*app.TransferKeeper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
odd design from IBC module to make them different types that don't inherit...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I didn't follow the docker file updates tho. (I'm just not familiar with whats going on)
Wait this was targetted at the wrong release, we need to undo it on v8.x 😬 |
) Closes: #XXX ## What is the purpose of the change Context: #1527 This change is a trivial rework / code cleanup without any test coverage. ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? no - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? yes - How is the feature or change documented? not applicable
Closes: #1502
What is the purpose of the change
The ultimate goal of this PR is to upgrade wasmd to v0.27.0.rc3. This change is done against
v8.x
branch to unblock the RC as soon as possible. A separate PR to main will be made, once this is mergedTo do so, the following updates were done:
chore: backport Add hooks to allow app modules to add things to state-sync #237 cosmos-sdk#238 to release v0.45.0x-osmo-v7.10
https://github.com/osmosis-labs/wasmd/releases/tag/v0.27.0.rc3-osmo
upgrade to ibc/go v3.0.0
upgrade bech32-ibc to v0.3.0-rc1
Brief Changelog
Testing and Verifying
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? (yes / no)x/<module>/spec/
) / Osmosis docs repo / not documented)