Plugin support

[cleanunicorn]

There is a need for this proxy to support receiving tokens. Unfortunately in its current form, the proxy does not support that.

Background

We use this proxy to enhance the user experience, but we hit the limitation of not being able to receive tokens (specifically EIP1155 and EIP721).

Thus, we had to fork the proxy, since an extension of this functionality was not possible. We achieved this by forking the proxy and inheriting ERC1155Holder and ERC721Holder from OpenZeppelin.

https://github.com/fiatdao/proxy/blob/553a28073011521fcf393842ceb10fa05dcb7fa7/contracts/PRBProxy.sol#L25

Proposal

My idea is a plugin system that allows each plugin to be installed/uninstalled by the owner.

This can be achieved when the proxy implements the fallback() method and checks for known, installed method signatures. If they are found, it can delegate execution to the plugin implementation.

Such an implementation of the fallback() method could look like this:

fallback(bytes calldata data) external payable returns (bytes memory response) {
    // Check function signature in fallback method list
    address implementation = installedMethods[msg.sig];
    if (implementation == address(0)) {
        // Exit if method is not found
        revert PRBProxyPlugin__MethodNotFound();
    }

    // Reserve some gas to ensure that the function has enough to finish the execution
    uint256 stipend = gasleft() - minGasReserve;

    // Call fallback method
    bool success;
    // solhint-disable-next-line avoid-low-level-calls
    (success, response) = implementation.delegatecall{gas: stipend}(data);

    // Check if the call was successful or not
    if (!success) {
        // If there is return data, the call reverted with a reason or a custom error
        if (response.length > 0) {
            assembly {
                let returndata_size := mload(response)
                revert(add(32, response), returndata_size)
            }
        } else {
            revert PRBProxyPlugin__ExecutionReverted();
        }
    }
}

Checking if a method is implemented can be done by maintaining a list of installed methods:

  // List of methods supported by the proxy
  mapping(bytes4 => address) public installedMethods;

The fallback method can use this mapping to check if the request should be delegated or exit early:

// Check function signature in fallback method list
address implementation = installedMethods[msg.sig];
if (implementation == address(0)) {
    // Exit if method is not found
    revert PRBProxyPlugin__MethodNotFound();
}

// Continue with execution
// ...

The rest of the execution can be very similar to how the proxy handles execution right now:

• make sure the owner is not overridden
• forward a limited amount of gas (gasleft() - minGasReserve) to the execution
• delegate the call to the implementation
• log execution
• return error

Functionality

The plugin should have a standardized way of installing and uninstalling plugins. For this, an interface needs to be provided and implemented.

interface IPRBProxyPlugin {
    function methods() external view returns (bytes4[] memory);
}

• methods() - returns a list of method signatures it supports

The proxy needs to implement and additional pair of methods:

• install(address plugin_) - installs a new plugin
• uninstall(address plugin_) - uninstalls an existing plugin

These methods need to be generalized and they should use the list provided by IPRBProxyPlugin.methods() to register or unregister implemented methods.

An example of these methods:

// Install a plugin
function install(address plugin_) public {
    // List methods
    bytes4[] memory methodsToInstall = IPRBProxyPlugin(plugin_).methods();

    // Add signatures to fallback method list
    for (uint256 i = 0; i < methodsToInstall.length; i++) {
        installedMethods[methodsToInstall[i]] = plugin_;
    }
}

// Uninstall a plugin
function uninstall(address plugin_) public {
    // List methods
    bytes4[] memory methodsToInstall = IPRBProxyPlugin(plugin_).methods();

    // Add signatures to fallback method list
    for (uint256 i = 0; i < methodsToInstall.length; i++) {
        installedMethods[methodsToInstall[i]] = address(0);
    }
}

More functionality needs to be added in these methods:

• check if a method is already registered
• emit event for each method
• emit event for the plugin install/uninstall

[Work in progress] Implementation

A working repository that extends the current PRBProxy and adds this functionality on top is here.

To help with navigating the repo, here are the important parts:

• PRBProxyPlugins.sol - extends the proxy and adds plugin functionality
• IPRBProxyPlugin.sol - interface each proxy should implement
• ERC1155ReceiverPlugin.sol - proxy that implements holding EIP1155 tokens
• testFail_proxyCantReceive_ERC1155Token - test that shows that the current vanilla proxy can't receive EIP1155 tokens
• test_proxyWithPluginsCanReceive_ERC1155Token - test that installs the plugin and tests it can receive EIP1155 tokens

References

These features can be implemented as plugins

• EIP-1271
• EIP-3156

[PaulRBerg]

Wow, this is such a detailed feature proposal. Thanks for taking the time to write it up!

I'm totally onboard with this - it does not add much overhead in terms of extra gas to pay for deploying the proxy, and generally it is non-invasive to the default user experience. A user has to opt-in to the plugin system by manually installing a plugin only when they need it, which effectively addresses the concerns I shared in #24.

I don't have an ETA for when I will finish implementing this, but it will be done!

[newts6]

@cleanunicorn awesome work! I'd be interested in using the plug-in system. Do you have plans to open a PR to merge it upstream?

[cleanunicorn]

I have some time these following weeks to create an implementation with complete tests that add the plugin system. I'll create a PR as soon as I have something.

@PaulRBerg would that be okay with you?

[PaulRBerg]

Totally! Happy to dedicate time to review your PR(s).

For context, the best place to start (fork from) would be the staging branch.

[cleanunicorn]

I started working on this. Will push a PR within a week.

[PaulRBerg]

Going to lock this discussion as this feature has been implemented in #53.