implement gas consume on denom creation #4983
Conversation
github-actions
bot
added
C:app-wiring
| return err | ||
| } | ||
| } | ||
|
|
||
| // if DenomCreationGasConsume is non-zero, consume the gas | ||
| if params.DenomCreationGasConsume != 0 { |
There was a problem hiding this comment.
What is the use case for allowing both DenomCreationFee and DenomCreationGasConsume?
The entire point of DenomCreationGasConsume is to remove the CreationFee token to make contract easier. What am I missing
There was a problem hiding this comment.
In your implementation the gas is only consumed if the fee is nil. However the variable name does not imply this behavior.
Imagine a developer setting gas consume to a non-zero value but the gas isn't consumed. How confusing would it be! The developer will need to read the comments or dig into the code to find out that they also need to set fee to nil.
In my opinion the logic in my implementation here is simpler and less likely to cause confusion.
There was a problem hiding this comment.
Gotcha good point. Will update our implementation
| return migrations, nil | ||
| } | ||
| } | ||
|
|
||
| func updateTokenFactoryParams(ctx sdk.Context, tokenFactoryKeeper *tokenfactorykeeper.Keeper) { | ||
| tokenFactoryKeeper.SetParams(ctx, tokenfactorytypes.NewParams(nil, NewDenomCreationGasConsume)) |
There was a problem hiding this comment.
I assume this is purposely setting the fee to nil, but commenting in case the intention was to add gas-consume without modifying the fee.
There was a problem hiding this comment.
@nicolaslara Proposal 489 (https://www.mintscan.io/osmosis/proposals/489) states that the fee is to be set "to zero in the UpgradeHandler of the next chain upgrade".
I'm going to add a test for the gas consumption then this PR should be ready.
|
One thing I want to get you guys' opinions on is the specific value for the gas consumption. Due to the very low gas price on Osmosis, the cost for creating a new denom will be significantly lower than the current 10 OSMO. If we set the gas consumption to 10M gas, then under What is Osmosis' block gas limit? |
120,000,000 osmosisd q params subspace baseapp BlockParams
key: BlockParams
subspace: baseapp
value: '{"max_bytes":"10485760","max_gas":"120000000"}' |
|
I'm setting the gas consume amount to 40M, which corresponds to 0.1 OSMO per denom created. |
|
|
||
| // if DenomCreationFee is non-zero, transfer the tokens from the creator | ||
| // account to community pool | ||
| if params.DenomCreationFee != nil { |
There was a problem hiding this comment.
would it be possible to add according unit tests to test this?
There was a problem hiding this comment.
@mattverse I've added the test, please see: e12c068
* implement gas consume on denom creation * apply state changes after running migrations * update readme * revert an unintended change * update changelog * adjust gas consume amount * add a test case for gas consumption
What is the purpose of the change
See on-chain proposal: https://www.mintscan.io/osmosis/proposals/489
This PR mirrors the implementation by Juno although implementation detail slightly differs (see
createdenom.go). cc @ReecepbcupsBrief Changelog
denom_creation_gas_consumeto tokenfactory paramsdenom_creation_gas_consumewhen creating a new denomdenom_creation_feeto nil anddenom_creation_gas_consumeto 10,000,000 gasTesting and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? yes