stableswap: Improve the scaling factor API #2908
Conversation
github-actions
bot
added
C:docs
ValarDragon
added
the
V:state/compatible/no_backport
| @@ -223,7 +223,13 @@ func solveCFMMBinarySearchMulti(constantFunction func(osmomath.BigDec, osmomath. | |||
| } | |||
| } | |||
|
|
|||
| func spotPrice(baseReserve, quoteReserve osmomath.BigDec, remReserves []osmomath.BigDec) osmomath.BigDec { | |||
| func (p Pool) spotPrice(baseDenom, quoteDenom string) (sdk.Dec, error) { | |||
| roundMode := osmomath.RoundBankers // TODO: | |||
There was a problem hiding this comment.
Did you mean to leave a todo here?
There was a problem hiding this comment.
yeah, need to figure out which rounding mode to use
| RoundBankers RoundingDirection = 3 | ||
| ) | ||
|
|
||
| func DivIntByU64ToBigDec(i sdk.Int, u uint64, round RoundingDirection) (BigDec, error) { |
| return BigDec{}, fmt.Errorf("invalid rounding mode %d", int(round)) | ||
| } | ||
|
|
||
| func DivCoinAmtsByU64ToBigDec(coins []sdk.Coin, scales []uint64, round RoundingDirection) ([]BigDec, error) { |
There was a problem hiding this comment.
a basic test for this function would be nice to have
| @@ -223,7 +223,13 @@ func solveCFMMBinarySearchMulti(constantFunction func(osmomath.BigDec, osmomath. | |||
| } | |||
| } | |||
|
|
|||
| func spotPrice(baseReserve, quoteReserve osmomath.BigDec, remReserves []osmomath.BigDec) osmomath.BigDec { | |||
| func (p Pool) spotPrice(baseDenom, quoteDenom string) (sdk.Dec, error) { | |||
| roundMode := osmomath.RoundBankers // TODO: | |||
There was a problem hiding this comment.
Is TODO meant for explaining the reason for the choice of the rounding direction? I think that would be useful
There was a problem hiding this comment.
Its for us to later choose the right rounding direction. All rounding direction choices are currenetly a TODO
| return osmomath.DivCoinAmtsByU64ToBigDec(reorderedLiquidity, reorderedScalingFactors, round) | ||
| } | ||
|
|
||
| // reorderReservesAndScalingFactors takes the pool liquidity and scaling factors, and reorders them s.t. |
There was a problem hiding this comment.
| // reorderReservesAndScalingFactors takes the pool liquidity and scaling factors, and reorders them s.t. | |
| // reorderReservesAndScalingFactors given two denoms - first and second, | |
| // it takes their pool liquidity and scaling factors and reorders them s.t. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return osmomath.DivCoinAmtsByU64ToBigDec(reorderedLiquidity, reorderedScalingFactors, round) |
There was a problem hiding this comment.
nit: every existing caller of this function grabs first and second element from the slice returned here. What do you think about simply having 3 returns here?
We could also have a sanity error check that the first 2 elements actually exist
There was a problem hiding this comment.
Yeah, could def add it in another PR!
| }, | ||
| "zero scaling factor": { | ||
| denoms: [2]string{"foo", "bar"}, | ||
| poolAssets: twoEvenStablePoolAssets, | ||
| scalingFactors: []uint64{0, 1}, | ||
| expPanic: true, | ||
| expError: true, |
Closes: #2903
What is the purpose of the change
Context in 2903, makes the API scale to/from BigDec.
Adds rounding mode for Int / U64 -> BigDec conversion
Testing and Verifying
This updates tests for existing behavior. We need to add tests for other rounding modes.