fix(x/twap): incorrect time delta due to nanoseconds in time #4359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
p0mvn
added
A:backport/v15.x
mattverse
approved these changes
Feb 20, 2023
| @@ -62,6 +60,12 @@ func LexicographicalOrderDenoms(denom0, denom1 string) (string, string, error) { | |||
| return denom0, denom1, nil | |||
| } | |||
|
|
|||
| // CanonicalTimeMs returns the canonical time in milleseconds used for twap | |||
| // math computations in UTC. Removes any monotonic clock reading prior to conversion to ms. | |||
| func CanonicalTimeMs(twapTime time.Time) int64 { | |||
There was a problem hiding this comment.
just curious, why is it called "canonical" time?
There was a problem hiding this comment.
Canonical with respect to block times. It strips away any mononic part from the clock and returns the time in UTC,
This should already be done given that we use ctx.BlockTime(). However, this is useful for tests since we might not be setting up the time correctly in tests
| @@ -182,7 +182,7 @@ func recordWithUpdatedAccumulators(record types.TwapRecord, newTime time.Time) t | |||
| return record | |||
| } | |||
| newRecord := record | |||
| timeDelta := newTime.Sub(record.Time) | |||
| timeDelta := types.CanonicalTimeMs(newTime) - types.CanonicalTimeMs(record.Time) | |||
There was a problem hiding this comment.
can we add a one line comment above here on why we use canonical time for the time delta?
There was a problem hiding this comment.
Since this is called in multiple places, I elaborated in the method spec instead:
937d148
Co-authored-by: Matt, Park <[email protected]>
ValarDragon
approved these changes
Feb 20, 2023
mergify bot
pushed a commit
that referenced
this pull request
Feb 20, 2023
* fix(x/twap): incorrect time delta due to nanoseconds in time * remove logs * more clean ups * changelog * restore arithmetic twap test * remove old * unskip geometric * Update x/twap/types/utils.go Co-authored-by: Matt, Park <[email protected]> * comment --------- Co-authored-by: Matt, Park <[email protected]> (cherry picked from commit f56280f) # Conflicts: # CHANGELOG.md
p0mvn
added a commit
that referenced
this pull request
Feb 20, 2023
p0mvn
added a commit
that referenced
this pull request
Feb 20, 2023
…#4359) (#4374) Co-authored-by: Roman <[email protected]>
pysel
pushed a commit
that referenced
this pull request
Feb 21, 2023
* fix(x/twap): incorrect time delta due to nanoseconds in time * remove logs * more clean ups * changelog * restore arithmetic twap test * remove old * unskip geometric * Update x/twap/types/utils.go Co-authored-by: Matt, Park <[email protected]> * comment --------- Co-authored-by: Matt, Park <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #4342
What is the purpose of the change
This PR fixes twap jitter that is caused by nanoseconds being accounted for in the time delta calculations.
In twap, we assume the time delta in milliseconds. However, the block time is given in nanoseconds. As a result, there were some cases such as:
where if
startRecord.Timecontained just a few nanoseconds, it would cause the timeDelta to fall by 1 ms below the expected value, causing a larger denominator in arithmetic mean computations. This would lead the final twap being off in the 10^-4-10^-2 digit.To fix this, a function
types.CanonicalTimeMsis introduced that converts the time stored in state into unix milliseconds with monotonic clock stripped away. This is consistent with how CometBFT handles time.Brief Changelog
types.CanonicalTimeMsfunctionTesting and Verifying
This change added tests and is covered by e2e
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? yes