Cleanup/Optimize gas for swaps #5850

stackman27 · 2023-07-16T01:54:45Z

Closes: #5842

What is the purpose of the change

Theres one trivial extra GetPool call we can eliminate from all swaps by making compute swap methods take in a pool instead of poolID
I think its maybe two GetPools eliminatable, including the one in updatePoolForSwap? This one not as clear
Similarly we can use the multi-send api's from bank to optimize the final fund transfers

Testing and Verifying

tested on localosmosis by performing swap and comparing outputs

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

stackman27 · 2023-07-16T23:34:43Z

x/concentrated-liquidity/swaps.go

+	// }
+
+	// Send the input token from the user to the pool's primary address
+	err := k.bankKeeper.SendCoins(ctx, swapDetails.Sender, pool.GetAddress(), sdk.Coins{


note: i tried using multisend api to replace multiple sendCoins call, but the keeper function doesnot take multiple input address
ref: https://github.com/osmosis-labs/cosmos-sdk/blob/osmosis-main/x/bank/types/msgs.go#L174-L175

Right, we will want to still do multiple multi-sends, just only one send per sender!

x/concentrated-liquidity/swaps.go

p0mvn · 2023-07-17T22:00:22Z

x/concentrated-liquidity/swaps.go

-	// Send the output token to the sender from the pool
-	err = k.bankKeeper.SendCoins(ctx, pool.GetAddress(), swapDetails.Sender, sdk.Coins{
-		swapDetails.TokenOut,
-	})
+	err = k.bankKeeper.InputOutputCoins(ctx, []banktypes.Input{{
+		Address: pool.GetAddress().String(),
+		Coins:   sdk.NewCoins(swapDetails.TokenOut),
+	}}, []banktypes.Output{
+		{
+			Address: swapDetails.Sender.String(),
+			Coins:   sdk.NewCoins(swapDetails.TokenOut),
+		}})


What is the benefit of using multi-send API when we replace each regular API with a multi-send one?

I would assume that we would want to batch all inputs and all outputs and run a single multi-send API at the end.

yea i think so, using InputOutputCoins made gas slightly lower. ref: https://github.com/osmosis-labs/osmosis/pull/5850/files#r1266377932

This change here isn't quite what I had in mind. I was expecting to be doing a MultiSend (not an InputOutputCoins!) and combining the two sends from the sender! My bad, I forgot that MultiSend is the name of the SDK message, which doesn't actually use the sendManyCoins API.

https://github.com/osmosis-labs/cosmos-sdk/blob/osmosis-main/x/bank/keeper/send.go#L21C7-L21C7

This will eliminate the repeated-get & write for the sender

Though your change suggests an interesting idea, thats actually better than mine.

We could do all the sends here as a single InputOutputCoins call! That will give us a minimal number of I/O ops

This still does not make sense to me, this still looks like three separate bank sends to me.

InputOutputCoins is VERY optimal when there is 1inputSender and ManyOutputReciever because all inputs and outputs gets handled into one single operation.

however, we cannot do that in our case because we have different inputs and different outputs. I wanted to revert back to SendCoins because it is simpler however, i saw that Input/OutputCoins was slightly gas efficient (https://github.com/osmosis-labs/osmosis/pull/5850/files/46ad00d4e066c02ce9fa6e2775a45c41898bd58e#r1266377932)

stackman27 · 2023-07-18T07:52:59Z

x/concentrated-liquidity/swaps_test.go

+	// Make a large swap in InGivenOut (negative tick direction) from currentTick = 1735 to 736
+	// Amount of tickCrossed = 998
+	// Make sure the gas is < 50M
+	_, _, _, err = s.clk.SwapInAmtGivenOut(s.Ctx, swapAddress, pool, sdk.NewCoin(USDC, sdk.NewInt(50_000_000_000)), ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec())


ref:
swapGasAmount(sendCoins): 22_367_874
swapGasAmount(InputOutputCoins): 22_345_380

ValarDragon · 2023-07-18T11:07:56Z

This is state breaking and can't be backported due to affecting gas.

ValarDragon · 2023-07-18T11:08:30Z

Can you add a changelog for improving gas of CL swaps?

stackman27 · 2023-07-18T19:21:42Z

Can you add a changelog for improving gas of CL swaps?

added changelog

CHANGELOG.md

czarcas7ic · 2023-07-25T12:37:40Z

x/concentrated-liquidity/fuzz_test.go

+	pool, err := s.clk.GetPoolById(s.Ctx, pool.GetId())
+	s.Require().NoError(err)


Why is this needed in place of the pool object that is provided in the args? No state change occurs prior to this call on the pool obj as far as I see.

So pool obj change when we create first position. It updates the current Sqr Price https://github.com/osmosis-labs/osmosis/blob/main/x/concentrated-liquidity/lp.go#L476

x/concentrated-liquidity/fuzz_test.go

x/concentrated-liquidity/position_test.go

czarcas7ic · 2023-07-25T12:50:42Z

x/concentrated-liquidity/swaps.go

-	// Send the output token to the sender from the pool
-	err = k.bankKeeper.SendCoins(ctx, pool.GetAddress(), swapDetails.Sender, sdk.Coins{
-		swapDetails.TokenOut,
-	})
+	err = k.bankKeeper.InputOutputCoins(ctx, []banktypes.Input{{
+		Address: pool.GetAddress().String(),
+		Coins:   sdk.NewCoins(swapDetails.TokenOut),
+	}}, []banktypes.Output{
+		{
+			Address: swapDetails.Sender.String(),
+			Coins:   sdk.NewCoins(swapDetails.TokenOut),
+		}})


This still does not make sense to me, this still looks like three separate bank sends to me.

x/concentrated-liquidity/swaps_test.go

stackman27 · 2023-08-02T21:55:35Z

okay just tried to refactor and there was lots of unintended behaviors in core swap logic around state get/set. So will probably close this PR to not break the current flow

github-actions bot added the C:x/concentrated-liquidity label Jul 16, 2023

stackman27 commented Jul 16, 2023

View reviewed changes

stackman27 added A:backport/v16.x backport patches to v16.x branch A:no-changelog V:state/compatible/backport State machine compatible PR, should be backported labels Jul 16, 2023

stackman27 marked this pull request as ready for review July 16, 2023 23:36

stackman27 requested review from czarcas7ic and p0mvn as code owners July 16, 2023 23:36

stackman27 requested a review from ValarDragon July 16, 2023 23:37

stackman27 marked this pull request as draft July 17, 2023 00:04

stackman27 force-pushed the stack/less-gas-swap branch from 2324df2 to da0d51e Compare July 17, 2023 16:52

stackman27 marked this pull request as ready for review July 17, 2023 17:05

p0mvn reviewed Jul 17, 2023

View reviewed changes

stackman27 commented Jul 18, 2023

View reviewed changes

stackman27 requested a review from p0mvn July 18, 2023 08:00

ValarDragon added V:state/breaking State machine breaking PR and removed V:state/compatible/backport State machine compatible PR, should be backported A:no-changelog A:backport/v16.x backport patches to v16.x branch labels Jul 18, 2023

czarcas7ic reviewed Jul 25, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

czarcas7ic reviewed Jul 25, 2023

View reviewed changes

removed unnecessary getPool and added multiSend

88331b5

stackman27 requested a review from czarcas7ic July 28, 2023 17:56

added

5a74192

stackman27 force-pushed the stack/less-gas-swap branch from 8247513 to 5a74192 Compare July 28, 2023 18:06

czarcas7ic marked this pull request as draft July 31, 2023 14:29

swap update issue logs added

971e63f

added more logs

971be70

stackman27 closed this Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup/Optimize gas for swaps #5850

Cleanup/Optimize gas for swaps #5850

stackman27 commented Jul 16, 2023 •

edited

Loading

stackman27 Jul 16, 2023

ValarDragon Jul 16, 2023

p0mvn Jul 17, 2023

stackman27 Jul 18, 2023

ValarDragon Jul 18, 2023 •

edited

Loading

ValarDragon Jul 18, 2023 •

edited

Loading

czarcas7ic Jul 25, 2023

stackman27 Jul 25, 2023

stackman27 Jul 18, 2023

ValarDragon commented Jul 18, 2023

ValarDragon commented Jul 18, 2023

stackman27 commented Jul 18, 2023

czarcas7ic Jul 25, 2023

stackman27 Jul 25, 2023 •

edited

Loading

czarcas7ic Jul 25, 2023

stackman27 commented Aug 2, 2023

		pool, err := s.clk.GetPoolById(s.Ctx, pool.GetId())
		s.Require().NoError(err)

Cleanup/Optimize gas for swaps #5850

Cleanup/Optimize gas for swaps #5850

Conversation

stackman27 commented Jul 16, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

ValarDragon Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Jul 18, 2023

ValarDragon commented Jul 18, 2023

stackman27 commented Jul 18, 2023

Choose a reason for hiding this comment

stackman27 Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 commented Aug 2, 2023

stackman27 commented Jul 16, 2023 •

edited

Loading

ValarDragon Jul 18, 2023 •

edited

Loading

ValarDragon Jul 18, 2023 •

edited

Loading

stackman27 Jul 25, 2023 •

edited

Loading