diff --git a/nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol b/nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol index 9e95fc3da..6ee16dd4a 100644 --- a/nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol +++ b/nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol @@ -105,7 +105,7 @@ contract StakingEscrow is Issuer { mapping (uint16 => uint256) public lockedPerPeriod; uint16 public minLockedPeriods; - uint16 public minWorkerPeriods; + uint16 public minWorkerPeriods; // TODO: What's a good minimum time to allow stakers to change/unset worker? (#1073) uint256 public minAllowableLockedTokens; uint256 public maxAllowableLockedTokens; PolicyManagerInterface public policyManager; @@ -357,33 +357,28 @@ contract StakingEscrow is Issuer { * @param _worker Worker address. Must be a real address, not a contract **/ function setWorker(address _worker) public onlyStaker { - uint16 currentPeriod = getCurrentPeriod(); StakerInfo storage info = stakerInfo[msg.sender]; require(_worker != info.worker, "Specified worker is already set for this staker"); - if (_worker != address(0)){ + uint16 currentPeriod = getCurrentPeriod(); + if(info.worker != address(0)){ // If this staker had a worker ... + // Check that enough time has passed to change it require(currentPeriod >= info.workerStartPeriod.add16(minWorkerPeriods), "Not enough time has passed since the previous setting worker"); + // Remove the old relation "worker->staker" + workerToStaker[info.worker] = address(0); + } + + if (_worker != address(0)){ require(workerToStaker[_worker] == address(0), "Specified worker is already in use"); require(stakerInfo[_worker].subStakes.length == 0 || _worker == msg.sender, "Specified worker is a staker"); - // Remove relation between the old worker and the staker, if there was one - if (info.worker != address(0)) { - workerToStaker[info.worker] = address(0); - } - info.worker = _worker; - info.workerStartPeriod = currentPeriod; // Set new worker->staker relation workerToStaker[_worker] = msg.sender; - } else { // Staker wants to unset her worker - // In this case, we don't require a minimum elapsed time with the worker - - // Remove relation between the old worker and the staker - workerToStaker[info.worker] = address(0); - // Unset worker - info.worker = address(0); - // Note we don't clear the previous worker start period, so staker - // can't bypass the time restriction by unsetting her previous worker } + + // Set new worker (or unset if _worker == address(0)) + info.worker = _worker; + info.workerStartPeriod = currentPeriod; emit WorkerSet(msg.sender, _worker, currentPeriod); } diff --git a/tests/blockchain/eth/contracts/main/staking_escrow/test_staking_escrow_additional.py b/tests/blockchain/eth/contracts/main/staking_escrow/test_staking_escrow_additional.py index 29d12eca1..9b2690063 100644 --- a/tests/blockchain/eth/contracts/main/staking_escrow/test_staking_escrow_additional.py +++ b/tests/blockchain/eth/contracts/main/staking_escrow/test_staking_escrow_additional.py @@ -512,8 +512,7 @@ def test_worker(testerchain, token, escrow_contract): event_args = events[-1]['args'] assert intermediary1.address == event_args['staker'] assert worker1 == event_args['worker'] - worker1_start_period = escrow.functions.getCurrentPeriod().call() - assert worker1_start_period == event_args['startPeriod'] + assert escrow.functions.getCurrentPeriod().call() == event_args['startPeriod'] # Only worker can confirm activity with pytest.raises((TransactionFailed, ValueError)): @@ -537,12 +536,18 @@ def test_worker(testerchain, token, escrow_contract): .transact({'from': worker1}) testerchain.wait_for_receipt(tx) - # Can't change worker twice in the same period + # Can't change worker twice too soon with pytest.raises((TransactionFailed, ValueError)): tx = intermediary1.functions.setWorker(worker2).transact({'from': ursula1}) testerchain.wait_for_receipt(tx) - # But she can unset her worker + # She can't unset her worker too, until enough time has passed + with pytest.raises((TransactionFailed, ValueError)): + tx = intermediary1.functions.setWorker(Blockchain.NULL_ADDRESS).transact({'from': ursula1}) + testerchain.wait_for_receipt(tx) + + # Let's advance one period and unset the worker + testerchain.time_travel(hours=1) tx = intermediary1.functions.setWorker(Blockchain.NULL_ADDRESS).transact({'from': ursula1}) testerchain.wait_for_receipt(tx) assert Blockchain.NULL_ADDRESS == escrow.functions.getWorkerFromStaker(intermediary1.address).call() @@ -552,18 +557,12 @@ def test_worker(testerchain, token, escrow_contract): assert number_of_events == len(events) event_args = events[-1]['args'] assert intermediary1.address == event_args['staker'] - # Even though the worker has been unset... + # Now the worker has been unset ... assert Blockchain.NULL_ADDRESS == event_args['worker'] - # ... the previous start period remains. - assert worker1_start_period == event_args['startPeriod'] + # ... with a new starting period. + assert escrow.functions.getCurrentPeriod().call() == event_args['startPeriod'] - # The staker can't set a new worker until the previous worker's time has passed - with pytest.raises((TransactionFailed, ValueError)): - tx = intermediary1.functions.setWorker(worker2).transact({'from': ursula1}) - testerchain.wait_for_receipt(tx) - - # Let's advance one period and change the worker - testerchain.time_travel(hours=1) + # The staker can set now a new worker, without waiting additional time. tx = intermediary1.functions.setWorker(worker2).transact({'from': ursula1}) testerchain.wait_for_receipt(tx) assert worker2 == escrow.functions.getWorkerFromStaker(intermediary1.address).call()