Skip to content

Commit

Permalink
Refactor gasprice.IsGTE to return an error instead of causing a panic.
Browse files Browse the repository at this point in the history
  • Loading branch information
piux2 committed Nov 28, 2024
1 parent 1504d8f commit 53dfbd3
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 52 deletions.
4 changes: 3 additions & 1 deletion contribs/gnodev/go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
module github.com/gnolang/gno/contribs/gnodev

go 1.22.0
go 1.22

toolchain go1.22.4

replace github.com/gnolang/gno => ../..

Expand Down
8 changes: 4 additions & 4 deletions contribs/gnodev/pkg/dev/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -388,10 +388,10 @@ func (n *Node) rebuildNodeFromState(ctx context.Context) error {
if err != nil {
return fmt.Errorf("unable to load pkgs: %w", err)
}

return n.rebuildNode(ctx, gnoland.GnoGenesisState{
Balances: n.config.BalancesList, Txs: txs,
})
genesis := gnoland.DefaultGenState()
genesis.Balances = n.config.BalancesList
genesis.Txs = txs
return n.rebuildNode(ctx, genesis)
}

state, err := n.getBlockStoreState(ctx)
Expand Down
8 changes: 7 additions & 1 deletion tm2/pkg/sdk/auth/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,13 @@ func EnsureSufficientMempoolFees(ctx sdk.Context, fee std.Fee) sdk.Result {
}
// check the block gas price
if blockGasPrice.Price.IsValid() && !blockGasPrice.Price.IsZero() {
if !feeGasPrice.IsGTE(blockGasPrice) {
ok, err := feeGasPrice.IsGTE(blockGasPrice)
if err != nil {
return abciResult(std.ErrInsufficientFee(
err.Error(),
))
}
if !ok {
return abciResult(std.ErrInsufficientFee(
fmt.Sprintf(
"insufficient fees; got: {Gas-Wanted: %d, Gas-Fee %s}, fee required: %+v as block gas price", feeGasPrice.Gas, feeGasPrice.Price, blockGasPrice,
Expand Down
28 changes: 28 additions & 0 deletions tm2/pkg/sdk/auth/ante_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/gnolang/gno/tm2/pkg/amino"
Expand Down Expand Up @@ -918,3 +919,30 @@ func TestEnsureBlockGasPrice(t *testing.T) {
)
}
}

func TestInvalidUserFee(t *testing.T) {
minGasPrice, err := std.ParseGasPrice("3ugnot/10gas") // 0.3ugnot
require.NoError(t, err)

blockGasPrice, err := std.ParseGasPrice("400ugnot/2000gas") // 0.2ugnot
require.NoError(t, err)

userFee1 := std.NewFee(0, std.NewCoin("ugnot", 50))
userFee2 := std.NewFee(100, std.NewCoin("uatom", 50))

// setup
env := setupTestEnv()
ctx := env.ctx

ctx = ctx.WithMinGasPrices(
[]std.GasPrice{minGasPrice},
)
ctx = ctx.WithValue(GasPriceContextKey{}, blockGasPrice)
res1 := EnsureSufficientMempoolFees(ctx, userFee1)
require.False(t, res1.IsOK())
assert.Contains(t, res1.Log, "GasPrice.Gas cannot be zero;")

res2 := EnsureSufficientMempoolFees(ctx, userFee2)
require.False(t, res2.IsOK())
assert.Contains(t, res2.Log, "Gas price denominations should be equal;")
}
13 changes: 6 additions & 7 deletions tm2/pkg/std/gasprice.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package std

import (
"fmt"
"math/big"
"strings"

Expand Down Expand Up @@ -54,14 +53,14 @@ func ParseGasPrices(gasprices string) (res []GasPrice, err error) {
return res, nil
}

// IsGTE compares the GasPrice with another gas price B. If coin denom matches AND fee per gas is
// greater or equal to the gas price B return true, other wise return false,
func (gp GasPrice) IsGTE(gpB GasPrice) bool {
// IsGTE compares the GasPrice with another gas price B. If the coin denom matches AND the fee per gas
// is greater than or equal to gas price B, return true; otherwise, return false.
func (gp GasPrice) IsGTE(gpB GasPrice) (bool, error) {
if gp.Price.Denom != gpB.Price.Denom {
panic(fmt.Sprintf("gas price denominations should be equal; %s, %s", gp.Price.Denom, gpB.Price.Denom))
return false, errors.New("Gas price denominations should be equal; %s, %s", gp.Price.Denom, gpB.Price.Denom)
}
if gp.Gas == 0 || gpB.Gas == 0 {
panic(fmt.Sprintf("GasPrice.Gas cannot be zero; %+v, %+v", gp, gpB))
return false, errors.New("GasPrice.Gas cannot be zero; %+v, %+v", gp, gpB)
}

gpg := big.NewInt(gp.Gas)
Expand All @@ -76,5 +75,5 @@ func (gp GasPrice) IsGTE(gpB GasPrice) bool {
// That the Fee / GasWanted ratio is greater than or equal to the minimum GasPrice per gas.
// This approach helps us avoid dealing with configurations where the value of
// the minimum gas price is set to 0.00001ugnot/gas.
return prod1.Cmp(prod2) >= 0
return prod1.Cmp(prod2) >= 0, nil
}
67 changes: 28 additions & 39 deletions tm2/pkg/std/gasprice_test.go
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
package std

import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestGasPriceGTE(t *testing.T) {
tests := []struct {
name string
gp GasPrice
gpB GasPrice
expectPanic bool
panicMsg string
expected bool // for non-panic cases: whether gp.IsGTE(gpB) should return true or false
expectError bool
errorMsg string
expected bool // for non-error cases: whether gp.IsGTE(gpB) should return true or false
}{
// Panic cases: Different denominations
// Error cases: Different denominations
{
name: "Different Denominations Panic",
name: "Different denominations error",
gp: GasPrice{
Gas: 100,
Price: Coin{
Expand All @@ -31,12 +33,12 @@ func TestGasPriceGTE(t *testing.T) {
Amount: 500,
},
},
expectPanic: true,
panicMsg: "gas price denominations should be equal",
expectError: true,
errorMsg: "Gas price denominations should be equal;",
},
// Panic cases: Zero Gas values
// Error cases: Zero Gas values
{
name: "Zero Gas in gp Panic",
name: "Zero Gas in gp error",
gp: GasPrice{
Gas: 0, // Zero Gas in gp
Price: Coin{
Expand All @@ -51,11 +53,11 @@ func TestGasPriceGTE(t *testing.T) {
Amount: 500,
},
},
expectPanic: true,
panicMsg: "GasPrice.Gas cannot be zero",
expectError: true,
errorMsg: "GasPrice.Gas cannot be zero;",
},
{
name: "Zero Gas in gpB Panic",
name: "Zero Gas in gpB error",
gp: GasPrice{
Gas: 100,
Price: Coin{
Expand All @@ -70,10 +72,10 @@ func TestGasPriceGTE(t *testing.T) {
Amount: 500,
},
},
expectPanic: true,
panicMsg: "GasPrice.Gas cannot be zero",
expectError: true,
errorMsg: "GasPrice.Gas cannot be zero;",
},
// Valid cases: No panic, just compare gas prices
// Valid cases: No errors, just compare gas prices
{
name: "Greater Gas Price",
gp: GasPrice{
Expand All @@ -90,7 +92,7 @@ func TestGasPriceGTE(t *testing.T) {
Amount: 500,
},
},
expectPanic: false,
expectError: false,
expected: true,
},
{
Expand All @@ -109,7 +111,7 @@ func TestGasPriceGTE(t *testing.T) {
Amount: 500,
},
},
expectPanic: false,
expectError: false,
expected: true,
},
{
Expand All @@ -128,37 +130,24 @@ func TestGasPriceGTE(t *testing.T) {
Amount: 500,
},
},
expectPanic: false,
expectError: false,
expected: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
defer func() {
if r := recover(); r != nil {
if !tt.expectPanic {
t.Errorf("Test %s failed: expected no panic, but got panic: %v", tt.name, r)
} else if tt.expectPanic && r != nil {
// Check if the panic message contains the expected substring
panicMsg := r.(string)
if tt.expectPanic && !strings.Contains(panicMsg, tt.panicMsg) {
t.Errorf("Test %s failed: expected panic message containing %q, but got %q", tt.name, tt.panicMsg, panicMsg)
}
}
} else if tt.expectPanic {
t.Errorf("Test %s failed: expected panic, but no panic occurred", tt.name)
}
}()

if !tt.expectPanic {
got := tt.gp.IsGTE(tt.gpB)
got, err := tt.gp.IsGTE(tt.gpB)
if !tt.expectError {
require.NoError(t, err)
assert.Equal(t, tt.expected, got, "Expect that %v is less than %v", tt.gp, tt.gpB)
if got != tt.expected {
t.Errorf("Test %s failed: expected result %v, got %v", tt.name, tt.expected, got)
}
} else {
// This will panic, but we handle it in the defer/recover above.
_ = tt.gp.IsGTE(tt.gpB)
require.Error(t, err)
errorMsg := err.Error()
assert.Contains(t, errorMsg, tt.errorMsg, "expected error message containing %q, but got %q", tt.errorMsg, errorMsg)
}
})
}
Expand Down

0 comments on commit 53dfbd3

Please sign in to comment.