Staking: Address PR #101 comments #351
Conversation
bingen
changed the title
| @@ -97,6 +104,10 @@ contract Staking is ERCStaking, AragonApp { | |||
| } | |||
| } | |||
|
|
|||
| function lockIndefinitely(uint256 amount, address unlocker, bytes32 metadata, bytes data) public returns(uint256 lockId) { | |||
There was a problem hiding this comment.
Is there any reason for adding a function for this? See a limited usecase and people can do it themselves anyway
There was a problem hiding this comment.
Well, I thought it may be convenient for the user to avoid the need of uint8(TimeUnit.Seconds), MAX_UINT64, but I can remove it if you don't think so.
| // process Stake | ||
| accounts[acct].amount = accounts[acct].amount.add(amount); | ||
|
|
||
| Staked(acct, amount, totalStakedFor(acct), data); | ||
|
|
||
| if (stakeScript.length > 0) { | ||
| runScript(stakeScript, data, new address[](0)); |
There was a problem hiding this comment.
I'm having second thoughts on whether we should add stakingToken to the blacklist for all script executions
| Lock memory newLock = Lock(amount, Timespan(lockEnds, TimeUnit(lockUnit)), unlocker, metadata); | ||
| uint256 lockId = accounts[msg.sender].locks.push(newLock) - 1; | ||
| lockId = accounts[msg.sender].locks.push(newLock) - 1; | ||
|
|
||
| Locked(msg.sender, lockId, metadata); |
There was a problem hiding this comment.
We should log the amount of locked tokens
| while (accounts[acct].locks.length > 0) { | ||
| if (canUnlock(acct, 0)) { | ||
| unlock(acct, 0); | ||
| uint256 lastAccountLock = accounts[acct].locks.length - 1; |
There was a problem hiding this comment.
I like the idea of this function, but we shouldn't re-implement unlock here. Why not just iterate over all as in unlockAll?
| accounts[from].amount -= amount; | ||
| accounts[to].amount += amount; | ||
| // make sure we don't move locked tokens, to avoid inconsistency | ||
| require(unlockedBalanceOf(from) >= amount); |
There was a problem hiding this comment.
I understand where you are coming from, but I think move tokens should be able to move them regardless of locks.
There was a problem hiding this comment.
But the (correct me if I'm wrong) the following situation may happen: an account could have more locked tokens than staked. So that account could try to stake more tokens for whatever reason but still wouldn't have available staked tokens. Even more, unlockedBalanceOf could revert trying to do the subtraction. I think that leaving such an inconsistent state where locked tokens amount can be greater than staked amount can give a lot of problems and be very confusing for the user. Let me put it this way: how can you have locked tokens that you don't actually have?
Another option would be to allow moveTokens to unlock first, but then it should require UNLOCK_ROLE. Maybe we can do an unlockAndMove function.
future-apps/staking/test/staking.js
Outdated
| // we just send dumb transactions to increment block number | ||
| let blockNumber | ||
| do { | ||
| await mockCounter.increment() |
There was a problem hiding this comment.
Nice hack 😂🙏 Though this is going to slow down the test unnecessarily. Two possible solutions:
- Make the lock shorter, 2 or 3 blocks should be ok.
- Mock block numbers like we do with timestamps (I thought we did this all over the system, but apparently just in Finance https://github.com/aragon/aragon-apps/blob/master/apps/finance/test/mocks/FinanceMock.sol)
Also rather than introducing a random contract, you can perform ETH value transfers to the same account web3.eth.sendTransaction({ from: owner, to: owner: value: 1, gasPrice: 1 }) to bump blocks.
future-apps/staking/test/staking.js
Outdated
| do { | ||
| await mockCounter.increment() | ||
| blockNumber = await getBlockNumber() | ||
| } while( blockNumber <= initialBlockNumber + blocks) |
There was a problem hiding this comment.
Also I hadn't seen this in a while 😉. We could just do a for (let x of new Array(blocks)) without the block number check, as ganache always creates a new block per tx submitted.
If we want these tests to properly run in geth or parity, we should await for the block to change before continuing sending txs
future-apps/staking/test/staking.js
Outdated
| assert.equal((await app.locksCount(owner)).valueOf(), 1, "there shouldn't be locks") | ||
| }) | ||
|
|
||
| it('fails trying to unlockAllOrNone', async () => { |
There was a problem hiding this comment.
'fails to unlockAllOrNone if a lock cannot be unlocked'
| return assertRevert(async () => { | ||
| await app.moveTokens(owner, other, amount + 1) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Add a test for failing to move more tokens from an account than unlocked.
| assert.equal((await app.totalStakedFor(owner)).valueOf(), amount, "staked value should match") | ||
| assert.equal(await executionTarget.stakeCounter(), 1, 'should have received execution call') | ||
| }) | ||
|
|
There was a problem hiding this comment.
Add test for stakeAndLock triggering both scripts
And add tests.
TODO: fix issue with
timeTraveland solidity coverage (it works fine withnpm run test)