Staking 4 deferred lock #409
Conversation
Now it's possible to pass a start date for locks
It was actually done in previous commit, but with this one locks array is converted to a mapping, as otherwise we could end up with a huge array with a lot of empty elements. Added an array for ids for loops and counting. Fixes #5.
bingen
force-pushed
the
staking_4_deferred_lock
branch
from
92523f7 to
3e9478c
Compare
There was a problem hiding this comment.
Very good job with this PR.
As we discussed offline, I'm a bit worried about the amount of complexity and increased gas costs as a result of adding overlocking.
If overlocking is off, then we don't need to keep the ordered list of locks per unlocker, but that would be weird as structs would be empty depending on the mode.
I'm not convinced that supporting both modes in the app is the best solution, a couple of alternative ideas to explore:
-
Have a separate 'StakingOverlocking' app that extends Staking to support overlocking, only overloading the methods needed to make it work. The main downside will be the fact that the structs cannot be extended and it could be end up being not so clean.
- Another way to do this could be to extract all the core Staking logic to a 'StakingBase' contract and then build two different Staking apps on top with the two overlocking modes. These apps would implement completely independent logic for lock management. An advantage of this solution would also be that the base Staking app could just implement the ERC900 spec which could be used as an standalone staking app without locks.
-
Maybe we don't need overlocking at the staking app level at all and it can be managed by the app that will be the unlocker directly, helped by a staking lock manager library that apps can use. We are already duplicating logic for checking locks in PLCR and the Curation app, which could maybe benefit from just using a library to check locks. This library maybe can also be used to mimic the overlocking functionality while keeping Staking locks simple.
| @@ -13,28 +13,41 @@ contract Staking is ERCStaking, IStaking, AragonApp { | |||
| using SafeMath for uint256; | |||
|
|
|||
| uint64 constant public MAX_UINT64 = uint64(-1); | |||
| address constant public ANY_ENTITY = address(uint160(-1)); | |||
There was a problem hiding this comment.
Simpler to just do address(-1)
There was a problem hiding this comment.
Also the name of ANY_ENTITY it is quite confusing for how it is used in the app.
| TimeUnit unit; | ||
| } | ||
|
|
||
| enum TimeUnit { Blocks, Seconds } | ||
|
|
||
| ERC20 public stakingToken; | ||
| bool public overlocking; // if true, an unlocker can use the same stake in different locks |
There was a problem hiding this comment.
It would be better to have the comment on what overlocking is in the initialize(...) function comment docs.
There was a problem hiding this comment.
On the name of the variable I think something like allowsOverlocking or allowsLockOverlap would be a bit more descriptive (not super convinced about these naming proposals either).
|
|
||
| return uint64(comparingValue) > timespan.end; | ||
| return comparingValue < timespan.start || comparingValue > timespan.end; |
There was a problem hiding this comment.
Looks like both start and end are inclusive now (makes sense that it is to avoid an unlock in the same block when doing lockNow), this should be clearly reflected by documenting it in a comment.
| public | ||
| returns(uint256 lockId) | ||
| { | ||
| // lock 0 tokens makes no sense | ||
| require(amount > 0); | ||
|
|
||
| Lock memory newLock = Lock(amount, Timespan(lockEnds, TimeUnit(lockUnit)), unlocker, metadata); | ||
| lockId = accounts[msg.sender].locks.push(newLock) - 1; | ||
| // TODO: should we prevent starting locks in the past? |
There was a problem hiding this comment.
I don't think we should, can't think of any problems this could cause. I see however an issue with someone sending a transaction with a particular lockStarts that doesn't get included in a block for some time and eventually fails for this reason.
| } | ||
|
|
||
| function canUnlock(address acct, uint256 lockId) public view returns (bool) { | ||
| Lock memory acctLock = accounts[acct].locks[lockId]; | ||
|
|
||
| return timespanEnded(acctLock.timespan) || msg.sender == acctLock.unlocker; | ||
| return outOfTimespan(acctLock.timespan) || msg.sender == acctLock.unlocker; |
There was a problem hiding this comment.
As pointed out here we should only allow for someone to unlock their own lock before it starts, to prevent someone else removing locks before the come into effect.
| } | ||
| } else { // without overlocking all locks must be subtracted | ||
| for (uint256 i = 0; i < accounts[acct].lockIds.length; i++) { | ||
| unlockedTokens = unlockedTokens.sub(accounts[acct].locks[accounts[acct].lockIds[i]].amount); | ||
| } |
There was a problem hiding this comment.
Should take into account if the locks can be be unlocked and not discount them.
| if (overlocking) { // with ovelocking we only discount biggest one for each unlocker | ||
| for (uint256 j = 0; j < account.unlockers.length; j++) { | ||
| if (unlocker == ANY_ENTITY || unlocker != account.unlockers[j].account) { | ||
| unlockedTokens = unlockedTokens.sub(accounts[acct].locks[account.unlockers[j].maxLockId].amount); |
There was a problem hiding this comment.
If the biggest lock id can be unlocked, then it should find which is the next biggest lock that cannot be unlocked for that unlocker.
| // delete from lockIds array | ||
| // find it first | ||
| uint256 locksLength = accounts[acct].lockIds.length; | ||
| for (uint256 j = 0; j < locksLength; j++) { |
There was a problem hiding this comment.
We should move this logic for finding an item in the array to another function:
function indexOf(uint256[] storage array, uint256 item) internal returns (uint256 index) {
uint256 arrayLength = array.length;
for (uint256 i = 0; i < arrayLength; i++) {
if (array[i] == item) {
return i;
}
}
revert();
}
| accounts[msg.sender].locks[lockId] = newLock; | ||
| accounts[msg.sender].lockIds.push(lockId); | ||
|
|
||
| insertUnlockerLock(unlocker, lockId); |
There was a problem hiding this comment.
As we talked during our review, when overlocking is off, there should be no need to insert this
| // remove from double linked list | ||
| Lock storage acctLock = accounts[acct].locks[lockId]; | ||
| // prev | ||
| if (acctLock.prevUnlockerLockId == 0) { // it was the first one |
There was a problem hiding this comment.
This entire block is only required if the staking app is in overlocking mode.
|
Closed in favor of https://github.com/aragon/staking/ |
Allow to create locks for the future, for simultaneous voting in PLCR.
Also fixes #406