PaulRBerg / prb-proxy

Proxy contract to compose Ethereum transactions
https://prbproxy.com
MIT License
273 stars 47 forks source link

Remove local variable "proxy" in "noProxy" modifier #153

Closed PaulRBerg closed 1 year ago

PaulRBerg commented 1 year ago

This is @IaroslavMazur's suggestion:

the initialization of a local IPRBProxy variable could be ditched for the direct access of the Proxy contract from storage. While the invalid calls would, indeed, need to access the storage twice in this case, the valid ones would become more efficient.

The reference is for the noProxy modifier defined here:

https://github.com/PaulRBerg/prb-proxy/blob/82d14b9d0b0f6be3df43268c41cffec4701368f8/src/PRBProxyRegistry.sol#L61-L67

However, we need to double-check if this would actually result in a gas optimization - it might be the case that, due to via IR, the gas cost is the same.

IaroslavMazur commented 1 year ago

If the gas cost of both of the options turns out to be the same, I'd vote for keeping the existing one (as it's undoubtedly more readable than the "reading from storage directly" one).

PaulRBerg commented 1 year ago

I made a gas comparison by using the same technique as the one described here.

Breakdown (for deploying proxies via the deploy function):

Tx Gas Cost
With local variable 425,154
Without local variable 425,214

Thus, again contrary to our expectations, removing the local variable proxy increases the gas cost - it does not decrease it. Via IR is strange.

Late edit: I correct the URL for "Without local variable" above.

IaroslavMazur commented 1 year ago

I made a gas comparison by using the same technique as the one described here.

Breakdown (for deploying proxies via the deploy function):

Tx Gas Cost With local variable 425,154 Without local variable 425,214 Thus, again contrary to our expectations, removing the local variable proxy increases the gas cost - it does not decrease it. Via IR is strange.

As I can see, the definitions of the noProxy modifier are the same in both of the instances of PRBProxyRegistry: image

image

So, whatever is different about those 2 contracts (which is causing the different tx gas cost), it's not the noProxy modifier... 🤔

PaulRBerg commented 1 year ago

Sorry @IaroslavMazur - I've linked the incorrect tx above (for "Without local variable"). The correct deployment transaction is this:

https://goerli.etherscan.io/tx/0x3760410599dcbd15e12d119d96f8d0c5e189e9a5f4a7b81b0185522f8b3f3677

And the correct address for the adjusted registry is this:

https://goerli.etherscan.io/address/0xc3d1039afbb4a511ee9188725abf437fa748fa72

As you can see, the modifier in this contract is this:

modifier noProxy(address owner) {
    if (address(_proxies[owner]) != address(0)) {
        revert PRBProxyRegistry_OwnerHasProxy(owner, _proxies[owner]);
    }
    _;
}

I'll let you close the issue once you double-check the updated URLs.

IaroslavMazur commented 1 year ago

Thank you for the clarification, @PaulRBerg!

I'm closing the issue.