-
Notifications
You must be signed in to change notification settings - Fork 3.2k
psbt_nostr: split generic and UI parts, implement for qml #9694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
5a498d2
to
85c36bb
Compare
8604376
to
78c6f7c
Compare
electrum/plugins/psbt_nostr/qml.py
Outdated
self.pending = (pubkey, event_id, tx) | ||
|
||
def accept_psbt(self, my_event_id): | ||
pubkey, event_id, tx = self.pending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the following exception when accepting 2 incoming cosigner requests at the same time (by broadcasting from 2 signers in a 3-of-3, then opening the third signer which will show 2 overlapping popups, clicking yes on both produces this exception):
Traceback (most recent call last):
File "/home/user/code/electrum-fork/electrum/plugins/psbt_nostr/qml.py", line 71, in acceptPsbt
cosigner_wallet.accept_psbt(event_id)
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
File "/home/user/code/electrum-fork/electrum/plugins/psbt_nostr/qml.py", line 140, in accept_psbt
pubkey, event_id, tx = self.pending
^^^^^^^^^^^^^^^^^^^^
TypeError: cannot unpack non-iterable NoneType object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch, testing this was indeed left on my TODO list :)
The desktop client handles this case by blocking the UI thread, which is not an option in QML. I was thinking it might be better to put the responsibility on the plugin to emit only one PSBT, until the PSBT/event is accepted. This way we don't have to implement this in every GUI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correction: it seems it is not blocking the entire UI thread, but the signal handler itself is blocking on the window.question()
electrum/electrum/plugins/psbt_nostr/qt.py
Lines 138 to 145 in 78c6f7c
def on_receive(self, pubkey, event_id, tx): | |
window = self.window | |
if not window.question( | |
_("An transaction was received from your cosigner.") + '\n' + | |
_("Do you want to open it now?")): | |
return | |
self.mark_event_rcvd(event_id) | |
show_transaction(tx, parent=window, prompt_if_unsaved=True) |
regardless, this is the result of a little spamming on desktop
What to do if the user answers no? Present the next psbt event popup (potentially many)? Or stop asking altogether? (maybe skip all received events? or disable receive for x minutes?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks more like an inbox pattern than an event pattern.. Maybe receive all to wallet and add another tab 'Sign Requests'? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also solve supercedes, e.g. remove older psbts that spend the same utxos, due to re-send of RbF-ed or same tx, or supercede PSBTs with more signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cancelling all of them when the user clicks no on one may cause an important one to get cancelled, i think it would be useful to see how many signatures the tx has in the message so the user sees which is the interesting one he actually wants to sign (e.g. in a 3-of-3 with 3 sequential sessions the third signer will get two messages, and could just cancel the one showing 1-of-3 sigs and accept the one with 2-of-3 to sign it). Or maybe just a little pie chart in the message?
But we should probably put the events on the known_events dict when the user clicks no, so they don't show up again on each wallet restart for the next 24h.
A Sign Requests tab would be nice as an optional thing maybe, i think the notification popups are cool because they are self explanatory as in you just get them shoved in the face and don't have to search for the right place to sign.
Considering that cosigning over nostr probably is done less frequently by users the simplicity of popups outweigh the benefits of an extra tab imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cancelling all of them when the user clicks no on one may cause an important one to get cancelled, i think it would be useful to see how many signatures the tx has in the message so the user sees which is the interesting one he actually wants to sign (e.g. in a 3-of-3 with 3 sequential sessions the third signer will get two messages, and could just cancel the one showing 1-of-3 sigs and accept the one with 2-of-3 to sign it). Or maybe just a little pie chart in the message?
But we should probably put the events on the known_events dict when the user clicks no, so they don't show up again on each wallet restart for the next 24h.
Not re-appearing for 24h is not very user friendly..
A Sign Requests tab would be nice as an optional thing maybe, i think the notification popups are cool because they are self explanatory as in you just get them shoved in the face and don't have to search for the right place to sign.
Well we could still show the popup of course
Considering that cosigning over nostr probably is done less frequently by users the simplicity of popups outweigh the benefits of an extra tab imo.
Not sure I agree. There's not many users of multisig, but those that do could use this frequently. It would basically just be a list. And we can hide the tab unless there's actual unhandled events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not re-appearing for 24h is not very user friendly..
I mean when the popup actually shows some information about the psbt (like the amount of sigs), and the user decides to click no because the tx is not relevant (e.g. already signed on another signer) i don't see a point in showing it again on every restart until it expires, the user will just click no again and it's annoying? In the current state i agree, as there is no information you don't really know which popup to accept or reject.
Well we could still show the popup of course
I think a nice middle way would be to show a single popup then (even if multiple requests come in), and clicking yes would just redirect to the inbox tab, so the user is aware of the tab and doesn't have to find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a nice middle way would be to show a single popup then (even if multiple requests come in), and clicking yes would just redirect to the inbox tab, so the user is aware of the tab and doesn't have to find it.
Hmm, we kinda-sorta already have a list of 'sign requests', if we consider storing transactions as Local (as in, 'Add to History' in transaction dialog), which already handles transaction replacements if they spend the same UTXOs.
So, maybe we could do a 'Add to History' implicitly for all remaining incoming PSBTs when saying No to the popup, so the tx(s) appear at the top of the history as a Local transaction.
Would be minimal effort to do it like this..
Also, for the initiating side we could 'Add to History' implicitly when sending to cosigner, so the initiator stays aware of the used utxos while waiting for cosigner(s) to complete the tx (and will not use these utxos in other txs). Currently it is possible to send to cosigner without adding to local history.
@ecdsa, @SomberNight what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if the user says 'no', the event should be added to self.known_events
. That would reduce the spam.
Also, if a tx has more than 2 cosigners, we should try to show only the most recent version of the tx, or the one that has the most signatures.
Tested QT and QML. I think it would also be useful to allow the user to configure relays (like in the QT gui), as they maybe want to use their own relay for privacy or the default relays could all be unavailable, rendering this function unusable until the next release. |
Correct, I haven't implemented this yet. We can hard-enable this, or create a Preferences option for it. Currently, it can only be tested on desktop after enabling through CLI.
Agreed |
78c6f7c
to
0d964d3
Compare
I've added an If the user responds No to 'open psbt now', it will add the transaction to the wallet and set a short cooldown timeout to process existing PSBTs in the nostr-queue without asking the user. Some extra PSBT handling is still TODO, e.g. check to only add 'newer' txs with more signatures, or auto-combine signatures if PSBTs are coming from multiple cosigners. Also GUI-wise there's some fighting between transaction dialog open and the popup for another PSBT opening at the same time. |
0d964d3
to
c36bb27
Compare
c36bb27
to
1a33863
Compare
also some boilerplate for plugins extending QML GUI