-
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
x/gamm: Add fixed gas cost for swaps #2016
x/gamm: Add fixed gas cost for swaps #2016
Conversation
x/gamm/types/constants.go
Outdated
@@ -12,6 +12,8 @@ const ( | |||
// Raise 10 to the power of SigFigsExponent to determine number of significant figures. | |||
// i.e. SigFigExponent = 8 is 10^8 which is 100000000. This gives 8 significant figures. | |||
SigFigsExponent = 8 | |||
// TODO: Current fixed cost gas fee per swap -- turn this into a param in the future. | |||
GasFeeForSwap = 10000 |
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.
Is there a reason to not directly just make this a param?
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.
We decided to make it a fixed cost for now to avoid needing to set up migration code for a change this small (#1903 (comment))
x/gamm/keeper/swap.go
Outdated
@@ -49,6 +49,7 @@ func (k Keeper) swapExactAmountIn( | |||
} | |||
tokensIn := sdk.Coins{tokenIn} | |||
|
|||
ctx.GasMeter().ConsumeGas(types.GasFeeForSwap, "swap computation") |
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.
It feels safer to move the gas consumption to the top-most level of abstraction, i.e the msg
handler for swapping.
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 think it should be in each core loop here right, so that we scale by number of hops in a multi-hop?
(Also needs to be in swapExactAmountOut)
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 guess really it should probably be defined per pool type
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.
Thoughts on applying it right here: https://github.com/osmosis-labs/osmosis/blob/main/x/gamm/pool-models/balancer/pool.go#L585-L599 ?
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.
That way we have it per Balancer swap, rather than per total swap. (And can then later charge a different gas amount for stableswap / next CFMM)
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 that makes sense, just updated! Should we update balancer pool docs with this or should we wait until it becomes a param?
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! Can we add a Changelog entry for this PR?
Also out of curiousity, how was the constant 10000 decided? |
x/gamm/types/constants.go
Outdated
@@ -12,6 +12,8 @@ const ( | |||
// Raise 10 to the power of SigFigsExponent to determine number of significant figures. | |||
// i.e. SigFigExponent = 8 is 10^8 which is 100000000. This gives 8 significant figures. | |||
SigFigsExponent = 8 | |||
// TODO: Turn this into a param in the future. |
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.
Great idea! Can we make an issue for this please and remove the comment?
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.
Yes, issue please with a reference 👍
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.
Created one! #2058
x/gamm/types/constants.go
Outdated
@@ -12,6 +12,8 @@ const ( | |||
// Raise 10 to the power of SigFigsExponent to determine number of significant figures. | |||
// i.e. SigFigExponent = 8 is 10^8 which is 100000000. This gives 8 significant figures. | |||
SigFigsExponent = 8 | |||
// TODO: Turn this into a param in the future. |
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.
Yes, issue please with a reference 👍
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
@mattverse oh I just benchmarked the swap i/o gas and it was around ~40000, so decided to have it be something around the same order of magnitude. No strong opinion on this though so open to changing. |
Thanks for making this! |
@rrrliu ah gotcha thanks for explaining! Nice work done here! |
* add gas for swap + test * test file comments * move swap consumption to balancer only * fix one more test case * add to changelog * Update CHANGELOG.md Co-authored-by: Aleksandr Bezobchuk <[email protected]> * rm TODO now that we have issue * underscore for clarity * gofmt Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Closes: #1903
What is the purpose of the change
To further prevent spam, adds fixed amount of gas we consume per swap in/out. Previously we had only been consuming gas for built-in I/O (store reads and writes), and in the future we might make
Brief Changelog
BalancerGasFeeForSwap
constant + TODO comment to turn into paramswapExactAmountIn
andswapExactAmountOut
consume gasswap_test.go
Testing and Verifying
This change added tests and can be verified as follows:
swap_test.go
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? yesx/<module>/spec/
) / Osmosis docs repo / not documented)