-
Notifications
You must be signed in to change notification settings - Fork 58
Performance Fee Calculator Adjust Fee Fix and Scenario Tests #637
Conversation
Pull Request Test Coverage Report for Build 8124
💛 - Coveralls |
uint256 rebalancingSetValue = SetUSDValuation.calculateRebalancingSetValue(msg.sender, oracleWhiteList); | ||
|
||
feeState[msg.sender].lastProfitFeeTimestamp = block.timestamp; | ||
feeState[msg.sender].highWatermark = rebalancingSetValue; |
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 needs to be checked to make sure its higher than the previous high watermark. We don't want to reduce the high watermark by accident
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.
good catch
// Thus, we need to reset the high water mark here so that users do not pay for profit fees | ||
// since inception. | ||
uint256 rebalancingSetValue = SetUSDValuation.calculateRebalancingSetValue(msg.sender, oracleWhiteList); | ||
uint256 existingHighwatermark = feeState[msg.sender].highWatermark; |
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 put this into its own variable?
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.
Thought it would be more clear
30c8297
to
e9d5ab0
Compare
* | ||
* CHANGELOG: | ||
* - 5/17/2020: Update adjustFee function to update high watermark to prevent unexpected fee actualizations | ||
* when the profitFee was initially 0 |
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.
Maybe add a note on the new require as well
@@ -258,6 +262,23 @@ contract PerformanceFeeCalculator is IFeeCalculator { | |||
} else { | |||
validateProfitFeePercentage(feePercentage); | |||
|
|||
// IMPORATNT: In the case that a profit fee is initially 0 and is set to a non-zero number, |
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.
IMPORTANT
await blockchain.mineBlockAsync(); | ||
}); | ||
|
||
describe('and there is a profit', async () => { |
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.
Think we'd want to test whether the new fee is set here as well
@@ -566,6 +568,109 @@ contract('PerformanceFeeCalculator', accounts => { | |||
expect(feeState.profitFeePercentage).to.be.bignumber.equal(newFeePercentage); | |||
}); | |||
|
|||
describe('when the profit fee is initially 0 and a profit is marked', async () => { |
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.
Don't think we need the and a profit is marked phrase here since it tests that and the complementary case below
No description provided.