Add RewardsDistributorAdmin, AutoRewardsDistributor and IRewardsDistributor#202
Add RewardsDistributorAdmin, AutoRewardsDistributor and IRewardsDistributor#202ElliotFriedman merged 22 commits intomasterfrom
Conversation
| import "../refs/CoreRef.sol"; | ||
| import "../external/rari/IRewardsDistributor.sol"; | ||
|
|
||
| /// Controller Contract to set tribe per block in Rewards Distributor Admin on Rari |
There was a problem hiding this comment.
here and elsewhere use canonical natspec annotations
xklob
left a comment
There was a problem hiding this comment.
Added some comments, but overall looks great so far!
| }); | ||
|
|
||
| it('compSupplySpeeds', async function () { | ||
| expect(await this.rewardsDistributorAdmin.compSupplySpeeds(ZERO_ADDRESS)).to.be.equal(toBN('0')); |
There was a problem hiding this comment.
what is this testing exactly?
There was a problem hiding this comment.
testing that initial compSupplySpeeds are 0. I pass 0 address as this doesn't matter in the test because it is just hitting a mock.
|
|
||
| contract RewardsDistributorAdmin is IRewardsDistributorAdmin, CoreRef, AccessControlEnumerable { | ||
|
|
||
| bytes32 public constant override AUTO_REWARDS_DISTRIBUTOR = keccak256("AUTO_REWARDS_DISTRIBUTOR"); |
There was a problem hiding this comment.
call this AUTO_REWARDS_DISTRIBUTOR_ROLE
| /// @notice Governance should create new admin role for this contract | ||
| /// and then grant this role to all AutoRewardsDistributors so that they can call this contract | ||
| _setContractAdminRole(keccak256("REWARDS_DISTRIBUTOR_ADMIN_ROLE")); | ||
| _setContractAdminRole(keccak256("TRIBAL_CHIEF_ADMIN_ROLE")); |
There was a problem hiding this comment.
Add a comment explaining why we reuse the TribalChief one
| } else { | ||
| rewardsDistributorAdmin._setCompSupplySpeed(cTokenAddress, compSpeed); | ||
| } | ||
| emit SpeedChanged(isBorrowIncentivized, compSpeed, cTokenAddress); |
There was a problem hiding this comment.
isborrowIncentivized is immutable state, same with cTokenAddress. These can be removed from the event. Normally events only want to emit changed/mutable state
| IRewardsDistributorAdmin public rewardsDistributorAdmin; | ||
| /// @notice tribal chief contract | ||
| ITribalChief public tribalChief; | ||
| /// @notice address of the CToken this contract controls rewards for | ||
| address public cTokenAddress; | ||
| /// @notice boolean which decides the action to incentivize | ||
| bool public isBorrowIncentivized; | ||
| /// @notice reward index on tribal chief to grab this staked token wrapper's index | ||
| uint256 public tribalChiefRewardIndex; |
There was a problem hiding this comment.
All of these can be immutable except rewardDistributorAdmin
| function setRewardsDistributorAdmin( | ||
| IRewardsDistributorAdmin _rewardsDistributorAdmin | ||
| ) external onlyGovernorOrAdmin { | ||
| rewardsDistributorAdmin = _rewardsDistributorAdmin; |
There was a problem hiding this comment.
This should have an event
| } | ||
|
|
||
| /** | ||
| * @notice Accepts transfer of admin rights. msg.sender must be pendingAdmin |
There was a problem hiding this comment.
this comment is slightly inaccurate. "this" must be pendingAdmin not the sender
There was a problem hiding this comment.
I just copy and pasted this comment from the rari codebase as it is a passthrough.
… & borrow speeds functionality
…nts to poolAllocPoints
Todo:
Done: