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

gettransaction: add an argument to decode the transaction #16185

Merged
merged 3 commits into from Sep 2, 2019

Conversation

@darosior
Copy link
Contributor

commented Jun 10, 2019

This PR adds a new parameter to the gettransaction call : decode. If set to true, it will add a new decoded field to the response. This mimics the behavior of getrawtransaction's verbose argument to avoid using 2 calls if we want to decode a wallet transaction (gettransaction then decoderawtransaction).

Fix #16181 .

Copy link
Member

left a comment

Concept ACK, please include tests and a release note.

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
@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch from 7894189 to 0622d5b Jun 10, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@promag thank you for the review. I added the changes and will include the tests and release note soon.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
  • #16199 (rpc: fix showing wrong amount when not all inputs are from me in gettransaction by shannon1916)
  • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

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.

@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Concept ACK, please include tests and a release note.

Since there was no functional test for the gettransaction I created a new one. However it only tests the decoded field since I don't know if testing other would not be overkill.

test/functional/rpc_gettransaction.py Outdated Show resolved Hide resolved
test/functional/rpc_gettransaction.py Outdated Show resolved Hide resolved
@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch 2 times, most recently from b351a12 to 9759da2 Jun 12, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Moved the test to test/functional/wallet_basic.py and used named arguments.

@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch from 9759da2 to 676b504 Jun 13, 2019
@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch 2 times, most recently from 8ccc657 to 7cf832a Jul 10, 2019
@DrahtBot DrahtBot removed the Needs rebase label Jul 10, 2019
@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch 2 times, most recently from 8e78c0b to 91f15c8 Jul 10, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Rebased.

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
test/functional/wallet_basic.py Show resolved Hide resolved
test/functional/wallet_basic.py Outdated Show resolved Hide resolved
test/functional/wallet_basic.py Show resolved Hide resolved
@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch 2 times, most recently from 769a0ea to 5f96ab3 Jul 18, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Some minor corrections according to last review.

Copy link
Member

left a comment

You could use 0b6ba66#diff-0f596a69ccfb4d07c007cd6fc993898dR50 (not yet in master) to do:

assert_no_key('decoded', self.nodes[0].gettransaction(txid, decode=False))
Copy link
Contributor

left a comment

Concept ACK

test/functional/wallet_basic.py Outdated Show resolved Hide resolved
doc/release-notes.md Outdated Show resolved Hide resolved
@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch from 5f96ab3 to 5c28a1f Aug 4, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Rebased.
(And addressed the spacing nits pointed by @jb55 :-) )

@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch 2 times, most recently from 957c11a to 50d0174 Aug 29, 2019
@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Aug 29, 2019
@promag

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

You must make linters happy.

@darosior

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

You must make linters happy.

No that's a Travis error : curl: (7) Failed to connect to storage.googleapis.com port 443: Connection timed out

darosior added 3 commits Jun 10, 2019
This adds a new boolean parameter 'decode' to the gettransaction call, which, if set to true, add a 'decoded' field to the result containing the decoded transaction
@darosior darosior force-pushed the darosior:decode_tx_gettransaction branch from 50d0174 to 9965940 Aug 30, 2019
Copy link
Member

left a comment

re-utACK 9965940

meshcollider added a commit that referenced this pull request Sep 2, 2019
9965940 doc: Add release note for the new gettransaction argument (darosior)
b8b3f04 tests: Add a new functional test for gettransaction (darosior)
7f3bb24 gettransaction: add an argument to decode the transaction (darosior)

Pull request description:

  This PR adds a new parameter to the `gettransaction` call : `decode`. If set to `true`, it will add a new `decoded` field to the response. This mimics the behavior of `getrawtransaction`'s `verbose` argument to avoid using 2 calls if we want to decode a wallet transaction (`gettransaction` then `decoderawtransaction`).

  Fix #16181 .

ACKs for top commit:
  meshcollider:
    re-utACK 9965940

Tree-SHA512: bcb6b4bd252b3488d6afc77659c499c2ad99fd58661eb24b6a2e17014c74f22e47fde70e00fedb4f4754915786622ad02483b2cf2c4dea0ab0eb4ac8276dbeee
@meshcollider meshcollider merged commit 9965940 into bitcoin:master Sep 2, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@darosior darosior deleted the darosior:decode_tx_gettransaction branch Sep 2, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 3, 2019
…ansaction

9965940 doc: Add release note for the new gettransaction argument (darosior)
b8b3f04 tests: Add a new functional test for gettransaction (darosior)
7f3bb24 gettransaction: add an argument to decode the transaction (darosior)

Pull request description:

  This PR adds a new parameter to the `gettransaction` call : `decode`. If set to `true`, it will add a new `decoded` field to the response. This mimics the behavior of `getrawtransaction`'s `verbose` argument to avoid using 2 calls if we want to decode a wallet transaction (`gettransaction` then `decoderawtransaction`).

  Fix bitcoin#16181 .

ACKs for top commit:
  meshcollider:
    re-utACK 9965940

Tree-SHA512: bcb6b4bd252b3488d6afc77659c499c2ad99fd58661eb24b6a2e17014c74f22e47fde70e00fedb4f4754915786622ad02483b2cf2c4dea0ab0eb4ac8276dbeee
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
This adds a new boolean parameter 'decode' to the gettransaction call, which, if set to true, add a 'decoded' field to the result containing the decoded transaction

Github-Pull: bitcoin#16185
Rebased-From: 7f3bb24
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
@@ -1684,11 +1685,13 @@ static UniValue gettransaction(const JSONRPCRequest& request)
" ,...\n"
" ],\n"
" \"hex\" : \"data\" (string) Raw data for transaction\n"
" \"decoded\" : transaction (json object) Optional, the decoded transaction\n"

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 4, 2019

Member

Should mention that the field is identical to decoderawtransaction?

@jnewbery

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Sorry for the post-merge review comment, but would it make sense to call the new parameter verbose, to mirror the verbose argument in getrawtransaction?

If so, we should make that change before v0.19 to avoid an API change.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Sorry for the post-merge review comment, but would it make sense to call the new parameter verbose, to mirror the verbose argument in getrawtransaction?

If so, we should make that change before v0.19 to avoid an API change.

This seems like a good idea. You could open a one-line PR and add the milestone to get more feedback.

@promag

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

+1 for API consistency.

jonatack added a commit to jonatack/bitcoin that referenced this pull request Sep 14, 2019
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.

However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.

This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.

It also addresses [this comment](bitcoin#16185 (comment)) to mention that the 'decoded' field is identical to decoderawtransaction.

Update the RPC help, functional test, and release note.
meshcollider added a commit that referenced this pull request Sep 15, 2019
1b41c2c test: improve gettransaction test coverage (Jon Atack)
0f34f54 rpc: fix regression in gettransaction (Jon Atack)

Pull request description:

  Closes #16872.

  PR #16866 renamed the `decode` argument in gettransaction to `verbose` to make it more consistent with other RPC calls like getrawtransaction. However, it inadvertently overloaded the "details" field when `verbose` is passed. The result is that the original "details" field is no longer returned correctly, which seems to be a breaking API change.

  This PR:

  - takes the simplest path to restoring the "details" field by renaming the decoded one back to "decoded" while leaving the `verbose` argument for API consistency, which was the main intent of #16866,

  - addresses [this comment](#16185 (comment)) by mentioning in the RPC help that the new decoded field is equivalent to decoderawtransaction, and

  - updates the help, functional test, and release note.

  Reviewers, to test this manually, build and run `bitcoin-cli help gettransaction` and `bitcoin-cli gettransaction <wallet txid> false true`, and verify that the command returns both `details` and `decoded` fields.

ACKs for top commit:
  jnewbery:
    tACK 1b41c2c

Tree-SHA512: 287edd5db7ed58fe8b548975aba58628bd45ed708b28f40174f10a35a455d89f796fbf27430aa881fc376f47aabda8803f74d4d100683bd86577a02279091cf3
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 16, 2019
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.

However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.

This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.

It also addresses [this comment](bitcoin#16185 (comment)) to mention that the 'decoded' field is identical to decoderawtransaction.

Update the RPC help, functional test, and release note.
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 19, 2019
This adds a new boolean parameter 'decode' to the gettransaction call, which, if set to true, add a 'decoded' field to the result containing the decoded transaction

Github-Pull: bitcoin#16185
Rebased-From: 7f3bb24
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 19, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 21, 2019
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.

However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.

This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.

It also addresses [this comment](bitcoin#16185 (comment)) to mention that the 'decoded' field is identical to decoderawtransaction.

Update the RPC help, functional test, and release note.

Github-Pull: bitcoin#16873
Rebased-From: 0f34f54
PierreRochard added a commit to PierreRochard/bitcoin that referenced this pull request Oct 12, 2019
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.

However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.

This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.

It also addresses [this comment](bitcoin#16185 (comment)) to mention that the 'decoded' field is identical to decoderawtransaction.

Update the RPC help, functional test, and release note.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.