-
Notifications
You must be signed in to change notification settings - Fork 608
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.
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
Update record sanity check #2741
Changes from 14 commits
29539a1
7b87cd8
0cbfc04
b09e65f
dfc2307
cd78749
5e9e64b
a7b94af
17618aa
5c3e98a
f7fcdc3
c43885d
acf03e3
0409367
77502d9
fa28336
95713c2
24da39c
ad6bd30
71991e9
bc79f48
c6f1b8f
277d7a7
2f5103d
6574fa6
a00dbcf
d9f3a6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,6 +9,8 @@ import ( | |||||
sdk "github.com/cosmos/cosmos-sdk/types" | ||||||
"github.com/stretchr/testify/require" | ||||||
|
||||||
tmproto "github.com/tendermint/tendermint/proto/tendermint/types" | ||||||
|
||||||
"github.com/osmosis-labs/osmosis/v12/app/apptesting/osmoassert" | ||||||
gammtypes "github.com/osmosis-labs/osmosis/v12/x/gamm/types" | ||||||
"github.com/osmosis-labs/osmosis/v12/x/twap" | ||||||
|
@@ -227,7 +229,8 @@ func (s *TestSuite) TestUpdateRecord() { | |||||
defaultTwoAssetCoins[1].Denom, defaultTwoAssetCoins[0].Denom, | ||||||
test.spotPriceResult1.Sp, test.spotPriceResult1.Err) | ||||||
|
||||||
newRecord := s.twapkeeper.UpdateRecord(s.Ctx, test.record) | ||||||
newRecord, err := s.twapkeeper.UpdateRecord(s.Ctx, test.record) | ||||||
s.Require().NoError(err) | ||||||
s.Equal(test.expRecord, newRecord) | ||||||
}) | ||||||
} | ||||||
|
@@ -756,6 +759,7 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
ammMock twapmock.ProgrammedAmmInterface | ||||||
spOverrides []spOverride | ||||||
blockTime time.Time | ||||||
blockHeight int64 | ||||||
|
||||||
expectedHistoricalRecords []expectedResults | ||||||
expectError error | ||||||
|
@@ -1005,9 +1009,8 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
}, | ||||||
}, | ||||||
}, | ||||||
// This case should never happen in-practice since ctx.BlockTime | ||||||
// should always be greater than the last record's time. | ||||||
"two-asset; pre-set at t and t + 1, new record inserted between existing": { | ||||||
"new record can't be inserted before the last record's update time": { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
preSetRecords: []types.TwapRecord{baseRecord, tPlus10sp5Record}, | ||||||
poolId: baseRecord.PoolId, | ||||||
|
||||||
|
@@ -1026,31 +1029,47 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
}, | ||||||
}, | ||||||
|
||||||
expectedHistoricalRecords: []expectedResults{ | ||||||
// The original record at t. | ||||||
{ | ||||||
spotPriceA: baseRecord.P0LastSpotPrice, | ||||||
spotPriceB: baseRecord.P1LastSpotPrice, | ||||||
}, | ||||||
// The new record added. | ||||||
// TODO: it should not be possible to add a record between existing records. | ||||||
// https://github.com/osmosis-labs/osmosis/issues/2686 | ||||||
expectError: types.InvalidRecordTimeError{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also add a test case for having block height greater than the last record's time and checking the error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes of course |
||||||
RecordBlockHeight: tPlus10sp5Record.Height, | ||||||
RecordTime: tPlus10sp5Record.Time, | ||||||
ActualBlockHeight: (baseRecord.Height + 1), | ||||||
ActualTime: baseRecord.Time.Add(time.Second * 5), | ||||||
}, | ||||||
}, | ||||||
// should always be greater than the last record's block. | ||||||
"new record can't be inserted before the last record's update block": { | ||||||
preSetRecords: []types.TwapRecord{mostRecentRecordPoolOne}, | ||||||
poolId: baseRecord.PoolId, | ||||||
|
||||||
// Even if lastRecord.Time < ctx.Time, | ||||||
// lastRecord.Height >= ctx.BlockHeight also throws error | ||||||
blockTime: mostRecentRecordPoolOne.Time.Add(time.Second), | ||||||
blockHeight: mostRecentRecordPoolOne.Height - 1, | ||||||
|
||||||
spOverrides: []spOverride{ | ||||||
{ | ||||||
spotPriceA: sdk.OneDec(), | ||||||
spotPriceB: sdk.OneDec().Add(sdk.OneDec()), | ||||||
isMostRecent: true, | ||||||
baseDenom: mostRecentRecordPoolOne.Asset0Denom, | ||||||
quoteDenom: mostRecentRecordPoolOne.Asset1Denom, | ||||||
overrideSp: sdk.OneDec(), | ||||||
}, | ||||||
// The original record at t + 1. | ||||||
{ | ||||||
spotPriceA: tPlus10sp5Record.P0LastSpotPrice, | ||||||
spotPriceB: tPlus10sp5Record.P1LastSpotPrice, | ||||||
baseDenom: mostRecentRecordPoolOne.Asset1Denom, | ||||||
quoteDenom: mostRecentRecordPoolOne.Asset0Denom, | ||||||
overrideSp: sdk.OneDec().Add(sdk.OneDec()), | ||||||
}, | ||||||
}, | ||||||
|
||||||
expectError: types.InvalidRecordTimeError{ | ||||||
RecordBlockHeight: mostRecentRecordPoolOne.Height, | ||||||
RecordTime: mostRecentRecordPoolOne.Time, | ||||||
ActualBlockHeight: mostRecentRecordPoolOne.Height - 1, | ||||||
ActualTime: mostRecentRecordPoolOne.Time.Add(time.Second), | ||||||
}, | ||||||
}, | ||||||
"multi-asset pool; pre-set at t and t + 1; creates new records": { | ||||||
preSetRecords: []types.TwapRecord{threeAssetRecordAB, threeAssetRecordAC, threeAssetRecordBC, tPlus10sp5ThreeAssetRecordAB, tPlus10sp5ThreeAssetRecordAC, tPlus10sp5ThreeAssetRecordBC}, | ||||||
poolId: threeAssetRecordAB.PoolId, | ||||||
blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), | ||||||
blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), | ||||||
spOverrides: []spOverride{ | ||||||
{ | ||||||
baseDenom: threeAssetRecordAB.Asset0Denom, | ||||||
|
@@ -1138,18 +1157,18 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
"multi-asset pool; pre-set at t and t + 1 with err, large spot price, overwrites error time": { | ||||||
preSetRecords: []types.TwapRecord{threeAssetRecordAB, threeAssetRecordAC, threeAssetRecordBC, withLastErrTime(tPlus10sp5ThreeAssetRecordAB, tPlus10sp5ThreeAssetRecordAB.Time), tPlus10sp5ThreeAssetRecordAC, tPlus10sp5ThreeAssetRecordBC}, | ||||||
poolId: threeAssetRecordAB.PoolId, | ||||||
blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), | ||||||
blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), | ||||||
spOverrides: []spOverride{ | ||||||
{ | ||||||
baseDenom: threeAssetRecordAB.Asset0Denom, | ||||||
quoteDenom: threeAssetRecordAB.Asset1Denom, | ||||||
overrideSp: sdk.OneDec(), | ||||||
}, | ||||||
{ | ||||||
baseDenom: threeAssetRecordAB.Asset1Denom, | ||||||
quoteDenom: threeAssetRecordAB.Asset0Denom, | ||||||
overrideSp: sdk.OneDec().Add(sdk.OneDec()), | ||||||
}, | ||||||
baseDenom: threeAssetRecordAB.Asset1Denom, | ||||||
quoteDenom: threeAssetRecordAB.Asset0Denom, | ||||||
overrideSp: sdk.OneDec().Add(sdk.OneDec()), | ||||||
}, | ||||||
{ | ||||||
baseDenom: threeAssetRecordAC.Asset0Denom, | ||||||
quoteDenom: threeAssetRecordAC.Asset1Denom, | ||||||
|
@@ -1167,9 +1186,9 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
overrideSp: sdk.OneDec(), | ||||||
}, | ||||||
{ | ||||||
baseDenom: threeAssetRecordBC.Asset1Denom, | ||||||
quoteDenom: threeAssetRecordBC.Asset0Denom, | ||||||
overrideSp: sdk.OneDec().Add(sdk.OneDec()), | ||||||
baseDenom: threeAssetRecordBC.Asset1Denom, | ||||||
quoteDenom: threeAssetRecordBC.Asset0Denom, | ||||||
overrideSp: sdk.OneDec().Add(sdk.OneDec()), | ||||||
}, | ||||||
}, | ||||||
|
||||||
|
@@ -1188,7 +1207,7 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
// The new record AB added. | ||||||
{ | ||||||
spotPriceA: sdk.OneDec(), | ||||||
spotPriceB: sdk.OneDec().Add(sdk.OneDec()), | ||||||
spotPriceB: sdk.OneDec().Add(sdk.OneDec()), | ||||||
lastErrorTime: tPlus10sp5ThreeAssetRecordAB.Time, | ||||||
isMostRecent: true, | ||||||
}, | ||||||
|
@@ -1199,8 +1218,8 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
}, | ||||||
// The original record AC at t + 1. | ||||||
{ | ||||||
spotPriceA: tPlus10sp5ThreeAssetRecordAC.P0LastSpotPrice, | ||||||
spotPriceB: tPlus10sp5ThreeAssetRecordAC.P1LastSpotPrice, | ||||||
spotPriceA: tPlus10sp5ThreeAssetRecordAC.P0LastSpotPrice, | ||||||
spotPriceB: tPlus10sp5ThreeAssetRecordAC.P1LastSpotPrice, | ||||||
}, | ||||||
// The new record AC added. | ||||||
{ | ||||||
|
@@ -1216,14 +1235,14 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
}, | ||||||
// The original record BC at t + 1. | ||||||
{ | ||||||
spotPriceA: tPlus10sp5ThreeAssetRecordBC.P0LastSpotPrice, | ||||||
spotPriceB: tPlus10sp5ThreeAssetRecordBC.P1LastSpotPrice, | ||||||
spotPriceA: tPlus10sp5ThreeAssetRecordBC.P0LastSpotPrice, | ||||||
spotPriceB: tPlus10sp5ThreeAssetRecordBC.P1LastSpotPrice, | ||||||
}, | ||||||
// The new record BC added. | ||||||
{ | ||||||
spotPriceA: sdk.OneDec(), | ||||||
spotPriceB: sdk.OneDec().Add(sdk.OneDec()), | ||||||
isMostRecent: true, | ||||||
spotPriceA: sdk.OneDec(), | ||||||
spotPriceB: sdk.OneDec().Add(sdk.OneDec()), | ||||||
isMostRecent: true, | ||||||
}, | ||||||
}, | ||||||
}, | ||||||
|
@@ -1234,6 +1253,9 @@ func (s *TestSuite) TestUpdateRecords() { | |||||
s.SetupTest() | ||||||
twapKeeper := s.App.TwapKeeper | ||||||
ctx := s.Ctx.WithBlockTime(tc.blockTime) | ||||||
if tc.blockHeight != 0 { | ||||||
ctx = s.Ctx.WithBlockHeader(tmproto.Header{Time: tc.blockTime, Height: tc.blockHeight}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use the |
||||||
} | ||||||
|
||||||
if len(tc.spOverrides) > 0 { | ||||||
ammMock := twapmock.NewProgrammedAmmInterface(s.App.GAMMKeeper) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,3 +62,14 @@ type InvalidRecordCountError struct { | |
func (e InvalidRecordCountError) Error() string { | ||
return fmt.Sprintf("The number of records do not match, expected: %d\n got: %d", e.Expected, e.Actual) | ||
} | ||
|
||
type InvalidRecordTimeError struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we should change this to |
||
RecordBlockHeight int64 | ||
RecordTime time.Time | ||
ActualBlockHeight int64 | ||
ActualTime time.Time | ||
} | ||
|
||
func (e InvalidRecordTimeError) Error() string { | ||
return fmt.Sprintf("failed to update the record, the context time must be greater than record time; record: block %d at %s, actual: block %d at %s", e.RecordBlockHeight, e.RecordTime, e.ActualBlockHeight, e.ActualTime) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it not
>
but>=
? Can the record not have same height as blockheight by design?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a case when creating new pool. New twap records will be created and updated immediately. I limit this by adding the condition
ArithmeticTwapAccumulator
must be zero (init twap record). But I should split it upThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet, can we add this context as inline comment?