Skip to content
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

Merged
merged 5 commits into from
Sep 13, 2021

Conversation

eswak
Copy link
Contributor

@eswak eswak commented Sep 12, 2021

No description provided.


/// @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");
Copy link
Contributor

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

Copy link
Contributor

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);
Copy link
Contributor

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

Copy link
Contributor

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;
Copy link
Contributor

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

Copy link
Contributor

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;
Copy link
Contributor

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

Copy link
Contributor

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 {
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Joeysantoro Joeysantoro merged commit 8552a81 into fei-protocol:feat/v2 Sep 13, 2021
Copy link
Contributor Author

@eswak eswak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants