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

rpcwallet: Add missing transaction categories to rpc helptexts #14653

Merged
merged 2 commits into from Dec 20, 2018

Conversation

@andrewtoth
Copy link
Contributor

@andrewtoth andrewtoth commented Nov 5, 2018

The current helptext for listtransactions, listsinceblock and gettransaction only list two of the five possible options for category. This incorrectly implies that these are the only two options, and can cause problems if the other three options aren't accounted for. Also, some of the documentation is incorrect when specifying which options are returned for which categories.

This PR updates the helptext for these RPCs and adds a functional regression test for the cases when the other three categories are returned.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Nov 5, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Coverage

Coverage Change (pull 14653) Reference (master)
Lines +0.0176 % 87.0960 %
Functions -0.0612 % 84.3822 %
Branches +0.0122 % 51.5722 %

Updated at: 2018-11-05T21:23:44.257742.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@meshcollider
Copy link
Member

@meshcollider meshcollider commented Nov 6, 2018

Thanks, utACK 31066f5
The listsinceblock RPC helptext also still refers to the 'move' category which was removed when accounts were, you could fix that up too in the same commit if you want :)

@andrewtoth andrewtoth force-pushed the listtransactions-help branch from 31066f5 to 4a78c48 Nov 7, 2018
@andrewtoth
Copy link
Contributor Author

@andrewtoth andrewtoth commented Nov 7, 2018

@meshcollider done. Also updated the gettransaction help.

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Nov 11, 2018

LGTM, re-utACK 4a78c48

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@andrewtoth andrewtoth force-pushed the listtransactions-help branch from 4a78c48 to 9828c53 Nov 13, 2018
@andrewtoth
Copy link
Contributor Author

@andrewtoth andrewtoth commented Nov 13, 2018

@promag Reformatted help docs, added tests for all rpcs that have transaction category.

@andrewtoth andrewtoth force-pushed the listtransactions-help branch from 9828c53 to a2c555d Nov 14, 2018
@andrewtoth andrewtoth changed the title Add all category options to listtransactions help Add all transaction category options to rpc helptexts Nov 17, 2018
@andrewtoth andrewtoth changed the title Add all transaction category options to rpc helptexts Add missing transaction category options to rpc helptexts Nov 17, 2018
@andrewtoth andrewtoth changed the title Add missing transaction category options to rpc helptexts Add missing transaction categories to rpc helptexts Nov 17, 2018
@chris-belcher
Copy link
Contributor

@chris-belcher chris-belcher commented Nov 18, 2018

ACK a2c555d

@andrewtoth
Copy link
Contributor Author

@andrewtoth andrewtoth commented Nov 20, 2018

@promag Are the formatting and tests ok?

@meshcollider Sorry to keep invalidating your utACKs. I updated to include category tests for all 3 rpcs.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@andrewtoth andrewtoth force-pushed the listtransactions-help branch 5 times, most recently from e539bbf to 1d25ee1 Nov 22, 2018
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@andrewtoth andrewtoth force-pushed the listtransactions-help branch from 1d25ee1 to 0742dcb Nov 23, 2018
Copy link
Contributor

@ryanofsky ryanofsky left a comment

utACK f3f6dde. Clearly a good change. It cleans up inaccurate documentation and adds a test.

@ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Dec 18, 2018

This looks like it could be merged.

@promag
Copy link
Member

@promag promag commented Dec 18, 2018

utACK f3f6dde.

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Dec 20, 2018

utACK

@MarcoFalke MarcoFalke changed the title Add missing transaction categories to rpc helptexts rpcwallet: Add missing transaction categories to rpc helptexts Dec 20, 2018
@MarcoFalke MarcoFalke merged commit f3f6dde into bitcoin:master Dec 20, 2018
2 checks passed
MarcoFalke added a commit that referenced this issue Dec 20, 2018
…lptexts

f3f6dde Test coinbase category in wallet rpcs (andrewtoth)
e982f0b Add all category options to wallet rpc help (andrewtoth)

Pull request description:

  The current helptext for `listtransactions`, `listsinceblock` and `gettransaction` only list two of the five possible options for `category`. This incorrectly implies that these are the only two options, and can cause problems if the other three options aren't accounted for. Also, some of the documentation is incorrect when specifying which options are returned for which categories.

  This PR updates the helptext for these RPCs and adds a functional regression test for the cases when the other three categories are returned.

Tree-SHA512: 67dd7ff6269a3b0f17f5d1a61b0ae1fb1f3778f05e1c440bfbb9b3a005c9c6d740abcace20f3d597cf2bd6779c494448690f13fab0bd2340f206213bc7890b51
@andrewtoth andrewtoth deleted the listtransactions-help branch Apr 29, 2019
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Sep 2, 2020
Summary:
bitcoin/bitcoin@e982f0b

---

Partial backport of Core [[bitcoin/bitcoin#14653 | PR14653]]

Test Plan:
  ninja
  ./src/bitcoind -regtest
  ./bitcoin-cli help listtransactions|listsinceblock|gettransaction

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7316
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Sep 2, 2020
Summary:
bitcoin/bitcoin@f3f6dde

---

Depends on D7316

Concludes backport of Core [[bitcoin/bitcoin#14653 | PR14653]]

Test Plan:
  ninja
  ./test/functional/test_runner.py wallet_coinbase_category

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants