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

Add a "safe way to send ether" i.e. address.transfer #610

Closed
chriseth opened this issue May 30, 2016 · 13 comments
Closed

Add a "safe way to send ether" i.e. address.transfer #610

chriseth opened this issue May 30, 2016 · 13 comments

Comments

@chriseth
Copy link
Contributor

address.send does not send along any gas and does not propagate exceptions. This is good for situation where we do not know the target, but those are limited in use as was discovered in the meantime. People resort to workarounds like recipient.call.value(x)(). Exactly this workaround should receive a name and it should propagate exceptions.

Suggestions for the name: transfer (sounds safer because transfer usually asks you to watch the whole process)

@ethers
Copy link
Member

ethers commented Jun 2, 2016

LGTM. When this is implemented, could a warning message when address.send is used, be displayed and to suggest that the user probably wants to use address.transfer ?

@chriseth chriseth changed the title Add a "save way to send ether" i.e. address.transfer Add a "safe way to send ether" i.e. address.transfer Jun 4, 2016
@PeterBorah
Copy link

As we've seen in the past couple weeks, recipient.call.value(x)() is very dangerous. I would therefore suggest that this method, if implemented, limit gas like send() does, with the only difference being the error propagation. Otherwise this will be exacerbating the reentry problem.

@chriseth chriseth added this to the 6-usability-features milestone Aug 11, 2016
@axic
Copy link
Member

axic commented Aug 12, 2016

Wouldn't the main suggested use for transfer be sending value to addresses or accounts? And in the case of accounts, strictly to the fallback function?

In that case it should by default include gas enough for that only and offer the .gas(n) modifier.

ie. address.transfer(1 ether) and address.transfer.gas(120000)(1 ether)

@axic
Copy link
Member

axic commented Feb 5, 2017

I think it may make sense rejecting 0 value transfers as those seem like circumventing the real purpose of transfer.

@federicobond
Copy link
Contributor

Is error bubbling the only difference between send and transfer? I am worried that people may confuse these two, since there is little semantic clue in those names to distinguish between them.

Also, if the plan is to eventually deprecate send (to avoid this confusion), what would be the recommended migration path? Contracts could easily lock themselves if they carelessly replace send with transfer, right?

@axic
Copy link
Member

axic commented Feb 9, 2017

Two differences:

  • throw on error
  • supports the .gas() modifier to override the supplied gas

if the plan is to eventually deprecate send

See #1666. Both will be available, send should be deprecated, maybe removed in the future.

Contracts could easily lock themselves if they carelessly replace send with transfer, right?

If they just replace it and the target throws, then I guess they could have situation their code won't progress forward. Hence this has a new method name and we're not changing send.

@federicobond
Copy link
Contributor

Thanks for the clarification!

@chriseth
Copy link
Contributor Author

chriseth commented Mar 8, 2017

This is implemented in develop.

@chriseth chriseth closed this as completed Mar 8, 2017
@chriseth chriseth removed the accepted label Mar 8, 2017
@ivan-gavran
Copy link

I am wondering about .gas() modifier: when I tried to use it, I got member gas not found or not visible compilation error (with compiler 0.4.13). Even after changing the compiler versions in Remix, the same compilation error happens.
what could be the problem here?

@axic
Copy link
Member

axic commented Aug 14, 2017

@Gergia the .transfer feature never supported the .gas modifier, if in certain cases it did that was a bug. Did you had such a case?

@ivan-gavran
Copy link

No, I thought it supported .gas modifier based on the answer on ethereum stackexchange and your comment in this thread from February 8th.
(Maybe it would make sense to comment on this stackexchange thread so that people are not misleaded, I don't have enough points for commenting yet)

@axic
Copy link
Member

axic commented Aug 14, 2017

Sorry, this issue was only an in-progress discussion. The documentation contains the final decision. Left a comment on stackexchange.

@naveedh27
Copy link

Still it is not working.
Ganache v1.1 + remix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants
@axic @federicobond @PeterBorah @ethers @ivan-gavran @naveedh27 @chriseth and others