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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Jul 17, 2022
Merged

x/gamm: Add fixed gas cost for swaps #2016

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
557fac4
add gas for swap + test
rrrliu Jul 11, 2022
88e6112
test file comments
rrrliu Jul 12, 2022
4bdfc68
move swap consumption to balancer only
rrrliu Jul 12, 2022
44abd66
fix one more test case
rrrliu Jul 12, 2022
7eed0c3
add to changelog
rrrliu Jul 13, 2022
5190924
Update CHANGELOG.md
rrrliu Jul 14, 2022
df0f26b
rm TODO now that we have issue
rrrliu Jul 14, 2022
5ea427f
underscore for clarity
rrrliu Jul 14, 2022
29ad32a
gofmt
rrrliu Jul 14, 2022
3cbb9dc
Merge branch 'main' into rrrliu/1903-charge-gas-for-computation
rrrliu Jul 15, 2022
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 x/gamm/keeper/swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ func (k Keeper) swapExactAmountIn(
}
tokensIn := sdk.Coins{tokenIn}

ctx.GasMeter().ConsumeGas(types.GasFeeForSwap, "swap computation")
Copy link
Contributor

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.

Copy link
Member

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)

Copy link
Member

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

Copy link
Member

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 ?

Copy link
Member

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)

Copy link
Contributor Author

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?

// Executes the swap in the pool and stores the output. Updates pool assets but
// does not actually transfer any tokens to or from the pool.
tokenOutCoin, err := pool.SwapOutAmtGivenIn(ctx, tokensIn, tokenOutDenom, swapFee)
Expand Down Expand Up @@ -113,6 +114,7 @@ func (k Keeper) swapExactAmountOut(
return sdk.Int{}, sdkerrors.Wrapf(types.ErrTooManyTokensOut,
"can't get more tokens out than there are tokens in the pool")
}
ctx.GasMeter().ConsumeGas(types.GasFeeForSwap, "swap computation")
tokenIn, err := pool.SwapInAmtGivenOut(ctx, sdk.Coins{tokenOut}, tokenInDenom, swapFee)
if err != nil {
return sdk.Int{}, err
Expand Down
8 changes: 8 additions & 0 deletions x/gamm/keeper/swap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,13 @@ func (suite *KeeperTestSuite) TestBalancerPoolSimpleSwapExactAmountIn() {
spotPriceBefore, err := keeper.CalculateSpotPrice(suite.Ctx, poolId, test.param.tokenIn.Denom, test.param.tokenOutDenom)
suite.NoError(err, "test: %v", test.name)

prevGasConsumed := suite.Ctx.GasMeter().GasConsumed()
tokenOutAmount, err := keeper.SwapExactAmountIn(suite.Ctx, suite.TestAccs[0], poolId, test.param.tokenIn, test.param.tokenOutDenom, test.param.tokenOutMinAmount)
suite.NoError(err, "test: %v", test.name)
suite.True(tokenOutAmount.Equal(test.param.expectedTokenOut), "test: %v", test.name)
gasConsumedForSwap := suite.Ctx.GasMeter().GasConsumed() - prevGasConsumed
// We consume 10,000 gas directly, so the extra I/O operation mean we end up consuming more.
suite.Assert().Greater(gasConsumedForSwap, uint64(10000))

spotPriceAfter, err := keeper.CalculateSpotPrice(suite.Ctx, poolId, test.param.tokenIn.Denom, test.param.tokenOutDenom)
suite.NoError(err, "test: %v", test.name)
Expand Down Expand Up @@ -188,11 +192,15 @@ func (suite *KeeperTestSuite) TestBalancerPoolSimpleSwapExactAmountOut() {
spotPriceBefore, err := keeper.CalculateSpotPrice(suite.Ctx, poolId, test.param.tokenInDenom, test.param.tokenOut.Denom)
suite.NoError(err, "test: %v", test.name)

prevGasConsumed := suite.Ctx.GasMeter().GasConsumed()
tokenInAmount, err := keeper.SwapExactAmountOut(suite.Ctx, suite.TestAccs[0], poolId, test.param.tokenInDenom, test.param.tokenInMaxAmount, test.param.tokenOut)
suite.NoError(err, "test: %v", test.name)
suite.True(tokenInAmount.Equal(test.param.expectedTokenInAmount),
"test: %v\n expect_eq actual: %s, expected: %s",
test.name, tokenInAmount, test.param.expectedTokenInAmount)
gasConsumedForSwap := suite.Ctx.GasMeter().GasConsumed() - prevGasConsumed
// We consume 10,000 gas directly, so the extra I/O operation mean we end up consuming more.
suite.Assert().Greater(gasConsumedForSwap, uint64(10000))
rrrliu marked this conversation as resolved.
Show resolved Hide resolved

spotPriceAfter, err := keeper.CalculateSpotPrice(suite.Ctx, poolId, test.param.tokenInDenom, test.param.tokenOut.Denom)
suite.NoError(err, "test: %v", test.name)
Expand Down
2 changes: 2 additions & 0 deletions x/gamm/types/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
rrrliu marked this conversation as resolved.
Show resolved Hide resolved
GasFeeForSwap = 10000
Copy link
Contributor

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?

Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu Jul 11, 2022 •
edited
Loading

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))

)

var (
Expand Down