[Fix] Add maximum for intent price deviation #468
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
arjun-io
reviewed
Oct 10, 2024
| /// @param price The oracle price to compare | ||
| /// @return The price deviation of the guarantee from the oracle price | ||
| function priceDeviation(Guarantee memory self, Fixed6 price) internal pure returns (UFixed6) { | ||
| if (takerTotal(self).isZero()) return UFixed6Lib.ZERO; |
There was a problem hiding this comment.
Should we use taker(self) here to correspond with the below div?
There was a problem hiding this comment.
takerTotal is more correct for measuring whether the guarantee is empty, since you can have an aggregated guarantee net to zero, but we need to use taker in the math below for the sign.
| if (takerTotal(self).isZero()) return UFixed6Lib.ZERO; | ||
|
|
||
| Fixed6 guarenteePrice = self.notional.div(taker(self)); | ||
| return guarenteePrice.sub(price).div(guarenteePrice.min(price)).abs(); |
There was a problem hiding this comment.
Is it safe to assume that price and guaranteePrice will not be 0 here?
There was a problem hiding this comment.
It should not be possible for price to be zero due to the latest oracle override.
The guaranteePrice could be zero, but this would correctly revert due to being infinitely out of range.
Any reverts here would only block a fresh update, it wouldn't brick the settlement flywheel.
There was a problem hiding this comment.
Might be worth adding a comment stating that, to avoid audit reports on the same.
arjun-io
approved these changes
Oct 10, 2024
EdNoepel
reviewed
Oct 11, 2024
| if (takerTotal(self).isZero()) return UFixed6Lib.ZERO; | ||
|
|
||
| Fixed6 guarenteePrice = self.notional.div(taker(self)); | ||
| return guarenteePrice.sub(price).div(guarenteePrice.min(price)).abs(); |
There was a problem hiding this comment.
Might be worth adding a comment stating that, to avoid audit reports on the same.
| @@ -49,7 +52,8 @@ using MarketParameterStorageLib for MarketParameterStorage global; | |||
| /// uint24 riskFee; // <= 1677% | |||
| /// uint16 maxPendingGlobal; // <= 65k | |||
| /// uint16 maxPendingLocal; // <= 65k | |||
| /// uint48 __unallocated__; // <= 281m | |||
| /// uint24 maxPriceDeviation; // <= 16.77x | |||
There was a problem hiding this comment.
nit: // <= 1677% seems more consistent and appropriate here
| function priceDeviation(Guarantee memory self, Fixed6 price) internal pure returns (UFixed6) { | ||
| if (takerTotal(self).isZero()) return UFixed6Lib.ZERO; | ||
|
|
||
| Fixed6 guarenteePrice = self.notional.div(taker(self)); |
There was a problem hiding this comment.
nit: spelling guaranteePrice
arjun-io
approved these changes
Oct 11, 2024
[Periphery] Unit Test Coverage ReportCoverage after merging britz-add-price-override-bound into v2.3-fix-review will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Integration Test Coverage ReportCoverage after merging britz-add-price-override-bound into v2.3-fix-review will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Periphery] Integration Test Coverage ReportCoverage after merging britz-add-price-override-bound into v2.3-fix-review will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Periphery] Combined Test Coverage ReportCoverage after merging britz-add-price-override-bound into v2.3-fix-review will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Unit Test Coverage ReportCoverage after merging britz-add-price-override-bound into v2.3-fix-review will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Combined Test Coverage ReportCoverage after merging britz-add-price-override-bound into v2.3-fix-review will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Adds a new market parameter:
maxPriceDeviationwhich caps the geometric distance that the quoted intent price can be from the underlying oracle price.Suggested change from: sherlock-audit/2024-08-perennial-v2-update-3-judging#42.