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
Remove claimRewards double call #228
Conversation
2304cf4
to
ed732e5
Compare
Morpho-compound-eth-mainnet gas diffs
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
Codecov Report
@@ Coverage Diff @@
## upgrade-0 #228 +/- ##
=============================================
+ Coverage 46.79% 47.22% +0.42%
=============================================
Files 15 15
Lines 359 360 +1
Branches 12 11 -1
=============================================
+ Hits 168 170 +2
Misses 189 189
+ Partials 2 1 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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! Can we do it for Aave V3 as well?
Morpho-aave-v3-avalanche-mainnet gas diffs
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
…ults into fix/claim-rewards-called-twice
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.
Just a suggestion
_accrueUnclaimedRewardsFromRewardsIndex(from, newRewardsIndex); | ||
_accrueUnclaimedRewardsFromRewardsIndex(to, newRewardsIndex); | ||
|
||
super._beforeTokenTransfer(from, to, amount); | ||
} | ||
|
||
function _claimRewards() internal returns (uint256 newRewardsIndex) { | ||
function _claimVaultRewards() internal returns (uint256 newRewardsIndex) { |
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.
why change the name? IMO it's clear enough
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.
Because there's the external function claimRewards
and it may suggest that this internal function is the implementation to claimRewards
@@ -469,21 +469,6 @@ contract TestSupplyVault is TestSetupVaults { | |||
assertApproxEqAbs(uint256(userReward1_1), userReward1_2, 1); | |||
} | |||
|
|||
// TODO: fix this test by using updated indexes in previewMint | |||
// function testShouldMintCorrectAmountWhenMorphoPoolIndexesOutdated() 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.
Did you re-try this test? Perhaps redundant to new tests though..
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.
This test is redundant yes
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.
And it is passing, except on aave-v3 on which the lens was not added
Wow the gas diff 🕺 |
Pull Request
Issue(s) fixed
This pull request fixes #220
It also harmonizes the way we manage rewards, so that most of the code of morpho-compound's tokenized vaults is duplicated in morpho-aave-v3's vault, with an additional dimension:
rewardToken
Look at the gas diff below (they are not broken @MathisGD)! 🚀