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.

Sign up for GitHub

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

Jump to bottom

WIP - Have Escrow forward all gas on withdraw. #1714

Closed
wants to merge 1 commit into from
Closed

WIP - Have Escrow forward all gas on withdraw. #1714

wants to merge 1 commit into from

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented Apr 9, 2019

Fixes #1668.

I'm not a fan of the library name, but this is the general idea. There's still ongoing discussion regarding whether or not this is actually a good idea, so tagging as WIP.

@nventuro nventuro added contracts Smart contract code. work in progress labels Apr 9, 2019
@nventuro nventuro changed the title Have Escrow forward all gas on withdraw. WIP - Have Escrow forward all gas on withdraw. Apr 9, 2019
@nventuro
Copy link
Contributor Author

This is being paused for now, see this comment in the related issue.

@frangio frangio added the on hold Put on hold for some reason that must be specified in a comment. label Apr 22, 2019
@nventuro
Copy link
Contributor Author

nventuro commented Sep 6, 2019

Given the recent discussions in the community around EIP 1884, it may make sense to review this and other usages of transfer. However, these should probably be considered breaking changes, since they allow for interactions that weren't possible before, and may introduce reentrancy risks.

@nventuro
Copy link
Contributor Author

nventuro commented Oct 23, 2019 •
edited
Loading

This should probably be implemented in terms of #1962 (which is pretty much the same as this PR).

I'm still not sure though regarding this breaking the API or not. Perhaps we should deprecate PullPayment and create a PullPayment2? @frangio

@frangio
Copy link
Contributor

frangio commented Oct 24, 2019

After some discussion we feel it would be irresponsible to change the behavior of PullPayment in this way, and we think the best way forward is to deprecate the current withdraw functions and add alternatives which do not limit gas.

@nventuro nventuro mentioned this pull request Oct 28, 2019
Merged
@nventuro
Copy link
Contributor Author

nventuro commented Oct 28, 2019 •
edited
Loading

Superceded by #1976

@nventuro nventuro closed this Oct 28, 2019
@nventuro nventuro deleted the escrow-transfer-call branch October 28, 2019 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
contracts Smart contract code. on hold Put on hold for some reason that must be specified in a comment.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Have Escrow forward all gas to recipient
2 participants
@nventuro @frangio