rpc listtransactions new argument options (paginatebypointer impl) #14898

hosseinzoda · 2018-12-08T11:54:36Z

Added fifth param to listtransactions named options, options may be an object containing paginatebypointer (boolean default: false) and nextpagepointer (string default: OMITTED).

With paginatebypointer output will have the following changes.

Return transactions is ordered by most recent transactions. Though the default does reverse the order after transactions are fetched and clipped.
skip argument has no effect. Instead nextpagepointer will be used for pagination.
Return value is an object containing, records (array of txs) and nextpagepointer (string)

src/wallet/rpcwallet.cpp

promag · 2018-12-09T18:27:34Z

Is there a strong reason to not improve existing listtransactions without being a breaking change?

hosseinzoda · 2018-12-09T20:51:20Z

@promag listtransactions has been used in many tests. I don't know what will happen. If change number one gets added to it.

Return transactions is ordered by most recent transactions. Though the original rpc does reverse the order after transactions are fetched and clipped.

There's an odd reverse call at listtransactions which is not in this one.
https://github.com/bitcoin/bitcoin/blob/fc1710fbf3ce840f0b647913be8b821089b00f5e/src/wallet/rpcwallet.cpp#L1494

Other than that adding nextpagepointer will break compatibility with listtransactions params. Only if one accepts it as interger and string and keeps the behaviour the same.

jonasschnelli · 2018-12-09T20:56:52Z

I general I like the idea of a pointer (seems more reliable) and the sort.
Not sure about cloning the call.

kristapsk · 2018-12-09T21:04:01Z

@promag listtransactions has been used in many tests. I don't know what will happen. If change number one gets added to it.

I would not worry about tests, you should anyway run the functional tests yourself if you change something in RPC. Sort order could be added as an additional optional boolean parameter IMO. As well as other changes.

hosseinzoda · 2018-12-09T21:09:13Z

Who thinks this line should get removed from listtransactions?

https://github.com/bitcoin/bitcoin/blob/fc1710fbf3ce840f0b647913be8b821089b00f5e/src/wallet/rpcwallet.cpp#L1494

sipa · 2018-12-09T22:12:47Z

@hosseinamin Well we can't remove that line; there are likely end users who rely on its behavior, even if it is suboptimal. We can make it optional by adding an extra flag, though.

kristapsk · 2018-12-09T22:22:09Z

Breaking userspace is bad, so changing default behaviour of any RPC should be avoided.

hosseinzoda · 2018-12-09T22:47:12Z

That's why i did cloned the rpc call. This is how new functionality can come in without messing with userspace.

jonasschnelli · 2018-12-09T22:53:32Z

That's why i did cloned the rpc call. This is how new functionality can come in without messing with userspace.

Cloning is one way, backward compatible parameterising would be another (see how the account system was removed).

hosseinzoda · 2018-12-09T22:56:25Z

Okay. How's this?

skip param can morph to nextpagepointer when string is given.
adding nonlegacy parameter at the end to avoid messing with userspace.

hosseinzoda · 2018-12-09T22:59:55Z

I think skip accepts string as input. It will convert to integer. A bit more work is needed to detect between the two.

hosseinzoda · 2018-12-10T00:22:56Z

It's also seems like changes on ListTransactions2 is trivial. ListTransactions with no change works.

hosseinzoda · 2018-12-10T04:08:46Z

@jonasschnelli Removed listransactions2, And did parametrize it.
I will try later to write some tests for it.

practicalswift · 2018-12-10T19:13:32Z

src/wallet/rpcwallet.cpp

+                } else if (i == 1) {
+                    try {
+                        nextVOut = std::stoul(s);
+                    } catch (std::invalid_argument &e) {


Drop &e? Applies to the three cases below as well :-)

kristapsk · 2018-12-10T21:16:30Z

I'm not sure a single parameter that changes multiple things is a right way to go. Wouln't be better to be able to specify sorting order and paging behaviour independently? Besides I don't like "old behaviour vs new behaviour" boolean also because in future there could be a different one "new new behaviour".

practicalswift · 2018-12-10T21:27:59Z

src/rpc/client.cpp

@@ -57,7 +57,9 @@ static const CRPCConvertParam vRPCConvertParams[] =
    { "waitfornewblock", 0, "timeout" },
    { "listtransactions", 1, "count" },
    { "listtransactions", 2, "skip" },
+    { "listtransactions", 2, "nextpagepointer" },


Nit: Isn't this typically called an index rather than a pointer?

practicalswift · 2018-12-10T21:28:42Z

src/rpc/client.cpp

    { "listtransactions", 3, "include_watchonly" },
+    { "listtransactions", 4, "newversion" },


Nit: The newversion won't be new forever :-) Can you come up with a time independent name?

DrahtBot · 2018-12-11T01:28:00Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16341 (WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)
#16240 (JSONRPCRequest-aware RPCHelpMan by kallewoof)

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.

hosseinzoda · 2018-12-11T11:10:41Z

Okay. I agree. Want to change newversion to flags. Though don't have time to do it now.

laanwj · 2019-01-22T12:45:54Z

Linter error from travis:

This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -1455,2 +1492,113 @@ UniValue listtransactions(const JSONRPCRequest& request)
+
+
+
+
+
^---- failure generated from test/lint/lint-whitespace.sh

hosseinzoda · 2019-01-23T21:45:32Z

This PR needs review.

src/wallet/rpcwallet.cpp

hosseinzoda · 2019-05-22T20:51:58Z

Hey guys. This commit is ready for merge.

src/wallet/rpcwallet.cpp

Added fifth param to `listtransactions` named `options`, `options` may be an object containing `paginatebypointer` (boolean default: false) and `nextpagepointer` (string default: OMITTED). With `paginatebypointer` output will have the following changes. 1. Return transactions is ordered by most recent transactions. Though the default does reverse the order after transactions are fetched and clipped. 2. `skip` argument has no effect. Instead `nextpagepointer` will be used for pagination. 3. Return value is an object containing, records (array of txs) and nextpagepointer (string)

DrahtBot · 2019-07-09T23:41:49Z

Needs rebase

adamjonas · 2020-05-19T01:19:48Z

Checking in on this. It's been a while since there was a rebase and there doesn't appear to be strong conviction from the reviewers on the concept/approach.

hosseinzoda · 2020-05-19T07:59:18Z

I wasted my time on this pull-request. No need for the merge anymore. Lets dump it in garbage.

kristapsk · 2020-05-19T12:10:18Z

I still think this would have been a good functionality conceptually.

fanquake added Wallet RPC/REST/ZMQ labels Dec 8, 2018

andrewtoth reviewed Dec 8, 2018

View reviewed changes

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

hosseinzoda force-pushed the master branch 3 times, most recently from 249c196 to 0115b7a Compare December 9, 2018 12:06

hosseinzoda force-pushed the master branch from fc1710f to 4829ba6 Compare December 10, 2018 04:03

hosseinzoda force-pushed the master branch 4 times, most recently from 6dd543e to d788aae Compare December 10, 2018 13:31

practicalswift reviewed Dec 10, 2018

View reviewed changes

hosseinzoda changed the title ~~nextpagepointer & list ordering options for listtransactions~~ nextpagepointer flag for listtransactions Jan 22, 2019

hosseinzoda force-pushed the master branch from e832d9e to af3b555 Compare January 22, 2019 16:08

DrahtBot added the Needs rebase label Jan 29, 2019

hosseinzoda force-pushed the master branch from af3b555 to 52a493b Compare January 30, 2019 21:55

DrahtBot removed the Needs rebase label Jan 30, 2019

maflcko reviewed Feb 7, 2019

View reviewed changes

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

DrahtBot added the Needs rebase label Feb 8, 2019

hosseinzoda force-pushed the master branch from 52a493b to ac127fc Compare May 22, 2019 02:31

hosseinzoda changed the title ~~nextpagepointer flag for listtransactions~~ rpc listtransactions new argument options (paginatebypointer impl) May 22, 2019

hosseinzoda force-pushed the master branch from ac127fc to f19bf7e Compare May 22, 2019 02:41

DrahtBot removed the Needs rebase label May 22, 2019

hosseinzoda force-pushed the master branch from f19bf7e to b3df008 Compare May 22, 2019 03:38

practicalswift reviewed May 23, 2019

View reviewed changes

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

hosseinzoda force-pushed the master branch from b3df008 to 52a493b Compare May 24, 2019 07:44

DrahtBot added the Needs rebase label May 24, 2019

hosseinzoda force-pushed the master branch from 52a493b to 8934f24 Compare May 24, 2019 08:48

DrahtBot removed the Needs rebase label May 24, 2019

DrahtBot added the Needs rebase label Jul 9, 2019

hosseinzoda closed this May 19, 2020

kristapsk mentioned this pull request Jul 4, 2020

rpc: Add options argument to listtransactions, with paginatebypointer options #19443

Closed

kristapsk mentioned this pull request Aug 30, 2021

RPC: Add universal options argument to listtransactions #22807

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		{ "listtransactions", 3, "include_watchonly" },
		{ "listtransactions", 4, "newversion" },

rpc listtransactions new argument options (paginatebypointer impl) #14898

rpc listtransactions new argument options (paginatebypointer impl) #14898

Uh oh!

Conversation

hosseinzoda commented Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

promag commented Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hosseinzoda commented Dec 9, 2018

Uh oh!

jonasschnelli commented Dec 9, 2018

Uh oh!

kristapsk commented Dec 9, 2018

Uh oh!

hosseinzoda commented Dec 9, 2018

Uh oh!

sipa commented Dec 9, 2018

Uh oh!

kristapsk commented Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hosseinzoda commented Dec 9, 2018

Uh oh!

jonasschnelli commented Dec 9, 2018

Uh oh!

hosseinzoda commented Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hosseinzoda commented Dec 9, 2018

Uh oh!

hosseinzoda commented Dec 10, 2018

Uh oh!

hosseinzoda commented Dec 10, 2018

Uh oh!

practicalswift Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Dec 10, 2018

Uh oh!

practicalswift Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Dec 10, 2018

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hosseinzoda commented Dec 11, 2018

Uh oh!

laanwj commented Jan 22, 2019

Uh oh!

hosseinzoda commented Jan 23, 2019

Uh oh!

Uh oh!

hosseinzoda commented May 22, 2019

Uh oh!

Uh oh!

DrahtBot commented Jul 9, 2019

Uh oh!

adamjonas commented May 19, 2020

Uh oh!

hosseinzoda commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented May 19, 2020

Uh oh!

Uh oh!

hosseinzoda commented Dec 8, 2018 •

edited

Loading

promag commented Dec 9, 2018 •

edited

Loading

kristapsk commented Dec 9, 2018 •

edited

Loading

hosseinzoda commented Dec 9, 2018 •

edited

Loading

practicalswift Dec 10, 2018 •

edited

Loading

DrahtBot commented Dec 11, 2018 •

edited

Loading

hosseinzoda commented May 19, 2020 •

edited

Loading