x/gamm: Catch overflow panics on CalculateSpotPrice
#2405
Conversation
github-actions
bot
added
the
C:x/gamm
AlpinYukseloglu
changed the title
x/gamm/pool-models/balancer/pool.go
Outdated
| // defer function to escape the panic when spot price overflows | ||
| if r := recover(); r != nil { | ||
| spotPrice = sdk.Dec{} | ||
| err = types.ErrSpotPriceOverflow |
There was a problem hiding this comment.
Lets mark this as a spot price internal error
x/gamm/pool-models/balancer/pool.go
Outdated
| spotPrice = osmomath.SigFigRound(fullRatio, types.SigFigs) | ||
|
|
||
| return spotPrice, err |
There was a problem hiding this comment.
Needs to add an overflow check here right? (E.g. a maximum of 2128 or 2160)?
There was a problem hiding this comment.
Right! I just added a check that capped at 2^160, although we could technically increase it to 2^196 without overflowing on equal weight pools. I personally think the buffer is useful regardless but would be curious to hear your thoughts
|
Comments should all be addressed! |
x/gamm/pool-models/balancer/pool.go
Outdated
| spotPrice = osmomath.SigFigRound(fullRatio, types.SigFigs) | ||
| if spotPrice.GT(sdk.NewDec(2).Power(160)) { | ||
| spotPrice = sdk.Dec{} | ||
| err = errors.New("spot price overflow") |
There was a problem hiding this comment.
err, the prior error type was good! We just need two errors:
ErrSpotPriceInternal, ErrSpotPriceOverflow
There was a problem hiding this comment.
Ah I see what you meant now – should be fixed!
AlpinYukseloglu
changed the title
CalculateSpotPrice
|
Oh I didn't catch this, this was done at the balancer level, which is the wrong place to do it. This should be done at the keeper level, will move in a follow-up PR. |
|
Yikes, spot price rounding is also at the wrong layer of abstraction |
Closes: #2207, #2445
What is the purpose of the change
This PR prevents
SpotPricefrom panicing and makes it return an error for invalid or overflowing spot prices instead. It also adds tests to ensure spot prices stay within the set bounds.Brief Changelog
SpotPricenow recovers panics and returns an error. It also sets a max spot price of 2^160Testing and Verifying
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? (yes / no)x/<module>/spec/) / Osmosis docs repo / not documented)