Remove superfluous receive() function from Proxy.sol #4434
Conversation
🦋 Changeset detectedLatest commit: 1715027 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
I'm approving but, can we try a bit more to reproduce the supposed warning with no receive function? I'm afraid this won't trigger for us but maybe will do for others importing the library. If we can make sure the warning was removed after a certain Solidity version would be better.
|
Note: tests show that is saved 1500~1900 gas per proxy deployment ( |
|
The warning wasn't removed. It's just really hard to reproduce. I couldn't figure out the conditions until I found the warning in the Solidity repo12. The warning comes up when there is a payable fallback function along with other functions, but no receive function. contract C {
fallback() external payable { }
function f() public pure { }
}contract A {
function f() external pure {}
}
contract C is A {
fallback() external payable { }
}The warning in both cases is:
This probably used to happen in the transparent proxy because we had a fallback and additional functions ( Footnotes |
https://forum.openzeppelin.com/t/proxy-sol-fallback/36951/8 The fallback alone would indeed be enough. Fallback is not limited to` msg.value == 0` (if its marked `payable`). Both functions can support value. The difference between receive and fallback is in the msg.data. If the calldata is empty and if there is a receive function, it fill be used. Otherwise, fallback is used. This means that regardless of the value, fallback will be called if there is some data. `fallback` is also the one that is called if there is no data, but receive is not defined. So why do we have a receive function that is not really needed? To silent solidity warnings that sometimes happen when you have a fallback function but no receive function. - @Amxx see <OpenZeppelin/openzeppelin-contracts#4434 (comment)>
https://forum.openzeppelin.com/t/proxy-sol-fallback/36951/8 The fallback alone would indeed be enough. Fallback is not limited to` msg.value == 0` (if its marked `payable`). Both functions can support value. The difference between receive and fallback is in the msg.data. If the calldata is empty and if there is a receive function, it fill be used. Otherwise, fallback is used. This means that regardless of the value, fallback will be called if there is some data. `fallback` is also the one that is called if there is no data, but receive is not defined. So why do we have a receive function that is not really needed? To silent solidity warnings that sometimes happen when you have a fallback function but no receive function. - @Amxx see <OpenZeppelin/openzeppelin-contracts#4434 (comment)>
Following this discussion in the forum
In the past, when declaring a
fallback(), solidity would trigger a warning if noreceive()is implemented. That is apparently no longer the case.In the case of Proxy, the
receivefunction is not necessary. Any call, with or without data, with or without value, will be catched byfallback() fallback.PR Checklist
npx changeset add)