PolicyManager: reorganize state layout, decrease size of some reward fields to uint128 to put state in fewer slots

nucypher/blockchain/eth/sol/source/contracts/PolicyManager.sol

@@ -14,7 +14,7 @@ import "contracts/proxy/Upgradeable.sol";
 
 /**
 * @notice Contract holds policy data and locks fees
-* @dev |v3.1.1|
+* @dev |v4.1.1|
 */
 contract PolicyManager is Upgradeable {
     using SafeERC20 for NuCypherToken;
@@ -61,13 +61,13 @@ contract PolicyManager is Upgradeable {
     }
 
     struct Policy {
+        bool disabled;
         address payable sponsor;
         address owner;
 
-        uint256 rewardRate;
+        uint128 rewardRate;
         uint64 startTimestamp;
         uint64 endTimestamp;
-        bool disabled;
 
         uint256 reservedSlot1;
         uint256 reservedSlot2;
@@ -79,21 +79,22 @@ contract PolicyManager is Upgradeable {
     }
 
     struct NodeInfo {
-        uint256 reward;
-        uint256 rewardRate;
+        uint128 reward;
         uint16 lastMinedPeriod;
-        mapping (uint16 => int256) rewardDelta;
+        uint256 rewardRate;
         uint256 minRewardRate;
+        mapping (uint16 => int256) rewardDelta;
     }
 
     struct Range {
-        uint256 min;
-        uint256 defaultValue;
-        uint256 max;
+        uint128 min;
+        uint128 defaultValue;
+        uint128 max;
     }
 
-    bytes16 constant RESERVED_POLICY_ID = bytes16(0);
-    address constant RESERVED_NODE = address(0);
+    bytes16 internal constant RESERVED_POLICY_ID = bytes16(0);
+    address internal constant RESERVED_NODE = address(0);
+    uint256 internal constant MAX_BALANCE = uint256(uint128(0) - 1);
     // controlled overflow to get max int256
     int256 public constant DEFAULT_REWARD_DELTA = int256((uint256(0) - 1) >> 1);
 
@@ -148,7 +149,7 @@ contract PolicyManager is Upgradeable {
     */
     // TODO # 1501
     // function setMinRewardRateRange(Range calldata _range) external onlyOwner {
-    function setMinRewardRateRange(uint256 _min, uint256 _default, uint256 _max) external onlyOwner {
+    function setMinRewardRateRange(uint128 _min, uint128 _default, uint128 _max) external onlyOwner {
         require(_min <= _default && _default <= _max);
         minRewardRateRange = Range(_min, _default, _max);
         emit MinRewardRateRangeSet(msg.sender, _min, _default, _max);
@@ -212,9 +213,11 @@ contract PolicyManager is Upgradeable {
         require(
             _policyId != RESERVED_POLICY_ID &&
             policy.rewardRate == 0 &&
+            !policy.disabled &&
             _endTimestamp > block.timestamp &&
             msg.value > 0
         );
+        require(address(this).balance <= MAX_BALANCE);
         uint16 currentPeriod = getCurrentPeriod();
         uint16 endPeriod = uint16(_endTimestamp / secondsPerPeriod) + 1;
         uint256 numberOfPeriods = endPeriod - currentPeriod;
@@ -222,7 +225,7 @@ contract PolicyManager is Upgradeable {
         policy.sponsor = msg.sender;
         policy.startTimestamp = uint64(block.timestamp);
         policy.endTimestamp = _endTimestamp;
-        policy.rewardRate = msg.value.div(_nodes.length) / numberOfPeriods;
+        policy.rewardRate = uint128(msg.value.div(_nodes.length) / numberOfPeriods);
         require(policy.rewardRate > 0 && policy.rewardRate * numberOfPeriods * _nodes.length  == msg.value);
         if (_policyOwner != msg.sender && _policyOwner != address(0)) {
             policy.owner = _policyOwner;
@@ -320,7 +323,7 @@ contract PolicyManager is Upgradeable {
             }
         }
         node.lastMinedPeriod = _period;
-        node.reward += node.rewardRate;
+        node.reward += uint128(node.rewardRate);
     }
 
     /**
@@ -463,9 +466,11 @@ contract PolicyManager is Upgradeable {
             if (numberOfActive == 0) {
                 policy.disabled = true;
                 // gas refund
-                // deletion more slots will increase gas usage instead of decreasing (in current code)
-                // because gas refund can be no more than half of all gas
                 policy.sponsor = address(0);
+                policy.owner = address(0);
+                policy.rewardRate = 0;
+                policy.startTimestamp = 0;
+                policy.endTimestamp = 0;
                 emit PolicyRevoked(_policyId, msg.sender, refundValue);
             } else {
                 emit RefundForPolicy(_policyId, msg.sender, refundValue);

tests/blockchain/eth/contracts/integration/test_intercontract_integration.py

@@ -30,7 +30,7 @@ from nucypher.blockchain.eth.constants import NULL_ADDRESS
 from nucypher.crypto.api import sha256_digest
 from nucypher.crypto.signing import SignatureStamp
 
-DISABLED_FIELD = 5
+DISABLED_FIELD = 0
 
 
 def pytest_namespace():

tests/blockchain/eth/contracts/main/policy_manager/test_policy_manager.py

@@ -25,16 +25,16 @@ from web3.contract import Contract
 
 from nucypher.blockchain.eth.constants import NULL_ADDRESS
 
-SPONSOR_FIELD = 0
-OWNER_FIELD = 1
-RATE_FIELD = 2
-START_TIMESTAMP_FIELD = 3
-END_TIMESTAMP_FIELD = 4
-DISABLED_FIELD = 5
+DISABLED_FIELD = 0
+SPONSOR_FIELD = 1
+OWNER_FIELD = 2
+RATE_FIELD = 3
+START_TIMESTAMP_FIELD = 4
+END_TIMESTAMP_FIELD = 5
 
 REWARD_FIELD = 0
-REWARD_RATE_FIELD = 1
-LAST_MINED_PERIOD_FIELD = 2
+LAST_MINED_PERIOD_FIELD = 1
+REWARD_RATE_FIELD = 2
 MIN_REWARD_RATE_FIELD = 3
 
 POLICY_ID_LENGTH = 16
@@ -176,6 +176,11 @@ def test_create_revoke(testerchain, escrow, policy_manager):
     with pytest.raises((TransactionFailed, ValueError)):
         tx = policy_manager.functions.revokeArrangement(policy_id, node1).transact({'from': policy_sponsor})
         testerchain.wait_for_receipt(tx)
+    # Can't create policy with the same id even after revoking
+    with pytest.raises((TransactionFailed, ValueError)):
+        tx = policy_manager.functions.createPolicy(policy_id, policy_sponsor, end_timestamp, [node1])\
+            .transact({'from': policy_sponsor, 'value': value})
+        testerchain.wait_for_receipt(tx)
 
     # Create new policy
     period = escrow.functions.getCurrentPeriod().call()
@@ -645,7 +650,7 @@ def test_handling_wrong_state(testerchain, deploy_contract):
         tx = escrow.functions.mint(current_period - 1, 1).transact({'from': node1})
         testerchain.wait_for_receipt(tx)
 
-    reward, reward_rate, last_mined_period, _min_reward_rate = policy_manager.functions.nodes(node1).call()
+    reward, last_mined_period, reward_rate, _min_reward_rate = policy_manager.functions.nodes(node1).call()
     assert reward == 0
     assert reward_rate == 0
     assert last_mined_period == current_period - 1
@@ -686,7 +691,7 @@ def test_handling_wrong_state(testerchain, deploy_contract):
         tx = escrow.functions.mint(current_period - 1, 1).transact({'from': node2})
         testerchain.wait_for_receipt(tx)
 
-    reward, reward_rate, last_mined_period, _min_reward_rate = policy_manager.functions.nodes(node2).call()
+    reward, last_mined_period, reward_rate, _min_reward_rate = policy_manager.functions.nodes(node2).call()
     assert reward == 50 * (number_of_periods - 2)
     assert reward_rate == 0
     assert last_mined_period == current_period - 1

tests/blockchain/eth/contracts/main/policy_manager/test_policy_manager_operations.py

@@ -23,12 +23,12 @@ from eth_tester.exceptions import TransactionFailed
 
 from nucypher.blockchain.eth.constants import NULL_ADDRESS
 
-SPONSOR_FIELD = 0
-OWNER_FIELD = 1
-RATE_FIELD = 2
-START_TIMESTAMP_FIELD = 3
-END_TIMESTAMP_FIELD = 4
-DISABLED_FIELD = 5
+DISABLED_FIELD = 0
+SPONSOR_FIELD = 1
+OWNER_FIELD = 2
+RATE_FIELD = 3
+START_TIMESTAMP_FIELD = 4
+END_TIMESTAMP_FIELD = 5
 
 REWARD_FIELD = 0
 REWARD_RATE_FIELD = 1