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.

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

Refactor createPosition to use struct param and return #5485

Closed
Pipello opened this issue Jun 9, 2023 · 3 comments
Closed

Refactor createPosition to use struct param and return #5485

Pipello opened this issue Jun 9, 2023 · 3 comments

Comments

@Pipello
Copy link
Contributor

Pipello commented Jun 9, 2023

Background

From discussion: #5484 (comment)

Suggested Design

This might be long refactoring but from what I saw the current signature of createPosition is as

func (k Keeper) createPosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, tokensProvided sdk.Coins, amount0Min, amount1Min sdk.Int, lowerTick, upperTick int64) (positionId uint64, actualAmount0 sdk.Int, actualAmount1 sdk.Int, liquidityDelta sdk.Dec, lowerTickResult int64, upperTickResult int64, err error)

This ends up with parent function calls looking like:

a, b, c, d, err = keepers.ConcentratedLiquidityKeeper.CreateFullRangePosition(...)

This is not really good for maintenance or new features.

The solution could be to provide a return struct and a param struct such as:

type createPos struct {
    poolId uint64
    owner sdk.AccAddress
    tokensProvided sdk.Coins
    amount0Min sdk.Int
    amount1Min sdk.Int
    lowerTick int64
    upperTick int64
}

type position struct {
    positionId uint64
    actualAmount0 sdk.Int
    actualAmount1 sdk.Int
    liquidityDelta sdk.Dec
    amount1Min sdk.Int
    lowerTickResult int64
    upperTickResult int64
}

func (k Keeper) createPosition(ctx sdk.Context, createPos *createPos) (pos *position, err error)

The parent callers are used across many files but solving this issue would reduce technical debt

Acceptance Criteria

  • existing calls are using the new structures
@Pipello
Copy link
Contributor Author

Pipello commented Jun 9, 2023

From what I see in the x/concentrated-liquidity/lp.go this might be the top of the iceberg and it would make sense to start by smaller pieces such as emitLiquidityChangeEvent and build up from there

@Pipello
Copy link
Contributor Author

Pipello commented Nov 9, 2023

This was partially solved by: #5983
I am fine with closing it

@ValarDragon
Copy link
Member

Thanks for that PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants