-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add resistantBalanceAndFei to Curve deposit #169
Add resistantBalanceAndFei to Curve deposit #169
Conversation
|
||
/// @notice set the minimum ratio threshold for a valid reading of restistant balances | ||
function setMinRatio(uint256 _minimumRatioThreshold) public onlyGovernor { | ||
require(_minimumRatioThreshold < maximumRatioThreshold, "Min ratio must be less than max ratio"); |
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.
A good pattern for this is to have an internal method that gets called by both the governor setter and constructor
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.
@Joeysantoro fixed
require(minimumRatioThreshold < maximumRatioThreshold, "Min ratio must be less than max ratio"); | ||
require(minimumRatioThreshold >= 0.01e18, "Min ratio must be at least 1%."); | ||
require(maximumRatioThreshold <= 100e18, "Max ratio cannot be above 100x."); | ||
setMinRatio(_minimumRatioThreshold); |
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.
Should call the internal ones, caller is unlikely to be a gov
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.
Fixed
minimumRatioThreshold = _minimumRatioThreshold; | ||
} | ||
|
||
/// @notice set the maximum ratio threshold for a valid reading of resistant balances | ||
function setMaxRatio(uint256 _maximumRatioThreshold) public onlyGovernor { | ||
require(_maximumRatioThreshold > minimumRatioThreshold, "Max ratio must be greater than min ratio"); | ||
require(_maximumRatioThreshold <= 100e18, "Max ratio cannot be above 100x."); | ||
|
||
maximumRatioThreshold = _maximumRatioThreshold; |
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.
Should call internal method here
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.
Done
@@ -97,22 +96,30 @@ contract StableSwapOperatorV1 is PCVDeposit { | |||
|
|||
/// @notice set the minimum ratio threshold for a valid reading of restistant balances | |||
function setMinRatio(uint256 _minimumRatioThreshold) public onlyGovernor { | |||
require(_minimumRatioThreshold < maximumRatioThreshold, "Min ratio must be less than max ratio"); | |||
require(_minimumRatioThreshold >= 0.01e18, "Min ratio must be at least 1%."); | |||
|
|||
minimumRatioThreshold = _minimumRatioThreshold; |
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.
Should call internal method here
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.
Done
_setMaxRatioThreshold(_maximumRatioThreshold); | ||
} | ||
|
||
function _setMinRatioThreshold(uint256 _minimumRatioThreshold) private { |
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.
I like to keep these methods internal
vs private in case we ever write a superclass that needs to overwrite these vars
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.
Done
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.
cool, thanks
No description provided.