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

[x/gamm][stableswap]: Consider making CalcJoinPoolShares not mutate pool assets by factoring out liquidity updates #2347

Closed
Tracked by #1451
AlpinYukseloglu opened this issue Aug 10, 2022 · 1 comment
Assignees
Labels
C:x/gamm Changes, features and bugs related to the gamm module.

Comments

@AlpinYukseloglu
Copy link
Contributor

AlpinYukseloglu commented Aug 10, 2022

Background

Our balancer implementation has separate functions for calculating pool join shares (CalcJoinPoolShares) and actually updating pool assets/liquidity (IncreaseLiquidity), whereas our stableswap implementation does all of it in CalcJoinPoolShares -> joinPoolSharesInternal.

These two implementations are inconsistent, so it would make sense to change one of them to fit the other. It's not clear to me which implementation is cleaner, but I am leaning towards factoring out the pool state updates from our stableswap implementation into a separate function like in balancer.

Note: our balancer implementation seems cleaner at a first glance, but it also requires setting up a local pool asset map to keep track of intermediate changes which adds a nontrivial amount of complexity to an already large function.

Suggested Design

  • Create a local pool asset map in joinPoolSharesInternal and redirect intermediate liquidity updates to local updates of this map
  • Remove any other liquidity updates at the end of joinPoolSharesInternal
  • Create a separate IncreaseLiquidity function that updates pool liquidity and add it where relevant (e.g. after CalcJoinPoolShares in JoinPool)

For reference, the main two places where pool assets are mutated are here and here

Acceptance Criteria

  • joinPoolSharesInternal (and therefore CalcJoinPoolShares) does not mutate pool assets
  • Existing tests pass
@AlpinYukseloglu AlpinYukseloglu added the C:x/gamm Changes, features and bugs related to the gamm module. label Aug 10, 2022
@AlpinYukseloglu AlpinYukseloglu changed the title stableswap: Consider making CalcJoinPoolShares not mutate pool assets by factoring out liquidity updates [x/gamm][stableswap]: Investigate critical precision issue with binary search CFMM solver: Consider making CalcJoinPoolShares not mutate pool assets by factoring out liquidity updates Aug 10, 2022
@AlpinYukseloglu AlpinYukseloglu changed the title [x/gamm][stableswap]: Investigate critical precision issue with binary search CFMM solver: Consider making CalcJoinPoolShares not mutate pool assets by factoring out liquidity updates [x/gamm][stableswap]: Consider making CalcJoinPoolShares not mutate pool assets by factoring out liquidity updates Aug 25, 2022
@AlpinYukseloglu AlpinYukseloglu self-assigned this Sep 26, 2022
@AlpinYukseloglu
Copy link
Contributor Author

Turns out this version is actually cleaner than what we have in Balancer & we should probably consider changing that implementation instead!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gamm Changes, features and bugs related to the gamm module.
Projects
Archived in project
Development

No branches or pull requests

1 participant