Conversation
gupadhyaya
force-pushed
the
vault-incentive-implementation
branch
from
e0b9280 to
bed1fdc
Compare
…ne/onebtc into vault-incentive-implementation
brucdarc
left a comment
brucdarc
left a comment
There was a problem hiding this comment.
A few comments/questions on different things. Also I plan to push a couple extra comments I added to the code while reviewing, hopefully that is ok and helps overall documentation.
Specifically, please take a look at the permissioning of setValueRewardAddress in vault registry and make sure it is correct.
…ne/onebtc into vault-incentive-implementation
| uint256 secPerDay = 60 * 60 * 24; | ||
| uint256 elapsedSecs; | ||
| if (block.timestamp <= vault.lockExpireAt) { | ||
| rewardClaimAt = block.timestamp.sub( |
There was a problem hiding this comment.
(not sure what the requirement) but do you need to check if block.timestamp - vault.rewardClaimAt >= 2 weeks. Otherwise, you're effectively shifting the rewardClaimAt back as far as +1 sec, and letting them claim anytime.
There was a problem hiding this comment.
The rewards can be claimed every 14 days. It's one of the requirements.
There was a problem hiding this comment.
Thanks. Wouldn't it make sense to add some checks on minimum period to collect?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah ok. Seems like if you check for period first, you don't need to mod at all.
| ); | ||
| } | ||
|
|
||
| function setVaultRewardAddress(address _vaultReward) external { |
There was a problem hiding this comment.
On the note or architecture, because we use inheritance, and expose various inherited classes as external, it be quite difficult to which is external layer, and internal layer. I think with code size refactor later as we move away from inheritance, we can make this clearer.
4 participants
close #90