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

feat: Change the default priority mechanism to be based on gas price #12953

Merged
merged 9 commits into from
Aug 22, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#12187](https://github.com/cosmos/cosmos-sdk/pull/12187) Add batch operation for x/nft module.
* [#12693](https://github.com/cosmos/cosmos-sdk/pull/12693) Make sure the order of each node is consistent when emitting proto events.
* [#12455](https://github.com/cosmos/cosmos-sdk/pull/12455) Show attempts count in error for signing.
* [#]() Change the default priority mechanism to be based on gas price.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* [#]() Change the default priority mechanism to be based on gas price.
* [#12953](https://github.com/cosmos/cosmos-sdk/pull/12953) Change the default priority mechanism to be based on gas price.

julienrbrt marked this conversation as resolved.
Show resolved Hide resolved

### State Machine Breaking

Expand Down
10 changes: 5 additions & 5 deletions x/auth/ante/fee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func TestEnsureMempoolFees(t *testing.T) {
// msg and signatures
msg := testdata.NewTestMsg(accs[0].acc.GetAddress())
feeAmount := testdata.NewTestFeeAmount()
gasLimit := testdata.NewTestGasLimit()
gasLimit := uint64(15)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea, it makes the test easier to understand / read. In my version of the fee prioritization I'm scaling the gas based on fee amount (using QuoRaw)

require.NoError(t, s.txBuilder.SetMsgs(msg))
s.txBuilder.SetFeeAmount(feeAmount)
s.txBuilder.SetGasLimit(gasLimit)
Expand All @@ -71,7 +71,7 @@ func TestEnsureMempoolFees(t *testing.T) {
require.NoError(t, err)

// Set high gas price so standard test fee fails
atomPrice := sdk.NewDecCoinFromDec("atom", math.LegacyNewDec(200).Quo(math.LegacyNewDec(100000)))
atomPrice := sdk.NewDecCoinFromDec("atom", math.LegacyNewDec(20))
highGasPrice := []sdk.DecCoin{atomPrice}
s.ctx = s.ctx.WithMinGasPrices(highGasPrice)

Expand Down Expand Up @@ -103,9 +103,9 @@ func TestEnsureMempoolFees(t *testing.T) {

newCtx, err := antehandler(s.ctx, tx, false)
require.Nil(t, err, "Decorator should not have errored on fee higher than local gasPrice")
// Priority is the smallest amount in any denom. Since we have only 1 fee
// of 150atom, the priority here is 150.
require.Equal(t, feeAmount.AmountOf("atom").Int64(), newCtx.Priority())
// Priority is the smallest gas price amount in any denom. Since we have only 1 gas price
// of 10atom, the priority here is 10.
require.Equal(t, int64(10), newCtx.Priority())
}

func TestDeductFees(t *testing.T) {
Expand Down
9 changes: 5 additions & 4 deletions x/auth/ante/validator_tx_fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,19 @@ func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins,
}
}

priority := getTxPriority(feeCoins)
priority := getTxPriority(feeCoins, int64(gas))
return feeCoins, priority, nil
}

// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the fee
yihuang marked this conversation as resolved.
Show resolved Hide resolved
// provided in a transaction.
func getTxPriority(fee sdk.Coins) int64 {
func getTxPriority(fee sdk.Coins, gas int64) int64 {
var priority int64
for _, c := range fee {
p := int64(math.MaxInt64)
if c.Amount.IsInt64() {
p = c.Amount.Int64()
price := c.Amount.QuoRaw(gas)
if price.IsInt64() {
p = price.Int64()
yihuang marked this conversation as resolved.
Show resolved Hide resolved
}
if priority == 0 || p < priority {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
priority = p
Expand Down