Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(x/twap): handle spot price error case in the context of geometric twap #3845

Merged
merged 3 commits into from
Dec 23, 2022
Merged

refactor(x/twap): handle spot price error case in the context of geometric twap #3845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6dcce8b
refactor(x/twap): handle spot price error case
p0mvn Dec 23, 2022
598c53a
supporting test cases
p0mvn Dec 23, 2022
a15947d
table-driven log tests
p0mvn Dec 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions osmomath/sigfig_round_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@ func TestSigFigRound(t *testing.T) {
tenToSigFig: sdk.NewInt(100),
expectedResult: sdk.MustNewDecFromStr("0.087"),
},

{
name: "minimum decimal is still kept",
decimal: sdk.NewDecWithPrec(1, 18),
tenToSigFig: sdk.NewInt(10),
expectedResult: sdk.NewDecWithPrec(1, 18),
},
}

for i, tc := range testCases {
Expand Down
16 changes: 16 additions & 0 deletions x/gamm/pool-models/balancer/pool_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,22 @@ func (suite *KeeperTestSuite) TestBalancerSpotPriceBounds() {
baseDenomWeight: sdk.NewInt(100),
expectError: true,
},
{
name: "internal error due to spot price precision being too small, resulting in 0 spot price",
quoteDenomPoolInput: sdk.NewCoin(baseDenom, sdk.OneInt()),
quoteDenomWeight: sdk.NewInt(100),
baseDenomPoolInput: sdk.NewCoin(quoteDenom, sdk.NewDec(10).PowerMut(19).TruncateInt().Sub(sdk.NewInt(2))),
baseDenomWeight: sdk.NewInt(100),
expectError: true,
},
{
name: "at min spot price",
quoteDenomPoolInput: sdk.NewCoin(baseDenom, sdk.OneInt()),
quoteDenomWeight: sdk.NewInt(100),
baseDenomPoolInput: sdk.NewCoin(quoteDenom, sdk.NewDec(10).PowerMut(18).TruncateInt()),
baseDenomWeight: sdk.NewInt(100),
expectedOutput: sdk.OneDec().Quo(sdk.NewDec(10).PowerMut(18)),
},
}

for _, tc := range tests {
Expand Down
13 changes: 13 additions & 0 deletions x/twap/logic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,14 @@ func recordWithUpdatedAccumulators(record types.TwapRecord, newTime time.Time) t
p1NewAccum := types.SpotPriceMulDuration(record.P1LastSpotPrice, timeDelta)
newRecord.P1ArithmeticTwapAccumulator = newRecord.P1ArithmeticTwapAccumulator.Add(p1NewAccum)

// If the last spot price is zero, then the logarithm is undefined.
// As a result, we cannot update the geometric accumulator.
// We set the last error time to be the new time, and return the record.
if record.P0LastSpotPrice.IsZero() {
newRecord.LastErrorTime = newTime
return newRecord
}

// logP0SpotPrice = log_{2}{P_0}
logP0SpotPrice := twapLog(record.P0LastSpotPrice)
// p0NewGeomAccum = log_{2}{P_0} * timeDelta
Expand Down Expand Up @@ -259,7 +267,12 @@ func computeTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quote
}

// twapLog returns the logarithm of the given spot price, base 2.
// Panics if zero is given.
func twapLog(price sdk.Dec) sdk.Dec {
if price.IsZero() {
panic("twap: cannot take logarithm of zero")
}
Comment on lines +272 to +274
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but I feel like the mental overhead is better here if we make this return an error in such case instead. (We have to be careful that this panic is never called due to this being end block logic. Vs if its an error, we can just check that caller handles errors correctly, which is already the case w/ our errcheck linter)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this panic can ever be reached because we have an additional safeguard in its only caller:

osmosis/x/twap/logic.go

Lines 200 to 203 in a15947d

if record.P0LastSpotPrice.IsZero() {
newRecord.LastErrorTime = newTime
return newRecord
}

This is just a defense-in-depth sanity check.

The problem with returning an error is that it would require a large refactor to propagate this error to multiple callers of recordWithUpdatedAccumulators.

I'm going to keep this as is and merge it. However, please let me know if you have further concerns, happy to discuss and address in another PR if needed


return osmomath.BigDecFromSDKDec(price).LogBase2().SDKDec()
}

Expand Down
65 changes: 61 additions & 4 deletions x/twap/logic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,10 +270,20 @@ func TestRecordWithUpdatedAccumulators(t *testing.T) {
newTime: time.Unix(1, 0),
expRecord: newExpRecord(oneDec, twoDec, pointFiveDec),
},
"zero spot price - panic": {
record: withPrice0Set(defaultRecord, sdk.ZeroDec()),
newTime: defaultRecord.Time.Add(time.Second),
expectPanic: true,
"sp0 - zero spot price - accum0 unchanged, accum1 updated, geom accum unchanged, last err time set": {
record: withPrice0Set(defaultRecord, sdk.ZeroDec()),
newTime: defaultRecord.Time.Add(time.Second),
expRecord: withLastErrTime(newExpRecord(oneDec, twoDec.Add(sdk.NewDecWithPrec(1, 1).Mul(OneSec)), pointFiveDec), defaultRecord.Time.Add(time.Second)),
},
"sp1 - zero spot price - accum0 updated, accum1 unchanged, geom accum updated correctly": {
record: withPrice1Set(defaultRecord, sdk.ZeroDec()),
newTime: defaultRecord.Time.Add(time.Second),
expRecord: newExpRecord(tenSecAccum.Add(oneDec), twoDec, pointFiveDec.Add(geometricTenSecAccum)),
},
"both sp - zero spot price - accum0 unchange, accum1 unchanged, geom accum unchanged": {
record: withPrice1Set(withPrice0Set(defaultRecord, sdk.ZeroDec()), sdk.ZeroDec()),
newTime: defaultRecord.Time.Add(time.Second),
expRecord: withLastErrTime(newExpRecord(oneDec, twoDec, pointFiveDec), defaultRecord.Time.Add(time.Second)),
},
"spot price of one - geom accumulator 0": {
record: withPrice1Set(withPrice0Set(defaultRecord, sdk.OneDec()), sdk.OneDec()),
Expand Down Expand Up @@ -1329,6 +1339,53 @@ func (s *TestSuite) TestTwapLog_CorrectBase() {
s.Require().Equal(expectedValue, result)
}

func (s *TestSuite) TestTwapLog() {
smallestAdditiveTolerance := osmomath.ErrTolerance{
AdditiveTolerance: sdk.SmallestDec(),
}

testcases := []struct {
name string
price sdk.Dec
expected sdk.Dec
expectPanic bool
}{
{
"max spot price",
gammtypes.MaxSpotPrice,
// log_2{2^128 - 1} = 128
sdk.MustNewDecFromStr("127.999999999999999999"),
false,
},
{
"zero price - panic",
sdk.ZeroDec(),
sdk.Dec{},
true,
},
{
"smallest dec",
sdk.SmallestDec(),
// https://www.wolframalpha.com/input?i=log+base+2+of+%2810%5E-18%29+with+20+digits
sdk.MustNewDecFromStr("59.794705707972522262").Neg(),
false,
},
}

for _, tc := range testcases {
s.Run(tc.name, func() {
osmoassert.ConditionalPanic(s.T(), tc.expectPanic, func() {
result := twap.TwapLog(tc.price)

smallestAdditiveTolerance.CompareBigDec(
osmomath.BigDecFromSDKDec(tc.expected),
osmomath.BigDecFromSDKDec(result),
)
})
})
}
}

// TestTwapPow_CorrectBase tests that the base of 2 is used for the twap power function.
// 2^3 = 8
func (s *TestSuite) TestTwapPow_CorrectBase() {
Expand Down