Skip to content

Commit

Permalink
Make apply func if no err, out of gas panic, match std behavior (#3746)
Browse files Browse the repository at this point in the history
* Remove logging for apply func if no err, out of gas

* Add test case

* Add changelog

* Actually Add changelog
  • Loading branch information
ValarDragon authored Feb 1, 2023
1 parent 3ae728c commit 62757d3
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 3 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Bug Fix

* [#3715](https://github.com/osmosis-labs/osmosis/pull/3715) Fix x/gamm (golang API) CalculateSpotPrice, balancer.SpotPrice and Stableswap.SpotPrice base and quote asset.
* [#3746](https://github.com/osmosis-labs/osmosis/pull/3746) Make ApplyFuncIfNoErr logic preserve panics for OutOfGas behavior.



## v14

Expand Down Expand Up @@ -124,7 +127,6 @@ Additionally, the swagger files for v13 have been updated to improve compatibili
* [#3711](https://github.com/osmosis-labs/osmosis/pull/3711) Use Dec instead of Int for additive `ErrTolerace` in `osmoutils`.
* [3647](https://github.com/osmosis-labs/osmosis/pull/3647), [3942](https://github.com/osmosis-labs/osmosis/pull/3942) (CLI) re-order the command line arguments for `osmosisd tx gamm join-swap-share-amount-out`


## v13.0.0

This release includes stableswap, and expands the IBC safety & composability functionality of Osmosis. The primary features are:
Expand Down
25 changes: 23 additions & 2 deletions osmoutils/cache_ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,20 @@ import (
// drop the state machine change and log the error.
// If there is no error, proceeds as normal (but with some slowdown due to SDK store weirdness)
// Try to avoid usage of iterators in f.
//
// If its an out of gas panic, this function will also panic like in normal tx execution flow.
// This is still safe for beginblock / endblock code though, as they do not have out of gas panics.
func ApplyFuncIfNoError(ctx sdk.Context, f func(ctx sdk.Context) error) (err error) {
// Add a panic safeguard
defer func() {
if recoveryError := recover(); recoveryError != nil {
PrintPanicRecoveryError(ctx, recoveryError)
err = errors.New("panic occurred during execution")
if isErr, _ := IsOutOfGasError(recoveryError); isErr {
// We panic with the same error, to replicate the normal tx execution flow.
panic(recoveryError)
} else {
PrintPanicRecoveryError(ctx, recoveryError)
err = errors.New("panic occurred during execution")
}
}
}()
// makes a new cache context, which all state changes get wrapped inside of.
Expand All @@ -35,6 +43,19 @@ func ApplyFuncIfNoError(ctx sdk.Context, f func(ctx sdk.Context) error) (err err
return err
}

// Frustratingly, this has to return the error descriptor, not an actual error itself
// because the SDK errors here are not actually errors. (They don't implement error interface)
func IsOutOfGasError(err any) (bool, string) {
switch e := err.(type) {
case types.ErrorOutOfGas:
return true, e.Descriptor
case types.ErrorGasOverflow:
return true, e.Descriptor
default:
return false, ""
}
}

// PrintPanicRecoveryError error logs the recoveryError, along with the stacktrace, if it can be parsed.
// If not emits them to stdout.
func PrintPanicRecoveryError(ctx sdk.Context, recoveryError interface{}) {
Expand Down
59 changes: 59 additions & 0 deletions osmoutils/cache_ctx_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package osmoutils_test

import (
"github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/osmosis-labs/osmosis/v13/osmoutils"
)

var expectedOutOfGasError = types.ErrorOutOfGas{Descriptor: "my func"}

func consumeGas(ctx sdk.Context, gas uint64, numTimes int) error {
for i := 0; i < numTimes; i++ {
ctx.GasMeter().ConsumeGas(gas, "my func")
}
return nil
}

func (s *TestSuite) TestCacheCtxConsumeGas() {
// every test case adds 1k gas 10 times
testcases := map[string]struct {
gasLimit uint64
gasUsedPreCtx uint64
gasUsedPostCtx uint64
expectPanic bool
}{
"no gas limit hit": {
gasLimit: 15_000,
gasUsedPreCtx: 111,
gasUsedPostCtx: 111 + 10_000,
expectPanic: false,
},
"gas limit hit": {
gasLimit: 10_000,
gasUsedPreCtx: 111,
gasUsedPostCtx: 111 + 10_000,
expectPanic: true,
},
}
for name, tc := range testcases {
s.Run(name, func() {
ctx := s.Ctx.WithGasMeter(sdk.NewGasMeter(tc.gasLimit))
ctx.GasMeter().ConsumeGas(tc.gasUsedPreCtx, "pre ctx")
var err error
f := func() {
osmoutils.ApplyFuncIfNoError(ctx, func(c sdk.Context) error {
return consumeGas(c, 1000, 10)
})
}
if tc.expectPanic {
s.PanicsWithValue(expectedOutOfGasError, f)
} else {
f()
s.Require().NoError(err)
}
s.Equal(tc.gasUsedPostCtx, ctx.GasMeter().GasConsumed())
})
}
}

0 comments on commit 62757d3

Please sign in to comment.