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.

Sign up for GitHub

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

Jump to bottom

Warn when adding the change output's value to fees. #811

Closed
darosior opened this issue Nov 14, 2023 · 3 comments
Closed

Warn when adding the change output's value to fees. #811

darosior opened this issue Nov 14, 2023 · 3 comments
Assignees
@jp1ac4
Labels
Daemon / Liana library This is about lianad or the liana library (not the GUI) Enhancement Improving an existing functionality GUI gui related

Comments

@darosior
Copy link
Member

When the change output value is below our own "dust" limit (5k sats at the moment, ie 1.5€) we do not include it at all thereby adding the value to fees.

We should display a warning when we do so. Maybe through a "warnings" entry in the result of createspend?
@darosior darosior added GUI gui related Feature New feature or functionality. Daemon / Liana library This is about lianad or the liana library (not the GUI) labels Nov 14, 2023
@darosior darosior mentioned this issue Nov 14, 2023
Closed
@pythcoiner
Copy link
Collaborator

related to #790

@darosior darosior mentioned this issue Nov 20, 2023
Closed
@darosior darosior added Enhancement Improving an existing functionality and removed Feature New feature or functionality. labels Nov 20, 2023
@darosior darosior added this to Liana v5 Dec 7, 2023
@darosior darosior moved this to Todo in Liana v5 Dec 7, 2023
@darosior darosior mentioned this issue Dec 15, 2023
Merged
@darosior darosior moved this from Todo to In Progress in Liana v5 Jan 3, 2024
@jp1ac4 jp1ac4 mentioned this issue Jan 3, 2024
Merged
darosior added a commit that referenced this issue Jan 13, 2024
@darosior
Merge #905: spend: warn when overpaying fee
501dce4 
5a15c74 commands: return warnings from spend creation (jp1ac4)
da474ad spend: add warning when adding change to fee (jp1ac4)
8d84f0d spend: return max possible change from coin selection (jp1ac4)
e4d8330 spend: use debug log level in coin selection (jp1ac4)

Pull request description:

  This is a first step towards #811. The GUI will be updated in a follow-up PR.

  It adds a warning if there is any change/excess available that has been added to the fee, building on the assumption from bitcoindevkit/coin-select#14 that the decision whether to include change depends on whether the excess is over a threshold. The function `select_coins_for_spend()` now returns this threshold so that the caller can check if the possible change amount is above it.

  I've used an `enum` for the different warnings, but could use strings directly.

  If the approach looks fine, I'll add some tests.

ACKs for top commit:
  darosior:
    ACK 5a15c74

Tree-SHA512: 12716b1e299d3154c520667c66aeb7a176a770f4a1d53125a8334d7c5a7e7bb2ce1dcb795ad5998bffc1303d9cb34c651cf8d3ef3dee8210572d75069012bddd
@darosior
Copy link
Member Author

Half of this was merged with #905. What's left now is to display the warnings in the GUI.

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Jan 26, 2024

I think we can display this warning, and the one from #873, when the user has generated a PSBT, but before saving it.

We can also consider whether to add the warning about change added to fee when the user creates a new RBF spend. We could display any such warnings in the RBF modal, but it might be more intuitive to link to the draft PSBT screen from the RBF modal and let the user view the warnings there. Currently, the PSBT is saved automatically from the RBF modal and we link to the saved PSBT.

We could also display these warnings on a saved PSBT screen, but in general we won't have access to these warnings after the PSBT is saved so I think it makes more sense to display them for draft PSBTs only.

For now, I could proceed on the assumption we'll display these warnings in the draft PSBT screen only and we can decide later whether to change the RBF modal.

@jp1ac4 jp1ac4 mentioned this issue Jan 29, 2024
Merged
edouardparis added a commit that referenced this issue Feb 27, 2024
@edouardparis
Merge #940: gui(spend): display warnings for draft PSBTs
7801fc7 
85f5a68 gui(spend): display warnings for draft PSBTs (jp1ac4)

Pull request description:

  This is to complete #811.

  It displays any warnings generated by `createspend` on the draft PSBT view, which could currently be those from #905 and #873. Once the PSBT has been saved, these warnings will no longer be shown.

  Below are some screenshots.

  With a warning:
  ![image](https://github.com/wizardsardine/liana/assets/121959000/fb2167d9-4800-4849-8622-ff20ac55623e)

  ![image](https://github.com/wizardsardine/liana/assets/121959000/e1ab9ec2-00ae-457f-81e1-9b3a2863af31)

  Without a warning:
  ![image](https://github.com/wizardsardine/liana/assets/121959000/73b5b4bd-55b5-40ee-b587-4f57bfd57b4c)

  ![image](https://github.com/wizardsardine/liana/assets/121959000/e9b23d4e-bb13-4ae7-944e-258f9c9a0abd)

ACKs for top commit:
  edouardparis:
    ACK 85f5a68

Tree-SHA512: b9875801f7607f6f824ad9ab5182e93aa533ba2bbb8f9a4168c4a2a779a8f39cf77dc0b24eac7c99a0fce36c486c126144d23b0cb7ecec27ebc145ea2ffdb212
@jp1ac4 jp1ac4 moved this from In Progress to Done in Liana v5 Feb 27, 2024
@jp1ac4 jp1ac4 closed this as completed Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jp1ac4 jp1ac4

Labels
Daemon / Liana library This is about lianad or the liana library (not the GUI) Enhancement Improving an existing functionality GUI gui related
Projects
No open projects
Liana v5
Status: Done
Milestone
No milestone
Development

No branches or pull requests
3 participants
@darosior @jp1ac4 @pythcoiner