-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix validation issue caused by truncation of risk params #465
Conversation
@@ -156,8 +156,8 @@ library RiskParameterStorageLib { | |||
|
|||
if (self.minMargin.lt(self.minMaintenance)) revert RiskParameterStorageInvalidError(); | |||
|
|||
UFixed6 scaleLimit = self.makerLimit.div(self.efficiencyLimit).mul(protocolParameter.minScale); | |||
if (self.takerFee.scale.lt(scaleLimit) || self.makerFee.scale.lt(scaleLimit)) | |||
UFixed6 scaleLimit = UFixed6Lib.from(self.makerLimit.truncate()).div(self.efficiencyLimit).mul(protocolParameter.minScale); |
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.
Not sure how we feel about line length here. Well over 120, like other parts of the file, but splitting out the lines reduces readability quite a bit.
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.
maybe this is a little more readable:
(UFixed6 makerLimitTruncated, UFixed6 takerFeeScaleTruncated, UFixed6 makerFeeScaleTruncated) = (
UFixed6Lib.from(self.makerLimit.truncate()),
UFixed6Lib.from(self.takerFee.scale.truncate()),
UFixed6Lib.from(self.makerFee.scale.truncate())
);
UFixed6 scaleLimit = makerLimitTruncated.div(self.efficiencyLimit).mul(protocolParameter.minScale);
if (takerFeeScaleTruncated.lt(scaleLimit) || makerFeeScaleTruncated.lt(scaleLimit))
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; happy to add those variables to improve readability. Done in 8e795c0.
[Periphery] Unit Test Coverage ReportCoverage after merging ed/41-limit-truncation-a into v2.3-fix-review will be
Coverage Report
|
[Core] Integration Test Coverage ReportCoverage after merging ed/41-limit-truncation-a into v2.3-fix-review will be
Coverage Report
|
[Periphery] Integration Test Coverage ReportCoverage after merging ed/41-limit-truncation-a into v2.3-fix-review will be
Coverage Report
|
[Periphery] Combined Test Coverage ReportCoverage after merging ed/41-limit-truncation-a into v2.3-fix-review will be
Coverage Report
|
[Core] Unit Test Coverage ReportCoverage after merging ed/41-limit-truncation-a into v2.3-fix-review will be
Coverage Report
|
[Core] Combined Test Coverage ReportCoverage after merging ed/41-limit-truncation-a into v2.3-fix-review will be
Coverage Report
|
Fixes sherlock-audit/2024-08-perennial-v2-update-3-judging#41