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 ethatomicswap and Ethereum AtomicSwap smart contract #76

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

GlenDC
Copy link
Contributor

@GlenDC GlenDC commented Jun 22, 2018

Closes #74:

  • add a smart contract, written in Solidity, as to support AtomicSwaps to/from ETH
  • also includes README with instructions how to test and deploy
    the (Solidity) atomic swap contract using truffle
  • update travis.yml to run the unit tests of the smart contract mentioned earlier;
  • ethatomicswap (binary) tool to be used to actually do the swaps

Documented a full TFT-ETH (testnet) atomic swap walkthrough. You can find the documentation here: https://github.com/threefoldfoundation/tfchain/blob/master/doc/atomicswaps/defaultethatomicswap.md

As part of creating this example I made the first testnet TFT-ETH atomic swap:

GlenDC and others added 2 commits June 22, 2018 13:20
includes README with instructions how to test and deploy
the (Solidity) atomic swap contract using truffle
@GlenDC GlenDC mentioned this pull request Jun 22, 2018
includes the generate.go file which allows you to
generate the atomicswap.go file yourself, using the Solidity AtomicSwap source file
this allows the deployment (through Go) of the smart contract
a tool to support atomic swaps for Ethereum
+ clearer error messages (by pre-validating method requirements),
  it makes the commands slightly slower, but gives clearer errors
+ improved authentication flow:
  + (a) authenticate by signing from the CLI itself:
    give path to fileKey as a flag and enter securily the
    passphrase using the STDIN to decrypt that file
  + (b) or give a (by the daemon known) account address,
    as to use it to sign with that account from the daemon
  + (c) or give no account info at all, and use the first found account
    address instead (works best if only one account is known by the daemon)

Note: (b) and (c) is not as secure, as it relies that your daemon
      has the accounts unlocked, making it possible for anyone
      that can access its RPC endpoints to use your unlocked accounts
@GlenDC GlenDC changed the title [WIP] add smart contract for the Ethereum atomic swaps add smart contract for the Ethereum atomic swaps Jul 19, 2018
@GlenDC GlenDC changed the title add smart contract for the Ethereum atomic swaps add ethatomicswap and Ethereum AtomicSwap smart contract Jul 20, 2018
@jmozah
Copy link

jmozah commented Sep 1, 2018

@GlenDC The documentation link is not working.

@GlenDC
Copy link
Contributor Author

GlenDC commented Sep 1, 2018

@GlenDC The documentation link is not working.

Sorry @jmozah, this PR is very old. Still very eager to get his merged into master though.

Updated the link to the documentation for you. The document is now already living in master of our tfchain repo.

@jmozah
Copy link

jmozah commented Sep 4, 2018

Thanks @GlenDC. Any reason why this was not merged?

@GlenDC
Copy link
Contributor Author

GlenDC commented Sep 4, 2018

You should ask the @decred maintainers. I haven’t received any feedback on this PR. They and community seemed enthusiastic in the initiator issue #74, but haven’t heard anything from anyone since.

@jrick
Copy link
Member

jrick commented Sep 4, 2018

Nobody has tested or reviewed this, and I don't have the ability to do so without taking time off other projects to learn solidity.

@jrick
Copy link
Member

jrick commented Sep 4, 2018

Thinking about this some more, I feel that a more Linux-like approach would be good here, where individual maintainers all maintain their own subprojects in the tree rather than one single party being responsible for everything. This is already how the repo is structured, but we just haven't been very clear about maintainer responsibilities before.

In other words, all 3rd party tools are not "official Decred" tools but are contributions from the community and the Decred project is not able to take responsibility for each of them. We will still try to fix obvious problems where we can (like the security fix done by #59) but for radically different tools such as this one, we will have to defer to you as the maintainer.

Sound good?

@jmozah
Copy link

jmozah commented Sep 4, 2018

@jrick Thanks for your response. I came here to build something like this and i see this PR already done. I feel this is a important missing piece.
@GlenDC I can test and review this PR if you are ok.

README.md Outdated
@@ -19,6 +19,7 @@ exists for the following coins and wallets:
* Vertcoin ([Vertcoin Core](https://github.com/vertcoin/vertcoin))
* Viacoin ([Viacoin Core](https://github.com/viacoin/viacoin))
* Zcoin ([Zcoin Core](https://github.com/zcoinofficial/zcoin))
* Ethereum ([Go Ethereum](https://github.com/ethereum/go-ethereum))
Copy link
Member

Choose a reason for hiding this comment

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

Keep this list alphabetized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, didn't notice there was a specific order to it, will put it in its correct place.

@@ -0,0 +1,186 @@
// Copyright (c) 2017 Altcoin Exchange, Inc
// Copyright (c) 2018 The Decred developers and Contributors
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't have a Decred copyright, it's not our code. You're welcome to add your own personal copyright if this contains your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds fair, I'll replace decred with my organisation than. You are correct.

@@ -0,0 +1,1364 @@
// Copyright (c) 2018 The Decred developers and Contributors
Copy link
Member

Choose a reason for hiding this comment

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

Remove "and Contributors" and add additional copyright below for yourself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You do want the decred developers here? All good for me though, just want to be clear where I do need to add decred copyright, if in any ethatomicswap related file at all.

Copy link
Member

Choose a reason for hiding this comment

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

For this file, yes. It includes code copied from the other tools and must retain the Decred developers copyright.


"github.com/bgentry/speakeasy"

ethereum "github.com/ethereum/go-ethereum"
Copy link
Member

Choose a reason for hiding this comment

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

I don't like these imports, as these packages, used as libraries, are under a LGPL license. Go will statically link against these, essentially turning the entire project GPL (it would be a GPL violation to make and distribute proprietary changes to this tool without following the terms of the GPL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. Replacing speakeasy should be simple enough, we can even do without a library for it perhaps.

Replacing go-ethereum is going to be trickier, and not really something I had planned to do. Will need to to think and plan what to do about this.

Copy link
Member

Choose a reason for hiding this comment

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

Speakeasy was never a problem, it's MIT/Apache2 licensed. go-ethereum was the one that made me raise a few eyebrows (LGPL licensed Go packages? really?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is your last remaining issue with this PR. For now I cannot however think of a solution that doesn't require me to reimplement all logic again.

Given that all sub projects are separate binaries, and given that you don't even make releases with prebuilt releases. Does it really matter that this binary becomes LGPL? Does it really affect the other binaries? You also don't embed the vendor (go-ethereum) code.

I'm not a legal person though, so perhaps it does have to impact the entire project. But as I said, there is not really a non-official go ethereum package that we could use as a replacement for the official one. I don't think anyone has ever bothered in providing that, and it seems a bit out of scope for me to start doing that.

If it really bothers you feel free to make suggestions, as I really have no clue how to make you happy with this one.

Copy link
Member

Choose a reason for hiding this comment

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

It wouldn't affect any of the other tools, but the issue is that this repo is currently entirely liberally licensed and all imports are as well. This is stated at the bottom of the project's readme. If these tools are included in the repo, then it will no longer be the case where you can clone, build, and freely distribute the ethatomicswap binary without complying with the additional rules of the LGPL.

I do agree with you that rewriting libraries is out of scope for providing this tool.

Would you be ok with supporting the code in a different repository, with a description of the licensing issues associated with the tool? We can link it in this project's readme like we already do for ThreeFold.

Copy link
Member

Choose a reason for hiding this comment

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

There was some project that was hoping to do an Apache licensed rewrite of ethereum but I think that is a long way off.

I think non-decred hosting of any tool that can't be done ISC (with preference for ISC work of course) is a great way to take care of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand. Our ethatomicswap lives in https://github.com/threefoldtech/atomicswap/tree/master/cmd/ethatomicswap so perhaps I could close this PR, and instead make a PR to link to ethatomicswap. We have some documentation and documented walk-through example as well somewhere, will add it to this repo as well.


I hope the community will find their way to ethatomicswap this way though. We also already support BTC electrum wallets for atomic-swaps, planning to give such support to ethereum in the future as well.

@GlenDC
Copy link
Contributor Author

GlenDC commented Sep 4, 2018

Thinking about this some more, I feel that a more Linux-like approach would be good here, where individual maintainers all maintain their own subprojects in the tree rather than one single party being responsible for everything.

Sounds like a sensible approach indeed. I wouldn't mind being the maintainer for ethatomicswap for the time being. I'm not sure how you want to do it. Do you want to grant me also access to the repo so I can merge changes for the subprojects I own, or how would you want to do that @jrick?

Will take your feedback into account later this week @jrick, for now I'm recovering and on sick holiday because of it.

@GlenDC I can test and review this PR if you are ok.

Of course @jmozah, please do test and review as much as you can and want. The more testing and the more feedback, the better.

@jrick
Copy link
Member

jrick commented Sep 4, 2018

I'm unsure how it would work logistically (and may depend on how frequently you wish to make changes), but even without direct write access, I would gladly pull any changes you have if they seem reasonable.

@jmozah
Copy link

jmozah commented Sep 10, 2018

FIXED: I had to run geth with -rpc

@GlenDC I took your 'ethatomicswap' branch from which you issued this PR

I am trying to do an atomicswap between BTC and ETH.

  1. Initiate from btc worked fine.
  2. auditcontract worked fine
  3. Participate from eth shows this error.
ethatomicswap -testnet -account 0xc33c7453fb06c9b19ea4eca55cf29f09214958f6  -s http://localhost:8545 participate 0xc33c7453fb06c9b19ea4eca55cf29f09214958f6 1.0 0b8aeb23474fecd2ace555a82bf93ee5bc792458b41e019322b3489793e5b09d

failed to create participate TX: unexpected error while checking for an existing contract: failed to get swap contract from smart contract (at 2661cbaa149721f7c5fab3fa88c1ea564a683631): abi: unmarshalling empty output

@GlenDC
Copy link
Contributor Author

GlenDC commented Nov 7, 2018

Happy you fixed it @jmozah. We do use it ourselves as well, and so far haven't found anything weird. Still, Would be more than happy to start taking contributions and feedback in any way.

@dajohi
Copy link
Member

dajohi commented Feb 20, 2019

@GlenDC please rebase now that go.11+ is required.

@GlenDC
Copy link
Contributor Author

GlenDC commented Feb 20, 2019

@GlenDC please rebase now that go.11+ is required.

Sure I don't mind @dajohi, are there still plans than to in the end merge this into the main branch? or do you mean that we should in our own repo require/update-to Go.11+ and ensure it works against it?

@xaur
Copy link

xaur commented Apr 4, 2021

Sad to see this stuck (why?) but there's a chance some of this work will live in DCRDEX, see the discussion of swap contracts at decred/dcrdex#1001 and the open PRs implementing those.

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

Successfully merging this pull request may close these issues.

Adding Ethereum support
6 participants