-
Notifications
You must be signed in to change notification settings - Fork 0
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
Ribbon fuse pool #1
base: fuse-final
Are you sure you want to change the base?
Conversation
import "./interfaces/ILiquidityGauge.sol"; | ||
import "./interfaces/IRibbonVault.sol"; | ||
import {DSMath} from "./libraries/DSMath.sol"; | ||
|
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.
tests for this
* @param cToken The market whose COMP speed to update | ||
*/ | ||
function _updateSpeedWithNewEpoch(CToken cToken) public { | ||
require(block.timestamp.sub(startTime) >= WEEK, "Must be at least week since latest epoch"); |
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.
potential issues:
1: if _updateSpeedWithNewEpoch not called from cron job w fast enough cadence it could be spreading rewards for longer duration than the speed duration was set for (longer than a week). But should not be a problem as long as we continue minting each week so even if we call late they can claim from balance of subsequent (essentially kick can down road indefinitely)
2: if AVG_BLOCKS_PER_WEEK is off, we could be giving out more rewards than speed duration set for (ex: we assume AVG is x but we end up having 1.0025x blocks so we spread out same rewards for more blocks. But should average out and again should not be a problem as long as we continue minting each week so even if we call late they can claim from balance of subsequent (essentially kick can down road indefinitely).
/** | ||
* @notice Set new borrow / supply speed | ||
* @param cToken The market whose COMP speed to update | ||
*/ |
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.
_updateSpeedWithNewEpoch needs to be called from cron job weekly
contracts/CErc20.sol
Outdated
// Minter contract for rbn gauge emissions | ||
RibbonMinter public constant RBN_MINTER = RibbonMinter(0x5B0655F938A72052c46d2e94D206ccB6FF625A3A); | ||
// RBN token | ||
IERC20Upgradeable public constant RBN = IERC20Upgradeable(0x6123b0049f904d730db3c36a31167d9d4121fa6b); |
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.
We can use IERC20 because we're not touching any of the upgradeable functionality
// Underlying is the gauge token like rETH-THETA-gauge | ||
RBN_MINTER.mint(underlying) | ||
|
||
uint256 toDistribute = RBN.balanceOf(address(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.
We can add these lines to return early
if (toDistribute == 0) {
return;
}
@@ -106,7 +140,7 @@ contract RewardsDistributorDelegate is RewardsDistributorDelegateStorageV1, Expo | |||
// Make sure distributor is added | |||
bool distributorAdded = false; | |||
address[] memory distributors = comptroller.getRewardsDistributors(); | |||
for (uint256 i = 0; i < distributors.length; i++) if (distributors[i] == address(this)) distributorAdded = true; | |||
for (uint256 i = 0; i < distributors.length; i++) if (distributors[i] == address(this)) distributorAdded = true; |
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.
Gas optimization for loop:
uint256 distributorsLen = distributors.length;
for (uint256 i = 0; i < distributorsLen; ++i)
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.
is distributors.length really calculated multiple times? this code was not changed its from original comp
* @notice Set borrower PCT | ||
*/ | ||
function _setBorrowerPCT(uint256 _borrowerPCT) public { | ||
require(msg.sender == admin, "only admin can set comp speed"); |
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.
nit only admin can set borrower percent
* @notice Set new borrow / supply speed | ||
* @param cToken The market whose COMP speed to update | ||
*/ | ||
function _updateSpeedWithNewEpoch(CToken cToken) public { |
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.
We can rename this to be without underscore since it's public updateSpeedWithNewEpoch
. We can also change it to external
function updateSpeedWithNewEpoch(CToken cToken) external {
* @notice Burn | ||
* @param Takes in RBN tokens | ||
*/ | ||
function burn(uint256 amount) public { |
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.
Can change this to external
contracts/CErc20.sol
Outdated
@@ -212,6 +216,10 @@ contract CErc20 is CToken, CErc20Interface { | |||
// Underlying is the gauge token like rETH-THETA-gauge | |||
RBN_MINTER.mint(underlying) | |||
|
|||
uint256 toDistribute = RBN.balanceOf(address(this)); | |||
|
|||
RBN.approve(rewardsDistributor, toDistribute) |
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.
you don't want to do an infinite approval once, like in becomeImplementationData
?
|
||
IRibbonVault vault = IRibbonVault(ILiquidityGauge(underlying).lp_token()); | ||
uint256 rVaultDecimals = vault.decimals(); | ||
uint256 rVaultToAssetExchangeRate = vault.pricePerShare(); // (ex: rETH-THETA -> ETH, rBTC-THETA -> BTC) |
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.
Do we not have to lookup the Uniswap v3 pool for this price?
@@ -1,18 +1,35 @@ | |||
pragma solidity ^0.5.16; | |||
|
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.
tests for this
|
||
/** | ||
* @title RewardsDistributorDelegate (COMP distribution logic extracted from `Comptroller`) | ||
* @author Compound | ||
*/ | ||
contract RewardsDistributorDelegate is RewardsDistributorDelegateStorageV1, ExponentialNoError { | ||
using SafeMath for uint; | ||
|
||
/// @dev Notice that this contract is a RewardsDistributor | ||
bool public constant isRewardsDistributor = true; | ||
|
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.
tests for this
Tests can be found in governance repo pr. It includes same RibbonVaultAssetOracle.sol and simplified versions of CERC20.sol and RewardsDistributorDelegate.sol