-
Notifications
You must be signed in to change notification settings - Fork 608
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
Changes from 8 commits
ea2f1a3
0143df5
f6a259b
9ae6a57
c4b6961
a7e17c2
c94a11f
ba70225
e12c068
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be possible to add according unit tests to test this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
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.
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.