osmosis
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
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
joinPoolSharesInternaland redirect intermediate liquidity updates to local updates of this map - Remove any other liquidity updates at the end of
joinPoolSharesInternal - Create a separate
IncreaseLiquidityfunction that updates pool liquidity and add it where relevant (e.g. afterCalcJoinPoolSharesinJoinPool)
For reference, the main two places where pool assets are mutated are here and here
Acceptance Criteria
joinPoolSharesInternal(and thereforeCalcJoinPoolShares) does not mutate pool assets- Existing tests pass