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

Ethereum HTLCs revert and return error message #37

Merged
merged 11 commits into from Jan 21, 2020
Merged

Conversation

@bonomat
Copy link
Member

bonomat commented Jan 17, 2020

Resolves #8
Resolves #30

We decided to go for revert with a reason (return data).

  • the data returned by revert can be shown in etherscan
  • a transaction failed due to revert will be shown as failed in metamask
  • we live with the fact that a contract calling this HTLC can't recover from a failure within this contract
  • return a string message. A client should be able to transform the returned bytes into text now :)

Note: I also updated solc to 0.5.16.
Version 0.6.x has breaking changes and a simple upgrade did not work.

--> Follow-up ticket:
comit-network/RFCs#135

bonomat added 3 commits Jan 15, 2020
@bonomat bonomat requested review from D4nte and thomaseizinger Jan 17, 2020
@bonomat bonomat force-pushed the etherhtlc-reverts-and-logs branch from 22f02bc to 321df62 Jan 17, 2020
@bonomat bonomat force-pushed the etherhtlc-reverts-and-logs branch from 7072331 to 0fc95a2 Jan 17, 2020
Copy link
Member

D4nte left a comment

Does not resolves #8 as some follow-up is needed (open RFC issue).
Please remove the keyword and tick the boxes in the issues to ensure we don't forget it to be done.

Co-Authored-By: Franck Royer <franck@coblox.tech>
tests/rfc003_erc20_htlc.rs Outdated Show resolved Hide resolved
tests/rfc003_erc20_htlc.rs Outdated Show resolved Hide resolved
tests/rfc003_erc20_htlc.rs Show resolved Hide resolved
tests/rfc003_ether_htlc.rs Show resolved Hide resolved
@D4nte

This comment has been minimized.

Copy link
Member

D4nte commented Jan 19, 2020

Does not resolves #8 as some follow-up is needed (open RFC issue).
Please remove the keyword and tick the boxes in the issues to ensure we don't forget it to be done.

Remove the resolve and ping me for approval. Other comments are minors. It'd be good if you can try out (or tell me it won't work) my contract optimisation proposal.

tests/rfc003_erc20_htlc.rs Outdated Show resolved Hide resolved
bonomat added 2 commits Jan 19, 2020
@bonomat bonomat requested a review from D4nte Jan 19, 2020
Copy link
Member

thomaseizinger left a comment

Small nit, otherwise looks fine to me :)

src/ethereum/rfc003/mod.rs Outdated Show resolved Hide resolved
src/ethereum/rfc003/mod.rs Outdated Show resolved Hide resolved
tests/rfc003_erc20_htlc.rs Show resolved Hide resolved
@bonomat bonomat added no-mergify and removed no-mergify labels Jan 20, 2020
@bonomat bonomat force-pushed the etherhtlc-reverts-and-logs branch from 95255ba to c77f4c3 Jan 20, 2020
@bonomat bonomat mentioned this pull request Jan 20, 2020
Copy link
Member

D4nte left a comment

LGTM

@D4nte
D4nte approved these changes Jan 20, 2020
…ract.asm

Co-Authored-By: Franck Royer <franck@coblox.tech>
@mergify

This comment has been minimized.

Copy link

mergify bot commented Jan 20, 2020

bors r+

bors bot added a commit that referenced this pull request Jan 20, 2020
Merge #37
37: Ethereum HTLCs revert and return error message r=mergify[bot] a=bonomat

Resolves #8 
Resolves #30

We decided to go for `revert` with a reason (return data). 

* the data returned by `revert` can be shown in etherscan
* a transaction failed due to `revert` will be shown as `failed` in metamask 
* we live with the fact that a contract calling this HTLC can't recover from a failure within this contract
* return a string message. A client should be able to transform the returned bytes into text now :)

Note: I also updated solc to 0.5.16. 
Version 0.6.x has breaking changes and a simple upgrade did not work.

 --> Follow-up ticket: 
comit-network/RFCs#135

Co-authored-by: Philipp Hoenisch <philipp@hoenisch.at>
@mergify

This comment has been minimized.

Copy link

mergify bot commented Jan 21, 2020

bors r+

bors bot added a commit that referenced this pull request Jan 21, 2020
Merge #37
37: Ethereum HTLCs revert and return error message r=mergify[bot] a=bonomat

Resolves #8 
Resolves #30

We decided to go for `revert` with a reason (return data). 

* the data returned by `revert` can be shown in etherscan
* a transaction failed due to `revert` will be shown as `failed` in metamask 
* we live with the fact that a contract calling this HTLC can't recover from a failure within this contract
* return a string message. A client should be able to transform the returned bytes into text now :)

Note: I also updated solc to 0.5.16. 
Version 0.6.x has breaking changes and a simple upgrade did not work.

 --> Follow-up ticket: 
comit-network/RFCs#135

Co-authored-by: Philipp Hoenisch <philipp@hoenisch.at>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jan 21, 2020

Build succeeded

  • ci (1.39.0)
  • ci (stable)
@bors bors bot merged commit 57f98c4 into master Jan 21, 2020
8 checks passed
8 checks passed
automation
Details
ci (1.39.0)
Details
ci (stable)
Details
ci (beta)
Details
ci (nightly)
Details
Summary 2 rules match
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@mergify mergify bot deleted the etherhtlc-reverts-and-logs branch Jan 21, 2020
@D4nte D4nte mentioned this pull request Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.