-
Notifications
You must be signed in to change notification settings - Fork 35.7k
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
rpc: improve getaddressinfo test coverage, help, code docs #17283
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK d73709a
Good cleanup. I would just recommend compressing the commits as indicated in my comments. I felt like having to review the same lines multiple times as I went through the commits and combining them will cut down on time needed for reviewing.
d73709a
to
90de8fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the gist of the CLI help output after this PR (same urls as before): |
90de8fe
to
e123923
Compare
Code review ACK e123923 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review and tested ACK e123923
IMO not sure it's worth it splitting getaddressinfo content and formatting change in diff commits.
Code review ACK e123923 I find the clarification on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, only read the change so far.
tACK e123923. CLI help output as described. Looking forward to reviewing the deprecation of label PR ;) |
e123923
to
d22d7fe
Compare
Rebased. |
appveyor failures are unrelated:
|
Appveyor error is apparently fixed by #17384 (comment) |
d22d7fe
to
8e1fbdb
Compare
Rebased and incorporated @promag's feedback. |
Concept ACK. Thanks Jon. I find the bitcoin/src/wallet/rpcwallet.cpp Line 3672 in 21ee676
was added in #12892 (from code originally in #7729). Prior to that, the My preference would be to:
Some feedback on this PR:
|
8e1fbdb
to
505a4ae
Compare
and separate the fields with a line break for readability.
dfbd281
to
33f5fc3
Compare
Deprecated the getaddressinfo |
Re-ACK 33f5fc3 Reviewed code changes. Build failure seems unrelated, would need a rebuild. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 33f5fc3.
I've left some nits. please don't invalidate review now, but you could consider addressing them in your follow-up PR.
33f5fc3 test: add rpc getaddressinfo labels test coverage (Jon Atack) 0f3539a test: add listlabels test in wallet_labels.py (Jon Atack) 1388de8 rpc: add getaddressinfo code documentation (Jon Atack) 2ee0cb3 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack) 8d1ed0c rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack) 5a0ed85 rpc: improve getaddressinfo RPCHelpman content (Jon Atack) 70cda34 rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack) Pull request description: This PR is a continuation of the work in #12892. Main motivations: - There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address. - `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers. Changes by order of commits: - [x] improve/fix getaddressinfo RPCHelpman layout formatting - [x] improve/fix getaddressinfo RPCHelpman content - [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman - [x] update getaddressinfo RPCExamples addresses to bech32 - [x] add getaddressinfo code docs - [x] add a `listlabels` test assertion in wallet_labels.py - [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests Here are gists of the CLI help output: [`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810) [`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba) It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._ ACKs for top commit: fjahr: Re-ACK 33f5fc3 jnewbery: ACK 33f5fc3. Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
… code docs 33f5fc3 test: add rpc getaddressinfo labels test coverage (Jon Atack) 0f3539a test: add listlabels test in wallet_labels.py (Jon Atack) 1388de8 rpc: add getaddressinfo code documentation (Jon Atack) 2ee0cb3 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack) 8d1ed0c rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack) 5a0ed85 rpc: improve getaddressinfo RPCHelpman content (Jon Atack) 70cda34 rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack) Pull request description: This PR is a continuation of the work in bitcoin#12892. Main motivations: - There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address. - `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers. Changes by order of commits: - [x] improve/fix getaddressinfo RPCHelpman layout formatting - [x] improve/fix getaddressinfo RPCHelpman content - [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman - [x] update getaddressinfo RPCExamples addresses to bech32 - [x] add getaddressinfo code docs - [x] add a `listlabels` test assertion in wallet_labels.py - [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests Here are gists of the CLI help output: [`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810) [`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba) It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups bitcoin#17578 and bitcoin#17585 now build on this PR._ ACKs for top commit: fjahr: Re-ACK 33f5fc3 jnewbery: ACK 33f5fc3. Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
This seemed to break CI / Trusty or at the very least, Trusty distribution seemed to not be able to be fetched from here on. |
… behavior 8925df8 doc: update release notes (Jon Atack) 8bb405b test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14 rpc: incorporate review feedback from PR 17283 (Jon Atack) Pull request description: This PR builds on #17283 (now merged) and is followed by #17585. It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs. before ``` "labels": [ { "name": "DOUBLE SPEND", "purpose": "receive" } ``` after ``` "labels": [ "DOUBLE SPEND" ] ``` The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`. For context, see: - #17283 (comment) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output. Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those. ACKs for top commit: jnewbery: reACK 8925df8 promag: Code review ACK 8925df8. meshcollider: Code review ACK 8925df8 Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
…revious behavior 8925df8 doc: update release notes (Jon Atack) 8bb405b test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14 rpc: incorporate review feedback from PR 17283 (Jon Atack) Pull request description: This PR builds on bitcoin#17283 (now merged) and is followed by bitcoin#17585. It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs. before ``` "labels": [ { "name": "DOUBLE SPEND", "purpose": "receive" } ``` after ``` "labels": [ "DOUBLE SPEND" ] ``` The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`. For context, see: - bitcoin#17283 (comment) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output. Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in bitcoin#17585) and add support for multiple labels per address. This PR will unblock those. ACKs for top commit: jnewbery: reACK 8925df8 promag: Code review ACK 8925df8. meshcollider: Code review ACK 8925df8 Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
d3bc184 doc: update release notes with getaddressinfo label deprecation (Jon Atack) 72af93f test: getaddressinfo label deprecation test (Jon Atack) d48875f rpc: deprecate getaddressinfo label field (Jon Atack) dc0cabe test: remove getaddressinfo label tests (Jon Atack) c7654af doc: address pr17578 review feedback (Jon Atack) Pull request description: This PR builds on bitcoin#17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`. See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and bitcoin#17283 (comment) for more context. Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text. Next step: add support for multiple labels. ACKs for top commit: jnewbery: ACK d3bc184 laanwj: ACK d3bc184 meshcollider: utACK d3bc184 Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
d3bc184 doc: update release notes with getaddressinfo label deprecation (Jon Atack) 72af93f test: getaddressinfo label deprecation test (Jon Atack) d48875f rpc: deprecate getaddressinfo label field (Jon Atack) dc0cabe test: remove getaddressinfo label tests (Jon Atack) c7654af doc: address pr17578 review feedback (Jon Atack) Pull request description: This PR builds on bitcoin#17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`. See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and bitcoin#17283 (comment) for more context. Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text. Next step: add support for multiple labels. ACKs for top commit: jnewbery: ACK d3bc184 laanwj: ACK d3bc184 meshcollider: utACK d3bc184 Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
…e docs Summary: 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151 test: add rpc getaddressinfo labels test coverage (Jon Atack) 0f3539ac6d772fc646b5f184fa1efe77bf632f6a test: add listlabels test in wallet_labels.py (Jon Atack) 1388de83900eaced906d369fe9e8887ae74b2dcf rpc: add getaddressinfo code documentation (Jon Atack) 2ee0cb3330ccf70f0540cb42370796e32eff1569 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack) 8d1ed0c263f8cdff7189f02040b5d02238d93da0 rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack) 5a0ed850700dfb19167d40b38f80313bd5e427ca rpc: improve getaddressinfo RPCHelpman content (Jon Atack) 70cda342cd20d0e0cd9f28405457544036968f2d rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack) Pull request description: This PR is a continuation of the work in bitcoin/bitcoin#12892. Main motivations: - There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address. - `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers. Changes by order of commits: - [x] improve/fix getaddressinfo RPCHelpman layout formatting - [x] improve/fix getaddressinfo RPCHelpman content - [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman - [x] update getaddressinfo RPCExamples addresses to bech32 - [x] add getaddressinfo code docs - [x] add a `listlabels` test assertion in wallet_labels.py - [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests Here are gists of the CLI help output: [`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810) [`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba) It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._ --- Depends on D7716 Backport of Core [[bitcoin/bitcoin#17283 | PR17283]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7718
…ous behavior Summary: 8925df86c4df16b1070343fef8e4d238f3cc3bd1 doc: update release notes (Jon Atack) 8bb405bbadf11391ccba7b334b4cfe66dc85b390 test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f2f11529add115d963d05599130288ae28 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14ccf2bcd1e9b2ad48e5e08881be06d9d21 rpc: incorporate review feedback from [[bitcoin/bitcoin#17283 | PR17283]] (Jon Atack) Pull request description: This PR builds on #17283 (now merged) and is followed by #17585. It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs. before ``` "labels": [ { "name": "DOUBLE SPEND", "purpose": "receive" } ``` after ``` "labels": [ "DOUBLE SPEND" ] ``` The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`. For context, see: - bitcoin/bitcoin#17283 (comment) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output. Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those. --- Depends on D7718 Backport of Core [[bitcoin/bitcoin#17578 | PR17578]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7724
… code docs 33f5fc3 test: add rpc getaddressinfo labels test coverage (Jon Atack) 0f3539a test: add listlabels test in wallet_labels.py (Jon Atack) 1388de8 rpc: add getaddressinfo code documentation (Jon Atack) 2ee0cb3 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack) 8d1ed0c rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack) 5a0ed85 rpc: improve getaddressinfo RPCHelpman content (Jon Atack) 70cda34 rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack) Pull request description: This PR is a continuation of the work in bitcoin#12892. Main motivations: - There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address. - `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers. Changes by order of commits: - [x] improve/fix getaddressinfo RPCHelpman layout formatting - [x] improve/fix getaddressinfo RPCHelpman content - [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman - [x] update getaddressinfo RPCExamples addresses to bech32 - [x] add getaddressinfo code docs - [x] add a `listlabels` test assertion in wallet_labels.py - [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests Here are gists of the CLI help output: [`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810) [`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba) It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups bitcoin#17578 and bitcoin#17585 now build on this PR._ ACKs for top commit: fjahr: Re-ACK 33f5fc3 jnewbery: ACK 33f5fc3. Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
…revious behavior 8925df8 doc: update release notes (Jon Atack) 8bb405b test: getaddressinfo labels purpose deprecation test (Jon Atack) 60aba1f rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack) 7851f14 rpc: incorporate review feedback from PR 17283 (Jon Atack) Pull request description: This PR builds on bitcoin#17283 (now merged) and is followed by bitcoin#17585. It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs. before ``` "labels": [ { "name": "DOUBLE SPEND", "purpose": "receive" } ``` after ``` "labels": [ "DOUBLE SPEND" ] ``` The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`. For context, see: - bitcoin#17283 (comment) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427) - http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output. Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in bitcoin#17585) and add support for multiple labels per address. This PR will unblock those. ACKs for top commit: jnewbery: reACK 8925df8 promag: Code review ACK 8925df8. meshcollider: Code review ACK 8925df8 Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
d3bc184 doc: update release notes with getaddressinfo label deprecation (Jon Atack) 72af93f test: getaddressinfo label deprecation test (Jon Atack) d48875f rpc: deprecate getaddressinfo label field (Jon Atack) dc0cabe test: remove getaddressinfo label tests (Jon Atack) c7654af doc: address pr17578 review feedback (Jon Atack) Pull request description: This PR builds on bitcoin#17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`. See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and bitcoin#17283 (comment) for more context. Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text. Next step: add support for multiple labels. ACKs for top commit: jnewbery: ACK d3bc184 laanwj: ACK d3bc184 meshcollider: utACK d3bc184 Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
This PR is a continuation of the work in #12892.
Main motivations:
labels
response. Coverage here is a prerequisite before deprecating thelabel
response or adding multiple labels per address.bitcoin-cli help getaddressinfo
returns a few content errors, difficult-to-read formatting, and no explanation why it returns bothlabel
andlabels
and how they relate, which can be confusing for application developers.Changes by order of commits:
label
andlabels
fields in getaddressinfo RPCHelpmanlistlabels
test assertion in wallet_labels.pylabels
test coverage and improve the existinglabel
testsHere are gists of the CLI help output:
bitcoin-cli help getaddressinfo
before this PRbitcoin-cli help getaddressinfo
after this PRIt seems we ought to begin a deprecation process for the getaddressinfo
label
field? If yes, I have a follow-up ready. --> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR.