From 446c6642c2bf0993082c38db1fbe66a9ebe9d9fe Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 23 Feb 2022 20:47:10 +0100 Subject: [PATCH 1/7] Redact errors returned in reply --- x/wasm/keeper/msg_dispatcher.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index edbfe162..ac610fbe 100644 --- a/x/wasm/keeper/msg_dispatcher.go +++ b/x/wasm/keeper/msg_dispatcher.go @@ -1,8 +1,11 @@ package keeper import ( + "fmt" + wasmvmtypes "github.com/CosmWasm/wasmvm/types" sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/errors" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" abci "github.com/tendermint/tendermint/abci/types" @@ -131,8 +134,10 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk }, } } else { + // Issue #759 - we don't return error string for worries of non-determinism + ctx.Logger().Info("Redacting submessage error", "error", err.Error()) result = wasmvmtypes.SubcallResult{ - Err: err.Error(), + Err: redactError(err), } } @@ -155,6 +160,11 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk return rsp, nil } +func redactError(err error) string { + codespace, code, _ := errors.ABCIInfo(err, false) + return fmt.Sprintf("Error: %s/%d", codespace, code) +} + func filterEvents(events []sdk.Event) []sdk.Event { // pre-allocate space for efficiency res := make([]sdk.Event, 0, len(events)) From 8f75ad1fafad8cb5bdbd1e9297e5f218708658c4 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 23 Feb 2022 21:13:33 +0100 Subject: [PATCH 2/7] Fix lint error, failing test --- x/wasm/keeper/msg_dispatcher.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index ac610fbe..1d96ef86 100644 --- a/x/wasm/keeper/msg_dispatcher.go +++ b/x/wasm/keeper/msg_dispatcher.go @@ -5,7 +5,6 @@ import ( wasmvmtypes "github.com/CosmWasm/wasmvm/types" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/types/errors" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" abci "github.com/tendermint/tendermint/abci/types" @@ -135,7 +134,7 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk } } else { // Issue #759 - we don't return error string for worries of non-determinism - ctx.Logger().Info("Redacting submessage error", "error", err.Error()) + logError(ctx, "Redacting submessage error", err) result = wasmvmtypes.SubcallResult{ Err: redactError(err), } @@ -160,8 +159,15 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk return rsp, nil } +func logError(ctx sdk.Context, msg string, err error) { + logger := ctx.Logger() + if logger != nil { + logger.Info(msg, "error", err.Error()) + } +} + func redactError(err error) string { - codespace, code, _ := errors.ABCIInfo(err, false) + codespace, code, _ := sdkerrors.ABCIInfo(err, false) return fmt.Sprintf("Error: %s/%d", codespace, code) } From 30bb2900c78e535e9253f8cb26036d06d1e673ae Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 23 Feb 2022 21:42:38 +0100 Subject: [PATCH 3/7] Fix submessage tests --- x/wasm/keeper/msg_dispatcher.go | 4 ++++ x/wasm/keeper/submsg_test.go | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index 1d96ef86..9a17f4a2 100644 --- a/x/wasm/keeper/msg_dispatcher.go +++ b/x/wasm/keeper/msg_dispatcher.go @@ -167,6 +167,10 @@ func logError(ctx sdk.Context, msg string, err error) { } func redactError(err error) string { + // FIXME: do we want to hardcode some constant string mappings here as well? + // Or better document them? (SDK error string may change on a patch release to fix wording) + // sdk/11 is out of gas + // sdk/5 is insufficient funds (on bank send) codespace, code, _ := sdkerrors.ABCIInfo(err, false) return fmt.Sprintf("Error: %s/%d", codespace, code) } diff --git a/x/wasm/keeper/submsg_test.go b/x/wasm/keeper/submsg_test.go index 2fdb17eb..bb2ab291 100644 --- a/x/wasm/keeper/submsg_test.go +++ b/x/wasm/keeper/submsg_test.go @@ -254,7 +254,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { msg: invalidBankSend, subMsgError: true, // uses less gas than the send tokens (cost of bank transfer) - resultAssertions: []assertion{assertGasUsed(76000, 79000), assertErrorString("insufficient funds")}, + resultAssertions: []assertion{assertGasUsed(76000, 79000), assertErrorString("sdk/5")}, }, "out of gas panic with no gas limit": { submsgID: 7, @@ -275,7 +275,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses same gas as call without limit (note we do not charge the 40k on reply) - resultAssertions: []assertion{assertGasUsed(79000, 79040), assertErrorString("insufficient funds")}, + resultAssertions: []assertion{assertGasUsed(77500, 77600), assertErrorString("sdk/5")}, }, "out of gas caught with gas limit": { submsgID: 17, @@ -283,7 +283,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses all the subGasLimit, plus the 52k or so for the main contract - resultAssertions: []assertion{assertGasUsed(subGasLimit+73000, subGasLimit+80000), assertErrorString("out of gas")}, + resultAssertions: []assertion{assertGasUsed(subGasLimit+72000, subGasLimit+73000), assertErrorString("sdk/11")}, }, "instantiate contract gets address in data and events": { submsgID: 21, From 3953bdafcc8628f624cb06803c1ac66b9db79f96 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 2 Mar 2022 10:07:28 +0100 Subject: [PATCH 4/7] Adjust error string --- x/wasm/keeper/msg_dispatcher.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index 9a17f4a2..51bb37cc 100644 --- a/x/wasm/keeper/msg_dispatcher.go +++ b/x/wasm/keeper/msg_dispatcher.go @@ -134,7 +134,7 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk } } else { // Issue #759 - we don't return error string for worries of non-determinism - logError(ctx, "Redacting submessage error", err) + moduleLogger(ctx).Info("Redacting submessage error", "cause", err) result = wasmvmtypes.SubcallResult{ Err: redactError(err), } @@ -159,20 +159,13 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk return rsp, nil } -func logError(ctx sdk.Context, msg string, err error) { - logger := ctx.Logger() - if logger != nil { - logger.Info(msg, "error", err.Error()) - } -} - func redactError(err error) string { // FIXME: do we want to hardcode some constant string mappings here as well? // Or better document them? (SDK error string may change on a patch release to fix wording) // sdk/11 is out of gas // sdk/5 is insufficient funds (on bank send) codespace, code, _ := sdkerrors.ABCIInfo(err, false) - return fmt.Sprintf("Error: %s/%d", codespace, code) + return fmt.Sprintf("codespace: %s, code: %d", codespace, code) } func filterEvents(events []sdk.Event) []sdk.Event { From 66ac1f58ce7e4976bd77f9236ced388cb9259fc1 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 2 Mar 2022 10:16:36 +0100 Subject: [PATCH 5/7] Handle nil logger again --- x/wasm/keeper/msg_dispatcher.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index 51bb37cc..c28e92d0 100644 --- a/x/wasm/keeper/msg_dispatcher.go +++ b/x/wasm/keeper/msg_dispatcher.go @@ -134,7 +134,7 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk } } else { // Issue #759 - we don't return error string for worries of non-determinism - moduleLogger(ctx).Info("Redacting submessage error", "cause", err) + logRedactError(ctx, err) result = wasmvmtypes.SubcallResult{ Err: redactError(err), } @@ -159,6 +159,13 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk return rsp, nil } +func logRedactError(ctx sdk.Context, err error) { + logger := moduleLogger(ctx) + if logger != nil { + logger.Info("Redacting submessage error", "cause", err) + } +} + func redactError(err error) string { // FIXME: do we want to hardcode some constant string mappings here as well? // Or better document them? (SDK error string may change on a patch release to fix wording) From 654fcec122c274a3dbe0a99eb3f76cc02e69085f Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Wed, 2 Mar 2022 14:15:11 +0100 Subject: [PATCH 6/7] Revert "Handle nil logger again" This reverts commit 66ac1f58ce7e4976bd77f9236ced388cb9259fc1. --- x/wasm/keeper/msg_dispatcher.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/x/wasm/keeper/msg_dispatcher.go b/x/wasm/keeper/msg_dispatcher.go index c28e92d0..51bb37cc 100644 --- a/x/wasm/keeper/msg_dispatcher.go +++ b/x/wasm/keeper/msg_dispatcher.go @@ -134,7 +134,7 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk } } else { // Issue #759 - we don't return error string for worries of non-determinism - logRedactError(ctx, err) + moduleLogger(ctx).Info("Redacting submessage error", "cause", err) result = wasmvmtypes.SubcallResult{ Err: redactError(err), } @@ -159,13 +159,6 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk return rsp, nil } -func logRedactError(ctx sdk.Context, err error) { - logger := moduleLogger(ctx) - if logger != nil { - logger.Info("Redacting submessage error", "cause", err) - } -} - func redactError(err error) string { // FIXME: do we want to hardcode some constant string mappings here as well? // Or better document them? (SDK error string may change on a patch release to fix wording) From b2217a3a44b773216ad6b2f4eaad757fc00ad9d3 Mon Sep 17 00:00:00 2001 From: Alex Peters Date: Wed, 2 Mar 2022 14:21:00 +0100 Subject: [PATCH 7/7] Fix test setup and assertions --- x/wasm/keeper/msg_dispatcher_test.go | 4 +++- x/wasm/keeper/submsg_test.go | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/x/wasm/keeper/msg_dispatcher_test.go b/x/wasm/keeper/msg_dispatcher_test.go index b1a806a6..b53eac72 100644 --- a/x/wasm/keeper/msg_dispatcher_test.go +++ b/x/wasm/keeper/msg_dispatcher_test.go @@ -5,6 +5,8 @@ import ( "fmt" "testing" + "github.com/tendermint/tendermint/libs/log" + wasmvmtypes "github.com/CosmWasm/wasmvm/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/assert" @@ -347,7 +349,7 @@ func TestDispatchSubmessages(t *testing.T) { em := sdk.NewEventManager() ctx := sdk.Context{}.WithMultiStore(&mockStore). WithGasMeter(sdk.NewGasMeter(100)). - WithEventManager(em) + WithEventManager(em).WithLogger(log.TestingLogger()) d := NewMessageDispatcher(spec.msgHandler, spec.replyer) gotData, gotErr := d.DispatchSubmessages(ctx, RandomAccountAddress(t), "any_port", spec.msgs) if spec.expErr { diff --git a/x/wasm/keeper/submsg_test.go b/x/wasm/keeper/submsg_test.go index bb2ab291..29c3ad8e 100644 --- a/x/wasm/keeper/submsg_test.go +++ b/x/wasm/keeper/submsg_test.go @@ -254,7 +254,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { msg: invalidBankSend, subMsgError: true, // uses less gas than the send tokens (cost of bank transfer) - resultAssertions: []assertion{assertGasUsed(76000, 79000), assertErrorString("sdk/5")}, + resultAssertions: []assertion{assertGasUsed(76000, 79000), assertErrorString("codespace: sdk, code: 5")}, }, "out of gas panic with no gas limit": { submsgID: 7, @@ -275,7 +275,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses same gas as call without limit (note we do not charge the 40k on reply) - resultAssertions: []assertion{assertGasUsed(77500, 77600), assertErrorString("sdk/5")}, + resultAssertions: []assertion{assertGasUsed(77800, 77900), assertErrorString("codespace: sdk, code: 5")}, }, "out of gas caught with gas limit": { submsgID: 17, @@ -283,7 +283,7 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { subMsgError: true, gasLimit: &subGasLimit, // uses all the subGasLimit, plus the 52k or so for the main contract - resultAssertions: []assertion{assertGasUsed(subGasLimit+72000, subGasLimit+73000), assertErrorString("sdk/11")}, + resultAssertions: []assertion{assertGasUsed(subGasLimit+73000, subGasLimit+74000), assertErrorString("codespace: sdk, code: 11")}, }, "instantiate contract gets address in data and events": { submsgID: 21,