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

[wallet] Backport(0.17): Restore ability to list incoming transactions by label #14441

Merged
merged 1 commit into from Nov 10, 2018

Conversation

Projects
None yet
7 participants
@jnewbery
Copy link
Member

jnewbery commented Oct 9, 2018

Backport of PR #14411 to v0.17.

This change partially reverts #13075 and #14023.

Fixes #14382

@jnewbery jnewbery changed the title [wallet] Restore ability to list incoming transactions by label [wallet] Backport(0.17): Restore ability to list incoming transactions by label Oct 9, 2018

@fanquake fanquake added this to the 0.17.1 milestone Oct 9, 2018

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK c8d1f15

@@ -71,7 +71,17 @@ Notable changes
0.17.x change log
=================

...
`listtransaction` label support

This comment has been minimized.

@ryanofsky

ryanofsky Oct 10, 2018

Contributor

Should be listtransactions with s.

This comment has been minimized.

@jnewbery
The `listtransactions` RPC `account` parameter which was deprecated in 0.17.0
and renamed to `dummy` has been un-deprecated and renamed again to `label`.

The behavior of the `label` argument is identical to previous behavior of the

This comment has been minimized.

@ryanofsky

ryanofsky Oct 10, 2018

Contributor

This paragraph doesn't really make sense when both behaviors are present in the same release. I'd maybe change it to:

When bitcoin is configured with the -deprecatedrpc=accounts setting,
specifying a label/account/dummy argument will return both outgoing and
incoming transactions. Without the -deprecatedrpc=accounts setting, it will
only return incoming transactions (because it used to be possible to create
transactions spending from specific accounts, but this is no longer possible
with labels).

This comment has been minimized.

@ryanofsky

ryanofsky Oct 10, 2018

Contributor

I think it'd be good to document the empty string change. Maybe:

Also for backwards compatibility when -deprecatedrpc=accounts is set, it's
possible to pass an empty string label "" to list transactions that aren't
labeled. Without -deprecatedrpc=accounts, passing the empty string is an
error because returning only non-labeled transactions isn't actually very
useful, and can be confusing.

This comment has been minimized.

@jnewbery

jnewbery Oct 10, 2018

Member

Both are good changes. Thanks!

@@ -2010,10 +2017,10 @@ UniValue listtransactions(const JSONRPCRequest& request)
pwallet->BlockUntilSyncedToCurrentChain();

std::string strAccount = "*";
if (!request.params[0].isNull()) {
if (!request.params[0].isNull() && request.params[0].get_str() != "*") {

This comment has been minimized.

@ryanofsky

ryanofsky Oct 10, 2018

Contributor

I don't see how the new check for "*" here changes anything. Maybe drop for clarity (unless I'm missing something).

This comment has been minimized.

@jnewbery

jnewbery Oct 10, 2018

Member

No, you're right. It's better without. The behaviour would be identical with or without the check for "*". I've removed it.

[wallet] Restore ability to list incoming transactions by label
Backport of PR 14411 to v0.17.

This change partially reverts #13075 and #14023.

Fixes #14382

@jnewbery jnewbery force-pushed the jnewbery:restore_listtransactions_label branch from c8d1f15 to 89306ab Oct 10, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 10, 2018

Thanks for the review @ryanofsky! I've updated the PR with your changes.

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 89306ab. Only changes since last review were code simplification and release notes updates suggested above.

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 10, 2018

utACK 89306ab.

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Nov 9, 2018

utACK 89306ab

This is slightly different from other backport PRs because the changes to src/wallet/rpcwallet.cpp are significantly different from the original PR to achieve the same functionality. The tests are basically a clean backport though.

@laanwj laanwj merged commit 89306ab into bitcoin:0.17 Nov 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 10, 2018

Merge #14441: [wallet] Backport(0.17): Restore ability to list incomi…
…ng transactions by label

89306ab [wallet] Restore ability to list incoming transactions by label (Russell Yanofsky)

Pull request description:

  Backport of PR #14411 to v0.17.

  This change partially reverts #13075 and #14023.

  Fixes #14382

Tree-SHA512: 1f8300e1a79e826cd706561265b8788deef505fa510be1a76ed9a62e5fca37cf6a741423ac0e5de2a36d6e8b9f25f141885455aacacbbf6474814e6eae406a27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment