-
Notifications
You must be signed in to change notification settings - Fork 607
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
Conversation
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.
Looks nice! I think we need a test case for checking error on block height and then should be good to merge!
Also curious if the issue / question you asked on the original issue is resolved or if its something that you need further answer on
x/twap/logic_test.go
Outdated
spotPriceA: tPlus10sp5Record.P0LastSpotPrice, | ||
spotPriceB: tPlus10sp5Record.P1LastSpotPrice, | ||
}, | ||
expectError: types.InvalidRecordTimeError{ |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yes of course
x/twap/logic_test.go
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the WithBlockTime
and WithBlockHeight
API instead?
x/twap/logic_test.go
Outdated
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
"new record can't be inserted before the last record's update time": { | |
"new record can't be inserted prior to the last record's update time": { |
x/twap/types/errors.go
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should change this to InvalidUpdateRecordError
or something similar, since this error does not only happen when it has a invalid record time but it also includes invlaid record height as well. Lmk what you think
nice thank u |
…is-labs/osmosis into update_record_sanity_check
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.
Nice work, logic lgtm, requesting for additional test cases before we can merge!
x/twap/logic.go
Outdated
// Returns error for last updated records in the same block. | ||
// Exception: record is initialized when creating a pool, | ||
// then the TwapAccumulator variables are zero. | ||
if (record.Height >= ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) && |
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.
if (record.Height >= ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) && | |
if (record.Height > ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) && |
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 up
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.
sweet, can we add this context as inline comment?
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.
One comment regarding returned error type. Otherwise lgtm
x/twap/logic.go
Outdated
if (record.Height == ctx.BlockHeight() || record.Time.Equal(ctx.BlockTime())) && | ||
!record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) && | ||
!record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) { | ||
return types.TwapRecord{}, fmt.Errorf("Invalid zero twap accumulator") |
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.
we should be using return types.TwapRecord{}, types.InvalidUpdateRecordError{
here as well
Sweet! Let's merge this after updating CHANGELOG please @hieuvubk |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
@mattverse hey sr I forgot about this pr :((. Changelog updated |
* add new error type * add condition to ensure ctx time > record time * format * replace * fix unused import * update co conditions, bypass the case when creating pool * add cmt * update test for checking block height * format * update err message * update ctx & err * resolve conflict * split condition * update condition & add more test * lint * update return err * update changelog --------- Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Matt, Park <[email protected]>
Closes: #2686
What is the purpose of the change
Brief Changelog
updateRecord
&UpdateRecords