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

kristapsk commented at 8:01 AM on July 4, 2020: contributor

This is #14898 rebased against current master, review comments addressed and help text updated with output changes when pageinatebypointer option is used.

~Added fifth param to listtransactions named options,~ Previous skip argument is replaced with options, with backwards compatibility, where it's treated as a skip if it's integer, not an object. 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)

fanquake added the label RPC/REST/ZMQ on Jul 4, 2020

in src/wallet/rpcwallet.cpp:1398 in 4d649021ef outdated

1395 |                  {
1396 |                      {"label|dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
1397 |                            "with the specified label, or \"*\" to disable filtering and return all transactions."},
1398 |                      {"count", RPCArg::Type::NUM, /* default */ "10", "The number of transactions to return"},
1399 | -                    {"skip", RPCArg::Type::NUM, /* default */ "0", "The number of transactions to skip"},
1400 | +                    {"skip", RPCArg::Type::NUM, /* default */ "0", "The number of transactions to skip. If \"usesnextpagepointer\" option is true, skip won't have an effect."},

luke-jr commented at 8:03 AM on July 4, 2020:

I wonder if we should just accept options as the 2nd argument in place of skip...

kristapsk commented at 8:06 AM on July 4, 2020:

That will break userland. Or we could have different behaviour depending if it's a number or JSON object?

luke-jr commented at 8:19 AM on July 4, 2020:

Yes, that's what I mean. We already can have multiple names for the same position.

promag commented at 5:57 PM on August 29, 2021:

#19443 (review)

An argument can have aliases but type is same.

DrahtBot commented at 2:51 PM on July 4, 2020: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	instagibbs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20012 (rpc: Remove duplicate name and argNames from CRPCCommand 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.

instagibbs commented at 3:30 PM on August 5, 2020: member

concept ACK, this is a pretty straight forward improvement I think as far as an API is concerned. Would be helpful to find people needing this API to test.

kristapsk force-pushed on Aug 26, 2020

kristapsk commented at 9:32 PM on August 26, 2020: contributor

Addressed review comment by @luke-jr , agree this way is better.

DrahtBot added the label Needs rebase on Sep 23, 2020

kristapsk force-pushed on Sep 25, 2020

kristapsk commented at 10:22 PM on September 25, 2020: contributor

Rebased (+ options|skip changes required by #19994).

DrahtBot removed the label Needs rebase on Sep 25, 2020

in src/wallet/rpcwallet.cpp:1366 in d2338091e0 outdated

1361 | + * @param  filter_ismine  The "is mine" filter flags.
1362 | + * @param  filter_label   Optional label string to filter incoming transactions.
1363 | + */
1364 | +static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter_ismine, const std::string* filter_label)  EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1365 | +{
1366 | +    (void)ListTransactions(pwallet, wtx, nMinDepth, fLong, ret, filter_ismine, filter_label, -1);

luke-jr commented at 9:27 PM on December 13, 2020:

Why not just set a default value for limit above?

kristapsk commented at 11:50 PM on December 13, 2020:

Good catch, will do!

rpc listtransactions new argument options (paginatebypointer impl)

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)

Co-authored-by: Kristaps Kaupe <kristaps@blogiem.lv>

480df47f8f

kristapsk force-pushed on Dec 14, 2020

in src/wallet/rpcwallet.cpp:4655 in 480df47f8f

4651 | @@ -4492,7 +4652,7 @@ static const CRPCCommand commands[] =
4652 |      { "wallet",             "listreceivedbyaddress",            &listreceivedbyaddress,         {"minconf","include_empty","include_watchonly","address_filter"} },
4653 |      { "wallet",             "listreceivedbylabel",              &listreceivedbylabel,           {"minconf","include_empty","include_watchonly"} },
4654 |      { "wallet",             "listsinceblock",                   &listsinceblock,                {"blockhash","target_confirmations","include_watchonly","include_removed"} },
4655 | -    { "wallet",             "listtransactions",                 &listtransactions,              {"label|dummy","count","skip","include_watchonly"} },
4656 | +    { "wallet",             "listtransactions",                 &listtransactions,              {"label|dummy","count","options|skip","include_watchonly"} },

maflcko commented at 6:41 PM on January 28, 2021:

can remove this diff, and then rebase

DrahtBot added the label Needs rebase on Jan 28, 2021

DrahtBot commented at 6:59 PM on January 28, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

ghost commented at 2:59 PM on August 25, 2021: none

Tried this with new argument added according to the examples. TBH this looks complex compared to PR 22775 or maybe I am not using it correctly. Only thing I noticed different which could be helpful in some cases was nextpagepointer in results.

kristapsk commented at 4:34 PM on August 25, 2021: contributor

@prayank23 Most of the complexity is for paginatebypointer. Probably I could rebase this and split into two commits, then adding options object as a second argument could be applied to #22775 separately.

kristapsk commented at 6:51 PM on August 26, 2021: contributor

I'm putting this on hold and not going to rebase until either #19762 or #22807 is merged, don't think anymore it's good idea to add additional positional arguments to listtransactions for this.

DrahtBot commented at 12:29 PM on December 22, 2021: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

glozow commented at 10:01 AM on September 26, 2022: member

@kristapsk are you still working on this?

glozow closed this on Sep 26, 2022

glozow reopened this on Sep 26, 2022

glozow added the label Waiting for author on Sep 26, 2022

kristapsk commented at 1:34 PM on September 26, 2022: contributor

@glozow Waiting on #19762, will it be merged or rejected.

luke-jr commented at 2:07 PM on September 26, 2022: member

#19762 shouldn't be relevant to this. Even if both named and positional are supported, the pure positional API should be clean.

kristapsk commented at 2:17 PM on September 26, 2022: contributor

@luke-jr So you think it's worth resuming work on #22807?

luke-jr commented at 2:20 PM on September 26, 2022: member

Yes

kristapsk commented at 8:25 AM on September 27, 2022: contributor

#22807 is rebased, don't think it's worth rebasing this before that one is either merged or rejected.

DrahtBot commented at 2:00 AM on December 26, 2022: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

fanquake commented at 1:44 PM on January 12, 2023: member

don't think it's worth rebasing this before that one is either merged or rejected.

Made this a draft for now then.

fanquake marked this as a draft on Jan 12, 2023

DrahtBot commented at 1:03 AM on April 12, 2023: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot commented at 1:47 AM on July 11, 2023: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko removed the label Waiting for author on Sep 20, 2023

maflcko commented at 4:54 PM on September 20, 2023: member

Are you still working on this? If not, maybe close for now?

DrahtBot commented at 1:53 AM on December 19, 2023: contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

glozow commented at 11:17 AM on December 20, 2023: member

Closing due to lack of activity. Feel free to ping if this is picked up again.

glozow closed this on Dec 20, 2023

bitcoin locked this on Dec 19, 2024