[ETHEREUM-CONTRACTS] distinguish between unsettled and claimable balance in pools#2084
[ETHEREUM-CONTRACTS] distinguish between unsettled and claimable balance in pools#2084
Conversation
Changelog ReminderReminder to update the CHANGELOG.md for any of the modified packages in this PR.
|
|
what prompted this issue? I prefer this be done after #2086 refactoring. |
|
UnsettledValue shouldn't be "leaked" as such. I will reject this one for now. I will continue from #2086, where we can touch this topic, too. |
|
this is now based on #2086 |
Note that it's not part of the interface. The PR doesn't change the public interface, it just fixes the behaviour of |
|
|
||
| function _claimAll(address memberAddr, uint32 time) internal returns (int256 amount) { | ||
| amount = getClaimable(memberAddr, time); | ||
| function _settle(address memberAddr, int256 amount) internal { |
There was a problem hiding this comment.
I will have to reject this impelemtation, for design reason:
This function is effectful and has no constraint on what "amount" can be provided to it. Because of this, now as a code reader, you will have to worry if all the call sites of this function provided this "amount" correctly. The more robust design is to avoid such possibility, keep reasoning of correctness local.
In this case, it is quite easy to do, I will do a change in the following commmit.
hellwolf
left a comment
hellwolf
left a comment
There was a problem hiding this comment.
checkout the updated _settle, then we can merge.
Signed-off-by: pavedroad <qcqs@outlook.com>
XKCD Comic RelifLink: https://xkcd.com/2084 |
3 participants
What & Why
getClaimableshould return the amount which will be added to the balance if callingclaimAll. This is currently not the case, because the implementation mixes up the concepts of claimable amount and unsettled amount.The point of the claiming concept is to allow pool members to fetch tokens allocated to them even when they are not connected to that pool. If the pool is connected, there's nothing to be claimed - the connected status could conceptually also be seen as being continuous auto-claiming.
If
getClaimablereturns a non-zero number, a call ofclaimAllis expected to add that amount to the user balance.With the current implementation that is true only for unconnected pools.
How
The inconsistency is solved by making
getClaimablealways return 0 for connected pools.This is achieved by having distinct logic for internal settlement and for claiming.