Solution Proposed about UI feature #20 #22

vincenzopalazzo · 2020-07-13T06:13:26Z

I read the UI feature published by @darosior and I try to implement a solution about the feature described inside the issue #20

This is a draft solution because I never tried before to check a bitcoin url from the camera. For this reason, I don't know if the solution proposed is good or is very stupid, and please if it is very stupid, ignore this PRs and don't lose your time here :)

The solution proposed, try to understand only if the URL has a standard format like a bitcoin address, I don't make the validation of it because we can know if the address is correct from the lightning command result.

So, at the moment, this solution runs in only in two situations (Tested only with the copy from clipboard):

You scan or copy only bitcoin address, such as: 3EktnHQD7RiAE6uzMj2ZifT9YgRrkSgzQX the command that clightning run is lightning-cli withdraw 3EktnHQD7RiAE6uzMj2ZifT9YgRrkSgzQX all
You scan an url, such as bitcoin:2NFmhFFEbR2ruAboRZ8gxCeDez81c3ByZeV?amount=102.00000000 lightning run the following command lightning-cli withdraw 2NFmhFFEbR2ruAboRZ8gxCeDez81c3ByZeV 102.00000000 (Maybe in this cases lightning can return an error because the amount has a bad format)

My transaction on testnet with lamp to make a small test. tx-status

The next feature to implement inside this solution is the following

Add a dialog in the middle from scan action and send action to permit the user to interact and make a double-check.
More test about the validation (if it doesn't is trash)
Create a method to store the transaction hash and make a double check on explorer (Maybe in a separate PRs it is a good feature)

Again, this is a draft solution to discuss with the developers is the Validator solution is a good solution or I'm losing something important. If the solution likes it to developers I will require the lock on issue #20.

@vincenzopalazzo

This is a draft solution to discuss with developer is the Validator solution is a good solution or I'm losing somethings important. If the solution, like at developer I will require the lock on the issue. Commit built by @vincenzopalazzo [email protected]

lvaccaro · 2020-07-13T19:24:35Z

Hi @vincenzopalazzo, I think you are enjoying with kotlin :-)

At 1st review, I would avoid that a mainnet clightning user scans a testnet bitcoin address and the validation pass.

vincenzopalazzo · 2020-07-13T19:44:43Z

Hi @lvaccaro,

I think you are enjoying with kotlin :-)

I little bit 😄

At 1st review, I would avoid that a mainnet clightning user scans a testnet bitcoin address and the validation pass.

Great, I will make this control

darosior · 2020-07-13T23:41:44Z

Just to echo the discussion in #20 :

At 1st review, I would avoid that a mainnet clightning user scans a testnet bitcoin address and the validation pass.

lightningd's withdraw will not accept such an address and return a nice JSONRPC error.

vincenzopalazzo · 2020-07-13T23:49:38Z

Hi @darosior,

lightningd's withdraw will not accept such an address and return a nice JSONRPC error.

I noted just now this with a test on my java RPC wrapper, anyway I think, that is possible to make a double check before that lamp runs the command. Maybe we can use the RPC error from withdraw to check any error in my bitcoin address type parser :) Today I will add this check

darosior · 2020-07-14T00:01:25Z

To be honest that seems to be belt and suspenders -------- Original Message --------

…

On Jul 14, 2020, 00:49, Vincenzo Palazzo wrote: Hi ***@***.***(https://github.com/lvaccaro), > lightningd's withdraw will not accept such an address and return a nice JSONRPC error. I noted just now this with a test on my java RPC wrapper, anyway I think, that is possible to make a double check before that lamp runs the command. Maybe we can use the RPC error from withdraw to check any error in my bitcoin address type parser :) Today I will add this check — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](https://github.com/lvaccaro/lamp/pull/22#issuecomment-657884484), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F27LEQ6HAN4XXGZH5LR3OMR7ANCNFSM4OYGI4FA).

@vincenzopalazzo

This commit contains the structure of the dialog and also contains a couple of comment about the problem that this solution have yet, and also about the possible solutions. Commit built by @vincenzopalazzo [email protected]

vincenzopalazzo · 2020-07-14T07:55:12Z

I hope that " to be belt and suspenders" have the same mining that has in America :)

@vincenzopalazzo

Comment built from @vincenzopalazzo vincenzopalazzo

vincenzopalazzo · 2020-07-15T18:40:03Z

Hi @lvaccaro and @darosior,

I made more tests on bitcoin URI validator and I fixed some bugs, also I added the java reference implementation lib to parse the URI.

I opted to use a dialog and not a new activity to display the data in the URI but is possible to create an activity and give the possibility to the user to create the withdraw also without URI, such as the withdraw setting.

At the moment I the app look like the gif above

In this case, the error is caused by the different networks, in fact, I have c-lightning node on testnet and the URI is from mainet.

The check is built with txprepare, the code looks like how the code above

https://github.com/vincenzopalazzo/lamp/blob/98addc499343172de94cd02846497faef3f251bc/app/src/main/java/com/lvaccaro/lamp/util/Validator.kt#L76-L87

If you want test it, you can found my testnet network with the following address

tb1qzy5mqyqpl6p67x8psu2phxwp0e7lz79w47e5ad
bitcoin:tb1qzy5mqyqpl6p67x8psu2phxwp0e7lz79w47e5ad

This is only a proposed for the bitcoin dialog, but I'm ready also to change it with activity and also, this commit includes the check for the network that we discussed on this PR. The are other work to do on the layout.

lvaccaro · 2020-07-18T21:17:09Z

Hi @vincenzopalazzo, adding some notes:

send btc fragment is like withdraw dialog from settings with sendall and label.. so I suggest to use only once for both, passing arguments to the fragment.
personal opinion: I prefer BottomSheetDialogFragment instead of DialogFragment , because we could open AlertDialog based on success/failure state in a better way.
on Refactoring Parsing uri #25, I move scanner function to a new class in order to cleanup MainActivity. It should be more clear how it works.
on Refactoring Parsing uri #25 I try to use withdraw command in a very fast way, but I don't know about bip21 uri support. So it could be paired with isBitcoinPayment()

vincenzopalazzo · 2020-07-18T23:21:08Z

Hi @lvaccaro,

Thanks for your comment

send btc fragment is like withdraw dialog from settings with sendall and label.. so I suggest to use only once for both, passing arguments to the fragment.

I agree with you, for the moment I will remove my dialog and we can you your fragment to make a withdraw. If will be necessary somethings a little too complex, e.g: Setting a personal fee. We can thing to migrate these inside a new activity a make the UI a little bit more complex.

personal opinion: I prefer BottomSheetDialogFragment instead of DialogFragment because we could open AlertDialog based on success/failure state in a better way.

I add the answer to the comment above, I agree with you. We can leave the BottomSheetDialogFragment

on #25, I move scanner function to a new class in order to clean up MainActivity. It should be more clear about how it works.

Yep, I love your refactoring, i think that the class MainActivity is too big 😄

on #25 I try to use withdraw command in a very fast way, but I don't know about bip21 uri support. So it could be paired with isBitcoinPayment()

I lose this passage when I read your PR #25, I will come back to read it and understend but I added an comment inside the method parse in the your PR. In conclusion, yes I think we can use the method isBitcoinPayment() and also after we can check the correct netowork with https://github.com/vincenzopalazzo/lamp/blob/98addc499343172de94cd02846497faef3f251bc/app/src/main/java/com/lvaccaro/lamp/util/Validator.kt#L76

Tomorrow I will conclude this PR and you can make the final check if you want marge it. After that I can help you on another task, also I want resolve the problem about the clean app data because sometimes is not possible and I don't understand why

@vincenzopalazzo

This is a draft solution to discuss with developer is the Validator solution is a good solution or I'm losing somethings important. If the solution, like at developer I will require the lock on the issue. Commit built by @vincenzopalazzo [email protected]

@vincenzopalazzo

This commit contains the structure of the dialog and also contains a couple of comment about the problem that this solution have yet, and also about the possible solutions. Commit built by @vincenzopalazzo [email protected]

@vincenzopalazzo

Comment built from @vincenzopalazzo vincenzopalazzo

…itcoin_action

@vincenzopalazzo

This contains a possible solution with the new refactoring of MainActivity. The next and last step is the refactoring and remove the bad&old code inside the app. Commit built by @vincenzopalazzo [email protected]

@vincenzopalazzo

Commit built by @vincenzopalazzo [email protected]

vincenzopalazzo · 2020-07-19T15:51:08Z

Hi @lvaccaro,

I want to make an update with my work on this PR, I think that this is my first complete proposed that support the following operations

Scan and past bitcoin address
Scan and past bitcoin URI (support all type of bitcoin URI)
Scan and past lightning node URI to connect
Scan and past bol11

All types of text are validated before to pass at the lightningd command with the Validator.kt class, and also, the address is validated with txprepare to verify if the network is correct.

In addition, I added a Switch component to support the send all amount inside the withdraw fragment (discussed inside the issue #26), and also I added a little padding inside the BottomSheetDialogFragment, I think that without the padding the component are on the screen border.

I introduced a class called LampKeys it contains all constant inside the json object and bundle object, this class help all develop to not commit the typing error, for instance, "massage" and not "message" is difficult to find sometimes.

This PR includes another dependence to import the java reference implementation of bip21

I hope this work can help you, and thanks for your time.

lvaccaro · 2020-07-19T21:29:18Z

Thanks..
I am pretty scared about the external library for reading an bitcoin uri and get the address.. if an attacker updates the bitcoin parsing library on remove cdn, it could provide a fake address. Could we mitigate the risk?

vincenzopalazzo · 2020-07-19T21:47:41Z

Hi @lvaccaro,

Absolutely yes I can restore my parsing string logic in a couple of commit before, sorry about this

lvaccaro · 2020-07-19T21:48:32Z

thanks, sorry but I starting the code review only now.

vincenzopalazzo · 2020-07-19T22:30:29Z

Hi @lvaccaro, I'm sorry I lost the jar particular, is my error :) but with the validator, the change is not very difficult :)
Tomorrow if the free head I will make this change :) and I'm happy if I will have also your review.

@vincenzopalazzo

Comment built by @vincenzopalazzo [email protected]

vincenzopalazzo · 2020-07-20T08:30:01Z

Hi @lvaccaro,

I update the version of code with the last version of the master branch, and I removed the dependence and I add more tests about the URI parser.

The Validator should support the bip21 and I remade the UI test about the scanning different types of QR and I received the correct behaviors, but more tests are welcome.

Please, if there are some other errors, let me know.

lvaccaro · 2020-08-02T10:39:01Z

ack 8bc3d6e

vincenzopalazzo · 2020-08-02T10:41:08Z

@lvaccaro I don't push the message sorry.

Yep, I change the version inside the travis.

Sorry about that.

lvaccaro · 2020-08-02T12:20:40Z

No problem, https://github.com/lvaccaro/lamp/pull/31 should fix

vincenzopalazzo · 2020-08-02T12:28:10Z

@lvaccaro ops, I create a java file. Sorry about that, I miss to check files extension

lvaccaro mentioned this pull request Jul 13, 2020

Feature request: detect onchain QR codes, and allow to spend from the onchain wallet instead #20

Open

vincenzopalazzo added 2 commits July 15, 2020 01:32

Fixed bugs validator and start work on bitcoin sent dialog

af8710d

Proposed Bitcoin send dialog and small change

98addc4

Comment built from @vincenzopalazzo vincenzopalazzo

lvaccaro added 4 commits July 18, 2020 16:35

Migrate uri parsing from MainActivity to UriResultActivity

a5f2712

Add withdraw command at uri parsing

0ce1c3b

Pre-parsing uri text to avoid multi strings for withdraw command

988c1dc

Add bitcoin & lightning schemes

3ffd280

vincenzopalazzo added 8 commits July 19, 2020 01:25

Fixed bugs validator and start work on bitcoin sent dialog

da9dd07

Proposed Bitcoin send dialog and small change

23ac425

Comment built from @vincenzopalazzo vincenzopalazzo

Pull last change about refatcotoring view

296bde3

Merge remote-tracking branch 'origin/sent_bitcoin_action' into sent_b…

156f50a

…itcoin_action

reintroduced missing function and added padding inside the fragments

a6018e8

Refactoring method parse inside the URI activity

cc32031

lvaccaro mentioned this pull request Jul 19, 2020

Refactoring Parsing uri #25

Merged

vincenzopalazzo added 2 commits July 19, 2020 15:50

Check inside the scan view before to run a lightningd commands

be22e7e

This contains a possible solution with the new refactoring of MainActivity. The next and last step is the refactoring and remove the bad&old code inside the app. Commit built by @vincenzopalazzo [email protected]

Solution proposed about the issue clightning4j#20

e939517

Commit built by @vincenzopalazzo [email protected]

vincenzopalazzo force-pushed the master branch from f8fb18e to e939517 Compare July 19, 2020 15:37

Merge branch 'master' into master

0eba011

vincenzopalazzo added 2 commits July 20, 2020 00:56

Removed reference implementation bip21

0bc0ad1

Update with the last master branch version and add more tests

2b07307

Comment built by @vincenzopalazzo [email protected]

vincenzopalazzo and others added 3 commits July 26, 2020 16:21

Validator.kt:isBolt11(): add support to regtest network

f9336e8

Validator.ktl: Fixed bug inside parsing node URL and add more tests

a274b2c

update travis build with the new version of android.

8bc3d6e

lvaccaro merged commit 8bc3d6e into clightning4j:master Aug 2, 2020

lvaccaro mentioned this pull request Aug 2, 2020

Withdraw all #26

Closed

This was referenced Aug 13, 2020

Logger Observer core implementation #38

Merged

Balance View to testing the lightningd notifications #39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solution Proposed about UI feature #20 #22

Solution Proposed about UI feature #20 #22

vincenzopalazzo commented Jul 13, 2020

lvaccaro commented Jul 13, 2020

vincenzopalazzo commented Jul 13, 2020

darosior commented Jul 13, 2020

vincenzopalazzo commented Jul 13, 2020 •

edited

Loading

darosior commented Jul 14, 2020 via email

vincenzopalazzo commented Jul 14, 2020

vincenzopalazzo commented Jul 15, 2020

lvaccaro commented Jul 18, 2020

vincenzopalazzo commented Jul 18, 2020

vincenzopalazzo commented Jul 19, 2020

lvaccaro commented Jul 19, 2020

vincenzopalazzo commented Jul 19, 2020

lvaccaro commented Jul 19, 2020

vincenzopalazzo commented Jul 19, 2020

vincenzopalazzo commented Jul 20, 2020

lvaccaro commented Aug 2, 2020

vincenzopalazzo commented Aug 2, 2020

lvaccaro commented Aug 2, 2020

vincenzopalazzo commented Aug 2, 2020

Solution Proposed about UI feature #20 #22

Solution Proposed about UI feature #20 #22

Conversation

vincenzopalazzo commented Jul 13, 2020

lvaccaro commented Jul 13, 2020

vincenzopalazzo commented Jul 13, 2020

darosior commented Jul 13, 2020

vincenzopalazzo commented Jul 13, 2020 • edited Loading

darosior commented Jul 14, 2020 via email

vincenzopalazzo commented Jul 14, 2020

vincenzopalazzo commented Jul 15, 2020

lvaccaro commented Jul 18, 2020

vincenzopalazzo commented Jul 18, 2020

vincenzopalazzo commented Jul 19, 2020

lvaccaro commented Jul 19, 2020

vincenzopalazzo commented Jul 19, 2020

lvaccaro commented Jul 19, 2020

vincenzopalazzo commented Jul 19, 2020

vincenzopalazzo commented Jul 20, 2020

lvaccaro commented Aug 2, 2020

vincenzopalazzo commented Aug 2, 2020

lvaccaro commented Aug 2, 2020

vincenzopalazzo commented Aug 2, 2020

vincenzopalazzo commented Jul 13, 2020 •

edited

Loading