Upgrade contracts to solc-6 #40
Conversation
contracts/Proxy.sol
Outdated
| receive() external payable { | ||
| revert(); | ||
| } |
There was a problem hiding this comment.
Do we really want this behaviour? It seem like e.g. the Safe (using the Proxy contract) should be able to receive ether without reverting.
There was a problem hiding this comment.
What should we do otherwise? Just leave it as an empty method?
There was a problem hiding this comment.
I think this implementation is incorrect. If this Proxy implementation would be used by the Safe for example, it would revert when someone sent you Eth, which I imagine is not what you want. I suspect the correct behaviour is to proxy the call to the _masterCopy as well, just without calldata.
The easier approach, would be to just remove the receive function as according to the docs https://docs.soliditylang.org/en/v0.6.12/contracts.html#fallback-function (emphasis my own):
The
fallbackfunction [..] is executed on a call to the contract if none of the other functions match the given function signature, or if no data was supplied at all and there is no receive Ether function.
There was a problem hiding this comment.
Leaving it empty also does not work, as some contracts (e.g. WETH) execute logic when they are sent ETH without any additional calldata. If the method was empty it would not trigger this logic on the proxied contract (as no delegatecall would be executed). I was under the assumption that just omitting receive would not work because of some solidity warning but agree that this would be the best option if possible. Otherwise I guess you'd have to copy some of the code from the fallback method to also invoke receive on the proxied contract.
There was a problem hiding this comment.
According to the docs, omitting the the receive function entirely should work. Maybe there is some way to silence the warnings if they show up?
There was a problem hiding this comment.
After looking closer at the OZ contract, it looks like perhaps we should simply be using their Proxy contracts instead of maintaining this one. Otherwise, it seems we are just doing the same thing twice. For example, I've recently been investigating making our GPv2Authenticator contract upgradable and I had no intention of using this library, but rather depending on openzepplin-cli for their contract upgrade SDK.
There was a problem hiding this comment.
Does this mean that we are not happy using just receive without fallback?
As Nick suggested this is likely going to cause problems if you are trying to proxy a regular method (one that has calldata like ERC20.transfer(to, amount)). If we did this, I'd like to see a unit test making sure this still works.
it looks like perhaps we should simply be using their Proxy contracts instead of maintaining this one
I think this is a good suggestion. I'm not sure if there are other than historic reasons that we built our own. It is however not really related to this PR and should be separate (could be done before if you want to avoid upgrading this piece).
There was a problem hiding this comment.
The proxy is from the Safe project and we wrote our own because the one from OZ uses up more gas
Edit: in this regard it might make more sense to fully rewrite the proxy in yul in the future to optimize this and for now remove the proxy
There was a problem hiding this comment.
Right, this is a good idea. However, since the purpose of this PR is merely to upgrade the contracts, I will create an issue to remove the Proxy separately.
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
|
Right yes. We should do something different here, but I wasn't really thinking we may actually want to receive ether at the time. Should this method just be included with an empty code block? Or maybe we can emit an event that says "received ether" with the value? Really not sure what to do here, but want to change things as little as possible. |
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
|
Sweet, so I will wait to merge this until your outstanding PR #38 has landed, then update, merge and publish |
| /** | ||
| * @dev Fallback function that delegates calls to the masterCopy. | ||
| * Runs when no other function in the contract matches the call data. | ||
| */ |
There was a problem hiding this comment.
ubernit: There is a mix of doc-comment styles in this file, both /// and /** */ are used.
There was a problem hiding this comment.
Thanks, I'll fix this ASAP. Plan to merge and publish on Monday. Please feel free to approve.
There was a problem hiding this comment.
Please feel free to approve
Already have 😉
Not too many changes necessary, just a few function signatures and had to explicitly make a receive function because of a warning.
Note that this PR is made explicitly to resolve the compiler clash issue we have in oba-contracts that will aid in making GPv2Authenticator StorageAccessible. Issue here