Skip to content

Conversation

brunoerg
Copy link
Contributor

@brunoerg brunoerg commented Aug 29, 2022

In some tests they have used sendtoaddress in order to empty a wallet. With the addition of sendall, it makes sense to use it for that.

@fanquake fanquake added the Tests label Aug 29, 2022
Copy link
Contributor

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

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

Does it make sense to also change the other occurrences of this usage scenario?
See git grep 'sendtoaddress.*getbalance'

@brunoerg
Copy link
Contributor Author

Does it make sense to also change the other occurrences of this usage scenario?

Make sense, let me check other cases and if so, I will update them and change the PR title/desc.

@brunoerg brunoerg force-pushed the 2022-08-sendall-wallet-groups branch from 53efdb1 to 923d245 Compare August 30, 2022 12:53
@brunoerg brunoerg changed the title test: use sendall in wallet_groups test: use sendall when emptying wallet Aug 30, 2022
@brunoerg brunoerg changed the title test: use sendall when emptying wallet test: use sendall when emptying wallet Aug 30, 2022
@brunoerg
Copy link
Contributor Author

Thanks for the suggestion, @kouloumos.

Force-pushed replacing sendtoaddress to sendall for all cases where they want to empty a wallet.

Copy link
Contributor

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

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

ACK 923d245, this change makes sense as that's one of the intended uses with the introduction of this RPC.

Similar change in #25159.

recipients receive equal share of the unspecified amount
Copy link
Contributor

@ishaanam ishaanam left a comment

Choose a reason for hiding this comment

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

utACK 28ea4c7
I think one of the reasons behind sendall was so that it could be used to simplify functional tests.

# Get all non-zero utxos together
chain_addrs = [self.nodes[0].getnewaddress(), self.nodes[0].getnewaddress()]
singletxid = self.nodes[0].sendtoaddress(chain_addrs[0], self.nodes[0].getbalance(), "", "", True)
singletxid = self.nodes[0].sendall(recipients=[chain_addrs[0]])['txid']
Copy link
Contributor

Choose a reason for hiding this comment

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

In 923d245 I think that this change can be removed as it is modified in the next commit anyways?

@achow101
Copy link
Member

ACK 28ea4c7

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

ACK 28ea4c7

@maflcko maflcko merged commit d16ef40 into bitcoin:master Aug 31, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 1, 2022
28ea4c7 test: simplify splitment with `sendall` in wallet_basic (brunoerg)
923d245 test: use `sendall` when emptying wallet (brunoerg)

Pull request description:

  In some tests they have used `sendtoaddress` in order to empty a wallet. With the addition of `sendall`, it makes sense to use it for that.

ACKs for top commit:
  achow101:
    ACK 28ea4c7
  ishaanam:
    utACK 28ea4c7
  w0xlt:
    ACK bitcoin@28ea4c7

Tree-SHA512: 903136d7df5c65d3c02310d5a84241c9fd11070f69d932b4e188b8ad45c38ab5bc1bd5a9242b3e52d2576665ead14be0a03971a9ad8c00431fed442eba4ca48f
@bitcoin bitcoin locked and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants