Fix: Last error time now updates during record interpolation #2923
Conversation
migueldingli1997
changed the title
|
This looks good to me! Can you add the test case you had in the github issue, which ensures the needed property at the api level? |
Sure thing |
|
I have not had the time to continue working on this, however please note that that after I added some additional rough test cases, there seems to be an issue at the API level as a result of my changes. |
| @@ -719,7 +742,7 @@ func (s *TestSuite) TestGetArithmeticTwapToNow() { | |||
|
|
|||
| if test.expectedError != nil { | |||
| s.Require().Error(err) | |||
| s.Require().ErrorIs(err, test.expectedError) | |||
| s.Require().Equal(test.expectedError, err) | |||
There was a problem hiding this comment.
The ErrorIs does not work for spotPriceError so I changed it to an Equals
|
Updated @ValarDragon :) Please review the PR in its entirety as I did not just add a test; I modified the fix as well. |
| if endRecord.LastErrorTime.After(startRecord.Time) || endRecord.LastErrorTime.Equal(startRecord.Time) { | ||
| if endRecord.LastErrorTime.After(startRecord.Time) || | ||
| endRecord.LastErrorTime.Equal(startRecord.Time) || | ||
| startRecord.LastErrorTime.Equal(startRecord.Time) { |
There was a problem hiding this comment.
curious, do we not need to add startRecord.LastErrorTime.Equal(startRecord.Time)?
There was a problem hiding this comment.
Consider the case where:
endRecord = {Time: 2, LastErrTime: 0}startRecord = {Time: 1, LastErrTime: 1}
The conditions will evaluate to the following:
endRecord.LastErrorTime.After(startRecord.Time)- FalseendRecord.LastErrorTime.Equal(startRecord.Time)- FalsestartRecord.LastErrorTime.Equal(startRecord.Time)- True
Without the third condition, the calculation proceeds. And we do not want it to proceed because the startRecord has an error at its time.
The case is definitely possible because the startRecord might have been interpolated before being passed to this function (e.g. from {Time: 0, LastErrTime: 0} to {Time: 1, LastErrTime: 1}) while the endRecord is untouched.
Co-authored-by: Matt, Park <[email protected]>
Closes: #2909
What is the purpose of the change
This pull request resolves the issue that interpolated records that have an error were no longer considered erroneous after interpolation. In other words, if the last error time matches the record's time, these will also match in the interpolated record.
Brief Changelog
Testing and Verifying
This change added tests and can be verified as follows:
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? yes