Get underlying assets of tick range #6595

hieuvubk · 2023-09-29T08:41:31Z

Closes: #6340

What is the purpose of the change

Create a query to get underlying assets of a tick range but currently we can only get the assets of 2 consecutive ticks, need to think of a way to get assets between any 2 ticks.

(E.g.: This pull request improves documentation of area A by adding ....

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added unit test that validates ...
Added integration tests for end-to-end deployment with ...
Extended integration test for ...
Manually verified the change by ...

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

pysel · 2023-09-29T19:55:56Z

hey @hieuvubk. could you please mark this as draft while CI is not passing? thanks!

…osis-labs/osmosis into hieu/query_tick_range_assets

pysel · 2023-10-02T15:58:43Z

x/concentrated-liquidity/client/query_proto_wrap.go

@@ -318,3 +318,16 @@ func (q Querier) NumNextInitializedTicks(ctx sdk.Context, req clquery.NumNextIni

 	return &clquery.NumNextInitializedTicksResponse{LiquidityDepths: liquidityDepths, CurrentLiquidity: pool.GetLiquidity(), CurrentTick: pool.GetCurrentTick()}, nil
 }
+
+// TickRangeUnderlyingAssets returns uderlying asset of a tick range


Suggested change

// TickRangeUnderlyingAssets returns uderlying asset of a tick range

// TickRangeUnderlyingAssets returns underlying asset of a tick range

pysel · 2023-10-02T15:59:26Z

x/concentrated-liquidity/query.go

@@ -336,3 +336,93 @@ func (k Keeper) GetNumNextInitializedTicks(ctx sdk.Context, poolId, numberOfNext

 	return liquidityDepths, nil
 }
+
+// GetNumNextInitializedTicks is a method that returns underlying assets of a tick range


I think godoc has a wrong function name here

pysel · 2023-10-02T16:00:15Z

x/concentrated-liquidity/query.go

+// GetNumNextInitializedTicks is a method that returns underlying assets of a tick range
+// Errors:
+// * ticks given not found
+// * There exists at least 1 tick inside the given range


I might be missing something, but why do we have to restrict a query for only consecutive ticks?

Same question for me as well

Sr guys for the late reply, I'm keeping the current logic limited to 2 consecutive ticks because it's quite confusing with the tick range overlaps different positions.
Lets say I have an example below, range [10, 13] is not the intersection of the 3 given positions while [10, 11] is the intersection of positions 10 and 50 have liquidity 60, [11, 13] is the intersection of 3 positions have liquidity 160. So what liquidity of range [10, 13] should be, I'm leaning towards 100 because the tick range is wider but I'm not sure.
If u can help me with this issue it would be great thank so so much
cc @pysel @mattverse

Sr guys for the late reply, I'm keeping the current logic limited to 2 consecutive ticks because it's quite confusing with the tick range overlaps different positions.
Lets say I have an example below, range [10, 13] is not the intersection of the 3 given positions while [10, 11] is the intersection of positions 10 and 50 have liquidity 60, [11, 13] is the intersection of 3 positions have liquidity 160. So what liquidity of range [10, 13] should be, I'm leaning towards 60 because the tick range is wider but I'm not sure.
If u can help me with this issue it would be great thank so so much
cc @pysel @mattverse

pysel · 2023-10-02T16:20:05Z

x/concentrated-liquidity/query.go

+	// Find first tick initialized
+	// use false for zeroForOne since we're going from lower tick -> upper tick
+	zeroForOne := false
+	swapStrategy := swapstrategy.New(zeroForOne, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())


I think we should not introduce a variable if we only use it once

Suggested change

swapStrategy := swapstrategy.New(zeroForOne, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())

swapStrategy := swapstrategy.New(false, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())

pysel · 2023-10-02T16:21:32Z

x/concentrated-liquidity/query.go

+	zeroForOne := false
+	swapStrategy := swapstrategy.New(zeroForOne, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())
+
+	nextTickIter := swapStrategy.InitializeNextTickIterator(ctx, poolId, types.MinCurrentTick)


asking to learn: is it possible to get an iterator without swap strategy? it is weird using swap strategy in this context

also, why are we using a minimum tick here and not a lower tick?

yeah we use swap strategy for iterating over ticks, perhaps we need to rename it some day

Yeah I feel like we should be starting from lower tick

pysel · 2023-10-02T23:08:21Z

x/concentrated-liquidity/query_test.go

+	// Create a full-range position
+	data0, err := s.App.ConcentratedLiquidityKeeper.CreatePosition(
+		s.Ctx,
+		1,


nit: just a personal pedantic suggestion: I think it would be better to dynamically get pool's id from returned by PrepareCustomConcentratedPool pool extension

pysel · 2023-10-02T23:09:19Z

x/concentrated-liquidity/query_test.go

+		s.Ctx,
+		1,
+		s.TestAccs[0],
+		sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(1000000)), sdk.NewCoin("atom", sdk.NewInt(1000000))),


nit: we can define these coins at the beginning of a test

pysel · 2023-10-02T23:11:34Z

x/concentrated-liquidity/query_test.go

+	tests := []struct {
+		name            string
+		req             queryproto.TickRangeUnderlyingAssetsRequest
+		expectedAmount0 sdk.Int


Suggested change

expectedAmount0 sdk.Int

expectedAmount0 osmomath.Int

pysel · 2023-10-02T23:13:47Z

x/concentrated-liquidity/query_test.go

+				UpperTick: types.MaxTick,
+			},
+			expectedAmount0: math.CalcAmount0Delta(osmomath.BigDecFromDec(data2.Liquidity), positive100TickSqrtPrice, maxTickSqrtPrice, true).Dec().TruncateInt(),
+			expectedAmount1: sdk.ZeroInt(),


we are using osmomath's numericals

Suggested change

expectedAmount1: sdk.ZeroInt(),

expectedAmount1: osmomath.ZeroInt(),

pysel · 2023-10-02T23:17:56Z

x/concentrated-liquidity/query_test.go

+		s.Run(test.name, func() {
+			res, err := s.App.ConcentratedLiquidityKeeper.TickRangeUnderlyingAssets(s.Ctx, test.req.PoolId, test.req.LowerTick, test.req.UpperTick)
+			if test.expectedError {
+				s.Require().Error(err)


nit: shall we check for a specific error in this test?

mattverse

Left some comments please take a look

Generally, I don't see how this differs from the GetTickLiquidityNetInDirection query, @hieuvubk can you explain how this is different from the GetTickLiquidityNetInDirection query?

mattverse · 2023-09-30T07:10:27Z

x/concentrated-liquidity/client/query_proto_wrap.go

+
+// TickRangeUnderlyingAssets returns uderlying asset of a tick range
+func (q Querier) TickRangeUnderlyingAssets(ctx sdk.Context, req clquery.TickRangeUnderlyingAssetsRequest) (*clquery.TickRangeUnderlyingAssetsResponse, error) {
+	if req.PoolId == 0 {


Lets move all checks into one place for code sanity please, I think we can move this check to query.go

mattverse · 2023-09-30T07:11:35Z

x/concentrated-liquidity/query.go

+// GetNumNextInitializedTicks is a method that returns underlying assets of a tick range
+// Errors:
+// * ticks given not found
+// * There exists at least 1 tick inside the given range


Suggested change

// * There exists at least 1 tick inside the given range

// * No tick initialized is in the given range

mattverse · 2023-10-05T09:55:52Z

x/concentrated-liquidity/query.go

+// GetNumNextInitializedTicks is a method that returns underlying assets of a tick range
+// Errors:
+// * ticks given not found
+// * There exists at least 1 tick inside the given range


Same question for me as well

mattverse · 2023-10-05T09:56:38Z

x/concentrated-liquidity/query.go

+// need to think of a way to get assets between any 2 ticks
+func (k Keeper) TickRangeUnderlyingAssets(ctx sdk.Context, poolId uint64, lowerTick, upperTick int64) (queryproto.TickRangeUnderlyingAssetsResponse, error) {
+	pool, err := k.getPoolById(ctx, poolId)
+	if err != nil {


Is it possible to return nils here instead of empty structs?

mattverse · 2023-10-05T09:57:35Z

x/concentrated-liquidity/query.go

+
+	// Find first tick initialized
+	// use false for zeroForOne since we're going from lower tick -> upper tick
+	zeroForOne := false


Why is that we are going zero for one as default? Should it not differ according to the asset?

mattverse · 2023-10-05T10:12:26Z

x/concentrated-liquidity/query.go

+	zeroForOne := false
+	swapStrategy := swapstrategy.New(zeroForOne, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())
+
+	nextTickIter := swapStrategy.InitializeNextTickIterator(ctx, poolId, types.MinCurrentTick)


yeah we use swap strategy for iterating over ticks, perhaps we need to rename it some day

mattverse · 2023-10-05T10:13:02Z

x/concentrated-liquidity/query.go

+	zeroForOne := false
+	swapStrategy := swapstrategy.New(zeroForOne, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())
+
+	nextTickIter := swapStrategy.InitializeNextTickIterator(ctx, poolId, types.MinCurrentTick)


Yeah I feel like we should be starting from lower tick

hieuvubk · 2023-10-10T07:06:46Z

Left some comments please take a look

Generally, I don't see how this differs from the GetTickLiquidityNetInDirection query, @hieuvubk can you explain how this is different from the GetTickLiquidityNetInDirection query?

I think GetTickLiquidityNetInDirection returns the LiquidityNet of all ticks in the range [start, bound] in one direction whilte this func aims to return how many tokens are locked within the price range (get liquidity of range then convert to virtual tokens locked)

github-actions · 2023-10-25T00:02:10Z

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

hieuvubk added 4 commits September 25, 2023 01:21

add new query

3a9d8f8

update function

1c1fad3

comment

e77d169

set up test

59ffd56

hieuvubk requested review from czarcas7ic, p0mvn and AlpinYukseloglu as code owners September 29, 2023 08:41

github-actions bot added C:CLI C:x/concentrated-liquidity labels Sep 29, 2023

Generated protofile changes

b90d070

mattverse self-assigned this Sep 29, 2023

hieuvubk marked this pull request as draft October 1, 2023 19:11

hieuvubk added 3 commits October 2, 2023 11:54

Merge branch 'main' into hieu/query_tick_range_assets

f1ef894

changelog

a9f77ca

Merge branch 'hieu/query_tick_range_assets' of https://github.com/osm…

e2492dd

…osis-labs/osmosis into hieu/query_tick_range_assets

hieuvubk added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Oct 2, 2023

hieuvubk and others added 4 commits October 2, 2023 12:01

msgServer

81c2b89

Generated protofile changes

74d2ae0

querygen

fdf746f

lint

51a6087

hieuvubk marked this pull request as ready for review October 2, 2023 11:19

pysel reviewed Oct 2, 2023

View reviewed changes

mattverse reviewed Oct 5, 2023

View reviewed changes

update go doc

1c030e7

hieuvubk added 2 commits October 10, 2023 14:59

fix return type

7acccbe

remove condition

ed8f918

github-actions bot added the Stale label Oct 25, 2023

github-actions bot closed this Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get underlying assets of tick range #6595

Get underlying assets of tick range #6595

hieuvubk commented Sep 29, 2023

pysel commented Sep 29, 2023

pysel Oct 2, 2023

pysel Oct 2, 2023

pysel Oct 2, 2023

mattverse Oct 5, 2023

hieuvubk Oct 9, 2023

hieuvubk Oct 9, 2023

pysel Oct 2, 2023

pysel Oct 2, 2023 •

edited

Loading

pysel Oct 2, 2023

mattverse Oct 5, 2023

mattverse Oct 5, 2023

pysel Oct 2, 2023

pysel Oct 2, 2023

pysel Oct 2, 2023

pysel Oct 2, 2023 •

edited

Loading

pysel Oct 2, 2023

mattverse left a comment

mattverse Sep 30, 2023

mattverse Sep 30, 2023

mattverse Oct 5, 2023

mattverse Oct 5, 2023

mattverse Oct 5, 2023

mattverse Oct 5, 2023

mattverse Oct 5, 2023

hieuvubk commented Oct 10, 2023

github-actions bot commented Oct 25, 2023

	// TickRangeUnderlyingAssets returns uderlying asset of a tick range
	// TickRangeUnderlyingAssets returns underlying asset of a tick range

	swapStrategy := swapstrategy.New(zeroForOne, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())
	swapStrategy := swapstrategy.New(false, osmomath.ZeroBigDec(), k.storeKey, osmomath.ZeroDec())

	expectedAmount1: sdk.ZeroInt(),
	expectedAmount1: osmomath.ZeroInt(),

	// * There exists at least 1 tick inside the given range
	// * No tick initialized is in the given range

Get underlying assets of tick range #6595

Get underlying assets of tick range #6595

Conversation

hieuvubk commented Sep 29, 2023

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

pysel commented Sep 29, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pysel Oct 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pysel Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse left a comment

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

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

hieuvubk commented Oct 10, 2023

github-actions bot commented Oct 25, 2023

pysel Oct 2, 2023 •

edited

Loading

pysel Oct 2, 2023 •

edited

Loading