Skip to content
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

Transfer replacement #1962

Merged
merged 4 commits into from
Oct 25, 2019
Merged

Conversation

nventuro
Copy link
Contributor

This PR adds a replacement for Solidity's transfer, which imposes a fixed gas limit of 2300. While useful to prevent reentrancy issues, this has proved to be an issue due to gas repricings, like the one EIP1884 will introduce on the Instanbul hardfork.

Given that the code to imitate transfer is tricky and cryptic, having a helper method to do it is a good idea.

@nventuro nventuro added feature New contracts, functions, or helpers. contracts Smart contract code. labels Oct 22, 2019
@nventuro nventuro requested a review from frangio October 22, 2019 23:28
@nventuro nventuro changed the base branch from master to release-v2.4.0 October 23, 2019 16:41
@nventuro nventuro changed the base branch from release-v2.4.0 to master October 23, 2019 16:41
@nventuro
Copy link
Contributor Author

This should go into the 2.4 release, but I started the branch from master instead of release-2.4. Should I rebase these commits on top of 2.4, or do we want to simply cherry-pick the merge commit there afterwards?

@frangio
Copy link
Contributor

frangio commented Oct 23, 2019

Cherry-picking aftewards is the "protocol".

CHANGELOG.md Outdated Show resolved Hide resolved
test/utils/Address.test.js Show resolved Hide resolved
contracts/utils/Address.sol Outdated Show resolved Hide resolved
@nventuro nventuro requested a review from frangio October 25, 2019 16:19
frangio
frangio previously approved these changes Oct 25, 2019
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@nventuro nventuro merged commit 8d166f3 into OpenZeppelin:master Oct 25, 2019
@nventuro nventuro deleted the transfer-replacement branch October 25, 2019 18:53
@@ -10,6 +10,7 @@
### New features:
* `Address.toPayable`: added a helper to convert between address types without having to resort to low-level casting. ([#1773](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1773))
* Facilities to make metatransaction-enabled contracts through the Gas Station Network. ([#1844](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1844))
* `Address.sendValue`: added a replacement to Solidity's `transfer`, removing the fixed gas stipend. ([#1961](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1961))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nventuro should it be 1962?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. I'll fix it on master.

frangio pushed a commit that referenced this pull request Oct 29, 2019
* Add Address.sendEther

* Add documentation to sendEther

* Add changelog entry

* Rename sendEther to sendValue

(cherry picked from commit 8d166f3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code. feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants