Get subset of investors at a checkpoint (#611)

* Updated checkpoint event and optimized st

* Optimized dividends push payment

* Added test

* Minor datastore optimization

* Test fixed

contracts/datastore/DataStore.sol

@@ -298,28 +298,48 @@ contract DataStore is DataStoreStorage, IDataStore {
     }
 
     function getUint256ArrayElements(bytes32 _key, uint256 _startIndex, uint256 _endIndex) external view returns(uint256[] memory array) {
-        uint256 size = _endIndex - _startIndex + 1;
+        uint256 size = uintArrayData[_key].length;
+        if (_endIndex >= size) {
+            size = size - _startIndex;
+        } else {
+            size = _endIndex - _startIndex + 1;
+        }
         array = new uint256[](size);
         for(uint256 i; i < size; i++)
             array[i] = uintArrayData[_key][i + _startIndex];
     }
 
     function getBytes32ArrayElements(bytes32 _key, uint256 _startIndex, uint256 _endIndex) external view returns(bytes32[] memory array) {
-        uint256 size = _endIndex - _startIndex + 1;
+        uint256 size = bytes32ArrayData[_key].length;
+        if (_endIndex >= size) {
+            size = size - _startIndex;
+        } else {
+            size = _endIndex - _startIndex + 1;
+        }
         array = new bytes32[](size);
         for(uint256 i; i < size; i++)
             array[i] = bytes32ArrayData[_key][i + _startIndex];
     }
 
     function getAddressArrayElements(bytes32 _key, uint256 _startIndex, uint256 _endIndex) external view returns(address[] memory array) {
-        uint256 size = _endIndex - _startIndex + 1;
+        uint256 size = addressArrayData[_key].length;
+        if (_endIndex >= size) {
+            size = size - _startIndex;
+        } else {
+            size = _endIndex - _startIndex + 1;
+        }
         array = new address[](size);
         for(uint256 i; i < size; i++)
             array[i] = addressArrayData[_key][i + _startIndex];
     }
 
     function getBoolArrayElements(bytes32 _key, uint256 _startIndex, uint256 _endIndex) external view returns(bool[] memory array) {
-        uint256 size = _endIndex - _startIndex + 1;
+        uint256 size = boolArrayData[_key].length;
+        if (_endIndex >= size) {
+            size = size - _startIndex;
+        } else {
+            size = _endIndex - _startIndex + 1;
+        }
         array = new bool[](size);
         for(uint256 i; i < size; i++)
             array[i] = boolArrayData[_key][i + _startIndex];

contracts/interfaces/ISecurityToken.sol

@@ -19,28 +19,28 @@ interface ISecurityToken {
 
     /**
      * @notice Transfers of securities may fail for a number of reasons. So this function will used to understand the
-     * cause of failure by getting the byte value. Which will be the ESC that follows the EIP 1066. ESC can be mapped 
+     * cause of failure by getting the byte value. Which will be the ESC that follows the EIP 1066. ESC can be mapped
      * with a reson string to understand the failure cause, table of Ethereum status code will always reside off-chain
      * @param _to address The address which you want to transfer to
      * @param _value uint256 the amount of tokens to be transferred
      * @param _data The `bytes _data` allows arbitrary data to be submitted alongside the transfer.
      * @return bool It signifies whether the transaction will be executed or not.
      * @return byte Ethereum status code (ESC)
-     * @return bytes32 Application specific reason code 
+     * @return bytes32 Application specific reason code
      */
     function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32);
 
     /**
      * @notice Transfers of securities may fail for a number of reasons. So this function will used to understand the
-     * cause of failure by getting the byte value. Which will be the ESC that follows the EIP 1066. ESC can be mapped 
+     * cause of failure by getting the byte value. Which will be the ESC that follows the EIP 1066. ESC can be mapped
      * with a reson string to understand the failure cause, table of Ethereum status code will always reside off-chain
      * @param _from address The address which you want to send tokens from
      * @param _to address The address which you want to transfer to
      * @param _value uint256 the amount of tokens to be transferred
      * @param _data The `bytes _data` allows arbitrary data to be submitted alongside the transfer.
      * @return bool It signifies whether the transaction will be executed or not.
      * @return byte Ethereum status code (ESC)
-     * @return bytes32 Application specific reason code 
+     * @return bytes32 Application specific reason code
      */
     function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32);
 
@@ -114,7 +114,7 @@ interface ISecurityToken {
     /**
      * @notice This function redeem an amount of the token of a msg.sender. For doing so msg.sender may incentivize
      * using different ways that could be implemented with in the `redeem` function definition. But those implementations
-     * are out of the scope of the ERC1594. 
+     * are out of the scope of the ERC1594.
      * @param _value The amount of tokens need to be redeemed
      * @param _data The `bytes _data` it can be used in the token contract to authenticate the redemption.
      */
@@ -123,7 +123,7 @@ interface ISecurityToken {
     /**
      * @notice This function redeem an amount of the token of a msg.sender. For doing so msg.sender may incentivize
      * using different ways that could be implemented with in the `redeem` function definition. But those implementations
-     * are out of the scope of the ERC1594. 
+     * are out of the scope of the ERC1594.
      * @dev It is analogy to `transferFrom`
      * @param _tokenHolder The account whose tokens gets redeemed.
      * @param _value The amount of tokens need to be redeemed
@@ -211,6 +211,15 @@ interface ISecurityToken {
      */
     function getInvestorsAt(uint256 _checkpointId) external view returns(address[] memory);
 
+    /**
+     * @notice returns an array of investors with non zero balance at a given checkpoint
+     * @param _checkpointId Checkpoint id at which investor list is to be populated
+     * @param _start Position of investor to start iteration from
+     * @param _end Position of investor to stop iteration at
+     * @return list of investors
+     */
+    function getInvestorsSubsetAt(uint256 _checkpointId, uint256 _start, uint256 _end) external view returns(address[] memory);
+
     /**
      * @notice generates subset of investors
      * NB - can be used in batches if investor list is large
@@ -347,8 +356,8 @@ interface ISecurityToken {
      * @param _value uint256 the amount of tokens to be transferred
      * @param _data data to validate the transfer. (It is not used in this reference implementation
      * because use of `_data` parameter is implementation specific).
-     * @param _operatorData data attached to the transfer by controller to emit in event. (It is more like a reason string 
-     * for calling this function (aka force transfer) which provides the transparency on-chain). 
+     * @param _operatorData data attached to the transfer by controller to emit in event. (It is more like a reason string
+     * for calling this function (aka force transfer) which provides the transparency on-chain).
      */
     function controllerTransfer(address _from, address _to, uint256 _value, bytes calldata _data, bytes calldata _operatorData) external;
 
@@ -361,8 +370,8 @@ interface ISecurityToken {
      * @param _value uint256 the amount of tokens need to be redeemed.
      * @param _data data to validate the transfer. (It is not used in this reference implementation
      * because use of `_data` parameter is implementation specific).
-     * @param _operatorData data attached to the transfer by controller to emit in event. (It is more like a reason string 
-     * for calling this function (aka force transfer) which provides the transparency on-chain). 
+     * @param _operatorData data attached to the transfer by controller to emit in event. (It is more like a reason string
+     * for calling this function (aka force transfer) which provides the transparency on-chain).
      */
     function controllerRedeem(address _tokenHolder, uint256 _value, bytes calldata _data, bytes calldata _operatorData) external;
 

contracts/modules/Checkpoint/DividendCheckpoint.sol

@@ -81,7 +81,7 @@ contract DividendCheckpoint is DividendCheckpointStorage, ICheckpoint, Module {
         if (wallet == address(0)) {
             address payable treasuryWallet = address(uint160(IDataStore(getDataStore()).getAddress(TREASURY)));
             require(address(treasuryWallet) != address(0), "Invalid address");
-            return treasuryWallet; 
+            return treasuryWallet;
         }
         else
             return wallet;
@@ -166,21 +166,24 @@ contract DividendCheckpoint is DividendCheckpointStorage, ICheckpoint, Module {
      * @notice Issuer can push dividends using the investor list from the security token
      * @param _dividendIndex Dividend to push
      * @param _start Index in investor list at which to start pushing dividends
-     * @param _iterations Number of addresses to push dividends for
+     * @param _end Index in investor list at which to stop pushing dividends
      */
     function pushDividendPayment(
         uint256 _dividendIndex,
         uint256 _start,
-        uint256 _iterations
-    ) public
-      withPerm(OPERATOR)
+        uint256 _end
+    )
+        public
+        withPerm(OPERATOR)
     {
+        //NB If possible, please use pushDividendPaymentToAddresses as it is cheaper than this function
         _validDividendIndex(_dividendIndex);
         Dividend storage dividend = dividends[_dividendIndex];
         uint256 checkpointId = dividend.checkpointId;
-        address[] memory investors = ISecurityToken(securityToken).getInvestorsAt(checkpointId);
-        uint256 numberInvestors = Math.min(investors.length, _start.add(_iterations));
-        for (uint256 i = _start; i < numberInvestors; i++) {
+        address[] memory investors = ISecurityToken(securityToken).getInvestorsSubsetAt(checkpointId, _start, _end);
+        // The investors list maybe smaller than _end - _start becuase it only contains addresses that had a positive balance
+        // the _start and _end used here are for the address list stored in the dataStore
+        for (uint256 i = 0; i < investors.length; i++) {
             address payable payee = address(uint160(investors[i]));
             if ((!dividend.claimed[payee]) && (!dividend.dividendExcluded[payee])) {
                 _payDividend(payee, dividend, _dividendIndex);

contracts/tokens/STGetter.sol

@@ -74,12 +74,44 @@ contract STGetter is OZStorage, SecurityTokenStorage {
         for (i = 0; i < investors.length; i++) {
             if (balanceOfAt(investors[i], _checkpointId) > 0) {
                 count++;
+            } else {
+                investors[i] = address(0);
             }
         }
         address[] memory holders = new address[](count);
         count = 0;
+        for (i = 0; i < investors.length; i++) {
+            if (investors[i] != address(0)) {
+                holders[count] = investors[i];
+                count++;
+            }
+        }
+        return holders;
+    }
+
+    /**
+     * @notice returns an array of investors with non zero balance at a given checkpoint
+     * @param _checkpointId Checkpoint id at which investor list is to be populated
+     * @param _start Position of investor to start iteration from
+     * @param _end Position of investor to stop iteration at
+     * @return list of investors
+     */
+    function getInvestorsSubsetAt(uint256 _checkpointId, uint256 _start, uint256 _end) external view returns(address[] memory) {
+        uint256 count;
+        uint256 i;
+        IDataStore dataStoreInstance = IDataStore(dataStore);
+        address[] memory investors = dataStoreInstance.getAddressArrayElements(INVESTORSKEY, _start, _end);
         for (i = 0; i < investors.length; i++) {
             if (balanceOfAt(investors[i], _checkpointId) > 0) {
+                count++;
+            } else {
+                investors[i] = address(0);
+            }
+        }
+        address[] memory holders = new address[](count);
+        count = 0;
+        for (i = 0; i < investors.length; i++) {
+            if (investors[i] != address(0)) {
                 holders[count] = investors[i];
                 count++;
             }
@@ -127,7 +159,7 @@ contract STGetter is OZStorage, SecurityTokenStorage {
     }
 
     /**
-     * @notice use to return the global treasury wallet 
+     * @notice use to return the global treasury wallet
      */
     function getTreasuryWallet() external view returns(address) {
         return IDataStore(dataStore).getAddress(TREASURY);
@@ -214,4 +246,4 @@ contract STGetter is OZStorage, SecurityTokenStorage {
         return _version;
     }
 
-}
+}

contracts/tokens/SecurityToken.sol

@@ -61,7 +61,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
     // Emit when the budget allocated to a module is changed
     event ModuleBudgetChanged(uint8[] _moduleTypes, address _module, uint256 _oldBudget, uint256 _budget);
     // Emit when new checkpoint created
-    event CheckpointCreated(uint256 indexed _checkpointId);
+    event CheckpointCreated(uint256 indexed _checkpointId, uint256 _investorLength);
     // Events to log controller actions
     event SetController(address indexed _oldController, address indexed _newController);
     //Event emit when the global treasury wallet address get changed
@@ -253,7 +253,6 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
     * @param _value amount of POLY to withdraw
     */
     function withdrawERC20(address _tokenContract, uint256 _value) external onlyOwner {
-        require(_tokenContract != address(0));
         IERC20 token = IERC20(_tokenContract);
         require(token.transfer(owner(), _value));
     }
@@ -320,7 +319,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
      * @notice freezes transfers
      */
     function freezeTransfers() external onlyOwner {
-        require(!transfersFrozen, "Already frozen");
+        require(!transfersFrozen);
         transfersFrozen = true;
         /*solium-disable-next-line security/no-block-members*/
         emit FreezeTransfers(true);
@@ -330,7 +329,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
      * @notice Unfreeze transfers
      */
     function unfreezeTransfers() external onlyOwner {
-        require(transfersFrozen, "Not frozen");
+        require(transfersFrozen);
         transfersFrozen = false;
         /*solium-disable-next-line security/no-block-members*/
         emit FreezeTransfers(false);
@@ -591,13 +590,13 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
      */
     function createCheckpoint() external returns(uint256) {
         _onlyModuleOrOwner(CHECKPOINT_KEY);
-        require(currentCheckpointId < 2 ** 256 - 1);
+        IDataStore dataStoreInstance = IDataStore(dataStore);
+        // currentCheckpointId can only be incremented by 1 and hence it can not be overflowed
         currentCheckpointId = currentCheckpointId + 1;
         /*solium-disable-next-line security/no-block-members*/
         checkpointTimes.push(now);
-        /*solium-disable-next-line security/no-block-members*/
         checkpointTotalSupply[currentCheckpointId] = totalSupply();
-        emit CheckpointCreated(currentCheckpointId);
+        emit CheckpointCreated(currentCheckpointId, dataStoreInstance.getAddressArrayLength(INVESTORSKEY));
         return currentCheckpointId;
     }
 
@@ -606,7 +605,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
      * @param _controller address of the controller
      */
     function setController(address _controller) public onlyOwner {
-        require(_isControllable());
+        require(isControllable());
         emit SetController(controller, _controller);
         controller = _controller;
     }
@@ -616,7 +615,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
      * @dev enabled via feature switch "disableControllerAllowed"
      */
     function disableController() external isEnabled("disableControllerAllowed") onlyOwner {
-        require(_isControllable());
+        require(isControllable());
         controllerDisabled = true;
         delete controller;
         emit DisableController();
@@ -653,8 +652,8 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
         (bool success, byte reasonCode, bytes32 appCode) = _canTransfer(_from, _to, _value, _data);
         if (success && _value > allowance(_from, msg.sender)) {
             return (false, 0x53, bytes32(0));
-        } else
-            return (success, reasonCode, appCode);
+        }
+        return (success, reasonCode, appCode);
     }
 
     function _canTransfer(address _from, address _to, uint256 _value, bytes memory _data) internal view returns (bool, byte, bytes32) {
@@ -713,24 +712,15 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
         delete _documents[_name];
     }
 
-    /**
-     * @notice Internal function to know whether the controller functionality
-     * allowed or not.
-     * @return bool `true` when controller address is non-zero otherwise return `false`.
-     */
-    function _isControllable() internal view returns (bool) {
-        return !controllerDisabled;
-    }
-
     /**
      * @notice In order to provide transparency over whether `controllerTransfer` / `controllerRedeem` are useable
      * or not `isControllable` function will be used.
      * @dev If `isControllable` returns `false` then it always return `false` and
      * `controllerTransfer` / `controllerRedeem` will always revert.
      * @return bool `true` when controller address is non-zero otherwise return `false`.
      */
-    function isControllable() external view returns (bool) {
-        return _isControllable();
+    function isControllable() public view returns (bool) {
+        return !controllerDisabled;
     }
 
     /**
@@ -747,7 +737,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
      * for calling this function (aka force transfer) which provides the transparency on-chain).
      */
     function controllerTransfer(address _from, address _to, uint256 _value, bytes calldata _data, bytes calldata _operatorData) external onlyController {
-        require(_isControllable());
+        require(isControllable());
         _updateTransfer(_from, _to, _value, _data);
         _transfer(_from, _to, _value);
         emit ControllerTransfer(msg.sender, _from, _to, _value, _data, _operatorData);
@@ -766,7 +756,7 @@ contract SecurityToken is ERC20, ERC20Detailed, Ownable, ReentrancyGuard, Securi
      * for calling this function (aka force transfer) which provides the transparency on-chain).
      */
     function controllerRedeem(address _tokenHolder, uint256 _value, bytes calldata _data, bytes calldata _operatorData) external onlyController {
-        require(_isControllable());
+        require(isControllable());
         _checkAndBurn(_tokenHolder, _value, _data);
         emit ControllerRedemption(msg.sender, _tokenHolder, _value, _data, _operatorData);
     }

test/c_checkpoints.js

@@ -247,7 +247,9 @@ contract("Checkpoints", async function(accounts) {
                         " TotalSupply: " +
                         JSON.stringify(totalSupply)
                 );
-                await I_SecurityToken.createCheckpoint({ from: token_owner });
+                let investorLength = await stGetter.getInvestorCount();
+                let tx = await I_SecurityToken.createCheckpoint({ from: token_owner });
+                assert.equal((tx.logs[0].args[1]).toString(), investorLength.toString());
                 let checkpointTimes = await stGetter.getCheckpointTimes();
                 assert.equal(checkpointTimes.length, j + 1);
                 console.log("Checkpoint Times: " + checkpointTimes);