osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

[x/gamm][stableswap]: Investigate critical precision issue with binary search CFMM solver: Consider making `CalcJoinPoolShares` not mutate pool assets by factoring out liquidity updates

Open AlpinYukseloglu opened this issue 3 years ago • 0 comments

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 avatar Aug 10 '22 08:08 AlpinYukseloglu