Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deployAndExecute function in the factory/ registry #62

Closed
PaulRBerg opened this issue Feb 12, 2023 · 10 comments
Closed

Add deployAndExecute function in the factory/ registry #62

PaulRBerg opened this issue Feb 12, 2023 · 10 comments
Milestone

Comments

@PaulRBerg
Copy link
Owner

As discussed in #25, it would be helpful to provide a deployAndExecute function that would deploy the proxy and immediately call a target contract in the same transaction.

To quote @SolcAndMe:

The main drawback for a protocol / app to use proxy accounts for their users is user onboarding - in particular the very first interaction between the user and the app. Sequential transactions still lead to pretty janky UX - primarily in conveying to the user for why two transactions are being sent instead of just one and secondly it also is more expensive for the user because the base tx cost has to be paid twice. Adding that method to the factory leads to very minimal gas overhead.

@PaulRBerg PaulRBerg self-assigned this Feb 12, 2023
@PaulRBerg PaulRBerg changed the title Add deployAndExecute Add deployAndExecute function Feb 12, 2023
@PaulRBerg PaulRBerg changed the title Add deployAndExecute function Add deployAndExecute function in the factory/ registry Feb 12, 2023
@PaulRBerg PaulRBerg removed their assignment Feb 12, 2023
@PaulRBerg PaulRBerg added this to the V4 milestone Feb 13, 2023
@SolcAndMe
Copy link

One additional user flow which should get implicitly unlocked with the addition of this method is that upon Proxy deployment the user can also install relevant plugins in the same transaction.

@PaulRBerg
Copy link
Owner Author

Interesting use case!

My first reaction was, "no, this ain't possible", but this could be done with a special target contract that has the same storage layout as the proxy.

@PaulRBerg
Copy link
Owner Author

PaulRBerg commented Feb 19, 2023

I just finished implementing this feature. @SolcAndMe let me know if the implementations match your expectations:

/// @inheritdoc IPRBProxyFactory
function deployAndExecute(
address target,
bytes calldata data
) external override returns (IPRBProxy proxy, bytes memory response) {
(proxy, response) = deployAndExecuteFor({ owner: msg.sender, target: target, data: data });
}
/// @inheritdoc IPRBProxyFactory
function deployAndExecuteFor(
address owner,
address target,
bytes calldata data
) public override returns (IPRBProxy proxy, bytes memory response) {
// Deploy the proxy.
proxy = _deploy(owner);
// Delegate call to the target contract.
response = proxy.execute(target, data);
// Transfer the ownership from this factory contract to the specified owner.
IPRBProxy(proxy).transferOwnership(owner);
}

And:

/// @inheritdoc IPRBProxyRegistry
function deployAndExecute(
address target,
bytes calldata data
) external override returns (IPRBProxy proxy, bytes memory response) {
(proxy, response) = deployAndExecuteFor({ owner: msg.sender, target: target, data: data });
}
/// @inheritdoc IPRBProxyRegistry
function deployAndExecuteFor(
address owner,
address target,
bytes calldata data
) public override noCurrentProxy(owner) returns (IPRBProxy proxy, bytes memory response) {
// Deploy the proxy via the factory, and delegate call to the target.
(proxy, response) = factory.deployAndExecuteFor(owner, target, data);
// Set or override the current proxy for the owner.
currentProxies[owner] = proxy;
}

All functions have 100% test coverage:

Screenshot 2023-02-19 at 5 00 23 PM

@SolcAndMe
Copy link

Interesting use case!

My first reaction was, "no, this ain't possible", but this could be done with a special target contract that has the same storage layout as the proxy.

Why does it have to have the same storage layout? Couldn't this just be a target contract which calls into the plugin configuration methods - since the Factory still has ownership over the proxy at that point in time?
The example use case would be the following:
Upon proxy deployment deployAndExecute will call a method defined in a contract called "OnboardActions.sol". This contract / method will first setup the plugins relevant to the user e.g. ERC1155 callback handlers etc., and will then perform further actions relevant to the protocol the users set up the Proxy for.

@SolcAndMe
Copy link

Yes that's exactly what I had in mind! I don't have any further input on the implementation.
Thanks!

@PaulRBerg
Copy link
Owner Author

PaulRBerg commented Feb 21, 2023

Why does it have to have the same storage layout?

Because of the DELEGATECALL. If you don't have the same storage layout, you will modify other storage properties. You can see an example for how this works in practice with the TargetChangeOwner contract. If there was another storage property defined before owner, changing the owner wouldn't work.

Couldn't this just be a target contract which calls into the plugin configuration methods - since the Factory still has ownership over the proxy at that point in time?

No, unfortunately.

By the time the target contract tries to access the installPlugin method on the proxy, the msg.sender will be the proxy itself (due to the DELEGATECALL). But the installPlugin method only allows the owner as the caller.

Upon proxy deployment deployAndExecute will call a method defined in a contract called OnboardActions.sol. This contract / method will first setup the plugins relevant to the user e.g. ERC1155 callback handlers etc

Totally doable! But, as explained above, it would have to be something like this:

contract OnboardActions {
    address public override owner;
    uint256 public override minGasReserve;
    mapping(bytes4 method => IPRBProxyPlugin plugin) internal plugins;

    function setUpPlugins(address[] calldata pluginsToInstall) external {
        for (uint256 i = 0; i < pluginsToInstall.length; ++i) {
            bytes4[] memory methodList = pluginsToInstall[i].methodList();
            for (uint256 j = 0; j < methodList.length; ++j) {
                plugins[methodList[j]] = pluginsToInstall[i];
            }
        }
    }
}

Seeing how this is done in practice makes me wonder if there's much value in keeping the installPlugin and the uninstallPlugin functions in the PRBProxy contract itself, given that the install method cannot be used in deployAndExecute, and that a target contract could technically achieve the same purpose.

Removing the said functions would definitely save some gas on deployment.

that's exactly what I had in mind!

Glad to hear! In terms of timeline, I will probably release a beta version later this week, and then I will wait a week or so before publishing the final V4 (just in case we find any last-minute issues).

@PaulRBerg
Copy link
Owner Author

As the lead proponent of the plugin system, I wanted to ask you @cleanunicorn what you think of the idea above to not include the plugin installation functions in the proxy itself?

@SolcAndMe
Copy link

Because of the DELEGATECALL. If you don't have the same storage layout, you will modify other storage properties.

Right, I blanked on the fact that this is an introspective change to the Proxy contract itself. Then it would make sense to provide a "Base" Target, Plugin / Setup contract to avoid user errors when users want to define their proxy setup strategies.
Maybe it makes sense to split PRBProxy into PRBProxyStorage for that reason.

Glad to hear! In terms of timeline, I will probably release a beta version later this week, and then I will wait a week or so before publishing the final V4 (just in case we find any last-minute issues).

Awesome!

@PaulRBerg
Copy link
Owner Author

Maybe it makes sense to split PRBProxy into PRBProxyStorage

Fantastic idea. I have just opened a separate issue for tracking - I 100% agree that this should be done.

it would make sense to provide a "Base" Target, Plugin / Setup contract

A base target will not be needed if I implement a separate storage contract.

@PaulRBerg
Copy link
Owner Author

I will close this issue since the deployAndExecute functionality has been implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants