feature/reward-master-pool #95
Conversation
|
@maxsam4 probably needs more extensive tests for the math, however shouldn't be too weird as its a copy of MasterChefV2 just isolated. |
contracts/mocks/ERC20Mock.sol
Outdated
| @@ -12,4 +12,9 @@ contract ERC20Mock is ERC20 { | |||
| ) ERC20(name, symbol) { | |||
| _mint(msg.sender, supply); | |||
| } | |||
|
|
|||
| function safeTransfer(address recipient, uint256 amount) external returns (bool) { | |||
There was a problem hiding this comment.
sushi doesn't have a safeTransfer function. Maybe you want to use a library that has safeTransfer function.
contracts/rewards/RewardsManager.sol
Outdated
|
|
||
| IRewarder _rewarder = rewarder[address(pool)]; | ||
| if (address(_rewarder) != address(0)) { | ||
| _rewarder.onSushiReward(address(pool), account, account, _pendingSushi, amount); |
There was a problem hiding this comment.
this will allow ops to hold lp hostage by making the _rewarder revert on any transfers. We need a way around that in emergencies.
There was a problem hiding this comment.
hmm, this is how MCv2 works, but I agree. Let's throw a try catch around this and ignore reverts. Wdyt?
There was a problem hiding this comment.
Yea, that's a bug, not a feature. Will be fixed in the next version.
As for how to handle these, I'm a bit split. Perhaps we should just add an emergencyTransfer function that bypasses the rewards stuff.
sometimes, the rewarders run out of tokens, and in those cases, we kinda want the reward claiming to revert for most users, or else they will lose all their rewards.
There was a problem hiding this comment.
The problem with the emergencyTransfer function is that its not very good UX, if someone transfers LP tokens from metamask or uses a contract that does a transferFrom, you'll always use transfer.
There was a problem hiding this comment.
Echoing Mudit on this, it's a pain point of MCv2, but was intended to avoid an exploit. It is a very clear problem for serious LP though whole need ability to be agile with their liquidity, if rewarders run out of tokens, or they emit them all malliciously, LP is held hostage which is a major problem IMO. Interesting to see the solutions around this though.
Btw @decanus, super cool to see rewards being handled on a pool level. A great showcase of the flexibility a system like this offers.
There was a problem hiding this comment.
Yeah I agree with the issue. Let's find a solution that works best so we can get this merged.
There was a problem hiding this comment.
@matthewlilley check the fix I made, this may work: 7ddc657
| try _rewarder.onSushiReward(address(pool), account, account, _pendingSushi + reward.pendingSushi, amount + reward.amount) { | ||
| unclaimedRewards[address(pool)][account] = RewardAmount({pendingSushi: 0, amount: 0}); | ||
| } catch { | ||
| unclaimedRewards[address(pool)][account] = RewardAmount({pendingSushi: reward.pendingSushi + _pendingSushi, amount: reward.amount + amount}); |
There was a problem hiding this comment.
Imagine if a user has 10 LP tokens. If they call it once while the rewarder fails, on the next call the rewarder is called with a new LP amount of 20, even though the user still only has 10 LP tokens. If the event is e.g. a transfer it is also problematic because the rewards continue to run for two accounts.
There was a problem hiding this comment.
Yeah we need to think about how best to do this. Maybe subtract the rewards.amount from pendingRewards if its greater than 0?
Better model for rewards