One-way communication offline funds transfer using QR-code #3358
Conversation
mitchellolsthoorn
changed the title
|
@devos50 Can you review the PR for merge? |
devos50
changed the title
|
@mitchellolsthoorn please prefix the PR title with |
|
#3364 fixes the unit tests. I would like to merge that one first. After that's done, please rebase this PR and I will review it. |
|
@mitchellolsthoorn please rebase this PR on devel again. The tests should pass now. |
|
@devos50 I tried to rebase it, but it gives a merge conflict in the main_window.ui and no matter which branch I chose, the tabs in the settings menu are shifted and options are moved. I do not know how to fix this |
|
@mitchellolsthoorn as a last resort, you might undo the changes to |
|
@devos50 I was planning on redoing the GUI changes on top of the latest commit like you suggested but I discovered that the latest commit also has the bug, so the bug is not introduced by the rebase.
All the settings tabs are shifted and some options are missing. Do you want me to rebase it on the broken version, or wait till its fixed? |
There was a problem hiding this comment.
-please fix the pylint errors and code coverage errors.
-make sure to write proper tests for the TrustchainBootstrapEndpoint class and bootstrap_new_identity method (plus any lines that are not covered, as reported by the code coverage check).
-could you add this QR code functionality to the GUI tests somehow?
GUI-related comments:
-it is not clear to the user what this option exactly does. Why should they scan a QR code and on what device? What happens to their funds?
-I would rename funds to ‘bandwidth tokens’ or something like that (probably needs discussion with Johan).
-please change the yellow warning text to ‘Warning: this is a one-way action that cannot be reverted. If the QR code is not scanned, funds will be lost.
-talking about that, can we save the QR code somewhere on the system? If users accidently click the QR code away, they can still access it.
-when clicking on ‘partically empty funds’ and then ‘cancel’ without entering anything, it gives an error.
-polish the buttons using custom CSS so it’s consistent across all major platforms and actually looks like a button.
-make sure that the titles in the buttons shown in the dialog are written in a consistent way (use capital characters).
-help the user a bit by specifying which unit they have to enter in the dialog when partially emptying their wallet.
-change the placehodler in the input dialog when partially emptying a wallet to ‘please enter the amount of bandwidth tokens in bytes). Talking about that, is bytes a straightforward unit? Maybe choose something like MB.
-change the title of the dialog with the QR code to something like ‘please scan the following QR code’.
| child_handler_dict = { | ||
| "statistics": TrustchainStatsEndpoint, | ||
| "blocks": TrustchainBlocksEndpoint, | ||
| "bootstrap": TrustchainBootstrapEndpoint} |
There was a problem hiding this comment.
Please position the } on a new line.
| try: | ||
| up = int(request.args['up'][0]) | ||
| except ValueError: | ||
| up = up |
There was a problem hiding this comment.
Please raise an error here when the input is not a valid integer so it's consistent with the code in other endpoints.
| def bootstrap_new_identity(self, up, down): | ||
| """ | ||
| One-way payment channel. | ||
| Create a new temporary identity , and transfer funds to the new identity. |
There was a problem hiding this comment.
Remove the whitespace before the comma.
| @@ -232,3 +266,8 @@ def on_introduction_response(self, messages): | |||
|
|
|||
| def start_walking(self): | |||
| self.register_task("take step", LoopingCall(self.take_step)).start(self.CrawlerDelay, now=False) | |||
|
|
|||
|
|
|||
| class DBShim: | |||
There was a problem hiding this comment.
Please inherit this class from object. Also, clarify why you need this class.
tribler-headless.sh
Outdated
| @@ -0,0 +1 @@ | |||
| python2 -m twisted tribler --restapi 8080 --manhole 3000 | |||
There was a problem hiding this comment.
Please remove this file. We already have a --headless option.
devos50
changed the title
|
@mitchellolsthoorn please rebase onto the latest version of Also, I thought a bit about the GUI test for the QR code. If you want to make this, you also need to adjust the fake Tribler API (https://github.com/Tribler/fake-tribler-api) and I don't think it's worth the time to dive into this so I'm fine with it if you don't create a GUI test. I would like to see a test for the API endpoints, however. Please refer to the other REST API tests to get an idea of how to write them. If you have any other questions, please let me know 👍 |
…so more can be ~ 1/3 can still be dropped without moving off json )
mitchellolsthoorn
force-pushed
the
devel
branch
3 times, most recently
from
ca678e3 to
5395398
Compare
mitchellolsthoorn
force-pushed
the
devel
branch
from
5395398 to
08f92e3
Compare
mitchellolsthoorn
changed the title
|
@devos50 I rebased it on devel, covered all the point you had and wrote some tests |
|
Looks good, thanks! |
devos50
changed the title
This PR adds the ability to export funds without a network connection by using a throw-away identity for transferring between the two different identities