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

Bisq Monero Wallet #110

Open
niyid opened this issue Aug 23, 2019 · 36 comments

Comments

@niyid
Copy link

commented Aug 23, 2019

This is a Bisq Network proposal. Please familiarize yourself with the submission and review process.

CC: @ManfredKarrer, @HarryMacfinned, @ripcurlx

Dear All,

This proposal is related to:

Taking it forward and as suggested by @ManfredKarrer, the user will be responsible for running monerod locally or setting up a remote Monero node via Tor (or any remote Monero node for that matter via Tor). Of course the user will be able to either select a local Monero node (monerod running on localhost) or a remote Monero node via Tor in User Preferences.

The wallet will be accessible at Account > Wallets > Monero. The Wallets screen will allow future support for other wallets.

The Monero wallet will be based on the monero-java-rpc library and support basic functionality as follows:

  • Create (or connect to) a wallet that uses a monero-wallet-rpc endpoint with authentication
  • Get wallet balance as BigInteger/long (for consistency in Bisq use long)
  • Get wallet primary address
  • Get address and balance of sub0-address [1, 0]
  • Get incoming and outgoing transfers
  • Get incoming transfers to account 0
  • Send to an address from account 0
  • Get all confirmed wallet transactions
  • Send to multiple destinations from multiple sub-addresses in account 1 which can be split into multiple transactions
  • Get a wallet transaction by id

Non-basic functionality and integration into Bisq are:

  • Automatically send/receive Monero from/to the wallet to in trading involving Monero.
  • Proof verification for Monero transactions.

ADDENDUM: screenshots with latest build below.

SCREENSHOTS - LATEST MERGED BUILD

Screenshot from 2019-09-18 06-59-35
Screenshot from 2019-09-18 07-00-13
Screenshot from 2019-09-18 07-00-35
Screenshot from 2019-09-18 07-01-05
Screenshot from 2019-09-18 07-01-34

@erciccione

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

Pinging @woodser (maintainer of Monero-java) and @rbrunner7, Monero dev who already explored the possibility to integrate monero into Bisq in past. Do you have any opinion/suggestion about the integration?

@rbrunner7

This comment has been minimized.

Copy link

commented Aug 27, 2019

Well, splendid if somebody starts to work on this in earnest! All the best wishes.

About the architecture of the integration: I think the potential for variations and choices is small. It's simply not possible with a reasonable effort to make a Monero integration into software like Bisq as slick and as seamless as their Bitcoin integration, which needs neither daemon (because it itself can connect directly to the network) nor any dedicated wallet program (because wallet can get fully integrated as well).

What remains and what is feasible is a setup with Monero daemon (local or remote) plus Monero wallet through RPC and the monero-wallet-rpc binary. This is what I currently implement for OpenBazaar, just in Go instead of Java like here. It's not very elegant for the user, but it works, and the integration code itself tends to stay refreshingly small.

@woodser

This comment has been minimized.

Copy link

commented Aug 27, 2019

The Java library supports most (all?) of the functionality listed in this issue so I expect most of the work will be in integrating the library into Bisq's current framework.

I'd recommend using the native Java binding to Monero's wallet now that it's available instead of shipping and starting monero-wallet-rpc as part of the Bisq application which might simplify things some.

Is multisig intended to be integrated? The library supports multisig but coordinating and exchanging multisig information among participants is outside of the scope of the library, which @rbrunner7's mms may be useful for (can/should something similar be implemented in Java? Would that help?). I expect some of the challenge will be in UX while the multsig wallet is synced among participants during creation and transacting.

I think a daemon should be shipped with Bisq but the user should be able to select their own daemon by URL.

This issue references proving transactions which I think getTxKey()/checkTxKey(), getTxProof()/checkTxProof or getSpendProof()/checkSpendProof() provides the functionality for.

Also, it is possible to register listeners to wallet events like when funds are sent or received if some action should automatically be done then.

@rbrunner7

This comment has been minimized.

Copy link

commented Aug 28, 2019

I'd recommend using the native Java binding to Monero's wallet now that it's available instead of shipping and starting monero-wallet-rpc as part of the Bisq application which might simplify things some.

I am curious about the details how that would work on a technical level, because right now I have difficulties to see it clearly: Would that mean to link a large part of Monero's code, compiled from C++, into the Bisq executable and call into the wallet2 class directly from Java code? Are things compatible on such a low binary level, with calling conventions, data types, stack, heap, etc.?

Or would you turn those large parts of Monero's C++ code into a dynamic library and call into that from Bisq's executable?

Or are my assumptions how that "native Java binding to Monero's wallet" works on a technical level simply off the mark by a wide margin?

Curious former 6502 assembly programmer wants to know :)

@niyid

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

@rbrunner7 @woodser @erciccione

I have been working on this for a couple of weeks now and made good progress with monero-wallet-rpc. Still early days though as I have a few more weeks to confidently announce completion.

I will also look at the alternative suggested - Monero Native Java bindings - once I am comfortable that the monero-wallet-rpc alternative is good enough to ship.

@woodser

This comment has been minimized.

Copy link

commented Aug 28, 2019

@rbrunner7 It is the latter of what you described: Monero's C++ code is compiled to a dynamic library which is specific to the platform it is running on. Java then uses that dynamic library to make native function calls to C++ using JNI to handle the mappings between languages.

Running monero-wallet-rpc is not technically that different; it is running C++ code which wraps core wallet logic but which has been compiled as a standalone executable instead of a dynamic library for other libraries to use. The difference of course is that monero-wallet-rpc must be launched as a server in the background and all communication must go through its port. Not sure how much of a burden that is, but I do hope it is all transparent to the user.

For comparison, the monero-wallet-rpc executable is 24.4 MBs on my system while the libmonero-cpp.dylib (the dynamic library built from C++ containing wallet2) is 24.9 MBs (similar because they wrap the same code).

@niyid The RPC and JNI wallet implementations conform to the same interfaces, so it should be easy to switch from one implementation to the other if you decide to.

@rbrunner7

This comment has been minimized.

Copy link

commented Aug 28, 2019

@woodser : Thanks for this interesting info.

Asthonishing that something like that can be made to work reliably. If it really does: I for one would probably always produce nagging questions in my mind, in a low "voice", questions like "Well, do callbacks really work?" or "Do threads and atomics and stuff still work as intended?" or "Will JNI cope if a C++ compiler update starts to produce a slightly different memory layout for some structs?" and so on.

But I guess the JNI creators and maintainers made and still make all this work in some rather monumental effort :)

@woodser

This comment has been minimized.

Copy link

commented Aug 28, 2019

Yeah I guess that was the challenge that the designers of JNI were confronted with. Importantly it's still running as native C++, so callbacks, threads, etc work exactly as they should. JNI just provides a layer on top to hook into and interact with the C++ execution.

Also worth noting that the library does use callbacks and threads, which are tested.

@niyid

This comment has been minimized.

Copy link
Author

commented Aug 28, 2019

@niyid The RPC and JNI wallet implementations conform to the same interfaces, so it should be easy to switch from one implementation to the other if you decide to.

@woodser Thanks for the information. It should make things a lot easier.

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 9, 2019

Hello All,

I need to get this clear: it appears to me that with the automated XMR trading that this provides, there will be no need for arbitration (as the funds get deposited directly from one XMR address to another). Is this a correct assessment?

@chimp1984

This comment has been minimized.

Copy link

commented Sep 10, 2019

Please be aware of some plans: #118

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

Please be aware of some plans: #118

Thanks for this piece of information. Since the 2 models will be maintained for a while, I do not believe the plans for the new arbitration model have a near term significant impact on this proposal. Are there any dissenting impact assessments?

@sqrrm

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@niyid I don't see why this proposal would rid us of the need for arbitrators. The transaction process for xmr trades would be smoother but there are no guarantees. You still need to handle the case of one side not sending the coin, either on purpose or due to technical issues.

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

@niyid I don't see why this proposal would rid us of the need for arbitrators. The transaction process for xmr trades would be smoother but there are no guarantees. You still need to handle the case of one side not sending the coin, either on purpose or due to technical issues.

@sqrrm Indeed after earlier posting and giving further thought, I reached the same conclusion as you did. Thanks for the feedback.

@chimp1984

This comment has been minimized.

Copy link

commented Sep 11, 2019

@niyid The off chain trade protocol (Bisq v2) app will not have a trade wallet (BTC) and also should not start to add other wallets. We want to reduce security risks. People should use any wallet for sending what thye like, the trade protocol is transparent to the currency.

@erciccione

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

@niyid Could you add an edit or something else when you make an important change to a post? I just noticed you added some screenshots in the OP, but we don't get any notification when something like that happens. Adding a short comment would ping everyone who contributed to the discussion, letting them know that you made a change. That would make your development progresses easier to follow :)

Also worth pointing out that @niyid opened a PR to implement the initial integration of the Bisq Monero wallet and it's waiting for reviews: bisq-network/bisq#3275

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 17, 2019

@wiz

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Why not simply launch an external app using monero: links, similar to how Bisq currently supports opening bitcoin: links? Then you can add any Monero wallet you want and have your OS open your wallet app of choice, without the trouble of merging code into Bisq.

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 20, 2019

@wiz

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

I understand what you want to do, but it's very ambitious to add this amount of new functionality into Bisq, and after looking at your PR a bit I think it will be very difficult, if not impossible, to actually get it merged. Like I mentioned before, another PR to add simple API functionality to Bisq is still being discussed for 2 years now, and you are proposing to do much more than that.

Also considering the current direction of removing the Bitcoin trade wallet from Bisq, as part of the new Bisq v2 trade protocol, I doubt the Monero trade wallet will get added, which is why I recommended to simply have Bisq call external bitcoin: and monero: and other alt-coin URI handlers from the OS level, which will allow the user to use Trezor or other HW wallets, bitcoind, monerod, etc. whatever they want.

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 20, 2019

I understand what you want to do, but it's very ambitious to add this amount of new functionality into Bisq, and after looking at your PR a bit I think it will be very difficult, if not impossible, to actually get it merged. Like I mentioned before, another PR to add simple API functionality to Bisq is still being discussed for 2 years now, and you are proposing to do much more than that.

Also considering the current direction of removing the Bitcoin trade wallet from Bisq, as part of the new Bisq v2 trade protocol, I doubt the Monero trade wallet will get added, which is why I recommended to simply have Bisq call external bitcoin: and monero: and other alt-coin URI handlers from the OS level, which will allow the user to use Trezor or other HW wallets, bitcoind, monerod, etc. whatever they want.

@wiz
Thanks for the suggestions. I am aware of the "current direction" referred to as Bisq v2 Trade protocol which I am aware is still in the very early stages and has not been cemented yet (this after discussing with a few developers in the know).

One thing I am very well aware of though is that software is made to please the market and not imposed on the market. That is one of the primary reasons that software fail.

The point here is, the work I am doing is to satisfy requests made by a substantial market population represented by over 90% by trade volume - Monero traders. When you have this overwhelming volume, it is a pointer to the fact that the users that fall in this group have a bias (for what ever reason) for Bisq and you risk losing them by not doing their bidding.

I am not working to satisfy my personal requirement; the purpose is to satisfy practically the population of the market who should not be disregarded. If they are not satisfied, they will find other software to suit their needs.

Regards.

@wiz

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

I'm not saying the functionality you want shouldn't be integrated with Bisq. I'm saying that instead of directly adding a Monero wallet into Bisq, it makes much more sense to add the functionality you want using an API, and keeping the applications separate. This also allows Bisq to be integrated with countless other alt-coin wallets directly using the same API calls, so it can benefit all alt-coins and not just Monero.

My suggestion is to take the 10 or so specific things you want to do (listed in the first comment of this PR) and turn them into a generic API specification. That API spec can be implemented on top of the current API that's being added in PR #3001, and you could either modify monerod to directly support this API, and/or create some middleware to allow the 2 applications to interoperate together seamlessly.

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 21, 2019

I'm not saying the functionality you want shouldn't be integrated with Bisq. I'm saying that instead of directly adding a Monero wallet into Bisq, it makes much more sense to add the functionality you want using an API, and keeping the applications separate. This also allows Bisq to be integrated with countless other alt-coin wallets directly using the same API calls, so it can benefit all alt-coins and not just Monero.

My suggestion is to take the 10 or so specific things you want to do (listed in the first comment of this PR) and turn them into a generic API specification. That API spec can be implemented on top of the current API that's being added in PR #3001, and you could either modify monerod to directly support this API, and/or create some middleware to allow the 2 applications to interoperate together seamlessly.

Great suggestion.

But I am not sure how this will work given that I have no control over monerod other than to create a fork that will force all Bisq users to use my specific fork of monerod. And your suggestion will have significantly higher cost in terms of spread of required technical skills - knowledge of Java, C/C++, JNI and more I have not even taken note of yet.

A more important question is: is this what the (vast number of) users want? The design you suggest from an architectural point of view makes sense to allow anything-for-anything trades. But as they say, "a bird in hand beats 2 in the bush." After you satisfy the vast majority of users (who trade mostly Monero), you can then start fishing for new users with bias for other currencies. There is no guarantee this new architecture will draw more users.

I suppose you have a timeline for the completion of v2 Trade Protocol?

@erciccione

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

I agree with @niyid. Bisq should have as priority to have an integrated Monero wallet. They are right when they say that software should be made to please the market. The vast majority of the people on Bisq trade mostly Monero, but right now they have to pass through BTC, which is really bad UX and should be fixed as soon as possible, an integrated Monero wallet is the long term solution.

so it can benefit all alt-coins and not just Monero

@wiz don't think we should care too much about other coins when the vast majority of the people trade Monero and right now they don't have a clean way to echange it.

I find a bit discouraging that a ready PR is being pushed away because complex and extensive to review, especially when has been worked on for quite some time. The fact that another extensive PR has been opened for two years without reviews sounds more like a big problem to me and i don't think shouldn't be used as a referral of the generic workflow.

Let's keep in mind that if another decentralized exchange with a better Monero integration appears, it's safe to assume XMR trader would move there, with the result of Bisq losing ~97% of its users. Would be a catastrophe and could potentially kill Bisq. Making trades for XMR users as smooth as possible should be a priority.

In short, my opinion is: Let's brake this integration in chunks easier to review and let's ask monero devs to help with the reviews (i could set a bounty for this), but let's have a Monero wallet integrated into bisq, because it is a priority and worth the added complexity.

@rbrunner7

This comment has been minimized.

Copy link

commented Sep 21, 2019

@erciccione , agree with most you say in your latest post. But:

The vast majority of the people on Bisq trade mostly Monero, but right now they have to pass through BTC, which is really bad UX and should be fixed as soon as possible, an integrated Monero wallet is the long term solution.

In my understanding this Monero wallet here, once finished and put into service, will not allow to completely bypass BTC. That would only be possible with XMR becoming a Bisq base currency which it does not with this wallet. See e.g. chapter XMR as Base Currency in my earlier analysis of the situation.

@erciccione

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

@rbrunner7 I think i explained myself poorly. When i talk about "a long term solution" i mean that an integrated wallet will be a much stronger starting point for a future deeper Monero integration (like using it as base currency) than a simple generic API.

@chimp1984

This comment has been minimized.

Copy link

commented Sep 21, 2019

Hi guys,
I just follow loosely the discussions here. Just want to chime in to make it a bit more understandable why there is quite some defensive reactions from a few Bisq devs for an idea which was pushed by Bisq devs earlier.

Over time it became more clear that having a wallet in Bisq (which is required for the current trade protocol due the multisig tx) is a liability and carries security risks. With the new off-chain trade protocol idea we have a much more future-proof solution in reach which makes that requirement for integrating a wallet obsolete. Wallet software which is directly integrated with a p2p network app carries higher risks as pure dedicated wallets - this is just simple engineering wisdom - the more you try to do the more risks and vulnerabilities you have to deal with. Also doing one thing only delivers better results than trying to do all at once.
Adding now Monero wallet integration extends that vulnerability surface. So however we proceed with that proposal, we should keep 2 things in mind:

  • There is the off-chain trade protocol which will be a huge game changer in many aspects (then you can trade XMR to any fiat as well)
  • Security is the highest priority in Bisq.

This focus on security does touch every development aspect and specially dependencies added to Bisq. Auditing an external depenency costs a lot of effort and money (BSQ). The DAO stakeholders are the management body of Bisq and need to be wise where to invest their resources.

To avoid stagnation and frustration I suggest to develop further the idea of incubator projects as a compromise and allow new riskier projects to develop in a risk limited environment where they also can proof if there is real user demand for that idea.

@erciccione

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

Thank you for chiming in @chimp1984, now i have the situation much more clear, but i see the possibility of stagnation very real.

Bisq is moving away from integrated wallets because of the new trade protocol and i think it's cool, (and make sense to not have a "full" Monero wallet integrated if we are stirring away from base currencies anyway) but what worries me is the timeline. How long do you guys think will take before the new protocol goes live?
If it's still in discussion and a broad timeline is not set, would make sense to give XMR trader a better integration in the meantime.

The incubator could help, but needs to be voted in the DAO and then developed. Correct me if i'm wrong but i think this will require a decent amount of time as well.

We have a ready proposal here, i understand the pushbacks and the reasons behind them, but i think waiting for the incubator and then for the new off-chain protocol for having a better xmr UX is not feasible. So the questions i think we should answer are: how can we have a better Monero integration without stirring too much away from the long term goals (off-chain protocol)? Would @wiz's suggestion be the best solution in this case? (since it only requires the PR with the API to be merged AFAIU), or should the integration proposed by @niyid be further trimmed, to reduce the amount of dependencies and therefore security risks (if that's possible)?

@chimp1984

This comment has been minimized.

Copy link

commented Sep 21, 2019

How long do you guys think will take before the new protocol goes live?

Depends on contributors. The core concept requires more work and analysis. Atm nobody is focussed on that because of other dev priorities. I think in the best case in 3-6 months it could be deployed. More likely with currnet dev power 6-12 months.

waiting for the incubator

This very proposal and the related PR can be implemented as incubator project. Only difference is that it is not merged into the official Bisq app and in case of security flaws it would not affect 100% of Bisq users. As the incubator idea is new it would require a bit of community discussion and consensus first though. But that could be completed in short time if there is interest in it and someone pushes that forward (I will not have time for that).

My suggestion is to run it as incubator project.

  • Get the dependency issue resolved to a reasonable level.
  • Take into account there is left some risks and make this very clear to users (popup warning,...).
  • Get it first deployed as compile-and-run only version to limit to developers and technical minded people as those are better with interpreting and dealing with the potential risks.
  • Meassure usage.
  • If usage and stability is good, go on with further audits, improvements and start deploying as incubator binary.
  • If all develops further well and it gets community consensus get it integrated in the main app.

Just my 5 cents and as said I did not follow all discussions and don't have time to take further part of discussion here.

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 21, 2019

@battleofwizards

This comment has been minimized.

Copy link

commented Sep 21, 2019

So the questions i think we should answer are: how can we have a better Monero integration without stirring too much away from the long term goals (off-chain protocol)?

This seems like a much lower hanging fruit: #86

@niyid

This comment has been minimized.

Copy link
Author

commented Sep 21, 2019

In short, my opinion is: Let's break this integration in chunks easier to review and let's ask monero devs to help with the reviews (i could set a bounty for this), but let's have a Monero wallet integrated into bisq, because it is a priority and worth the added complexity.

@erciccione
I earlier missed this. How can I help with this?

So the questions i think we should answer are: how can we have a better Monero integration without stirring too much away from the long term goals (off-chain protocol)?

This seems like a much lower hanging fruit: #86

This is already part of the PR.

@wiz

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

@niyid To put things another way, it seems like there is consensus for integrating the Monero functionality you want into Bisq, but not in the way you want to do it. I don't think the PR to directly integrate Monero into Bisq will get approved/merged, but we could likely add the API methods you need to indirectly integrate the Monero functionality you want into Bisq. This way, you can easily make some middleware to glue together Bisq and Monero, and the end result would be the same. Would this be an acceptable compromise for everyone?

@binaryFate

This comment has been minimized.

Copy link

commented Sep 28, 2019

I agree with @niyid. Bisq should have as priority to have an integrated Monero wallet. They are right when they say that software should be made to please the market.

I agree too. If it turns out the functionality as implemented by @niyid indeed answers a real market need, but it never get merged into Bisq for Bisq-centric reasons (even if justified from Bisq perspective), it is also possible to simply fork and explore two different paths further.
Monero users would likely be happy to go and run their own version of the project at some point if it keeps diverging too much from the tool they want and need. Finding trusted arbitrators would not be too hard.

@wiz

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

@binaryFate Yeah so again, instead of the PR submitted by @niyid that proposes to directly add a Monero wallet into Bisq (which seems to lack consensus), I think you guys (on behalf of the Monero community) should propose to create a generic API specification that could be implemented into Bisq to allow the 2 applications bisqd and monerod to work together. How about we create an API specification to implement the functions you want to implement:

  • Create (or connect to) a wallet that uses a monero-wallet-rpc endpoint with authentication
  • Get wallet balance as BigInteger/long (for consistency in Bisq use long)
  • Get wallet primary address
  • Get address and balance of sub0-address [1, 0]
  • Get incoming and outgoing transfers
  • Get incoming transfers to account 0
  • Send to an address from account 0
  • Get all confirmed wallet transactions
  • Send to multiple destinations from multiple sub-addresses in account 1 which can be split into multiple transactions
  • Get a wallet transaction by id
  • Automatically send/receive Monero from/to the wallet to in trading involving Monero.
  • Proof verification for Monero transactions.

This method is better for multiple reasons (security, maintainability, etc.), and the same API spec would also enable other crypto-coins to integrate with Bisq in a similar fashion.

And actually it seems Bisq needs this API regardless, because the Bisq v2 trading protocol plans to remove the Bitcoin trade wallet from Bisq, so we would need to integrate with the user's bitcoind or whatever wallet software are using somehow anyway.

@chimp1984

This comment has been minimized.

Copy link

commented Sep 29, 2019

@binaryFate

Bisq for Bisq-centric reasons

I think you misinterpret the situation and you show a lack of sense for security aspects. Bisq is not a normal wallet but an application with a p2p network. This comes with different security aspects as a standard wallet. Over the past months those concerns have become more aware under Bisq devs and the initially bullish push to integrate with monerod is seen in a bit different light now.

from the tool they want and need

I would not be so sure that many Monero users - who are often very technical minded and more on the "paranoid" side of things - would use an external application connecting to their monerod. I would not use it as I would not trust it without a profound audit... So lets proof in an incubator project that there is real demand for the feature and start a risky project in a risk limited environment.

See: #122 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.