RewardsManager test refactor and cleanup #718
Conversation
| assertEq(bETimestamp, timestamp); | ||
| assertEq(bEInterest, interest); | ||
| assertEq(bEBurned, burned); | ||
| assertEq(burned, tokensToBurn); |
There was a problem hiding this comment.
@MikeHathaway I was curious on how you would reccomend I compare these. The equation should be rewardsToClaimer + rewardsToUpdater = burned * 0.9 is that correct?
There was a problem hiding this comment.
Not quite. The claimer has a 5% claim factor, and the updaters have a 5% claim factor, but the claim must actually be made so it's unclear how much rewards will actually be distributed. Also, there's a bit of a first come first serve element as total rewards are capped at burned * 0.8
There was a problem hiding this comment.
After a discussion with Mike I deceided to not go through the cumbersome effort of attempting to compute the rewards in _assertBurn(). Updated things accordingly.
| /***********************/ | ||
|
|
||
| // need to retrieve the position managers index set since positionIndexes are stored unordered in EnnumerableSets | ||
| secondIndexes = _positionManager.getPositionIndexes(tokenIdOne); //TODO: investigate why commenting this out or keeping it doesn't do anything |
There was a problem hiding this comment.
@MikeHathaway noticing that by removing this, the function still passes. Wanted to confirm I'm not missing something here.
There was a problem hiding this comment.
You should need that line in order to pass as otherwise it should attempt to make the same set of movements that were made in the previous movement (secondIndexes) not updated. The comment is about getting the current order from the contract since the contract's order won't maintain order
There was a problem hiding this comment.
I am able to pass without this line in the tests... Here are the two outputs:
commented out, uses original secondIndexes variable above:
secondIndexes[0] = 2556;
secondIndexes[1] = 2557;
secondIndexes[2] = 2558;
secondIndexes[3] = 2559;
secondIndexes[4] = 2560;
Tests still pass / no change needed if I comment it out.
uncommented:
Logs:
secondInd[0]: 2559
secondInd[1]: 2556
secondInd[2]: 2557
secondInd[3]: 2558
I will uncomment them as requested but want you to know it doesn't have an effect on the test.
ith-harvey
changed the title
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
nit: doge says "much blank, very whitespace"
There was a problem hiding this comment.
; ) 🐶 that he does. Fixed -> a636196
| uint256 tokenId, | ||
| uint256 reward, | ||
| uint256[] memory epochsClaimed | ||
| ) internal { |
| deal(address(quote), minter, mintAmount * indexes.length); | ||
|
|
||
| // approve tokens | ||
| collateral.approve(address(pool), type(uint256).max); |
There was a problem hiding this comment.
Why does the minter need collateral?
There was a problem hiding this comment.
nope! fixed -> a636196
| uint256 randomIndex = randomInRange(0, copyOfArray.length - i - 1); | ||
| subsetArray[i] = copyOfArray[randomIndex]; | ||
| copyOfArray[randomIndex] = copyOfArray[copyOfArray.length - i - 1]; | ||
| } |
There was a problem hiding this comment.
This algorithm copies an element from the back of the array into the random index you plucked from copyOfArray? Is this a strategy to avoid duplicates?
There was a problem hiding this comment.
This existed in the previous versions of the tests... I merely copied it over.
After some review it looks like it does in fact limit duplicates from existing in the subsetArray ill be it in a very confusing way.
Also noticed that randomInRange appears to not reproduce random results when I tested it in a unit test...
However I think this work should be done in a separate PR as it's outside of scope of this change. Let me know if you see it differently.
Description of change
High level
RewardsManager.t.solintoRewards/RewardsManager.t.sol&Rewards/RewadsDSTestPlus.t.sol