feat(multi-rewards): Add support for multi-reward Ubeswap farms #9
Conversation
| } | ||
|
|
||
| assert(_paths.length == _rewardsTokens.length); | ||
| paths = _paths; |
There was a problem hiding this comment.
This copying of a dynamic nested array is weird... Solidity has (had? no idea...) a bunch of weird constraints around nested arrays. This does compile, but I'm not sure if we'll hit runtime errors or not.
| revoBounty = IRevoBounty(_revoBounty); | ||
|
|
||
| stakingToken = IUniswapV2Pair(address(stakingRewards.stakingToken())); | ||
| stakingToken = IUniswapV2Pair(_stakingToken); |
There was a problem hiding this comment.
We have to pass this in manually now rather than getting it from the stakingRewards contract, since the IMoolaStakingRewards interface does not publicly expose the stakingToken address. The actual implementation of MoolaStakingRewards does expose it publicly, but the interface does... We could just update the interface to expose the stakingToken address, but that feels a little weird to me.
| _rewardsTokenBalances[0] = TokenAmount(rewardsToken, _interestEarned + _leftoverBalance); | ||
| // Annoyingly, the MoolaStakingRewards.earnedExternal method is not declared as a view, so we cannot declare this | ||
| // method as a view itself. | ||
| function previewBounty() external returns (TokenAmount[] memory) { |
There was a problem hiding this comment.
This is annoying... but not sure how to work around this.
There was a problem hiding this comment.
does that mean it will cost gas now to call this? not great..
There was a problem hiding this comment.
Not necessarily? I think you can do an e.g., farmBotContract.methods.previewBounty().call() using web3 which will emulate the call locally which should be fine? Although not really sure to be honest.
| path0 = _path0; | ||
| path1 = _path1; | ||
| function updatePaths(address[][2][] memory _paths) external onlyOwner { | ||
| paths = _paths; |
There was a problem hiding this comment.
should there be a check here to make sure the rewards tokens are still at the ends of the new paths?
There was a problem hiding this comment.
otherwise the owner could force farm bots to buy an unrelated token, and drive up its price or something. kind of a weird threat model admittedly but easy enough to prevent
There was a problem hiding this comment.
Yeah, that's probably a good idea, although that check is not super trivial to implement. It's not obvious, but the claimRewards call where the swapping occurs will actually fail if the end tokens in the path don't align with the constituent tokens of the LP, since we won't have as much of token0/token1 as we say we do when calling the router's addLiquidity method. (I'm pretty sure this is true, not 100% though.)
| _rewardsTokenBalances[0] = TokenAmount(rewardsToken, _interestEarned + _leftoverBalance); | ||
| // Annoyingly, the MoolaStakingRewards.earnedExternal method is not declared as a view, so we cannot declare this | ||
| // method as a view itself. | ||
| function previewBounty() external returns (TokenAmount[] memory) { |
There was a problem hiding this comment.
does that mean it will cost gas now to call this? not great..
Does what it says on the tin. This is untested, but does compile. The deploy script hasn't been updated since I don't even know if any MoolaStakingReward contracts are deployed on Alfajores, so you'll have to comment
mainout of the deploy script if you want to compile.This definitely needs tests. Ideally, we spin up/tear down a local chain and deploy "dummy" contracts each time to test against for quicker iteration/validation. If getting that set up proves to be too much frontloaded overhead, a stopgap could be manually deploying contracts and testing on Alfajores, but that's obviously not a long-term solution.