[Protocol3] Support DEX upgradability be default

[dong77]

OwnedUpgradeabilityProxy is very convenient to make sure a DEX is upgradable. The issue is that user assets are at risk -- the owner of the proxy can always upgrade the proxy to point to another contract that may move user fund.

My idea is to disable instant proxy upgrade to arbitrary contract and only allow the proxy owner to schedule an upgrade to a declared address in advance, for example, 30 days. When a scheduled upgrade is still pending, the owner can cancel it or schedule a new one to replace it, and the waiting period shall restart.

@letsgoustc @Brechtpd @hosschao please let me know what you think.

[dong77]

BTW, we are of course assuming DAOs are not ready. The objective is not to prevent DEX owners from upgrading to a malicious contract, but allows users of a DEX to have enough time to respond during the waiting period.

[dong77]

After talking to @Brechtpd and @letsgoustc, a better way to offer transparent upgradability is to create a VersionManager that always maintain the latest version and force each DEX's proxy to point to the lastest version.

[dong77]

Here is the basic idea.

Right now we have ILoopring3, but in the future, we may have ILoopring4, ILoopring5. And we have to assume they are not compatible. For Loopring3, the current design allows the creation of multiple exchange instances (each with its own id), but if we use a proxy instead for each DEX, we only need to have one single exchange instance behind all the proxies.

First, we can ask ILoopring3/4/5 to implement a new interface called ILoopring, which has the following methods:

interface ILoopring {
   function getProtocolInfo() returns (uint16 version, address dexInstanceAddress);
   function registerProxy(address proxy) returns (uint dexId); // this will also initialize the proxy's exchange ID
}

When ILoopring3 is deployed, its constructor will automatically calls the createInstance function -- currently public but should be private -- to initialize the address _dexInstanceAddress field that will be retuned by the getProtocolInfo function. But the dexInstance's id will be 0.

We then create anIVersionManager instance which implements the following:

contract IVersionManager {
  struct Protocol {
     address loopring;
     address instance;
  }
  mapping (uint16 => Protocol) protocols;

  function getProtocol(uint16 version) returns (address loopring, address instance) {
    Protocol protocol = protocols[version];
    loopring = protocol.loopring;
    instance = protocol.instance;
  }

  function registerProtocol(address loopring) {
    ILoopring loopring = ILoopring(loopring);
    (ver, instance) = loopring.getProtocolInfo();
    protocols[ver] = Protocol(loopring, instance);
  }

  function createExchange(uint16 ver) {
      Protocol protocol = protocols[ver];
      ILoopring loopring = ILoopring(protocol.loopring);
      ExchangeProxy proxy = new ExchangeProxy(ver, address(this));
      loopring.registerProxy(proxy);
  }
}

The ExchangeProxy will be aware of the IVersionManager:

contract ExchangeProxy {
  constructor(uint16 _version, address _versionManager) public {
     setVersion(_version);
     setVersionManager(_versionManager);
  }

  function implementation() view public returns (address impl) {
    IVersionManager vm = IVersionManager(versionManager());
    (_, impl) = vm.getProtocol(version());
    assert(impl != address(0));
  }
}

@Brechtpd please comment.

[Brechtpd]

Looks good. How would we do small backwards compatible changes to the Exchange implementation? It seems like currently it's a single Exchange implementation / Loopring version (a single call to createInstance in the Loopring contract to create the implementation contract). Because all the staking is done on the Loopring level for these exchanges the Loopring contract cannot be easily swapped for another (so doing registerProtocol on the same version somehow cannot work easily). Will it be possible to update the implementation contract address for an existing Loopring contract (when, of course, we are sure it's backward compatible)? Or maybe this is possible on some other way (a similar proxy design pattern on the Loopring contract)?

[dong77]

Update: We can create the IProtocalRegistry (previously IVersionManager) using the addresses of ILoopring contracts as the real "version" keys.

contract IProtocalRegistry is Claimable {
  struct Protocol {
     address instance;
     string version;
  }
  mapping (address => Protocol) protocols; // map keys are  ILoopring contract addresses

  function getProtocol(address loopring) returns (address instance, string version) {
    Protocol protocol = protocols[loopring];
    instance = protocol.instance;
    version = protocol.version;
  }

  function registerProtocol(address loopring, address instance, string version)  {
    protocols[loopring] = Protocol(instance, version);
  }

  function createExchange(address loopring) returns (address exchangeProxy, uint exchangeId) {
      // assert loopring is a valid address
      ExchangeProxy proxy = new ExchangeProxy(address(this), loopring);
      exchangeProxy = address(proxy);
      exchangeId = ILoopring(loopring).registerProxy(exchangeProxy);
  }
}

By doing this, we allow the owner to replace an instance for an existing loopring protocol version, and the instance does not have to deployed by the loopring contract.

The ExchangeProxy will be aware of the IProtocolRegistry:

contract ExchangeProxy {
  constructor(address _registry, address loopring) public {
     setRegistry(_registry);
     setProtocol(_loopring);
  }

  function implementation() view public returns (address instance) {
    IProtocolRegistry registry = IProtocolRegistry(registry());
    (instance, _) = registry.getProtocol(loopring());
    assert(instance != address(0));
  }

  function version() view public returns (string version) {
    IProtocalRegistry registry = IProtocalRegistry(registry());
    (, version) = registry.getProtocol(loopring());
  }
}

[Brechtpd]

I think this is a good solution!

Something that doesn't really matter and is just an implementation detail: Because storing data in a proxy contract is done in a way that is a bit strange, we can choose to not store the loopring address there. In IProtocolRegistry we can keep a map of proxy -> loopring and when getProtocol is called we lookup the loopring address for the proxy in that map using msg.sender. This way we only need to store the single registry address in the proxy.

[dong77]

I think this is a good solution!

Something that doesn't really matter and is just an implementation detail: Because storing data in a proxy contract is done in a way that is a bit strange, we can choose to not store the loopring address there. In IProtocolRegistry we can keep a map of proxy -> loopring and when getProtocol is called we lookup the loopring address for the proxy in that map using msg.sender. This way we only need to store the single registry address in the proxy.

I like your recommendation. Will change it later that way in the PR.

[dong77]

I created these images to illustrate our upgradability solution.
(an error in the image, ExchangeRegistry should really be ProtocolRegistry)

1. Only one protocol version is registered:

2. After we deploy and register a compatible implementation:
Note that the upgrading is transparent and controlled by the owner of ProtocolRegistry.

3. When we deploy a new protocol which is incompatible with the previous version:
Note that in such scenario the existing exchanges cannot upgrade to the new protocol. Users can create new exchanges based on either version.

4. deploy a proxy to make the registry upgradable:

5. When we create another exchange:

@kongliangzhong @letsgoustc please take a look

[Brechtpd]

Will we use a proxy for the Loopring contracts as well? There's not that much logic in it, but I guess it could be useful to be able to update it as well when the Exchange <-> Loopring interaction changes a bit when updating Exchange implementations.

[dong77]

Will we use a proxy for the Loopring contracts as well? There's not that much logic in it, but I guess it could be useful to be able to update it as well when the Exchange <-> Loopring interaction changes a bit when updating Exchange implementations.

We can call ILoopringVxxx represents the major versions, and IExchangeVxxx represents the minor versions.
If we modify the ILoopringVxxx contracts, we have to deploy a major version. Transparent upgradability is only available for minor versions. As illustrated by the (new) set of images below:

1. Multiple exchange instances using the same major version (LoopringV3):

2. Exchanges will be upgraded automatically to a new default minor version:

3. Exchanges will NOT be able to upgrade to any major version:

4. Upgradability can be disabled

The upgrade is triggered by the owner of the ProtocolRegistry owner. If DEX owners decided not to trust the ProtocolRegistry owner, they can choose to disable upgradability by setting supportUpgradability to false.

    function forgeExchange(
        address protocol,
        bool    supportUpgradability,
        bool    onchainDataAvailability
        )
        external
        nonReentrant
        returns (
            address exchangeAddress,
            uint    exchangeId
        );

5. DEX owners can also manage their own customized proxies for upgradability

[dong77]

Implemented by PR #384

[dong77]

DONE