-
Notifications
You must be signed in to change notification settings - Fork 607
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
Consensus min gas fee of .0025 uosmo #4244
Conversation
name: "doesn't work with not enough converted fee in checktx", | ||
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 1)), | ||
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom, | ||
sdk.MustNewDecFromStr("0.1"))), | ||
gasRequested: 10000, | ||
isCheckTx: true, | ||
expectPass: false, | ||
baseDenomGas: false, | ||
}, | ||
{ | ||
name: "works with not enough converted fee in delivertx", | ||
txFee: sdk.NewCoins(sdk.NewInt64Coin(uion, 1)), | ||
minGasPrices: sdk.NewDecCoins(sdk.NewDecCoinFromDec(baseDenom, | ||
sdk.MustNewDecFromStr("0.1"))), | ||
gasRequested: 10000, | ||
isCheckTx: false, | ||
expectPass: true, | ||
baseDenomGas: false, | ||
}, | ||
{ | ||
name: "multiple fee coins - checktx", | ||
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1), sdk.NewInt64Coin(uion, 1)), | ||
minGasPrices: sdk.NewDecCoins(), | ||
gasRequested: 10000, | ||
isCheckTx: true, | ||
expectPass: false, | ||
baseDenomGas: false, | ||
}, | ||
{ | ||
name: "multiple fee coins - delivertx", | ||
txFee: sdk.NewCoins(sdk.NewInt64Coin(baseDenom, 1), sdk.NewInt64Coin(uion, 1)), | ||
minGasPrices: sdk.NewDecCoins(), | ||
gasRequested: 10000, | ||
isCheckTx: false, | ||
expectPass: false, | ||
baseDenomGas: false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to be deduplicated across check/deliver
x/txfees/keeper/feedecorator_test.go
Outdated
expectPass: true, | ||
baseDenomGas: true, | ||
isCheckTx: true, | ||
expectPass: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deduplicated above
{ | ||
name: "no min gas price - checktx", | ||
txFee: sdk.NewCoins(), | ||
minGasPrices: sdk.NewDecCoins(), | ||
gasRequested: 10000, | ||
isCheckTx: true, | ||
expectPass: true, | ||
baseDenomGas: true, | ||
}, | ||
{ | ||
name: "no min gas price - delivertx", | ||
txFee: sdk.NewCoins(), | ||
minGasPrices: sdk.NewDecCoins(), | ||
gasRequested: 10000, | ||
isCheckTx: false, | ||
expectPass: true, | ||
baseDenomGas: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to be consensus min gas failure tests.
if tc.txFee[0].Denom != baseDenom { | ||
moduleName = types.NonNativeFeeCollectorName | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base denom gas was an unnecessary field, its role is automated with this if statement.
The failure in go tests is that our ibc-hooks tests use ibctesting. Ibctesting uses 0 fees in its tests, right here: https://github.com/cosmos/ibc-go/blob/v4.3.0/testing/chain.go#L312-L322 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌮
// setup fee pool, between "e2e_default_fee_token" and "uosmo" | ||
feePoolParams := balancer.NewPoolParams(sdk.MustNewDecFromStr("0.01"), sdk.ZeroDec(), nil) | ||
feePoolAssets := []balancer.PoolAsset{ | ||
{ | ||
Weight: sdk.NewInt(100), | ||
Token: sdk.NewCoin("uosmo", sdk.NewInt(100000000000)), | ||
}, | ||
{ | ||
Weight: sdk.NewInt(100), | ||
Token: sdk.NewCoin(E2EFeeToken, sdk.NewInt(100000000000)), | ||
}, | ||
} | ||
pool1, err := balancer.NewBalancerPool(1, feePoolParams, feePoolAssets, "", time.Unix(0, 0)) | ||
if err != nil { | ||
panic(err) | ||
} | ||
anyPool, err := types1.NewAnyWithValue(&pool1) | ||
if err != nil { | ||
panic(err) | ||
} | ||
gammGenState.Pools = []*types1.Any{anyPool} | ||
gammGenState.NextPoolNumber = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this and instead re-create this via CLI?
This interferes with pool IDs that we are creating via CLI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions:
- Why does it interfere?
- Where do I do it in CLI such that it comes first? It needs to be first so that other txs can pay fees, and we'd have to make distinct custom methods for paying fees to set this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
The reason is that we also create pre-upgrade pools as a result the pool id of 1 is already assigned after upgrade. It is possible to work around this by first making a separate PR to v14 genesis.
-
Makes sense. I will work around number 1 then and make the changes to v14 before main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, for 2, it should be possible to create a pool with the fee via pool file, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pool file doesn't contain the tx fee. (Adding something custom for the tx fee for initialization isn't really a big deal. It just has to come first, if you point me to where I do that, I can add it in relatively easily)
@@ -127,7 +128,7 @@ func (n *NodeConfig) SendIBCTransfer(from, recipient, amount, memo string) { | |||
|
|||
cmd := []string{"osmosisd", "tx", "ibc-transfer", "transfer", "transfer", "channel-0", recipient, amount, fmt.Sprintf("--from=%s", from), "--memo", memo} | |||
|
|||
_, _, err := n.containerManager.ExecTxCmdWithSuccessString(n.t, n.chainId, n.Name, cmd, "code: 0") | |||
_, _, err := n.containerManager.ExecTxCmd(n.t, n.chainId, n.Name, cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a driveby change to get the execution stack to have one entry point. (At the time I did this line, I thought I'd do the gas if statement in this method, ended up not doing that)
So can be reverted, but looks identical to the ExecTxCmd as far as I can tell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this is fine, was getting sidetracked by this but the change LGTM now.
I think I found the main problem. Should have the fix soon
I think we can disable geometric TWAP as well. These are likely related. I'm still investigating separately |
* Refactor tests a bit * Add consensus min gas fee * Fix tests * Delete more crud * Cleanup more duplicate test cases * flip isCheckTx sign * One more test needed * Run invalid fee denom test across both check & deliver * Remove extra line * Add hack to get around txfee problem in ibctesting * Handle genesis * Minor code cleanup * tryfix simulation * Fix for legacy simulator * Update x/txfees/keeper/feedecorator_test.go Co-authored-by: Matt, Park <[email protected]> * Apply matt comments * See what happens by adding fees * Add genesis logic update * Try fixing e2e * Add some better logging to find error * remove stat, fix typo * fix(e2e): fix e2e test for consensus min gas fee #4244 (#4317) * chore(e2e): update init image on main for #4244 * changes * fix more problems * more fixes * lint * twap gebug logs * arithmetic fix * skip twap test * link issue * Temp disable geo twap E2E --------- Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Roman <[email protected]> (cherry picked from commit 05fda4d)
* Refactor tests a bit * Add consensus min gas fee * Fix tests * Delete more crud * Cleanup more duplicate test cases * flip isCheckTx sign * One more test needed * Run invalid fee denom test across both check & deliver * Remove extra line * Add hack to get around txfee problem in ibctesting * Handle genesis * Minor code cleanup * tryfix simulation * Fix for legacy simulator * Update x/txfees/keeper/feedecorator_test.go Co-authored-by: Matt, Park <[email protected]> * Apply matt comments * See what happens by adding fees * Add genesis logic update * Try fixing e2e * Add some better logging to find error * remove stat, fix typo * fix(e2e): fix e2e test for consensus min gas fee #4244 (#4317) * chore(e2e): update init image on main for #4244 * changes * fix more problems * more fixes * lint * twap gebug logs * arithmetic fix * skip twap test * link issue * Temp disable geo twap E2E --------- Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Roman <[email protected]> (cherry picked from commit 05fda4d) Co-authored-by: Dev Ojha <[email protected]>
* Refactor tests a bit * Add consensus min gas fee * Fix tests * Delete more crud * Cleanup more duplicate test cases * flip isCheckTx sign * One more test needed * Run invalid fee denom test across both check & deliver * Remove extra line * Add hack to get around txfee problem in ibctesting * Handle genesis * Minor code cleanup * tryfix simulation * Fix for legacy simulator * Update x/txfees/keeper/feedecorator_test.go Co-authored-by: Matt, Park <[email protected]> * Apply matt comments * See what happens by adding fees * Add genesis logic update * Try fixing e2e * Add some better logging to find error * remove stat, fix typo * fix(e2e): fix e2e test for consensus min gas fee #4244 (#4317) * chore(e2e): update init image on main for #4244 * changes * fix more problems * more fixes * lint * twap gebug logs * arithmetic fix * skip twap test * link issue * Temp disable geo twap E2E --------- Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Roman <[email protected]>
* Refactor tests a bit * Add consensus min gas fee * Fix tests * Delete more crud * Cleanup more duplicate test cases * flip isCheckTx sign * One more test needed * Run invalid fee denom test across both check & deliver * Remove extra line * Add hack to get around txfee problem in ibctesting * Handle genesis * Minor code cleanup * tryfix simulation * Fix for legacy simulator * Update x/txfees/keeper/feedecorator_test.go Co-authored-by: Matt, Park <[email protected]> * Apply matt comments * See what happens by adding fees * Add genesis logic update * Try fixing e2e * Add some better logging to find error * remove stat, fix typo * fix(e2e): fix e2e test for consensus min gas fee #4244 (#4317) * chore(e2e): update init image on main for #4244 * changes * fix more problems * more fixes * lint * twap gebug logs * arithmetic fix * skip twap test * link issue * Temp disable geo twap E2E --------- Co-authored-by: Matt, Park <[email protected]> Co-authored-by: Roman <[email protected]>
Cref #3320 , does the raw osmo amount component, not the TWAP.
What is the purpose of the change
Implements a consensus min gas fee of .0025 uosmo / gas. The mempool policy is set to max(localMempoolPolicy, Consensus minimum)
This consensus minimum is not being exposed as a governance parameter right now, as it seems far more effective to get to adaptive, demand based fee markets (ala EIP-1559).
Brief Changelog
Testing and Verifying
I've updated the existing test vectors to account for the consensus minimum, which includes adapting no fee and sub-consensus minimum test vectors to fail. Furthermore this PR refactors the test vector setup to be more succinct. (Removing extraneous fields, and using a for loop to combine Check and Deliver)
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? yes