-
Notifications
You must be signed in to change notification settings - Fork 10
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
Optional claim/stash SP ETH rewards #158
Conversation
d9d84ac
to
c087ca6
Compare
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!
contracts/src/StabilityPool.sol
Outdated
|
||
// If they have a deposit, update it and update its snapshots | ||
if (initialDeposit > 0) { | ||
currentETHGain = getDepositorETHGain(msg.sender); // Only active deposits can only have a current ETH gain |
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.
I guess only one only is 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.
Only one comment? I was trying to say two things here: the second line aims to point out that their current gain must be zero if their deposit initial value is zero.
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.
I meant that the second comment says “only” twice. And I guess it doesn’t mean that active deposits cannot have stashed gains.
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.
Oh! haha, right. Should just be one. Fixed.
contracts/src/StabilityPool.sol
Outdated
uint256 stashedETHGain = stashedETH[msg.sender]; | ||
uint256 ETHToSend = currentETHGain + stashedETHGain; | ||
|
||
stashedETH[msg.sender] = 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.
Aren’t these 3 lines the same as _getTotalETHGainAndZeroStash
? Can’t we reuse that function?
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.
Right, good point! I've refactored that
doClaim
when providing/withdrawing from SP. They can choose to either have their ETH gains paid out, or stashed for future claimingclaimAllETHGains
pays out a depositor's total ETH gain (current gains accrued since last touch, and stashed gains)Closes #122