-
Couldn't load subscription status.
- Fork 6.2k
Add various deprecation warnings #16174
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
base: develop
Are you sure you want to change the base?
Conversation
|
Just a quick heads up: note that the issues you listed in the description should not be closed when this PR is merged. They're about feature removals and adding a warning is just an intermediate step, not a complete solution. |
3c00eb6 to
f052fe2
Compare
6d32b8c to
2531e30
Compare
2531e30 to
cb21835
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
cb21835 to
0c67bef
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
0c67bef to
2cf221d
Compare
2cf221d to
ab2e083
Compare
| you have to implement a receive Ether function (using payable fallback functions for receiving Ether is | ||
| not recommended, since the fallback is invoked and would not fail for interface confusions | ||
| on the part of the sender). | ||
| Note that ``send`` and ``transfer`` are scheduled to be deprecated in the next breaking version (0.9). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we deprecate them sooner, with this PR, and then with 0.9 (or another future breaking release) they are removed? Also I think this could be its own warning block. Perhaps even the first one.
| call to the contract with empty calldata. This is the function that is executed | ||
| on plain Ether transfers (e.g. via ``.send()`` or ``.transfer()``). If no such | ||
| function exists, but a payable :ref:`fallback function <fallback-function>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps not in this PR but I think we will have to go through the docs regarding send and transfer and see where we have to update them wrt best practices regarding send and transfer
| // If someone sends Ether to that contract, | ||
| // the transfer will fail, i.e. this returns false here. | ||
| // This will report a warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // This will report a warning | |
| // This will emit a deprecation warning |
| // If someone sends Ether to that contract, | ||
| // the transfer will fail, i.e. this returns false here. | ||
| // This will report a warning | ||
| return testPayable.send(2 ether); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new way of writing this without the gas limits would be this right?
(bool sent,) = address(test).call{value: 2 ether}("");
return sent;| } | ||
| .. warning:: | ||
| virtual modifiers will be deprecated in the next breaking version (0.9). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| virtual modifiers will be deprecated in the next breaking version (0.9). | |
| ``virtual`` modifiers are deprecated and scheduled for removal in the next breaking version (0.9). |
|
|
||
| .. _modifier-overriding: | ||
|
|
||
| Modifier Overriding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wouldn't hurt to clarify that this is deprecated already in the headline?
| Modifier Overriding (deprecated) |
| // msg.sender is not of type `address payable` and must be | ||
| // explicitly converted using `payable(msg.sender)` in order | ||
| // use the member function `send()`. | ||
| // This will report a warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is everywhere... I'll only remark it once more :)
| // This will report a warning | |
| // This emits a deprecation warning |
| (this can always be forced by the caller) and it also fails if the recipient runs out of gas. So in order | ||
| to make safe Ether transfers, always check the return value of ``send``, use ``transfer`` or even better: | ||
| use a pattern where the recipient withdraws the Ether. | ||
| Please, be aware that ``send`` will be deprecated in the next breaking version (0.9). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a warning block
| backward-compatibility with older compiler versions, prefer using the dialect string. | ||
| .. warning:: | ||
| The ``memory-safe-assembly`` special comment will be deprecated in the next breaking version (0.9). | ||
| So, if you are not concerned with backward-compatibility with older compiler versions, prefer using the dialect string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, do we have a backwards-compatible way of handling this?
| 9170_error, | ||
| _operation.location(), | ||
| "Comparison of variables of contract type will be deprecated in the next breaking version." | ||
| "Consider using an explicit cast to address type." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the same semantic behavior, you could also emit
| "Consider using an explicit cast to address type." | |
| "Instead, use an explicit cast to address type." |
|
Please go over the docs again, we will deprecate things before 0.9 and with 0.9 or a later version they will be removed. So "Note that XYZ will be deprecated in 0.9" should be "Note that XYZ are deprecated and scheduled for removal in 0.9". I find it a bit problematic to show deprecated code in the examples. This should be updated to be according to best practices. I am not sure if this PR is the right place for it, but if you find it's not, it should be done in a follow-up. Generally I'd have liked that alongside the .. warning::
The following patterns are deprecated and will be removed in v0.9:
- ``address.send()`` - Use ... instead
- ``address.transfer()`` - Use ...Also the code-emitted warnings have to be updated to reflect that things are deprecated now and will be removed in a future breaking release. Please present alternatives there (like for the send stuff to use call) and if possible it would be great to provide a link to the docs where this is described. Otherwise users are left stranded. Think what you would like to see if a compiler presents you with a deprecation warning. Implementation-wise it looks good. |
Partially solves #15795.
Deprecation warnings for:
.sendand.transferRemove .send and .transfer. #7455./// @solidity memory-safe-assemblynatspec comment.