feat(x/twap): use exp2 in twapPow; add overflow test #3809
Merged
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
the
V:state/compatible/no_backport
p0mvn
commented
Dec 21, 2022
p0mvn
commented
Dec 21, 2022
Comment on lines
+269
to
+271
| if exponent.IsNegative() { | ||
| return osmomath.OneDec().Quo(exp2).SDKDec() | ||
| } |
There was a problem hiding this comment.
Note: this is possible if sp0 is < 1
Currently, there is a micro-optimization possible to avoid taking 1 / result twice. This happens when:
arithmeticMeanOfLogPrices.IsNegative() && quoteAsset == startRecord.Asset1Denom
I'm planning to revisit this in the future
p0mvn
requested review from
czarcas7ic,
stackman27,
nicolaslara and
ValarDragon
|
amazing the TWAP testing time spent in pow has 10x decreased, great sign for our Exp2 fn :) |
ValarDragon
reviewed
Dec 21, 2022
ValarDragon
reviewed
Dec 21, 2022
x/twap/strategy_test.go
Outdated
| RoundingDir: osmomath.RoundDown, | ||
| } | ||
|
|
||
| oneYear := OneSec.MulInt64(60 * 60 * 24 * 365) |
There was a problem hiding this comment.
Do we also need to test min spot price, since this can get underflow? (or is that a future PR?)
There was a problem hiding this comment.
(not blocking merge, as long as we track in an issue otherwise)
There was a problem hiding this comment.
Tested with 100 years - will address underflow in #3815
ValarDragon
approved these changes
Dec 21, 2022
Co-authored-by: Dev Ojha <[email protected]>
This was referenced Dec 21, 2022
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: #3540
What is the purpose of the change
This PR switches to use the new
Exp2function (#3708) in x/twap for computing geometric twap.It also adds a test at max spot price, showing that there is no overflow.
Testing and Verifying
This change added overflow test and refactored twap math function tests to be simpler.
Note that adding more hand-calculated tests is tracked in: #3541
On top of this PR, I made a PR with query tests that currently pass: #3802
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no