From efe52f265d88a16f0d639ed77afe708a521e83a4 Mon Sep 17 00:00:00 2001 From: Alex Johnson Date: Mon, 15 Jul 2024 14:00:24 -0400 Subject: [PATCH] fix: gas sim (#129) * small updates * protect * update ante * testing * lint fix --- .github/workflows/ictest.yml | 2 +- .github/workflows/lint.yml | 2 +- .github/workflows/test.yml | 6 +- x/feemarket/ante/fee.go | 34 ++++++--- x/feemarket/ante/suite/suite.go | 20 ++++-- x/feemarket/post/fee.go | 3 +- x/feemarket/post/fee_test.go | 118 +++++++++++++++++--------------- 7 files changed, 107 insertions(+), 78 deletions(-) diff --git a/.github/workflows/ictest.yml b/.github/workflows/ictest.yml index c5a7c23..9c17ac7 100644 --- a/.github/workflows/ictest.yml +++ b/.github/workflows/ictest.yml @@ -12,7 +12,7 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v4 with: - go-version: 1.22.3 + go-version: 1.22.5 cache: true cache-dependency-path: go.sum - uses: technote-space/get-diff-action@v6.1.2 diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index d12397a..ed6676d 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -18,7 +18,7 @@ jobs: steps: - uses: actions/setup-go@v4 with: - go-version: 1.22.3 + go-version: 1.22.5 - uses: actions/checkout@v4 - name: golangci-lint uses: golangci/golangci-lint-action@v3 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 83a648c..4c352cf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -20,7 +20,7 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v4 with: - go-version: 1.22.3 + go-version: 1.22.5 cache: true cache-dependency-path: go.sum - uses: technote-space/get-diff-action@v6.1.2 @@ -40,7 +40,7 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v4 with: - go-version: 1.22.3 + go-version: 1.22.5 cache: true cache-dependency-path: go.sum - uses: technote-space/get-diff-action@v6.1.2 @@ -60,7 +60,7 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-go@v4 with: - go-version: 1.22.3 + go-version: 1.22.5 cache: true cache-dependency-path: go.sum - uses: technote-space/get-diff-action@v6.1.2 diff --git a/x/feemarket/ante/fee.go b/x/feemarket/ante/fee.go index 1e6c6dc..dac2e86 100644 --- a/x/feemarket/ante/fee.go +++ b/x/feemarket/ante/fee.go @@ -104,7 +104,8 @@ func (dfd feeMarketCheckDecorator) anteHandle(ctx sdk.Context, tx sdk.Tx, simula var feeCoin sdk.Coin if simulate && len(feeCoins) == 0 { - feeCoin = sdk.NewCoin(params.FeeDenom, sdkmath.ZeroInt()) + // if simulating and user did not provider a fee - create a dummy value for them + feeCoin = sdk.NewCoin(params.FeeDenom, sdkmath.OneInt()) } else { feeCoin = feeCoins[0] } @@ -128,19 +129,20 @@ func (dfd feeMarketCheckDecorator) anteHandle(ctx sdk.Context, tx sdk.Tx, simula if err != nil { return ctx, errorsmod.Wrapf(err, "error checking fee") } + } - priorityFee, err := dfd.resolveTxPriorityCoins(ctx, feeCoin, params.FeeDenom) - if err != nil { - return ctx, errorsmod.Wrapf(err, "error resolving fee priority") - } - - baseGasPrice, err := dfd.feemarketKeeper.GetMinGasPrice(ctx, params.FeeDenom) - if err != nil { - return ctx, err - } + priorityFee, err := dfd.resolveTxPriorityCoins(ctx, feeCoin, params.FeeDenom) + if err != nil { + return ctx, errorsmod.Wrapf(err, "error resolving fee priority") + } - ctx = ctx.WithPriority(GetTxPriority(priorityFee, int64(gas), baseGasPrice)) + baseGasPrice, err := dfd.feemarketKeeper.GetMinGasPrice(ctx, params.FeeDenom) + if err != nil { + return ctx, err } + + ctx = ctx.WithPriority(GetTxPriority(priorityFee, int64(gas), baseGasPrice)) + return next(ctx, tx, simulate) } @@ -220,6 +222,16 @@ const ( // normalizedGasPrice = effectiveGasPrice / currentGasPrice (floor is 1. The minimum effective gas price can ever be is current gas price) // scaledGasPrice = normalizedGasPrice * 10 ^ gasPricePrecision (amount of decimal places in the normalized gas price to consider when converting to int64). func GetTxPriority(fee sdk.Coin, gasLimit int64, currentGasPrice sdk.DecCoin) int64 { + // protections from dividing by 0 + if gasLimit == 0 { + return 0 + } + + // if the gas price is 0, just use a raw amount + if currentGasPrice.IsZero() { + return fee.Amount.Int64() + } + effectiveGasPrice := fee.Amount.ToLegacyDec().QuoInt64(gasLimit) normalizedGasPrice := effectiveGasPrice.Quo(currentGasPrice.Amount) scaledGasPrice := normalizedGasPrice.MulInt64(int64(math.Pow10(gasPricePrecision))) diff --git a/x/feemarket/ante/suite/suite.go b/x/feemarket/ante/suite/suite.go index 3489583..d996eef 100644 --- a/x/feemarket/ante/suite/suite.go +++ b/x/feemarket/ante/suite/suite.go @@ -133,13 +133,14 @@ func (s *TestSuite) SetupHandlers(mock bool) { // TestCase represents a test case used in test tables. type TestCase struct { - Name string - Malleate func(*TestSuite) TestCaseArgs - RunAnte bool - RunPost bool - Simulate bool - ExpPass bool - ExpErr error + Name string + Malleate func(*TestSuite) TestCaseArgs + RunAnte bool + RunPost bool + Simulate bool + ExpPass bool + ExpErr error + ExpectConsumedGas uint64 } type TestCaseArgs struct { @@ -193,6 +194,11 @@ func (s *TestSuite) RunTestCase(t *testing.T, tc TestCase, args TestCaseArgs) { require.NotNil(t, newCtx) s.Ctx = newCtx + if tc.RunPost { + consumedGas := newCtx.GasMeter().GasConsumed() + require.Equal(t, tc.ExpectConsumedGas, consumedGas) + } + } else { switch { case txErr != nil: diff --git a/x/feemarket/post/fee.go b/x/feemarket/post/fee.go index 0433d0b..1b59755 100644 --- a/x/feemarket/post/fee.go +++ b/x/feemarket/post/fee.go @@ -85,7 +85,8 @@ func (dfd FeeMarketDeductDecorator) PostHandle(ctx sdk.Context, tx sdk.Tx, simul var feeCoin sdk.Coin if simulate && len(feeCoins) == 0 { - feeCoin = sdk.NewCoin(params.FeeDenom, math.ZeroInt()) + // if simulating and user did not provider a fee - create a dummy value for them + feeCoin = sdk.NewCoin(params.FeeDenom, math.OneInt()) } else { feeCoin = feeCoins[0] } diff --git a/x/feemarket/post/fee_test.go b/x/feemarket/post/fee_test.go index 4a6d5a0..4ed1f0e 100644 --- a/x/feemarket/post/fee_test.go +++ b/x/feemarket/post/fee_test.go @@ -124,12 +124,12 @@ func TestSendTip(t *testing.T) { func TestPostHandle(t *testing.T) { // Same data for every test case const ( - baseDenom = "stake" - resolvableDenom = "atom" + baseDenom = "stake" + resolvableDenom = "atom" + expectedConsumedGas = 33339 + gasLimit = expectedConsumedGas ) - // exact cost of transaction - gasLimit := uint64(27284) validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit)) validFeeAmountWithTip := validFeeAmount.Add(math.LegacyNewDec(100)) validFee := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmount.TruncateInt())) @@ -204,11 +204,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validFee, } }, - RunAnte: true, - RunPost: true, - Simulate: true, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: true, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "signer has enough funds, should pass, no tip", @@ -223,11 +224,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validFee, } }, - RunAnte: true, - RunPost: true, - Simulate: false, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: false, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "signer has enough funds, should pass with tip", @@ -242,11 +244,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validFeeWithTip, } }, - RunAnte: true, - RunPost: true, - Simulate: false, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: false, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "signer has enough funds, should pass with tip - simulate", @@ -261,11 +264,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validFeeWithTip, } }, - RunAnte: true, - RunPost: true, - Simulate: true, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: true, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "signer has enough funds, should pass, no tip - resolvable denom", @@ -280,11 +284,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validResolvableFee, } }, - RunAnte: true, - RunPost: true, - Simulate: false, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: false, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "signer has enough funds, should pass, no tip - resolvable denom - simulate", @@ -299,11 +304,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validResolvableFee, } }, - RunAnte: true, - RunPost: true, - Simulate: true, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: true, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "signer has enough funds, should pass with tip - resolvable denom", @@ -318,11 +324,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validResolvableFeeWithTip, } }, - RunAnte: true, - RunPost: true, - Simulate: false, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: false, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "signer has enough funds, should pass with tip - resolvable denom - simulate", @@ -337,11 +344,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validResolvableFeeWithTip, } }, - RunAnte: true, - RunPost: true, - Simulate: true, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: true, + Simulate: true, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "0 gas given should pass in simulate - no fee", @@ -354,11 +362,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: nil, } }, - RunAnte: true, - RunPost: false, - Simulate: true, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: false, + Simulate: true, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "0 gas given should pass in simulate - fee", @@ -371,11 +380,12 @@ func TestPostHandle(t *testing.T) { FeeAmount: validFee, } }, - RunAnte: true, - RunPost: false, - Simulate: true, - ExpPass: true, - ExpErr: nil, + RunAnte: true, + RunPost: false, + Simulate: true, + ExpPass: true, + ExpErr: nil, + ExpectConsumedGas: expectedConsumedGas, }, { Name: "no fee - fail",