osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

proposal(twap): consider using last spot price instead of zero on error in `getSpotPrices`

Open p0mvn opened this issue 2 years ago • 5 comments

Background

Context: https://github.com/osmosis-labs/osmosis/blob/4bbd3fb45cef4d9c88207822858f6eeaf9fe95cf/x/twap/logic.go#L46-L54

I think this is a worthwhile improvement to make before v12.

Acceptance Criteria

  • decide if including in v12

p0mvn avatar Sep 09 '22 23:09 p0mvn

I agree with switching to last spot price as this causes TWAPs to level out and stay stable after an error, which is the property that I believe we wanted to protect by leaving TWAPs open after spot price errors

AlpinYukseloglu avatar Sep 10 '22 04:09 AlpinYukseloglu

Let me think about the possible edge case here, is it going to error when the pool does not have the last spot price? By last spot price, are we referring to the spot price recorded in the previous epoch?

mattverse avatar Sep 10 '22 11:09 mattverse

Let me think about the possible edge case here, is it going to error when the pool does not have the last spot price? By last spot price, are we referring to the spot price recorded in the previous epoch?

Assume that we have a twap record at time t. Then, at t + 1 we have a swap so a new record needs to be created with the updated spot prices. Let us get a spot price error.

Currently, we would set the spot price to 0 at t + 1 since it errored. The problem with that is that it negatively impacts the calculations by diverging them significantly from the true value.

Instead of setting it to zero, we might want to reuse the spot price from the previous record (at time ``t`)

One edge case I can think of is when we get an error but there is no older record.

I don't think it should be hard to implement the change. I will try getting it out before Monday to further discuss

p0mvn avatar Sep 11 '22 01:09 p0mvn

Currently, we would set the spot price to 0 at t + 1 since it errored. The problem with that is that it negatively impacts the calculations by diverging them significantly from the true value

If this is the case, I'm definitely for what this issue is proposing! (I wonder if we have a test case showing that rn)

mattverse avatar Sep 11 '22 12:09 mattverse

Decided on the call that this is not v12 blocking but good to have in the future releases

p0mvn avatar Sep 12 '22 17:09 p0mvn