-
Notifications
You must be signed in to change notification settings - Fork 2
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
Integrate $BB in YieldDistributor #94
Conversation
Probably should make a new deploy script for "from scratch" deploys, however for practical purposes having these deploy scripts separated is useful. I elected to not replicate the tests for buttered bread as it is also an ERC20Votes and all that functionality is already tested for $BREAD |
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.
Looks good other than potential non-set revert issue
Will be adding a test soon |
@@ -66,8 +69,15 @@ contract YieldDistributor is IYieldDistributor, OwnableUpgradeable { | |||
address[] memory _projects | |||
) public initializer { | |||
__Ownable_init(msg.sender); | |||
|
|||
if ( |
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 think this is more readable personally
if (
_bread == address(0) ||
_butteredBread == address(0) ||
_precision == 0 ||
_minRequiredVotingPower == 0 ||
_maxPoints == 0 ||
_cycleLength == 0 ||
_yieldFixedSplitDivisor == 0 ||
_lastClaimedBlockNumber == 0 ||
_projects.length == 0
) {
revert MustBeGreaterThanZero();
}
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.
formatter overrides this.. do you know if there's a comment I can add for it to ignore this?
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.
Aside from changing line_length
, which i wouldn't recommend, not really. all good
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 few comments
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.
lgtm
uint256 _lastClaimedBlockNumber = stdJson.readUint(config_data, "._lastClaimedBlockNumber"); | ||
uint256 _yieldFixedSplitDivisor = stdJson.readUint(config_data, "._yieldFixedSplitDivisor"); | ||
// See test/YieldDistributor.t.sol for explanation of these values | ||
uint256 _minRequiredVotingPower = stdJson.readUint(config_data, "._minRequiredVotingPower"); |
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 recommend in the future, inheriting the deployment script from the Scripts folder into the test setup, which will also inherit the stdJson datas. but for now, this works
Opened #97 to address issues introduced with this PR , merging for now :) |
#77