Fulfilling Proposal #110 #3275

niyid · 2019-09-16T20:39:17Z

This is to initiate the update necessary for the fulfillment of proposal
bisq-network/proposals#110:

Fully functional and basic Monero (XMR) wallet integrated to Monero RPC Wallet
running on localhost with the following features:

Viewing balance (total and unlocked).
Sending XMR.
Viewing XMR Primary address to receive XMR.
Viewing and searching transactions.
Transaction/payment proof/verification (almost certified complete).

TODOs:

Completion of automation of Monero related trades (integration to
wallet).
Unit Tests (first order - XmrFormatter and XmrValidator).
3.Payment proof testing completion.

bisq-network#110: Fully functional and basic Monero (XMR) wallet integrated to Monero RPC Wallet running on localhost with the following features: 1. Viewing balance (total and unlocked). 2. Sending XMR. 3. Viewing XMR Primary address to receive XMR. 4. Viewing and searching transactions. 5. Transaction/payment proof/verification (almost certified complete). TODOs: 1. Completion of automation of Monero related trades (integration to wallet). 2. Unit Tests (first order - XmrFormatter and XmrValidator). 3.Payment proof testing completion.

wiz · 2019-09-16T23:57:10Z

@niyid Before reviewing, would you mind squashing all the "cleaning up" commits into a single commit, and also fixing the merge conflicts?

build.gradle

woodser · 2019-09-17T16:17:50Z

@niyid Thanks for your effort. I noticed an outdated version of the Monero Java library is being used, v0.0.2. I recommend updating to v0.1.0 which includes a lot of clean up work and does make some breaking changes to the API. Specifically, createTx() and send() now return a MoneroTxSet which includes the individual transactions.

niyid · 2019-09-17T16:22:01Z

@niyid Thanks for your effort. I noticed an outdated version of the Monero Java library is being used, v0.0.2. I recommend updating to v0.1.0 which includes a lot of clean up work and does make some breaking changes to the API. Specifically, createTx() and send() now return a MoneroTxSet which includes the individual transactions.

@woodser
Great. I will make that happen ASAP. Thanks a lot.

Edit:
@woodser
I checked the current POM again here https://github.com/monero-ecosystem/monero-java/blob/master/pom.xml and I see it reports v0.0.2. Something may be missing somewhere. Even the v0.1.3 branch https://github.com/monero-ecosystem/monero-java/blob/v0.13.0/pom.xml has this: <version>0.0.2-SNAPSHOT</version>.

I will try updating to <version>0.1.0-SNAPSHOT</version>

Please ignore earlier suggestion on conversion to Gradle. I found the solution. Apologies on that.

niyid · 2019-09-17T16:40:01Z

@ripcurlx @sqrrm

The conficts I am seeing below seem to be transient or recently occurring. For instance in the case of bisq.desktop.main.account.AccountView refer to the screenshots below: one screen shows the same class file in Green in the PR diff page while the "Resolve conflicts" button shows recent addition of package imports.

erciccione · 2019-09-17T17:59:21Z

@niyid Yes, those conflicts happened because your PR touches files that have been changed with the recent merges.
My suggestion is to use git pull upstream master --rebase, so that you will be able to manually fix all conflicts.

woodser · 2019-09-17T18:04:39Z

@niyid

any chance of transforming (from Maven) to a Gradle project?

Is it only necessary to add a Gradle project file to be usable as a Gradle project? I don't mind such a file being added. Do you mind providing what you are looking for? Otherwise I can look into it.

I checked the current POM again here https://github.com/monero-ecosystem/monero-java/blob/master/pom.xml and I see it reports v0.0.2. Something may be missing somewhere.

Thanks for pointing that out. I forgot to bump the version in the POM. I will release an update shortly which does that.

niyid · 2019-09-17T18:11:17Z

@niyid

any chance of transforming (from Maven) to a Gradle project?

Is it only necessary to add a Gradle project file to be usable as a Gradle project? I don't mind such a file being added. Do you mind providing what you are looking for? Otherwise I can look into it.

@woodser
I am trying to get around the issue of having to add the Maven project as a JAR dependency to the Bisq Gradle project; which is about the best workaround I could find.

For instance, see the issue below as the Maven project was not automatically built into a JAR:

woodser · 2019-09-17T18:24:07Z

@niyid I updated the POM version and tagged a new release.

niyid · 2019-09-17T18:31:35Z

@niyid I updated the POM version and tagged a new release.

@woodser Splendid. Thanks a lot.

# Conflicts: # common/src/main/proto/pb.proto # core/src/main/resources/i18n/displayStrings.properties # desktop/src/main/java/bisq/desktop/main/account/AccountView.java # desktop/src/main/java/bisq/desktop/main/settings/preferences/PreferencesView.java # desktop/src/test/java/bisq/desktop/GuiceSetupTest.java

niyid · 2019-09-17T18:42:57Z

@niyid Yes, those conflicts happened because your PR touches files that have been changed with the recent merges.
My suggestion is to use git pull upstream master --rebase, so that you will be able to manually fix all conflicts.

@woodser Thanks for the hint. Now no conflicts; but it won't build because the monero-wallet-rpc Maven project first needs to be built and added to the Local Maven Repository. I will have to look for a way to automate this.

battleofwizards · 2019-09-17T20:45:22Z

Hi @niyid, thanks a lot for working on this!

My fear is that this great effort may stuck in limbo simply because of the sheer volume of changes.

Of course, this is inherently a big feature. However, there is virtually always a way to split into smaller, thinner "features", even if they are artificial. As a side note, in modern software engineering, individual pull requests should be pieces of work on the order of hour(s) to 2-3 days max. In no case many weeks of active coding should be merged at once.

niyid · 2019-09-17T20:50:54Z

Hi @niyid, thanks a lot for working on this!

My fear is that this great effort may stuck in limbo simply because of the sheer volume of changes.

Of course, this is inherently a big feature. However, there is virtually always a way to split into smaller, thinner "features", even if they are artificial. In modern software engineering, individual pull requests should be pieces of work on the order of hour(s) to 2-3 days max. In no case many weeks of active coding should be merged at once.

Thanks @battleofwizards

I acknowledge the volume of work that has been done. But I had to also separate the delivery to units otherwise I would still not have done a PR yet. This PR is simply the Monero Wallet UI without any integration to Bisq trades. That is the next unit (integration to trades) I am delivering hopefully before the end of this cycle.

Also, the only thing keeping this in limbo right now is the monero-wallet-rpc library which I am working on to play nicely with Gradle (as it is on Maven). The project has been completely merged and rebased and good to go. I have built on my workspace and successfully launched the application with no issues.

Once again, thanks for the nice words.

Regards.

woodser · 2019-09-17T20:51:09Z

build.gradle

@@ -82,7 +82,7 @@ configure(subprojects) {

    dependencies {
        testCompile "junit:junit:$junitVersion"
-        compile 'woodser:monero-wallet-java:0.0.2-SNAPSHOT'
+        compile 'woodser:monero-wallet-java:0.1.1-SNAPSHOT'


Probably rename to woodser:monero-java:0.1.1-SNAPSHOT

Instead of adding the new jar dependency, it would be better to simply add this code to Bisq so it can be reviewed together with this PR.

@wiz

That is a pragmatic suggestion. But I actually restrained myself from doing just that. I am exploring what I had earlier missed: the use of the apply plugin: 'maven' directive.

niyid · 2019-09-18T18:24:53Z

@battleofwizards @woodser

There is no headache. We all agree to stick with best software development practices.

I will update the PR over a 2/3 day period
that will feature the barest necessities of bridging to monero-wallet-rpc and you should expect a massive reduction in the code.

Apologies for the inconvenience.

Regards.

woodser · 2019-09-18T18:55:07Z

@battleofwizards Yes, I will release a lite version shortly with the minimum necessary for integration with Bisq.

woodser · 2019-09-19T15:16:59Z

I have released a minimum version of monero-java which provides only basic wallet functionality using monero-wallet-rpc: https://github.com/woodser/monero-java-lite. It preserves RPC tests from monero-java. I estimate the total lines of code to be ~8K, of which ~2K are non-trivial logic and the rest are POJOs or auto-generated.

battleofwizards · 2019-09-19T19:31:05Z

Thanks a lot @woodser! This looks like a good step forward.

The next order of business would be dependencies of the monero-java-lite itself.

Unfortunately, we can barely accept any dependencies :( This reluctance extends even to dependencies that are already part of Bisq (for various reasons).

Please consider if it makes sense for you to get rid off virtually all runtime dependencies. My hunch is that maybe only http client is vital and then even that should be out when we finally move to Java 11+. Regarding JSON I am not aware how much advanced processing you really need.

classes.

niyid · 2019-09-20T01:23:49Z

@woodser @battleofwizards @erciccione

Thanks everyone for your cooperation and understanding.

I have replaced monero-java with a very compact version which you will find under the packages bisq.core.xmr.jsonrpc and bisq.core.xmr.jsonrpc.result. They retain just 10 of the classes from monero-java and are in total just 19 classes (8 of which are mere POJOs).

I expect this is more manageable on the code review front as @woodser and I push for a separate PR to include monero-java into Bisq.

Regards.

niyid · 2019-09-20T01:31:44Z

I have released a minimum version of monero-java which provides only basic wallet functionality using monero-wallet-rpc: https://github.com/woodser/monero-java-lite. It preserves RPC tests from monero-java. I estimate the total lines of code to be ~8K, of which ~2K are non-trivial logic and the rest are POJOs or auto-generated.

@woodser

Thanks a lot. Even within the limited time, you have made it easier to integrate monero-java. Please refer to suggestions from @battleofwizards. I expect we should be able to achieve our secondary goal within a short period.

Regards.

woodser · 2019-09-20T15:02:58Z

@battleofwizards I have removed all dependencies except Jackson, HttpClient, JUnit, and log4j. The library utilizes advanced JSON processing using Jackson including managed and back references, field aliasing, and polymorphic deserialization. I expect it will be difficult to switch this functionality to another library, but do you suggest another library to try? JUnit is not a runtime dependency so it should be ok? log4j logging can be removed if needed.

battleofwizards · 2019-09-20T15:18:07Z

@woodser thanks!

Please use standard Java logging so your lib is flexible for inclusion in any project (so no log4j). The Jackson is a no-go. In Bisq we use Google gson and that seems cemented. If you decide to use it, make sure you are flexible version wise ("dumb down" when in doubt) because the version used will be the Bisq one. The same goes for HttpClient. JUnit as test-only dependency is fine.

battleofwizards · 2019-09-20T15:29:45Z

@niyid just a quick minor comment. I have not looked into the code (because there is 5200+ lines of it ;)) but glimpsed the build.gradle. Everything looks like a no-go to me: the maven plugin, the new log4j dependency, and making junit a compile dependency (why on Earth?).

niyid · 2019-09-20T15:51:20Z

@niyid just a quick minor comment. I have not looked into the code (because there is 5200+ lines of it ;)) but glimpsed the build.gradle. Everything looks like a no-go to me: the maven plugin, the new log4j dependency, and making junit a compile dependency (why on Earth?).

@battleofwizards

Apologies for that. I made several changes to make the monero-java Maven project plugin into Bisq. Nearly all (if not all) of these changes are no longer required. I will update in a few hours.

You can be reassured that most of those quoted lines of code are from pre-existing classes that were touched (with maybe total of less than 30 lines added across all). The new additions are the real meat; and they are less than 40 classes in total.

niyid · 2019-09-20T17:40:12Z

@battleofwizards I have removed all dependencies except Jackson, HttpClient, JUnit, and log4j. The library utilizes advanced JSON processing using Jackson including managed and back references, field aliasing, and polymorphic deserialization. I expect it will be difficult to switch this functionality to another library, but do you suggest another library to try? JUnit is not a runtime dependency so it should be ok? log4j logging can be removed if needed.

@woodser

Good work you are doing.

I was able to easily replace the validations you did with JUnit in non test-case classes with org.springframework.util.Assert.

You can replace Log4J with org.slf4j/java.util.logging/ch.qos.logback

niyid · 2019-09-20T17:47:08Z

@woodser thanks!

Please use standard Java logging so your lib is flexible for inclusion in any project (so no log4j). The Jackson is a no-go. In Bisq we use Google gson and that seems cemented. If you decide to use it, make sure you are flexible version wise ("dumb down" when in doubt) because the version used will be the Bisq one. The same goes for HttpClient. JUnit as test-only dependency is fine.

@battleofwizards
I prefer com.google.code.gson (based on familarity only) but I can attest to the fact that com.fasterxml.jackson.core was already in use in the Bisq core project.

battleofwizards · 2019-09-20T18:28:03Z

I prefer com.google.code.gson but I can attest to the fact that com.fasterxml.jackson.core was already in use in the Bisq core project.

Yes, and that is enough of a problem. BTW, up until recently we had 3 libs for the same purpose: gson, jakcson, and json-simple.

niyid · 2019-09-20T18:45:40Z

I prefer com.google.code.gson but I can attest to the fact that com.fasterxml.jackson.core was already in use in the Bisq core project.

Yes, and that is enough of a problem. BTW, up until recently we had 3 libs for the same purpose: gson, jakcson, and json-simple.

@battleofwizards

Are you aware of any performance matrix across these libraries? I like the "faster" in com.fasterxml.jackson.core but I would really want to know how it compares with com.google.code.gson.

EDIT:

I did find a few benchmarks and the most recent pages are below:

https://github.com/fabienrenaud/java-json-benchmark
https://medium.com/@dannydamsky99/heres-why-you-probably-shouldn-t-be-using-the-gson-library-in-2018-4bed5698b78b
https://programmer.help/blogs/5c0c2d51272fb.html

We need to look into this.

Regards.

battleofwizards · 2019-09-20T22:53:06Z

@battleofwizards
Are you aware of any performance matrix across these libraries?

This is pretty much irrelevant because we are not making any blank slate decision here - not even close.

From what I looked the codebase is simply cemented on using Google gson.

woodser · 2019-09-26T12:22:13Z

I updated monero-java-lite so the only compile dependencies are gson 2.7 (Bisq's version) and HttpClient and the scope of the JUnit dependency is test.

~~Update: the asserts need removed or the build breaks. I will do this shortly.~~ The asserts have been removed and JUnit is now a test dependency.

battleofwizards · 2019-09-26T12:30:11Z

@woodser wow, very nice! Thanks a lot.

mrosseel · 2019-09-26T13:05:17Z

hi all, solid work as far as I can tell, but in one of the last dev calls which you can see here: https://www.youtube.com/watch?v=YnTA3p-5v00
it seems like integrating wallets is no longer a goal for the project. The btc wallet might also be removed in the future. My impression is that this PR as-is will not be merged. Having extra XMR features in bisq might however still be needed, either integrated (without wallet) in bisq or by using a bisq api. Sorry to bring this news - and devs, please correct me if you have a different opinion.

erciccione · 2019-09-26T13:29:09Z

@mrosseel integrating wallets is not a long term goal, right, but keep in mind that implementing the new protocol could take a year or more and a lot of discussions, but the integrated Monero wallet is basically ready (this PR).

The problem with the integration was mostly related to the amount of dependencies that would have needed to be included, now that @woodser trimmed most of them i think could be possible to to move forward with this faster than we expected (correct me if i'm wrong). @niyid @wiz @battleofwizards what's your opinion?

niyid · 2019-09-26T14:17:19Z

@mrosseel integrating wallets is not a long term goal, right, but keep in mind that implementing the new protocol could take a year or more and a lot of discussions, but the integrated Monero wallet is basically ready (this PR).

The problem with the integration was mostly related to the amount of dependencies that would have needed to be included, now that @woodser trimmed most of them i think could be possible to to move forward with this faster than we expected (correct me if i'm wrong). @niyid @wiz @battleofwizards what's your opinion?

@woodser
Great work. I will check to confirm and give you feedback.

@erciccione
Thanks for chipping in. I believe if everything is in place, we can proceed with the test-bed/incubation procedure.

Regards.

ripcurlx · 2019-11-12T11:58:27Z

There are conflicts to be resolved in this PR.

niyid · 2019-11-12T12:09:32Z

This PR has since been migrated to the Monero Integration Incubator Repository at https://github.com/bisq-network/incubator-bisq-xmr-integration. I think this should be closed.

…

On Tue, Nov 12, 2019, 12:58 Christoph Atteneder ***@***.***> wrote: There are conflicts to be resolved in this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3275?email_source=notifications&email_token=AE2MWR2IUVQZBVAHELTRQS3QTKK7DA5CNFSM4IXG65PKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOED2APUA#issuecomment-552863696>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE2MWR5TAEZX6UKTC2NMFQTQTKK7DANCNFSM4IXG65PA> .

niyid added 6 commits September 16, 2019 20:30

Merging messages from proposal bisq-network#110 messages with HEAD.

c879121

Cleaning up.

3383f71

Cleaning up.

39f89b5

Cleaning up formatting.

7a2e54f

Cleaning up formatting.

1ac19a2

niyid requested review from ripcurlx and sqrrm as code owners September 16, 2019 20:39

niyid added 2 commits September 16, 2019 21:43

Quick fix; misplaced bracket.

4100fe4

Switching mode to debug from info.

f06f171

wiz reviewed Sep 16, 2019

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

wiz reviewed Sep 17, 2019

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

erciccione mentioned this pull request Sep 17, 2019

Bisq Monero Wallet bisq-network/proposals#110

Closed

Keeping abreast of new changes.

35643bc

niyid added 2 commits September 17, 2019 20:15

Refixing merged items.

471960c

Upgraded to monero-wallet-java v0.1.1

2825e23

woodser reviewed Sep 17, 2019

View reviewed changes

niyid added 3 commits September 20, 2019 02:00

Replacing monero-java with a stripped down version of less than 10

c117b3f

classes.

Replacing monero-java references in the view.

3f50cce

Replacement of monero-java with compact version complete.

1235780

Test ignore for now.

63cff66

Display destination address; filter out transactions older than 90 days.

4596d29

References to Log4J and JUnit in non test-case classes removed.

b8e84ae

mrosseel mentioned this pull request Nov 11, 2019

For Cycle 7 bisq-network/compensation#407

Closed

niyid closed this Nov 12, 2019

Fulfilling Proposal #110 #3275

Fulfilling Proposal #110 #3275

Conversation

niyid commented Sep 16, 2019 • edited Loading

wiz commented Sep 16, 2019

woodser commented Sep 17, 2019

niyid commented Sep 17, 2019 • edited Loading

niyid commented Sep 17, 2019 • edited Loading

erciccione commented Sep 17, 2019

woodser commented Sep 17, 2019

niyid commented Sep 17, 2019 • edited Loading

woodser commented Sep 17, 2019

niyid commented Sep 17, 2019

niyid commented Sep 17, 2019

battleofwizards commented Sep 17, 2019 • edited Loading

niyid commented Sep 17, 2019 • edited Loading

woodser Sep 17, 2019

Choose a reason for hiding this comment

wiz Sep 17, 2019

Choose a reason for hiding this comment

niyid Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

niyid commented Sep 18, 2019

woodser commented Sep 18, 2019

woodser commented Sep 19, 2019

battleofwizards commented Sep 19, 2019

niyid commented Sep 20, 2019

niyid commented Sep 20, 2019 • edited Loading

woodser commented Sep 20, 2019

battleofwizards commented Sep 20, 2019 • edited Loading

battleofwizards commented Sep 20, 2019

niyid commented Sep 20, 2019 • edited Loading

niyid commented Sep 20, 2019 • edited Loading

niyid commented Sep 20, 2019 • edited Loading

battleofwizards commented Sep 20, 2019

niyid commented Sep 20, 2019 • edited Loading

battleofwizards commented Sep 20, 2019 • edited Loading

woodser commented Sep 26, 2019 • edited Loading

battleofwizards commented Sep 26, 2019

mrosseel commented Sep 26, 2019

erciccione commented Sep 26, 2019

niyid commented Sep 26, 2019

ripcurlx commented Nov 12, 2019

niyid commented Nov 12, 2019 via email • edited Loading

niyid commented Sep 16, 2019 •

edited

Loading

niyid commented Sep 17, 2019 •

edited

Loading

niyid commented Sep 17, 2019 •

edited

Loading

niyid commented Sep 17, 2019 •

edited

Loading

battleofwizards commented Sep 17, 2019 •

edited

Loading

niyid commented Sep 17, 2019 •

edited

Loading

niyid Sep 17, 2019 •

edited

Loading

niyid commented Sep 20, 2019 •

edited

Loading

battleofwizards commented Sep 20, 2019 •

edited

Loading

niyid commented Sep 20, 2019 •

edited

Loading

niyid commented Sep 20, 2019 •

edited

Loading

niyid commented Sep 20, 2019 •

edited

Loading

niyid commented Sep 20, 2019 •

edited

Loading

battleofwizards commented Sep 20, 2019 •

edited

Loading

woodser commented Sep 26, 2019 •

edited

Loading

niyid commented Nov 12, 2019 via email •

edited

Loading