WIP: upgrade-str

[satyamakgec]

No description provided.

[adamdossa]

Hey @satyamakgec

Overall pattern looks great.

I think we can simplify the code a lot by using something like the below - taking advantage of overloaded functions in Solidity. Let me know what you think - should let us easily represent named variables and single-level maps in EternalStorage without all of the boilerplate (I only implemented a subset of the helper functions).

So for example, a line like:
stringStorage[keccak256(abi.encodePacked("securityTokens", newSecurityTokenAddress, "symbol"))] = symbol;
becomes:
setMap("securityTokenToSymbol", newSecurityTokenAddress, symbol)

and:
uintStorage[keccak256(abi.encodePacked('expiryLimit'))] = 15 * 1 days;
becomes:
set('expiryLimit', 15 * 1 days);

pragma solidity ^0.4.24;

/**
 * @title EternalStorage
 * @dev This contract holds all the necessary state variables to carry out the storage of any contract.
 */
contract EternalStorage {

  mapping(bytes32 => uint256) public uintStorage;
  mapping(bytes32 => string) public stringStorage;
  mapping(bytes32 => address) public addressStorage;
  mapping(bytes32 => bytes) public bytesStorage;
  mapping(bytes32 => bool) public boolStorage;
  mapping(bytes32 => int256) public intStorage;
  mapping(bytes32 => bytes32) public bytes32Storage;

  function set(string _name, uint256 _value) internal {
      uintStorage[keccak256(abi.encodePacked(_name))] = _value;
  }

  function set(string _name, address _value) internal {
      addressStorage[keccak256(abi.encodePacked(_name))] = _value;
  }

  function set(string _name, string _value) internal {
      stringStorage[keccak256(abi.encodePacked(_name))] = _value;
  }

  function setMap(string _name, address _key, uint256 _value) internal {
      uintStorage[keccak256(abi.encodePacked(_name, _key))] = _value;
  }

  function setMap(string _name, address _key, address _value) internal {
      addressStorage[keccak256(abi.encodePacked(_name, _key))] = _value;
  }

  function setMap(string _name, uint256 _key, uint256 _value) internal {
      uintStorage[keccak256(abi.encodePacked(_name, _key))] = _value;
  }

  function setMap(string _name, uint256 _key, address _value) internal {
      addressStorage[keccak256(abi.encodePacked(_name, _key))] = _value;
  }
  
  function getUint(string _name) internal view returns (uint256) {
      return uintStorage[keccak256(abi.encodePacked(_name))];
  }
  
  function getMapAddress(string _name, uint256 _key) internal view returns (address) {
      return addressStorage[keccak256(abi.encodePacked(_name, _key))];
  }

  function getMapAddress(string _name, address _key) internal view returns (address) {
      return addressStorage[keccak256(abi.encodePacked(_name, _key))];
  }

  function getMapUint(string _name, address _key) internal view returns (uint256) {
      return uintStorage[keccak256(abi.encodePacked(_name, _key))];
  }

  function getMapUint(string _name, uint256 _key) internal view returns (uint256) {
      return uintStorage[keccak256(abi.encodePacked(_name, _key))];
  }

  function example() public {
      doSomeStuff(10, 0x1, "blah", 0x2);
  }

  function doSomeStuff(uint256 _someInt, address _someAddr, string _someString, address _someAddr2) public {
      //NB - explicit casts only needed as inline constants aren't typed - shouldn't usually be needed
      
      //uint256 someInteger = _someInt;
      set('someInteger', _someInt);
      //address someAddress = _someAddr;
      set('someAddress', _someAddr);
      //string someString = _someString
      set('someString', _someString);
      //mapping(address => address) issuerToToken -- issuerToToken[_someAddr] = _someAddr2;
      setMap('issuerToToken', _someAddr2, _someAddr);
      //mapping(uint256 => address) indexToIssuer -- indexToIssuer[_someInt] = _someAddr;
      setMap('indexToIssuer', _someInt, _someAddr);
      //uint256 temp = someInteger;
      uint256 temp = getUint('someInteger');
      assert(temp == _someInt);
      address tempAddress = getMapAddress('indexToIssuer', _someInt);
      address tempAddress2 = getMapAddress('issuerToToken', _someAddr2);
      assert(tempAddress == tempAddress2);

  }


}

[adamdossa]

To make it clearer what storage a contract is using, could we list all storage at the top in comments.

i.e.

//uint256 - 'tickerRegFee'
//mapping (address => string) - 'securityTokenToSymbol'

[adamdossa]

Would prefer to see storage structured along the lines of:

symbol (string) maps to:

• registrationTime (uint256)
• registrationExpiry (uint256)
• symbolOwner (address)
• securityToken (address - 0x0 means not yet generated)

securityToken maps to:

• creationTime (uint256)
• symbol (string)

Other securityToken details can be looked up dynamically from the ST (e.g. ST owner, ST name, ST version) - we can provide getter functions for these.

If we want to make the above mappings iterable, we could also store:

• a list of valid symbols in an array
• a mapping from symbol owner to an array of symbols
  e.g. with something like:
  mapping(bytes32 => string[]) public stringArrayStorage;

[adamdossa]

Some feedback (not all of these relate to the upgrading functionality):

• is it possible to put the initialize function logic into the SecurityTokenRegistryProxy constructor instead? AFAICT this would only ever be run once (i.e. would not be rerun on an upgrade)?

• get PolyToken from the PolymathRegistry rather than storing it locally.

• rename ‘flag’ to ‘initialised’ (if we can’t move logic to Proxy constructor)

• can we make _checkValidity a view function and use _storeSymbolData function instead (from generateSecurityToken).

• same for _isReserved - both these functions sound like they shouldn’t be modifying state.

• rename symbols to tickerToSecurityTokens (in general lets be consistent with using ticker or symbol everywhere - don’t mind which)

• registeredSymbols_stauts is a typo - do we have a test case to catch this?

• generally the addCustomSecurityToken should allow Polymath to add / update both the ticker & token data without restriction (i.e. whether or not a token has already been generated, or a ticker reserved), so maybe we don’t need _isReserved.

• what happens if someones ticker expires - is there a way to extend the expiry time by the user?

• can we be consistent with using bytes32 or strings for ticker symbols? IIRC the reason to use bytes32 was because we couldn’t previously return variable length data internally in a smart contract - this is now possible after 0.4.22.

• rename tokensOwnedByUser to userToTickers

• tokenName looks like it is set when registering a symbol, then re-saved when generating a token. I wonder whether we should be storing this at all, or just looking up in the token once a token has been generated (and not storing as part of the ticker registry)

• lets do Util.upper(_ticker) on external / public functions, then assume it is set for internal functions, or arguably better, have a modifier isUpper and insist that symbol is always passed in in upper case rather than converting it on-chain (this would be more efficient).

• probably good to wrap up:

        pushMapArray("tokensOwnedByUser", _newOwner, Util.stringToBytes32(ticker));
        setMap("tickerIndex", ticker, (getMapArrayBytes32("tokensOwnedByUser", _newOwner).length - 1));

in a function.

• should transferTickerOwnership work when the ST has already been generated (at this point the ticker owner is pretty meaningless)

• bit confused by _renounceTickerOwnership - isn’t:
  bytes32 _symbol = getMapArrayBytes32("tokensOwnedByUser", getMapAddress("registeredSymbols_owner", _ticker))[_index];
  equivalent to:
  bytes32 _symbol = getMapArrayBytes32("tokensOwnedByUser", _currentOwner)[_index];
  so you’ll end up getting back some other random symbol owned by the currentOwner, or an array out of bounds if none? Not sure why this functionality is split between _renounceTickerOwnership & transferTickerOwnership - I think it will be easier to have it all in the same function (i.e. you set the ticker index in _renounceTickerOwnership and then reset it in transferTickerOwnership)

[satyamakgec]

• I keep flexible because if we want to intialise some some variables in our next version of the contract then the only option is left to hard code those variables. So we can change in a way that for first time we set implementation contract using the constructor of the SecurityTokenRegistryProxy and for later versions we can use the upgradeToAndCall function to intialise the new variables.

• Yes we can do that. But I avoid it because there is only 1 % chance of changing the address of the polyToken. and If we follow that way then I need to copy the whole code of RegistryUpdater contract and the interface of the PolymathRegistry that also increase the size of the contract.

• Done

• I agree with you according to naming it should be a view function but when I use the _storeSymbolDetails to store the data then It consume more gas in comparison to previous logic because we are assigining 5 state variables in comparison to 2 state variables in earlier logic. We can solve it by creating a new internal function which only works upon the 2 state variables. Or we can simply change the name of the function to make it more sensible according to functionality. as validityAndChangeStatus() or something else I am not good with the naming.

• same as above.

• Done, ticker is being used consistently.

• Correct the typo. we have the test cases but need to add more (WIP).

• I don't think so. addCustomSecurityToken() function is used to add the securityToken by Polymath without paying the fee and It can be used to migrate the data(for this it can't be used anymore). So if we are adding any securityToken using the addCustomSecurityToken() then it should respect the details of the existed deployed securityTokens. It is not being used to change the details of the existing securityToken.

• (Current Approach) If ticker get expired and token is not deployed then ticker will be available to other issuers. If token is deployed then it doesn't matters whether the ticker is expired or not. So do we really required this functionality to change the expiryDate of the ticker ?

• For ticker we are using the string everywhere. but dApp team requires a list of tickers owned by particular issuer. so for that I need to return an array of bytes32 as array of dynamic length data is not permited outside the contract. I agree it is available internally but i need to return to dApp team so I created another mapping that hold this data mapping(address => bytes32[]) tokensOwnedByUser.

• Done

• As I remember correctly Issuer can use the different tokenName at the time of generation of securityToken compare to ticker registeration time only if issuer is using the smart contract . and if it is use differrent name then we are also changing the name in tickerRegistry data store to make the name consistent everywhere (in tickerRegistry and in securityToken). We could dynamically get the tokenName from the securityToken address. but if the token is not deployed and Issuer wants to check the ticker details getTickerDetails() then we are not able to fetch the tokenName at all if we are not storing it.

• If we use the isUpper() modifier then only difference between the current approach and new approach is the cost of storing a memory variable. and we will also decereased the user expirence if user directly interacts with the smart contract. I think we have the keep the same functionality as you said Util.upper() is mandatory for all the external/public function and for internal it is assume that is already set.

• Done

• Done

• Done

[satyamakgec]

@adamdossa I think constructor call is also not possible to store the state variables in proxy contract. I try to do it but I am not able to access the variables. Some insights are written here as well zeppelinos docs

[adamdossa]

I agree with you according to naming it should be a view function but when I use the _storeSymbolDetails to store the data then It consume more gas in comparison to previous logic because we are assigining 5 state variables in comparison to 2 state variables in earlier logic. We can solve it by creating a new internal function which only works upon the 2 state variables. Or we can simply change the name of the function to make it more sensible according to functionality. as validityAndChangeStatus() or something else I am not good with the naming.

Can we do something like:

    function setCheap(address _addr, uint256 _ui, bool _boo) public {
        if (addr != _addr) addr = _addr;
        if (ui != _ui) ui = _ui;
        if (boo != _boo) boo = _boo;
    }

so we only write out the variable if the value changes - this should be cheap from a gas perspective.

[satyamakgec]

recent commit has the setCheap logic but we are stuck in the contract size it again start giving the out-of-gas error at the migration time.

[adamdossa]

Re. polyToken - if we're storing it in the contract, can we include a function to change it.

I didn't follow the bit:

addCustomSecurityToken() function is used to add the securityToken by Polymath without paying the fee and It can be used to migrate the data(for this it can't be used anymore)

could you explain a bit more what you mean?

In terms of updating state, the owner needs to be able to account for situations like:

• reverting a token generation (e.g. by setting status to false) so that issuer can re-generate
• adding a token generated outside of the protocol
• removing a bad ticker / token registration
• extending the expiry time of a reservation
  so I thought the most flexible approach would be to give owner the rights to update either the ticker or token table.

[satyamakgec]

Yes, we can add the function to change the PolyToken address.
Re. for the addCustomSecurityToken()

could you explain a bit more what you mean?

The initially the idea of using the addCustomSecurityToken() are -

1. To add tokens those are generated outside the network.
2. Use to migrate the data from previous registry to new registry. (No use anymore as we have the upgradeable approach)

following these above requirements in mind we had created addCustomSecurityToken() function with some restriction on it. Restriction as follows -

1. Not able to change the details of the already deployed token.
2. Not able to use any pre-owned ticker. (if Bob buy the ABC ticker at 10 sept with the expiry date of 25 sept and did not deployed the token yet and if STR owner calls addCustomSecurityToken() at 11 sept with ticker ABC and having the different _owner other than Bob then the txn will revert. So the idea was to maintain the integrity into the system).

if we are thinking to change that functionality then we can rework on the logic of the addCustomSecurityToken() and amend all the new requirements that we want.

removing a bad ticker / token registration
extending the expiry time of a reservation

we have another function called modifyTickerDetails() that is used to change all the details of the ticker only if the token with same ticker is not deployed.
And we have addCustomTicker() to add a new ticker by the STR owner in the ecosystem with out paying the POLY fee. This function also checks the "already existence" of the ticker if ticker is already present and not expired then addCustomTicker() will revert.

To make the approach simpler we can merge modifyTickerDetails() and addCustomTicker().

[satyamakgec]

As you suggested to create the library I try to make a one. I did not change lot of things in the code only factored out the hashing logic from the contract and use a helper library for that. you can check the code of EternalStorage.sol and the helper library.

Using the library logic we saved more than 1 million gas in STR deployment and I also try to calculate the size of the contract to understand the difference so I followed the suggestion on stackexchange to calculate the size. I don't know it is a right way or not but after using library approach we got the reduction of 8KB in size (47 KB to 39 KB). But this will contradict EIP according to it contract size limit is 24 KB. Maybe the method I opt to find the size of contract is wrong I have to find a new one.

But still, it is a good improvement related to gas. I am thinking to optimize it more. Could we move any other functionality into our helper.sol library from the EternalSotrage.sol suggestions?

[satyamakgec]

ah! got the answer as It depends on the deployedbytecode and in bytecode each byte refers to 2 character so 47 KB turns to 23.5 KB (close to hit the limit) and 39 KB turns to 19.5KB so we are able to reduce the 4KB size of the contract.

[pabloruiz55]

I'm wondering if we really need to have this function:
function renounceOwnership() external onlyOwner
I don't see a reason why we would ever want to call this on the registries at all...

[satyamakgec]

Yes we can remove it as we will never gonna use it.

[satyamakgec]

Test cases are passed in the build but getting a failure in docs.sh. we already fixed this in our different branches. Once STR passes all the review procedure then I will merge all the changes from the dev-1.5.0 to upgrade-str. We also need to merge so I can create the new branch for the development of upgradable MR