millisat precision#10586
Conversation
ce4fb8e to
eda429f
Compare
62db370 to
d8dfce5
Compare
d8dfce5 to
102df84
Compare
102df84 to
f7d2a9e
Compare
|
I looked through the PR, but the issues above were all found by Claude. For those i verified (point 1 - 6) i wrote the comments above. I didn't verify the remaining points but considering the first were all real it makes sense to look at the remaining ones as well (8. -12.). This is the output:## 🔴 High — confirmed crashes / broken core flows1.
|
754820c to
19432c7
Compare
Thanks for reviewing. I missed these Claude findings due to the green checkmark, and cirrus-ci is by now unavailable.. |
The results i commented were from a local run, not sure what the CI said, but yeah it often finds some things even though the checkmark is green, so makes sense to look at the output anyway. |
SomberNight
left a comment
There was a problem hiding this comment.
I just looked at the first commit.
Please don't think just because the genai finds no concerns, there are no bugs.
Hmm, don't mind if I do :) |
19432c7 to
40a169a
Compare
|
rebased on master, commits reorganised for better reviewability |
be0695d to
5f13e7b
Compare
…imal point and extra_precision up to AmountEdit AmountEdit defaults to decimal_point = 0, declare FiatAmountEdit to encapsulate fx base unit and precision, extra_precision now a callable and retrieved dynamically. BTCAmountEdit extended with millisat_precision option kwarg to allow 3 digits extra precision.
…with parameter combinations to constructor. in QEConfig, add formatMilliSatsForEditing, update formatMilliSats passing Decimal to config.format_amount/_and_units
add amount_msat parameter and make mutually exclusive with amount_sat
qt: send_tab LN checks millisat precision
payment_identifier: mark pi invalid if multiline and any output uses msat precision qt: update send_tab for lnurl msat precision qml: update qeinvoice, qerequestdetails for lnurl msat precision
…oiceParser, QEFX, QEWallet
QEConfig.satsToUnits to QEConfig.amountToBaseunitStr with the latter now also taking a QEAmount instance
Using javascript or QML `int` type is a long-standing issue, as there is not enough range to express millisats or even sats. A short summary of the issues: - integers in javascript context have a max range of +/- 2^54-1, which allows 21M BTC expressed in sats, but not in msats. - QML `int` properties have a max range of +/- 2^31-1, which only allows 21 BTC expressed in sats, 21 mBTC expressed in msats - `int` parameters declared in the `pyqtProperty` and `pyqtSlot` decorators have a max range of 2^31-1, although this is somewhat alleviated in the QML->Python direction by using `q(u)int64`. Returning a `q(u)int64` does not work around the `int` limitation In most of the QML code, `QEAmount` is already used for storing and passing around BTC values. The only exception is where amounts are compared (e.g. invoice amount < available balance etc), so the `<`, `>`, `<=`, `>=` and `!=` operators, and where these operators are implied, like `Math.min()` and `Math.max()` This commit delegates these operators to python scope.
…precision on send_tab if recipient is onchain
5f13e7b to
6bc0bcd
Compare
|
This would also benefit from using the new electrum/electrum/plugins/nwc/nwcserver.py Lines 463 to 466 in 32f2c18 |
| def satoshiValue(self, fiat, plain=True): | ||
| @pyqtSlot(str, result='QVariant') | ||
| def satoshiValue(self, fiat): | ||
| self._amount = QEAmount() |
There was a problem hiding this comment.
Why is _amount declared as instance attribute here, isn't it just used in the scope of this method?
| try: | ||
| fd = Decimal(fiat) | ||
| except Exception: | ||
| return '' |
There was a problem hiding this comment.
Returning a str here violates the specified QVariant result type, e.g. when entering . as fiat amount:
11.04 | W | gui.qml.qeapp | "Could not convert argument 0 at"
11.04 | W | gui.qml.qeapp | "expression for onTextChanged@file:///var/home/user/code/code_vm/electrum/electrum/gui/qml/components/controls/FiatField.qml:22"
11.04 | W | gui.qml.qeapp | file:///var/home/user/code/code_vm/electrum/electrum/gui/qml/components/controls/FiatField.qml:22: TypeError: Passing incompatible arguments to C++ functions from JavaScript is not allowed.
| Label { | ||
| visible: valid | ||
| text: amount.msatsInt != 0 ? Config.formatMilliSats(amount) : Config.formatSats(amount) | ||
| text: Config.formatMilliSats(amount) |
There was a problem hiding this comment.
This passes None to Config.formatMilliSats() and raises an Exception, e.g. when creating a lightning invoice:
3.70 | I | lnworker.LNWallet.[default_wallet] | creating bolt11 invoice with routing_hints: [('r', [('02c3be1bcedffe1675617fc7ae9e4c43ce0f54cc3e58c48ef51398dc87a78e0ce0', '10052759x15511793x59990', 1000, 1, 144)]), ('r', [('02c3be1bcedffe1675617fc7ae9e4c43ce0f54cc3e58c48ef51398dc87a78e0ce0', '8427041x8597475x4058', 1000, 1, 144)]), ('r', [('02465ed5be53d04fde66c9418ff14a5f2267723810176c9212b722e542dc1afb1b', '16000000x0x18483', 1000, 1, 80)])], sat: 1898
3.71 | E | gui.qml.qeapp.Exception_Hook | exception caught by crash reporter
Traceback (most recent call last):
File "/var/home/user/code/code_vm/electrum/electrum/gui/qml/qeconfig.py", line 400, in formatMilliSats
raise Exception(f"Unknown amount type: {str(type(amount))}")
Exception: Unknown amount type: <class 'NoneType'>
3.73 | D | gui.qml.qerequestdetails | key=f3cd542ad64f230e65dfe4e7312d3a00394a3ce651ed4a5ab1c8072535c10061
| return "%d" % amount | ||
| x = to_decimal(amount) | ||
| scale_factor = pow(10, self.decimal_point()) | ||
| nfmt = "{:." + str(self.decimal_point() + self.extra_precision()) + "f}" |
There was a problem hiding this comment.
When calling .setAmount() on a FeerateEdit with a 0 extra_precision currency like KRW this swallows trailing 0, though nothing seems to be affected by this:
>>> from electrum.gui.qt import amountedit
>>> fiat = amountedit.FiatAmountEdit(fx=daemon.fx) # fx is KRW
>>> fiat.setAmount(100000)
>>> fiat.get_amount()
1
| def __init__(self, *, amount_sat: int = 0, amount_msat: int = 0, is_max: bool = False, from_invoice=None, parent=None): | ||
| valueChanged = pyqtSignal() | ||
|
|
||
| def __init__(self, *, amount_sat: int = None, amount_msat: int = None, is_max: bool = False, from_invoice=None, parent=None): |
There was a problem hiding this comment.
The default argument change from 0 to None causes the log in satsInt() and msatsInt() to spam a bit when using default QEAmount(). Maybe the log could be removed as it is to be expected that the amount is None sometimes?
E.g. when opening a swap view:
24.86 | D | gui.qml.qeswaphelper | run_swap_manager
24.87 | I | submarine_swaps.NostrTransport | starting nostr transport with pubkey: 7992c2f29bf56da19b032e2aca6f785e8413058dd57f1c1482aa2486c4531299
24.87 | I | submarine_swaps.NostrTransport | nostr relays: ['wss://nostr.einundzwanzig.space', 'wss://relay.primal.net', 'wss://relay.damus.io', 'wss://eu.purplerelay.com', 'wss://ftp.halifax.rwth-aachen.de/nostr', 'wss://relay.getalby.com/v1', 'wss://nostr.mom', 'wss://brb.io', 'wss://nos.lol']
24.87 | W | gui.qml.qetypes | amount_msat is undefined, returning 0
24.87 | W | gui.qml.qetypes | amount_msat is undefined, returning 0
24.87 | W | gui.qml.qetypes | amount_msat is undefined, returning 0
24.87 | W | gui.qml.qetypes | amount_msat is undefined, returning 0
| Layout.preferredWidth: 1 | ||
| enabled: Daemon.currentWallet.isLightning && (Daemon.currentWallet.lightningCanReceive.satsInt | ||
| > amountBtc.textAsSats.satsInt || Daemon.currentWallet.canGetZeroconfChannel) | ||
| enabled: Daemon.currentWallet.isLightning && (Daemon.currentWallet.lightningCanReceive.msatsInt |
There was a problem hiding this comment.
The Lightning button doesn't react to user input anymore, when entering an amount exceeding lightningCanReceive the button stays enabled.
| else: | ||
| self.amount_e.setToolTip('') | ||
|
|
||
| # resolve '!' in amount editor if it was set before PI |
There was a problem hiding this comment.
This behavior seems to have regressed, when entering ! first and then pasting an address the amount turns empty instead of resolving max.
support millisat precision throughout the codebase.
ints are passedDecimalis used on the GUI side where it's easier to assume sats are the base unitsatandmsatfields/parametersQEAmountis now synced between sats and millisats, simplifying assumptions and reducing if-else checksshould fix #6253, #10412