-
Notifications
You must be signed in to change notification settings - Fork 231
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
base: master
Are you sure you want to change the base?
Conversation
includes README with instructions how to test and deploy the (Solidity) atomic swap contract using truffle
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
and added some more code comments as well
@GlenDC The documentation link is not working. |
Thanks @GlenDC. Any reason why this was not merged? |
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. |
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? |
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this list alphabetized
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
cmd/ethatomicswap/main.go
Outdated
@@ -0,0 +1,1364 @@ | |||
// Copyright (c) 2018 The Decred developers and Contributors |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Sounds like a sensible approach indeed. I wouldn't mind being the maintainer for Will take your feedback into account later this week @jrick, for now I'm recovering and on sick holiday because of it.
Of course @jmozah, please do test and review as much as you can and want. The more testing and the more feedback, the better. |
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. |
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.
|
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. |
@GlenDC please rebase now that go.11+ is required. |
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. |
Closes #74:
the (Solidity) atomic swap contract using truffle
travis.yml
to run the unit tests of the smart contract mentioned earlier;ethatomicswap
(binary) tool to be used to actually do the swapsDocumented 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: