Synchronise the ISTR with STR

[satyamakgec]

Did not inherit the ISTR to STR because of invalidation of many getter functions that are present in the STRGetter. But Update the ISTR with the proper declarations of the STR and STRGetter functions

[coveralls]

Coverage remained the same at 96.267% when pulling 758ff15 on task_3.18 into 3ff1d69 on audit-fixes.

[F-OBrien]

@satyamakgec in ISecurityTokenRegistry.
function generateSecurityToken( string calldata _name, string calldata _ticker, string calldata _tokenDetails, bool _divisible, address _treasuryWallet, uint256 _protocolVersion ) external;
should be
function generateSecurityToken( string calldata _name, string calldata _ticker, string calldata _tokenDetails, bool _divisible ) external;

When I was working on the CLI updates ref PR #659 I actually went through this and ISecurityToken.sol and already fixed some broken interfaces. I also include the functions from the respective getter contracts as there were already some getters included. I'm not sure if you wanted all the getter functions in there but it made it simpler to reference the ABI from the interface files combining all functions, events and getters for public variables. The work I already did may be of help if you wanted them in the interface....and it would save me a need to refactor the CLI updates 😄

Edit:
Also
function getSecurityTokenData(address _securityToken) external view returns(string memory, address, string memory, uint256, uint8[] memory);
should be
function getSecurityTokenData(address _securityToken) external view returns (string memory, address, string memory, uint256);

[tintinweb]

Hi,

as mentioned by @F-OBrien there're some mismatching declarations

• generateSecurityToken
• getSecurityTokenData
• updateFromRegistry ...

Since the interface already had some mismatches and inheriting from it does not appear to be an option (also having two interface contracts ISTR,ISTRGetter could be an option but isn't as nice as having one ISTR with all interfaces) I'd recommend to add a prominent comment/note to STR to make sure devs always update the interface when modifying the implementation. (ISecurityTokenRegistry is not heavily used atm, therefore risk is not that high but that can change)

tin/diligence

[F-OBrien]

These have probably intentionally been left out but the below functions, which are part of the STR ABI, are not included in this interface. It's again down to what the purpose is for this interface file and if some of these may ever be used externally or not.
If it is intended to be used to create a combined ABI for STR and SRTGetter for dApp and CLI developers then some of these should possibly be added (along with events). I'm assuming that is what is intended as it's not suitable for inheriting.
function pause ( ) external;
function unpause ( ) external;
function reclaimERC20 ( address _tokenContract ) external;
function initialize ( address _polymathRegistry, uint256 _stLaunchFee, uint256 _tickerRegFee, address _owner, address _getterContract ) external;

Eternal Storage getters (probably less useful)
function getBytes32Value ( bytes32 _variable ) external view returns ( bytes32 );
function getBytesValue ( bytes32 _variable ) external view returns ( bytes );
function getAddressValue ( bytes32 _variable ) external view returns ( address );
function getArrayAddress ( bytes32 _key ) external view returns ( address[] );
function getBoolValue ( bytes32 _variable ) external view returns ( bool );
function getStringValue ( bytes32 _variable ) external view returns ( string );
function getArrayBytes32 ( bytes32 _key ) external view returns ( bytes32[] );
function getUintValue ( bytes32 _variable ) external view returns ( uint256 );
function getArrayUint ( bytes32 _key ) external view returns ( uint256[] );

[satyamakgec]

Hey, @F-OBrien For pause() and unpause() these functions can be covered by the Pausable contract of (OZ) similarly reclaimERC20() can be covered from the ReclaimTokens contract. initialize() will never be called again after being executed once (just after deployment). And for EternalStorage contract getters, as you say these are very less useful, we are expecting that all the contract data reading requirement will be covered by our STR getters and these EternalStorage getters will never be used by the outside world.

[F-OBrien]

I don't disagree. In fact one could just use the STR and STRGetter contract ABI's instead of the interface file and have all functions and events covered from just those two contracts. My point was more it makes it easier for those interacting with the contract if they are all consolidated into a single interface. If that's not your preference I've no issue, it just means I may need to modify some of the CLI updates made.

[F-OBrien]

@satyamakgec I reviewed ISecurityToken.sol against the functions in the ABI of SecurityToken.sol and STGetter.sol and have the following observations.

Errors

• function initialize() external; should be
  function initialize(address _getterDelegate) external;
• function freezeIssuance() external; should be
  function freezeIssuance(bytes calldata _signature) external;
• function decreaseApproval(address _spender, uint _subtractedValue) external returns (bool); should be
  function decreaseAllowance(address spender, uint256 subtractedValue) external returns (bool);
• function increaseApproval(address _spender, uint _addedValue) external returns (bool); should be
  function increaseAllowance(address spender, uint256 addedValue) external returns (bool);

Different/missing variable names

• function approve(address _spender, uint256 _value) external returns (bool);
function approve(address spender, uint256 value) external returns (bool);

• function balanceOf(address _owner) external view returns (uint256);
function balanceOf(address owner) external view returns (uint256);

• function allowance(address _owner, address _spender) external view returns (uint256);
function allowance(address owner, address spender) external view returns (uint256);

• function transfer(address _to, uint256 _value) external returns (bool);
function transfer(address _to, uint256 _value) external returns (bool success);

• function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32);
function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (bool, bytes1, bytes32);

• function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (bool, bytes1, bytes32);
function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (bool success, bytes1 reasonCode, bytes32 appCode);

• function canTransferByPartition(address _from, address _to, bytes32 _partition, uint256 _value, bytes calldata _data) external view returns (bytes1, bytes32, bytes32);
function canTransferByPartition(address _from, address _to, bytes32 _partition, uint256 _value, bytes calldata _data) external view returns (bytes1 esc, bytes32 appStatusCode, bytes32 toPartition);

• function getInvestors() external view returns (address[] memory);
function getInvestors() external view returns (address[] memory investors);

Missing functions

• function name() external view returns (string);
• function initialized() external view returns (bool);
• function getterDelegate() external view returns (address);
• function UNLOCKED() external view returns (bytes32);
• function polyToken() external view returns (address);
• function controllerDisabled() external view returns (bool);
• function symbol() external view returns (string);
• function LOCKED() external view returns (bytes32);
• function _owner() external view returns (address);
• function moduleRegistry() external view returns (address);
• function securityTokenRegistry() external view returns (address);
• function tokenDetails() external view returns (string);
• function controller() external view returns (address);
• function _totalSupply() external view returns (uint256);
• function _balances(address) external view returns (uint256);
• function _allowed(address, address) external view returns (uint256);

[satyamakgec]

@F-OBrien Thanks for the detailed review. :) Let me know if you find anything wrong.

[F-OBrien]

@F-OBrien Thanks for the detailed review. :) Let me know if you find anything wrong.

ISTR and IST Look good to me. I haven't looked at the other changes in detail but from a quick review they also look good and the fact they are inherited if there were issues it should have been caught by the compiler.

This tidy up will be a big help for CLI/dApp developers. Sorry if I pushed you into doing it 😄

minor comment - It might be an idea to add a note the below one from the STR to STRGetter, ST and STGetter

IMPORTANT: Developer should update the ISecurityTokenRegistry.sol (Interface) if there is any change in function signature or addition/removal of the functions from SecurityTokenRegistry & STRGetter contract.

If I had to find something else I could say that there is inconsistent use of white space after returns 😁 🤣

[adamdossa]

@satyamakgec Please could you review merge conflicts

[F-OBrien]

If this is to be merged to Audit fixes you should also update the interface for new functions and changed ones from PR #706 e.g registerNewTicker

[F-OBrien]

There are other discrepancies also.
706 has 2 instances of the event RegisterTicker with different parameters.
Missing new function modifyExistingTicker
Missing new function modifyExistingSecurityToken

I'm checking on my phone so may have missed other changes