Qt: widget for entering amounts, supports both BTC and sats #530

kristapsk · 2020-03-01T17:51:58Z

It has dropdown to switch between entering BTC or sats. From caller's perspective tries to behave like QLineEdit and always returns sats.

Currently used only in single join / send form. Should be added to other places, like Tumbler wizard too, but that takes extra work as QWizard does not know how to handle them by default and also needs more testing.

Changed code that creates controls in single send from loops to adding each control seperately. It's more readable and also gives more flexibility with using different controls in future (for example, mixdepth chooser could be drop down with mixdepth number and available balance in brackets).

Commented out unimplemented SchStaticPage with line comments, because """ approach sometimes messes up Vim's syntax highlightning.

kristapsk · 2020-03-05T00:56:46Z

Rebased

kristapsk · 2020-03-23T23:08:11Z

Rebased again. Planning to work on the remaining stuff mentioned above, but some concept ACKs / NACKs would not hurt.

chris-belcher · 2020-03-23T23:43:16Z

Concept ACK

AdamISZ · 2020-03-30T14:47:03Z

First, sorry that this one's been a bit neglected!
Ran a few simple tests, this is what I observed:

the UI looks good, simple and intuitive toggle.
did coinjoins with btc and sat amounts and worked as intended
counter-intuitive behaviour: you can enter non-integer sats values, and the pre-existing auto-correction to btc occurs. the same thing happens if you enter a large (>21M) integer value for BTC. So basically the settings work but are not enforced. IIRC you can use something like QIntValidator or similar in Qt to enforce the right kind of input, there is some usage of that already, somewhere in joinmarket-qt.py or qtsupport.py.

Once the above is cleared up I'll check it again. I do think it's a good addition to the app though, thanks.

kristapsk · 2020-03-30T19:29:55Z

@AdamISZ , thanks for testing, will check the bug you mentioned (I really think it's a bug, not just "counter-intuitive behaviour"). Also still haven't managed to get tumbler wizards working with this, want to do that too.

If this gets merged before, should be useful for #487 too.

kristapsk · 2020-05-16T17:23:22Z

Rebased and addressed comments by @AdamISZ. Currently works in a single send and settings tabs. Not yet in tumbler wizard. I tried to get that working some weeks ago, didn't finish, will get back to it at some point, but I think this is already useful, even without that.

AdamISZ · 2020-05-18T15:27:30Z

Test result:
I got a stack trace when clicking out of the "address" entry field to the next (entered a valid regtest address). It appears not to be related to the PR, but rather to the BIP21 thing?

(jmvenv) waxwing@here~/testjminstall/joinmarket-clientserver/scripts$ python joinmarket-qt.py --datadir=.
User data location: .
2020-05-18 16:22:25,680 [DEBUG]  rpc: getnewaddress []
2020-05-18 16:22:25,712 [DEBUG]  rpc: sendrawtransaction ['0000000000000000000000000000000000000000']
2020-05-18 16:22:25,714 [DEBUG]  error pushing = -22 TX decode failed
2020-05-18 16:22:38,789 [DEBUG]  rpc: listaddressgroupings []
2020-05-18 16:22:38,830 [DEBUG]  Fast sync in progress. Got this many used addresses: 4
2020-05-18 16:22:40,119 [DEBUG]  rpc: listunspent [0]
2020-05-18 16:22:40,229 [DEBUG]  bitcoind sync_unspent took 0.11186861991882324sec
2020-05-18 16:22:40,281 [INFO]  Starting transaction monitor in walletservice
Traceback (most recent call last):
  File "joinmarket-qt.py", line 486, in <lambda>
    lambda: checkAddress(self, self.addressInput.text()))
  File "joinmarket-qt.py", line 119, in checkAddress
    parent.widgets[0][1].setText(addr)
AttributeError: 'SpendTab' object has no attribute 'widgets'

~~At a first glance I don't know where this came from; it does indeed appear that QWidget does not have a widgets member variable? https://doc.qt.io/qt-5/qwidget.html~~ see #530 (comment)

scripts/joinmarket-qt.py

AdamISZ · 2020-05-18T15:37:00Z

scripts/joinmarket-qt.py

@@ -527,7 +530,7 @@ def initUI(self):
        buttons.addWidget(self.startButton)
        buttons.addWidget(self.abortButton)
        self.abortButton.clicked.connect(self.abortTransactions)
-        innerTopLayout.addLayout(buttons, len(self.widgets) + 1, 0, 1, 2)


It's a shame to lose flexibility by hardcoding 5. But a really minor point so OK.

scripts/qtsupport.py

AdamISZ · 2020-05-18T15:38:45Z

scripts/qtsupport.py

-    def __init__(self, parent):
-        super(SchStaticPage, self).__init__(parent)
-        self.setTitle("Manually create a schedule entry")
+# TODO implement this option


Why change this in this PR?

Mentioned in initial comment when opening PR. It messes up syntax highlighting in vim sometimes, makes no difference otherwise. Can be removed if you insist.

To be clear: you're saying that using """ style comment sections is a problem for formatting sometimes? You think that should always be avoided outside of doc strings, then? Not unreasonable, if so, just I wasn't aware.

Yes, it looks so. Can't say with 100% certainty, vim syntax highlighting acts strange sometimes. Also """ is meant for doc strings, # is a proper way to comment out code blocks, I think.

kristapsk · 2020-05-18T21:26:03Z

Addressed stack trace mentioned by @AdamISZ, my mistake at rebasing. Had this in mind while doing #567, but forgot.

AdamISZ · 2020-05-19T11:21:03Z

Testing: 2c54542

I notice something that is probably a mistake: I can enter more than 8 decimal places for BTC amounts (specifically, 15 decimal places). From trying a coinjoin, I can confirm it simply rounds to the nearest 8 decimal places, as you'd expect, but this is not the correct behaviour imo: users should be constrained to 8 decimal places for an exact amount (and indeed all other wallets do this).

AFAIR this can be set in a QValidator.

Apart from this point, everything else so far looks correct, including rejection of invalid input (negative or non-numeric input). Also the number of significant figures allowed for satoshi amounts looks correct, and non-integers are correctly rejected.

I'll test a little more but the above point seems like it needs to be fixed.

kristapsk · 2020-05-19T11:27:17Z

Good catch, BitcoinAmountBTCValidator should be improved, it also currently allows entering multiple decimal points, looking into it.

scripts/qtsupport.py

kristapsk · 2020-05-19T11:33:42Z

Fixed

AdamISZ · 2020-05-19T12:58:21Z

Fix confirmed to above with 55f7f5f

Testing autoconvert, I notice a behaviour which I don't think was intended: 34 sats converts to 3.4E-7 BTC.

However, it does bring up "amount below dust threshold".
It seems two digit sat amounts are like that: covert to E notation, but are below dust threshold. 3 digit sat amounts convert to decimal as intended.

Edit: this is not really relevant as a test case, but it amused me: although you can't enter non-numerics into the field because of the validator, if you switch say "10" in sats to BTC, it goes to 1.0E-7 and then you can edit the -7 to 30 and try to do a coinjoin of 10**30 BTC, unfortunately there was not enough liquidity in the orderbook ;) (i.e. the code got that far before it gave up; no crashes or errors, which is both good and bad).

kristapsk · 2020-05-19T13:32:30Z

Fixed E notation issue, the same fix as in #573.

AdamISZ · 2020-05-19T14:11:52Z

For future reference, it's better to add commits one at a time and then squash at the end. That way I don't need to spend time finding the new code that fixes each bug.

AdamISZ · 2020-05-19T14:13:24Z

I'm guessing 6a659ac added this? 6a659ac#diff-3e361749da5986d8e0337a42d1eb755bR598

kristapsk · 2020-05-19T14:18:22Z

I'm guessing 6a659ac added this? 6a659ac#diff-3e361749da5986d8e0337a42d1eb755bR598

Yes

AdamISZ · 2020-05-19T14:30:16Z

OK tACK, I will merge this.
Edit: just for reference, I tested that a small absurd_fee_per_kb is respected (it is, but see #581 - not related to this PR), and I tested the tumbler as a sanity check.

AdamISZ · 2020-05-19T14:47:54Z

Sorry, before I merge: you have tabs on empty lines at line 645 and 650, should be removed (don't know if other places, didn't check every line. Edited: checked again, it's just those two. Everything else looks good in the diff.

kristapsk · 2020-05-19T14:54:10Z

Fixed. These two were the only places, checked with git diff HEAD~.

AdamISZ · 2020-05-19T15:01:31Z

I'm still seeing them in 2645c1b ?

kristapsk · 2020-05-19T15:09:19Z

I forgot to git add before commit. :)

kristapsk mentioned this pull request May 9, 2020

Add BIP21 bitcoin payment URI support to sendpayment.py #560

Merged

AdamISZ reviewed May 18, 2020

View reviewed changes

scripts/joinmarket-qt.py Show resolved Hide resolved

AdamISZ reviewed May 18, 2020

View reviewed changes

scripts/qtsupport.py Show resolved Hide resolved

AdamISZ reviewed May 18, 2020

View reviewed changes

AdamISZ reviewed May 19, 2020

View reviewed changes

scripts/qtsupport.py Show resolved Hide resolved

BitcoinAmountEdit Qt widget

d1323f6

AdamISZ merged commit 7748da5 into JoinMarket-Org:master May 19, 2020

kristapsk deleted the qt-amount-widget branch May 19, 2020 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qt: widget for entering amounts, supports both BTC and sats #530

Qt: widget for entering amounts, supports both BTC and sats #530

kristapsk commented Mar 1, 2020

kristapsk commented Mar 5, 2020

kristapsk commented Mar 23, 2020

chris-belcher commented Mar 23, 2020

AdamISZ commented Mar 30, 2020

kristapsk commented Mar 30, 2020

kristapsk commented May 16, 2020

AdamISZ commented May 18, 2020 •

edited

Loading

AdamISZ May 18, 2020

AdamISZ May 18, 2020

kristapsk May 18, 2020

AdamISZ May 19, 2020

kristapsk May 19, 2020

kristapsk commented May 18, 2020

AdamISZ commented May 19, 2020

kristapsk commented May 19, 2020

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020 •

edited

Loading

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020

AdamISZ commented May 19, 2020

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020 •

edited

Loading

AdamISZ commented May 19, 2020 •

edited

Loading

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020

kristapsk commented May 19, 2020

Qt: widget for entering amounts, supports both BTC and sats #530

Qt: widget for entering amounts, supports both BTC and sats #530

Conversation

kristapsk commented Mar 1, 2020

kristapsk commented Mar 5, 2020

kristapsk commented Mar 23, 2020

chris-belcher commented Mar 23, 2020

AdamISZ commented Mar 30, 2020

kristapsk commented Mar 30, 2020

kristapsk commented May 16, 2020

AdamISZ commented May 18, 2020 • edited Loading

AdamISZ May 18, 2020

Choose a reason for hiding this comment

AdamISZ May 18, 2020

Choose a reason for hiding this comment

kristapsk May 18, 2020

Choose a reason for hiding this comment

AdamISZ May 19, 2020

Choose a reason for hiding this comment

kristapsk May 19, 2020

Choose a reason for hiding this comment

kristapsk commented May 18, 2020

AdamISZ commented May 19, 2020

kristapsk commented May 19, 2020

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020 • edited Loading

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020

AdamISZ commented May 19, 2020

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020 • edited Loading

AdamISZ commented May 19, 2020 • edited Loading

kristapsk commented May 19, 2020

AdamISZ commented May 19, 2020

kristapsk commented May 19, 2020

AdamISZ commented May 18, 2020 •

edited

Loading

AdamISZ commented May 19, 2020 •

edited

Loading

AdamISZ commented May 19, 2020 •

edited

Loading

AdamISZ commented May 19, 2020 •

edited

Loading