Skip to content
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

qml: QETxDetails: defer to wallet.get_tx_info() for rbf/cpfp #8894

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

SomberNight
Copy link
Member

I think this logic should be encapsulated in the wallet.

Looks like the extra guards were added in 84cb210
(except for can_save_as_local -- added in 9cb8dea)

I don't really see a reason why this logic should be GUI-specific.
Also, note that it can make sense to allow bump_fee on a local tx, and so wallet.get_tx_info() allows it (and hence so does the qt gui). For example, if your tx falls out of the mempool, you might want to bump it. (though it could also make sense to allow dscancel in the same scenario but we don't allow that)
For cpfp, as long as there is no package relay, it does not make sense to allow it for a local tx.

I don't really understand the guard on can_save_as_local - maybe @accumulator can comment if he remembers why it was added.

(My direct motivation is simply that I sometimes manually test rbf/cpfp and it would be easier to only have to patch a single localised thing.)

It might make sense to allow bump_fee on a local tx, and so wallet.get_tx_info() allows it.
For dscancel/cpfp, it does not allow it either. Still, I think this logic should be encapsulated in the wallet.
@accumulator
Copy link
Member

accumulator commented Feb 16, 2024

I don't really understand the guard on can_save_as_local - maybe @accumulator can comment if he remembers why it was added.

I believe the extra and not txinfo.can_remove is intended to only enable the save button if the transaction is not recorded in the wallet yet (e.g. a scanned raw tx -> can_remove==False).
Not sure why txinfo.can_save_as_local was not sufficient, but it could be that it is not updated immediately after saving a TX. I don't remember exactly (it's been a while), but at the time of the commit I was very careful not to mess with the semantics of the backend wallet :)

@ecdsa
Copy link
Member

ecdsa commented Mar 15, 2024

ACK. this should not be GUI specific

@ecdsa ecdsa merged commit 61dc8b7 into spesmilo:master Mar 15, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants