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] Restore ability to list incoming transactions by label #14411

Merged
merged 2 commits into from Nov 14, 2018

Conversation

Projects
None yet
10 participants
@ryanofsky
Copy link
Contributor

ryanofsky commented Oct 6, 2018

This change partially reverts #13075 and #14023.

Fixes #14382

@fanquake fanquake added the Wallet label Oct 6, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 6, 2018

Is this for backport to 0.17.1? While not necessary, I believe it would simplify upgrade from 0.17.1+ to 0.18.0, because 0.17.1 could be used without -deprecatedrpc as workaround.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Oct 6, 2018

I think it'd be reasonable to backport this, since it's a small change that restores a removed feature. But I think it won't cherry-pick cleanly because #14023 was merged since the 0.17 branch. @Saicere originally reported the issue so they might be able to say whether there's a benefit to backporting.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 6, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14726 (Use RPCHelpMan for all RPCs by MarcoFalke)

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.

@Saicere

This comment has been minimized.

Copy link

Saicere commented Oct 7, 2018

@Saicere originally reported the issue so they might be able to say whether there's a benefit to backporting.

As far as I am concerned, it's not really a big deal whether or not it's backported to the 0.17 branch as long as it's in before accounts are removed entirely. If you were using accounts in your application, you are probably running 0.17 with -deprecatedrpc=accounts anyway while RPC-facing code is updated.

@kristapsk

This comment has been minimized.

Copy link
Contributor

kristapsk commented Oct 7, 2018

ACK 8fcb765

@promag
Copy link
Member

promag left a comment

Concept ACK.

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 8, 2018

Concept ACK. For completeness, this functionality was hidden behind a deprecation switch in #12953 (based on #11497). The functionality was marked deprecated in #5575.

Apologies - it was my oversight that this functionality got dropped. I'm happy to backport this to V0.17 once this gets merged. I aim to review this week.

@jnewbery
Copy link
Member

jnewbery left a comment

Tested ACK 8fcb765 with a couple of nits.

@MarcoFalke - I'll go ahead and open a PR for backporting. A couple of questions:

  • does the backport need release notes?
  • if we add release notes for the backport, do we also need release notes here?
Show resolved Hide resolved test/functional/wallet_listtransactions.py Outdated
Show resolved Hide resolved test/functional/wallet_listtransactions.py Outdated
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 9, 2018

We assume the reader goes through the release notes for each release, so it should be sufficient to only mention on the 0.17. branch.

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 9, 2018

Backport is #14441. PR numbering was totally intentional.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/list-label branch from 8fcb765 to fa7fae5 Oct 10, 2018

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

Added 1 commit 8fcb765 -> 665d00f (pr/list-label.1 -> pr/list-label.2, compare) with suggested fixes.
Squashed 665d00f -> fa7fae5 (pr/list-label.2 -> pr/list-label.3)
Updated fa7fae5 -> 4deba4c (pr/list-label.3 -> pr/list-label.4) with release notes changes to reflect the intent to backport this to 0.17.1

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_listtransactions.py Outdated
Show resolved Hide resolved test/functional/wallet_listtransactions.py Outdated

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/list-label branch from fa7fae5 to 4deba4c Oct 10, 2018

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 10, 2018

ACK 4deba4c

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 10, 2018

The release note should be removed from this PR (assuming #14441 is merged)

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/list-label branch from 4deba4c to 7cbe74f Oct 15, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Oct 15, 2018

Updated 4deba4c -> 7cbe74f (pr/list-label.4 -> pr/list-label.5) removing release notes and changing whitespace as suggested.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 15, 2018

Very nitty comment; it feels like this functionality is closer to listreceivedbylabel than to listtransactions (as the latter is about all transactions which affect the balance of the wallet - and formerly account) rather than just incoming payments. Would it make sense to add it as an extra option to listreceivedbylabel (verbose or so), or a separate RPC, instead?

@Saicere

This comment has been minimized.

Copy link

Saicere commented Oct 15, 2018

Would it make sense to add it as an extra option to listreceivedbylabel (verbose or so), or a separate RPC, instead?

listreceivedbylabel doesn't take a label argument or list transactions in any way, it just lists total received coins per label, as per the code.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 16, 2018

ACK 7cbe74f. Only change since 4deba4c is removing release notes and changing whitespace.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Oct 16, 2018

Would it make sense to add it as an extra option to listreceivedbylabel (verbose or so), or a separate RPC, instead?

It'd be good to improve listreceivedbylabel. But filtering by label in listtransactions used to work previously and I think was just removed by accident.

You could maybe say that listtransactions shouldn't return labels or filter by label at all, since outgoing transactions won't have labels. But I don't think it makes sense to keep returning labels and only remove the ability to filter them.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 12, 2018

Rename ListTransactions filter variable
Suggested by MeshCollider <dobsonsa68@gmail.com> in
bitcoin#14411 (comment)
@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Nov 12, 2018

Rebased 7cbe74f -> 671d8ee (pr/list-label.5 -> pr/list-label.6) due to conflict with #14437. Also renamed filter variable as suggested by MeshCollider.

@MarcoFalke MarcoFalke modified the milestones: 0.17.1, 0.18.0 Nov 12, 2018

@MarcoFalke MarcoFalke modified the milestones: 0.18.0, 0.17.1 Nov 12, 2018

@MarcoFalke MarcoFalke modified the milestones: 0.17.1, 0.18.0 Nov 12, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 12, 2018

Unless I am mistaken this has been "backported" in 5150acc. So this must go into 0.18.0 instead.
(assigned new milestone)

@DrahtBot DrahtBot removed the Needs rebase label Nov 12, 2018

@@ -1352,10 +1356,12 @@ UniValue listtransactions(const JSONRPCRequest& request)

if (request.fHelp || request.params.size() > 4)
throw std::runtime_error(
"listtransactions (dummy count skip include_watchonly)\n"
"listtransactions ( label count skip include_watchonly )\n"

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 13, 2018

Member

From the documentation below, label is a string, so should be surrounded by quotation marks in the oneline doc.

Suggested change Beta
"listtransactions ( label count skip include_watchonly )\n"
"listtransactions ( \"label\" count skip include_watchonly )\n"

This comment has been minimized.

@ryanofsky

ryanofsky Nov 13, 2018

Contributor

re: #14411 (comment)

From the documentation below, label is a string, so should be surrounded by quotation marks in the oneline doc.

Done in rebase.

ryanofsky added some commits Oct 6, 2018

Rename ListTransactions filter variable
Suggested by MeshCollider <dobsonsa68@gmail.com> in
#14411 (comment)

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/list-label branch from 671d8ee to ae81320 Nov 13, 2018

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Nov 13, 2018

Rename ListTransactions filter variable
Suggested by MeshCollider <dobsonsa68@gmail.com> in
bitcoin#14411 (comment)

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/list-label branch 3 times, most recently from ae81320 to da427db Nov 13, 2018

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

Rebased 671d8ee -> da427db (pr/list-label.6 -> pr/list-label.7) due to conflict with #14720

@@ -1352,10 +1356,12 @@ UniValue listtransactions(const JSONRPCRequest& request)

if (request.fHelp || request.params.size() > 4)
throw std::runtime_error(
"listtransactions (dummy count skip include_watchonly)\n"
"listtransactions ( label count skip include_watchonly )\n"

This comment has been minimized.

@ryanofsky

ryanofsky Nov 13, 2018

Contributor

re: #14411 (comment)

From the documentation below, label is a string, so should be surrounded by quotation marks in the oneline doc.

Done in rebase.

@DrahtBot DrahtBot removed the Needs rebase label Nov 13, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 14, 2018

ACK da427db (Only checked that the rebase was done properly from the last time people reviewed this. Also checked that the tests are the same as in the backport 5150acc, didn't look at the code)

@MarcoFalke MarcoFalke merged commit da427db into bitcoin:master Nov 14, 2018

2 checks passed

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

MarcoFalke added a commit that referenced this pull request Nov 14, 2018

Merge #14411: [wallet] Restore ability to list incoming transactions …
…by label

da427db Rename ListTransactions filter variable (Russell Yanofsky)
65b740f [wallet] Restore ability to list incoming transactions by label (Russell Yanofsky)

Pull request description:

  This change partially reverts #13075 and #14023.

  Fixes #14382

Tree-SHA512: 8c4e56104b3a45784cdc06bae8e5facdfff04fe3545b63a35e0ec2e440a41b79d84833ca4c4e728d8af7ebb8a519303a9eda7bee4bbfb92bd50c58587a33eb30
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Nov 21, 2018

utACK da427db

JeremyRubin added a commit to JeremyRubin/bitcoin that referenced this pull request Nov 24, 2018

Rename ListTransactions filter variable
Suggested by MeshCollider <dobsonsa68@gmail.com> in
bitcoin#14411 (comment)

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 26, 2018

Rename ListTransactions filter variable
Suggested by MeshCollider <dobsonsa68@gmail.com> in
bitcoin#14411 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment