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

implement gas consume on denom creation #4983

Merged
merged 9 commits into from
May 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#4682](https://github.com/osmosis-labs/osmosis/pull/4682) Deprecate x/gamm SpotPrice v2 query. The new one is located in x/poolmanager.
* [#4801](https://github.com/osmosis-labs/osmosis/pull/4801) remove GetTotalShares, GetTotalLiquidity and GetExitFee from PoolI. Define all on CFMMPoolI, define GetTotalLiquidity on PoolModuleI only.
* [#4868](https://github.com/osmosis-labs/osmosis/pull/4868) Remove wasmEnabledProposals []wasm.ProposalType from NewOsmosisApp
* [#4983](https://github.com/osmosis-labs/osmosis/pull/4983) Consume a gas when creating a new token using tokenfactory as a spam deterrence mechanism.
* [#4951](https://github.com/osmosis-labs/osmosis/pull/4951) Implement pool liquidity query in pool manager, deprecate the one in gamm


## v15.1.0

### Security
Expand Down
6 changes: 6 additions & 0 deletions app/upgrades/v16/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ import (
// UpgradeName defines the on-chain upgrade name for the Osmosis v16 upgrade.
const UpgradeName = "v16"

// new token factory parameters
//
// at the current gas price of 0.0025uosmo, this corresponds to 0.1 OSMO per
// denom creation.
const NewDenomCreationGasConsume uint64 = 40_000_000

var Upgrade = upgrades.Upgrade{
UpgradeName: UpgradeName,
CreateUpgradeHandler: CreateUpgradeHandler,
Expand Down
5 changes: 5 additions & 0 deletions app/upgrades/v16/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
gammkeeper "github.com/osmosis-labs/osmosis/v15/x/gamm/keeper"
"github.com/osmosis-labs/osmosis/v15/x/poolmanager"
poolmanagertypes "github.com/osmosis-labs/osmosis/v15/x/poolmanager/types"
tokenfactorykeeper "github.com/osmosis-labs/osmosis/v15/x/tokenfactory/keeper"
)

var (
Expand All @@ -22,3 +23,7 @@ func CreateConcentratedPoolFromCFMM(ctx sdk.Context, cfmmPoolIdToLinkWith uint64
func CreateCanonicalConcentratedLiuqidityPoolAndMigrationLink(ctx sdk.Context, cfmmPoolId uint64, desiredDenom0 string, keepers *keepers.AppKeepers) error {
return createCanonicalConcentratedLiquidityPoolAndMigrationLink(ctx, cfmmPoolId, desiredDenom0, keepers)
}

func UpdateTokenFactoryParams(ctx sdk.Context, tokenFactoryKeeper *tokenfactorykeeper.Keeper) {
updateTokenFactoryParams(ctx, tokenFactoryKeeper)
}
9 changes: 9 additions & 0 deletions app/upgrades/v16/upgrades.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import (

"github.com/osmosis-labs/osmosis/v15/app/keepers"
"github.com/osmosis-labs/osmosis/v15/app/upgrades"

tokenfactorykeeper "github.com/osmosis-labs/osmosis/v15/x/tokenfactory/keeper"
tokenfactorytypes "github.com/osmosis-labs/osmosis/v15/x/tokenfactory/types"
)

const (
Expand Down Expand Up @@ -81,6 +84,12 @@ func CreateUpgradeHandler(
return nil, err
}

updateTokenFactoryParams(ctx, keepers.TokenFactoryKeeper)

return migrations, nil
}
}

func updateTokenFactoryParams(ctx sdk.Context, tokenFactoryKeeper *tokenfactorykeeper.Keeper) {
tokenFactoryKeeper.SetParams(ctx, tokenfactorytypes.NewParams(nil, NewDenomCreationGasConsume))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

mattverse marked this conversation as resolved.
Show resolved Hide resolved
}
4 changes: 4 additions & 0 deletions app/upgrades/v16/upgrades_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ func (suite *UpgradeTestSuite) TestUpgrade() {
suite.Require().False(params.IsPermissionlessPoolCreationEnabled)
},
func() {
// Validate that tokenfactory params have been updated
params := suite.App.TokenFactoryKeeper.GetParams(suite.Ctx)
suite.Require().Nil(params.DenomCreationFee)
suite.Require().Equal(v16.NewDenomCreationGasConsume, params.DenomCreationGasConsume)
},
},
{
Expand Down
12 changes: 12 additions & 0 deletions proto/osmosis/tokenfactory/v1beta1/params.proto
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,21 @@ option go_package = "github.com/osmosis-labs/osmosis/v15/x/tokenfactory/types";

// Params defines the parameters for the tokenfactory module.
message Params {
// DenomCreationFee defines the fee to be charged on the creation of a new
// denom. The fee is drawn from the MsgCreateDenom's sender account, and
// transferred to the community pool.
repeated cosmos.base.v1beta1.Coin denom_creation_fee = 1 [
(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
(gogoproto.moretags) = "yaml:\"denom_creation_fee\"",
(gogoproto.nullable) = false
];

// DenomCreationGasConsume defines the gas cost for creating a new denom.
// This is intended as a spam deterrence mechanism.
//
// See: https://github.com/CosmWasm/token-factory/issues/11
uint64 denom_creation_gas_consume = 2 [
(gogoproto.moretags) = "yaml:\"denom_creation_gas_consume\"",
(gogoproto.nullable) = true
];
}
2 changes: 2 additions & 0 deletions x/tokenfactory/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ message MsgCreateDenom {

- Fund community pool with the denom creation fee from the creator address, set
in `Params`.
- Consume an amount of gas corresponding to the `DenomCreationGasConsume` parameter
specified in `Params`.
- Set `DenomMetaData` via bank keeper.
- Set `AuthorityMetadata` for the given denom to store the admin for the created
denom `factory/{creator address}/{subdenom}`. Admin is automatically set as the
Expand Down
25 changes: 17 additions & 8 deletions x/tokenfactory/keeper/createdenom.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,25 @@ func (k Keeper) validateCreateDenom(ctx sdk.Context, creatorAddr string, subdeno
}

func (k Keeper) chargeForCreateDenom(ctx sdk.Context, creatorAddr string, subdenom string) (err error) {
// Send creation fee to community pool
creationFee := k.GetParams(ctx).DenomCreationFee
accAddr, err := sdk.AccAddressFromBech32(creatorAddr)
if err != nil {
return err
}
if creationFee != nil {
if err := k.communityPoolKeeper.FundCommunityPool(ctx, creationFee, accAddr); err != nil {
params := k.GetParams(ctx)

// if DenomCreationFee is non-zero, transfer the tokens from the creator
// account to community pool
if params.DenomCreationFee != nil {
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to add according unit tests to test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattverse I've added the test, please see: e12c068

accAddr, err := sdk.AccAddressFromBech32(creatorAddr)
if err != nil {
return err
}

if err := k.communityPoolKeeper.FundCommunityPool(ctx, params.DenomCreationFee, accAddr); err != nil {
return err
}
}

// if DenomCreationGasConsume is non-zero, consume the gas
if params.DenomCreationGasConsume != 0 {
Copy link
Contributor

@Reecepbcups Reecepbcups Apr 22, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha good point. Will update our implementation

ctx.GasMeter().ConsumeGas(params.DenomCreationGasConsume, "consume denom creation gas")
}

return nil
}
65 changes: 65 additions & 0 deletions x/tokenfactory/keeper/createdenom_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,68 @@ func (suite *KeeperTestSuite) TestCreateDenom() {
})
}
}

func (suite *KeeperTestSuite) TestGasConsume() {
// It's hard to estimate exactly how much gas will be consumed when creating a
// denom, because besides consuming the gas specified by the params, the keeper
// also does a bunch of other things that consume gas.
//
// Rather, we test whether the gas consumed is within a range. Specifically,
// the range [gasConsume, gasConsume + offset]. If the actual gas consumption
// falls within the range for all test cases, we consider the test passed.
//
// In experience, the total amount of gas consumed should consume be ~30k more
// than the set amount.
const offset = 50000

for _, tc := range []struct {
desc string
gasConsume uint64
}{
{
desc: "gas consume zero",
gasConsume: 0,
},
{
desc: "gas consume 1,000,000",
gasConsume: 1_000_000,
},
{
desc: "gas consume 10,000,000",
gasConsume: 10_000_000,
},
{
desc: "gas consume 25,000,000",
gasConsume: 25_000_000,
},
{
desc: "gas consume 50,000,000",
gasConsume: 50_000_000,
},
{
desc: "gas consume 200,000,000",
gasConsume: 200_000_000,
},
} {
suite.SetupTest()
suite.Run(fmt.Sprintf("Case %s", tc.desc), func() {
// set params with the gas consume amount
suite.App.TokenFactoryKeeper.SetParams(suite.Ctx, types.NewParams(nil, tc.gasConsume))

// amount of gas consumed prior to the denom creation
gasConsumedBefore := suite.Ctx.GasMeter().GasConsumed()

// create a denom
_, err := suite.msgServer.CreateDenom(sdk.WrapSDKContext(suite.Ctx), types.NewMsgCreateDenom(suite.TestAccs[0].String(), "larry"))
suite.Require().NoError(err)

// amount of gas consumed after the denom creation
gasConsumedAfter := suite.Ctx.GasMeter().GasConsumed()

// the amount of gas consumed must be within the range
gasConsumed := gasConsumedAfter - gasConsumedBefore
suite.Require().Greater(gasConsumed, tc.gasConsume)
suite.Require().Less(gasConsumed, tc.gasConsume+offset)
})
}
}
21 changes: 17 additions & 4 deletions x/tokenfactory/types/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,27 @@ import (

// Parameter store keys.
var (
KeyDenomCreationFee = []byte("DenomCreationFee")
KeyDenomCreationFee = []byte("DenomCreationFee")
KeyDenomCreationGasConsume = []byte("DenomCreationGasConsume")
)

// ParamTable for gamm module.
func ParamKeyTable() paramtypes.KeyTable {
return paramtypes.NewKeyTable().RegisterParamSet(&Params{})
}

func NewParams(denomCreationFee sdk.Coins) Params {
func NewParams(denomCreationFee sdk.Coins, denomCreationGasConsume uint64) Params {
return Params{
DenomCreationFee: denomCreationFee,
DenomCreationFee: denomCreationFee,
DenomCreationGasConsume: denomCreationGasConsume,
}
}

// default gamm module parameters.
func DefaultParams() Params {
return Params{
DenomCreationFee: sdk.NewCoins(sdk.NewInt64Coin(appparams.BaseCoinUnit, 10_000_000)), // 10 OSMO
DenomCreationFee: sdk.NewCoins(sdk.NewInt64Coin(appparams.BaseCoinUnit, 10_000_000)), // 10 OSMO
DenomCreationGasConsume: 0,
}
}

Expand All @@ -45,6 +48,7 @@ func (p Params) Validate() error {
func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
return paramtypes.ParamSetPairs{
paramtypes.NewParamSetPair(KeyDenomCreationFee, &p.DenomCreationFee, validateDenomCreationFee),
paramtypes.NewParamSetPair(KeyDenomCreationGasConsume, &p.DenomCreationGasConsume, validateDenomCreationGasConsume),
}
}

Expand All @@ -60,3 +64,12 @@ func validateDenomCreationFee(i interface{}) error {

return nil
}

func validateDenomCreationGasConsume(i interface{}) error {
_, ok := i.(uint64)
if !ok {
return fmt.Errorf("invalid parameter type: %T", i)
}

return nil
}
87 changes: 66 additions & 21 deletions x/tokenfactory/types/params.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.