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

commands: include missing amount in spend response #927

Merged

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Jan 17, 2024

This PR follows a discussion around #873 (comment).

The GUI uses the InsufficientFunds error to get the missing amount when the user is creating a new spend, but it is not straightforward to extract this information in a general way from the RPC error (see #822 (comment)) and instead the spend module's create_spend is currently used (see #863).

With this PR, the missing amount will be included in the createspend response rather than as an error.

These changes are based on suggestions from @darosior and @edouardparis.

In a follow-up PR, the GUI should revert to using the createspend command to calculate the amount left to select.

@jp1ac4 jp1ac4 force-pushed the commands-include-missing-in-spend-result branch 2 times, most recently from 8cd021d to e9e46b1 Compare January 17, 2024 20:45
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e9e46b1. Can you just merge the two functional tests into one?

tests/test_rpc.py Outdated Show resolved Hide resolved
doc/API.md Show resolved Hide resolved
The GUI uses the InsufficientFunds error to get the missing
amount when the user is creating a new spend.

It is not straightforward to extract this information in a
general way from the RPC error. Instead, this missing amount
will be included in the command response.

These changes are based on suggestions from darosior
and edouardparis.
@jp1ac4 jp1ac4 force-pushed the commands-include-missing-in-spend-result branch from e9e46b1 to 1339898 Compare January 23, 2024 15:04
@darosior
Copy link
Member

darosior commented Jan 23, 2024

re-ACK 1339898

@darosior darosior merged commit 79141e2 into wizardsardine:master Jan 23, 2024
18 checks passed
@jp1ac4 jp1ac4 deleted the commands-include-missing-in-spend-result branch January 26, 2024 10:19
edouardparis added a commit that referenced this pull request Jan 30, 2024
…sing amount

cb5073c gui(spend): use create_spend_tx for missing amount (jp1ac4)
fae32df gui: pass change_address param to createspend (jp1ac4)

Pull request description:

  This PR is a follow-up to #927 and uses changes made in #938.

  It reverts to using the `createspend` command in the GUI for automated coin selection and to determine the amount left to select when creating a new spend (which was previously changed in #863).

  With this PR, the changes from #873 will become effective in the GUI so that (some) unconfirmed coins are used as candidates and any additional fee to pay for ancestors is included when calculating the amount left to select.

ACKs for top commit:
  edouardparis:
    ACK cb5073c

Tree-SHA512: d780b8a0238b595301701d889c45b263682867cdff1ec054872f717de7ae3d325fc5010c8df29333ae2a44ae2e92a86689d332a26ac7334c7e92fd9ffc7a6397
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.

2 participants