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

Remove iterator calls in two CL operations #7499

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Remove iterator calls in two CL operations #7499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aaef4a3
Remove iterator calls in two CL operations
ValarDragon Feb 16, 2024
0e5e50e
add changelog
ValarDragon Feb 16, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### State Breaking

* [#7250](https://github.com/osmosis-labs/osmosis/pull/7250) Further filter spam gauges from epoch distribution.
* [#7499](https://github.com/osmosis-labs/osmosis/pull/7499) Slight speed/gas improvements to CL CreatePosition and AddToPosition

## v23.0.0

Expand Down
19 changes: 10 additions & 9 deletions x/concentrated-liquidity/lp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,8 @@ func (k Keeper) CreatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddr

// If this is the first position created in this pool, ensure that the position includes both asset0 and asset1
// in order to assign an initial spot price.
hasPositions, err := k.HasAnyPositionForPool(ctx, poolId)
if err != nil {
return CreatePositionData{}, err
}
// N.B. the pool is not mutated between fetching and this call.
hasPositions := k.PoolHasPosition(ctx, pool)

// Trigger before hook for CreatePosition prior to mutating state.
// If no contract is set, this will be a no-op.
Expand Down Expand Up @@ -307,6 +305,11 @@ func (k Keeper) WithdrawPosition(ctx sdk.Context, owner sdk.AccAddress, position
return osmomath.Int{}, osmomath.Int{}, err
}

// Note that here we currently use the iterator based definition to search
// for a remaining position in the pool. Since we have removed a position we need to
// search if there are more.
// Ideally in the future we could have a simple "num positions" counter to make this logic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean adding a state entry for num positions where key is address and value is num positions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! But key is poolID

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right pool id. Would the speedup of using num counter be worthy for making an github issue and coming back to it to address it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, but there are other higher priorities in the CL performance line of work.

// much faster.
anyPositionsRemainingInPool, err := k.HasAnyPositionForPool(ctx, position.PoolId)
if err != nil {
return osmomath.Int{}, osmomath.Int{}, err
Expand Down Expand Up @@ -417,22 +420,20 @@ func (k Keeper) addToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId
return 0, osmomath.Int{}, osmomath.Int{}, err
}

anyPositionsRemainingInPool, err := k.HasAnyPositionForPool(ctx, position.PoolId)
pool, err := k.GetConcentratedPoolById(ctx, position.PoolId)
if err != nil {
return 0, osmomath.Int{}, osmomath.Int{}, err
}

anyPositionsRemainingInPool := k.PoolHasPosition(ctx, pool)
Comment on lines +423 to +428
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also give us speed up compared to calling HasAnyPositionForPool directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! HasAnyPositionForPool runs an iterator. PoolHasPosition just checks data off the pool struct directly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, isn't it also possible to change HasAnyPositionForPool to do GetConcentratedPoolById then call PoolHasPosition instead of running an iterator?

Copy link
Member Author

@ValarDragon ValarDragon Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it was mostly that we still need the old method as well for withdraw position.

Maybe we rename this

if !anyPositionsRemainingInPool {
return 0, osmomath.Int{}, osmomath.Int{}, types.AddToLastPositionInPoolError{PoolId: position.PoolId, PositionId: position.PositionId}
}

// Create new position with updated liquidity.
amount0Desired := amount0Withdrawn.Add(amount0Added)
amount1Desired := amount1Withdrawn.Add(amount1Added)
pool, err := k.GetConcentratedPoolById(ctx, position.PoolId)
if err != nil {
return 0, osmomath.Int{}, osmomath.Int{}, err
}

tokensProvided := sdk.NewCoins(sdk.NewCoin(pool.GetToken0(), amount0Desired), sdk.NewCoin(pool.GetToken1(), amount1Desired))
minimumAmount0 := amount0Withdrawn
minimumAmount1 := amount1Withdrawn
Expand Down