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

Upgrade to WasmVM v0.15.1 #548

Merged
merged 3 commits into from
Jul 21, 2021
Merged

Upgrade to WasmVM v0.15.1 #548

merged 3 commits into from
Jul 21, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Jun 25, 2021

Closes #532

  • Messages and submessages are merged; the replyOn type now got a new "never" case for the fire and forget messages
  • replyOn is not an integer but supports the Stringer interface (i.e. .String())
  • The interface version was bumped from interface_version_5 to interface_version_6 such that CosmWasm 0.15.0 contracts need to be used
  • In some Wasm messages "send" was renamed to "funds" for consistency
  • The IBC acknowledgement type was changed slightly
  • There is a new Gov message (MsgVote) that needs to be connected
  • Deserializing contract results now cost gas. This needs to be configured in a new argument of vm.Execute and friends. We messed up storing the value in the vm instance, such that it needs to be passed to every call. Sorry for that! The deserialization cost is in gas per byte. It is a fraction, so you can use 20/1, 10/1, 1/1, 1/2, 1/30, ... in there.
  • Contract responses now contain events. The exiting Attributes should go into a main wasm event as before. All other events should be added with a wasm- prefix on the event type.
  • Set correct release version in go.mod
  • Upgrade Dockerfile with proper version checksum

Blocker

CosmWasm/wasmvm#235
CosmWasm/wasmvm#236

Inconsistent

#552
#553

Improvements:

CosmWasm/wasmvm#239
CosmWasm/wasmvm#238

@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

Merging #548 (32ba924) into master (8bec411) will increase coverage by 0.35%.
The diff coverage is 98.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
+ Coverage   59.26%   59.61%   +0.35%     
==========================================
  Files          44       45       +1     
  Lines        5165     5220      +55     
==========================================
+ Hits         3061     3112      +51     
- Misses       1881     1884       +3     
- Partials      223      224       +1     
Impacted Files Coverage Δ
x/wasm/keeper/api.go 42.85% <ø> (ø)
x/wasm/types/types.go 52.14% <ø> (+2.14%) ⬆️
x/wasm/keeper/handler_plugin_encoders.go 80.88% <91.30%> (+0.98%) ⬆️
x/wasm/ibc.go 62.58% <100.00%> (ø)
x/wasm/keeper/events.go 100.00% <100.00%> (ø)
x/wasm/keeper/gas_register.go 97.29% <100.00%> (+0.52%) ⬆️
x/wasm/keeper/keeper.go 85.60% <100.00%> (+0.05%) ⬆️
x/wasm/keeper/msg_dispatcher.go 86.20% <100.00%> (-10.23%) ⬇️
x/wasm/keeper/relay.go 100.00% <100.00%> (ø)
... and 1 more

@alpe alpe force-pushed the wasmvm_upgrade_532 branch from 4b5b78e to 82aa0c4 Compare July 5, 2021 10:45
@alpe alpe added the blocked label Jul 5, 2021
@alpe alpe force-pushed the wasmvm_upgrade_532 branch from 82aa0c4 to a13db5d Compare July 5, 2021 12:09
@alpe alpe mentioned this pull request Jul 5, 2021
3 tasks
@webmaster128
Copy link
Member

A patched wasmvm 0.15.1 is released that should solve the two blocker tickets as well as the missing pointer in GovMsg.

@alpe alpe removed the blocked label Jul 7, 2021
@alpe alpe force-pushed the wasmvm_upgrade_532 branch from a13db5d to 1bbd292 Compare July 7, 2021 07:50
@alpe alpe changed the title Upgrade to WasmVM v0.15.0 Upgrade to WasmVM v0.15.1 Jul 7, 2021
@alpe alpe force-pushed the wasmvm_upgrade_532 branch from 1bbd292 to b0b1fa8 Compare July 7, 2021 07:56
@alpe alpe marked this pull request as ready for review July 7, 2021 08:26
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Good stuff. Will merge.

Left some minor comments that may be worth addressing in the future

return sdk.Events{sdk.NewEvent(types.WasmModuleEventType, attrs...)}
}

// returns true when a wasm module event was emitted for this contract already
Copy link
Member

Choose a reason for hiding this comment

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

nice to avoid duplicates with reply, etc

func newCustomEvents(evts wasmvmtypes.Events, contractAddr sdk.AccAddress) sdk.Events {
events := make(sdk.Events, 0, len(evts))
for _, e := range evts {
if len(e.Type) <= eventTypeMinLength {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like silent failures/filters. I would prefer returning an error (even panic-ing in Go), Devs learn quick when things panic. They ignore errors for months if they are silently ignored

func (g WasmGasRegister) EventCosts(attrs []wasmvmtypes.EventAttribute, events wasmvmtypes.Events) sdk.Gas {
gas, remainingFreeTier := g.eventAttributeCosts(attrs, g.c.EventAttributeDataFreeTier)
for _, e := range events {
gas += g.c.CustomEventCost
Copy link
Member

Choose a reason for hiding this comment

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

looks good to me

Copy link
Member

Choose a reason for hiding this comment

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

we really need some overview docs on gas pricing (including from the sdk and wasmer) so devs understand this all


func EncodeGovMsg(sender sdk.AccAddress, msg *wasmvmtypes.GovMsg) ([]sdk.Msg, error) {
var option govtypes.VoteOption
switch msg.Vote.Vote {
Copy link
Member

Choose a reason for hiding this comment

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

I would pull this switch out into a function. But if it is only used here, I guess no need

events := types.ParseEvents(attrs, contractAddr)
ctx.EventManager().EmitEvents(events)
return k.wasmVMResponseHandler.Handle(ctx, contractAddr, ibcPort, subMsg, msgs, data)
if len(attrs) != 0 || !hasWasmModuleEvent(ctx, contractAddr) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice check. If we don't add any info, and they already emitted some event in this tx (recursive calls, reply), then no need to emit one 👍

replyer: &mockReplyer{},
msgHandler: &wasmtesting.MockMessageHandler{
DispatchMsgFn: func(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) {
return nil, [][]byte{{}}, nil
Copy link
Member

Choose a reason for hiding this comment

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

Nil vs empty is an odd test. What is being tested here anyway?

require.NoError(t, err)
//coordinator.CommitBlock(chainA, chainB)
err = coordinator.UpdateClient(chainA, chainB, clientA, ibcexported.Tendermint)
err := coordinator.RelayAndAckPendingPackets(chainA, chainB, clientA, clientB)
Copy link
Member

Choose a reason for hiding this comment

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

Did that just get 90% smaller? 😄

@@ -297,23 +297,6 @@ func NewWasmCoins(cosmosCoins sdk.Coins) (wasmCoins []wasmvmtypes.Coin) {
return wasmCoins
}

// ParseEvents converts wasm LogAttributes into an sdk.Events. Returns events and number of bytes for custom attributes
func ParseEvents(wasmOutputAttrs []wasmvmtypes.EventAttribute, contractAddr sdk.AccAddress) sdk.Events {
Copy link
Member

Choose a reason for hiding this comment

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

Yup, I prefer your more complex functions now

) (*wasmvmtypes.IBCBasicResponse, uint64, error)

// IBCPacketTimeout is available on IBC-enabled contracts and is called when an
// outgoing packet (previously sent by this contract) will provably never be executed.
// Usually handled like ack returning an error
IBCPacketTimeout(
codeID wasmvm.Checksum,
checksum wasmvm.Checksum,
Copy link
Member

Choose a reason for hiding this comment

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

nice typo to fix

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.

Upgrade wasmvm to 0.15.0
3 participants