OpenZeppelin / openzeppelin-labs

A space for the community to interact and exchange ideas on the OpenZeppelin platform. Do not use in production!
https://openzeppelin.com/
MIT License
374 stars 115 forks source link

Why not use standard Solidity delegatecall instead of Assembly delegatecall in Proxy? #1

Closed willjgriff closed 6 years ago

willjgriff commented 6 years ago

I was recently experimenting with a similar structure and realised it wasn't necessary to use the low-level assembly delegatecall but could use the standard Solidity delegatecall, see: UpgradableContractProxy

To me the less assembly there is the more readable it is. Is there a reason for using the assembly delegatecall? I'm not sure how the gas estimation works in the higher-level Solidity delegatecall so I imagine the lower-level assembly delegatecall is more efficient but I'm not sure by how much.

frangio commented 6 years ago

Thanks for the question @willjgriff!

Completely agree that the less assembly the better. In this case the assembly was needed because the higher-level delegatecall doesn't give access to the called function's return value. Most of the assembly code is actually dedicated to that:

https://github.com/zeppelinos/labs/blob/6f0b5d9b0b20766d67948b5cd7e203df36b8b0bd/contracts/Proxy.sol#L22-L29

It may be possible to use the high-level delegatecall and immediately afterwards get the return value using assembly. But this isn't guaranteed to work, and the code may actually end up being messier.

I think it's fine to create small and well-defined building blocks using assembly, which can later be used safely in a greater higher-level context.

willjgriff commented 6 years ago

Ah fair enough, the primitive tests I wrote for the proxy work for both Solidity and assembly implementations so thought it might just work in all cases. However, I've now compared the assembly output of both implementations and there's clearly more going on in the higher-level Solidity delegatecall. I'm interested to understand exactly what's happening. I don't suppose you know of any literature that explains how the Solidity delegatecall is compiled? I'll try to work it out anyway. Thanks for the response!

frangio commented 6 years ago

I haven't seen anything explaining it. Perhaps the compiler source code can help!

willjgriff commented 6 years ago

I'm curious to know what situations the higher-level call isn't guaranteed to work, is there any chance you could expand on that or give an example?

frangio commented 6 years ago

Well, it's not guaranteed to work because the compiled code is free to do whatever it pleases with the lower levels components (such as memory and stack contents) as long as the higher level Solidity semantics are unaffected. That is unless the compiler guarantees otherwise, which is not the case as far as I can tell. This means that anything can happen in the middle of the high-level delegatecall and the assembly block that would get the return value, including changing the returndata.

willjgriff commented 6 years ago

I traced through the compiled Solidity delegatecall assembly in Solidity 0.4.18 and it ultimately executed as delegatecall(gas - 25710, contractAddress, mload(0x40), callDataSize, mload(0x40), 0) Separate tests I've done of the low-level delegatecall show all that's needed to get the return value using returndatacopy(mload(0x40), 0x0, returndatasize) is a 0 in the final parameter of the delegatecall, which is the case for the Solidity delegatecall.

So do you mean it could work for the current Solidity version but may not work for a future version? Or do you mean even with the current Solidity version it might not work in some cases? In which case do you have any examples where you think it might not work? I'm trying to learn as much as I can about the delegate call since it turns out we'll be using it a lot.

frangio commented 6 years ago

Yes, I mean both of those things. I don't have any examples. It is just that since it is not guaranteed by the compiler, it can't be relied to work.

willjgriff commented 6 years ago

Ah okay, I will avoid doing it like that then for now. Thanks for the response!