RPC: Add universal options argument to listtransactions

kristapsk commented at 5:06 pm on August 26, 2021: contributor

Taken from #19443. There two additional arguments paginatebypointer and nextpagepointer were added and @luke-jr proposed that they could be united with existing skip argument into single options JSON argument. This is better approach than continue adding more and more different arguments for different output options. #22775 proposes to add sorting order as additional argument, I think it’s better to use the same approach there too. Having this as a separate PR will make reviews and testing simpler.

Tests for old and new way of providing skip argument should be added, but didn’t do it here as functional tests currently don’t have tests for skip at all. I plan to add them later in a separate PR.

ryanofsky commented at 5:26 pm on August 26, 2021: contributor

I think #19762 is a better solution to the problem of supporting large numbers of optional arguments. #19762 allows options to be specified without having to quote JSON on the command line and provides a more natural calling interface for languages like python that support named arguments. Unlike this PR, it is fully generic and does not require adding and maintaining confusing backwards compatibility code inside individual RPC method implementations.

#19762 can also be a foundation for supporting future usability improvements like being able to declare individual arguments keyword-only (only allowed to be passed by name and not position).

DrahtBot added the label RPC/REST/ZMQ on Aug 26, 2021

DrahtBot added the label Wallet on Aug 26, 2021

luke-jr commented at 9:25 pm on August 26, 2021: member

Concept ACK

I think #19762 is a better solution to the problem of supporting large numbers of optional arguments.

It’s a good idea, but non-standard and IMO complimentary.

#19762 can also be a foundation for supporting future usability improvements like being able to declare individual arguments keyword-only (only allowed to be passed by name and not position).

This would make sense if positional use was deprecated, which it is not (and should not be sabotaged by making poor parameter positions or options unavailable).

in src/wallet/rpcwallet.cpp:1481 in aab6389b10 outdated

1478     int nFrom = 0;
1479-    if (!request.params[2].isNull())
1480-        nFrom = request.params[2].get_int();
1481+    const UniValue& options = request.params[2];
1482+    if (!options.isNull()) {
1483+        if (options.type() == UniValue::VNUM) {

luke-jr commented at 9:27 pm on August 26, 2021:

0    if (options.type() == UniValue::VNUM) {
1    ...
2    } else if (!options.isNull()) {

in src/wallet/rpcwallet.cpp:1490 in aab6389b10 outdated

1487+        else {
1488+            RPCTypeCheckArgument(options, UniValue::VOBJ);
1489+            RPCTypeCheckObj(options,
1490+                {
1491+                    {"skip", UniValueType(UniValue::VNUM)},
1492+                }, true, true);

luke-jr commented at 9:30 pm on August 26, 2021:

0                }, /* allow missing/null keys */ true, /* disallow unknown keys */ true);

luke-jr approved

luke-jr commented at 9:30 pm on August 26, 2021: member

utACK, a few minor nits

ryanofsky commented at 9:59 pm on August 26, 2021: contributor

Concept ACK

I think #19762 is a better solution to the problem of supporting large numbers of optional arguments.

It’s a good idea

Great, review is welcome.

, but non-standard

non-standard != not standardized

The “args” parameter added by #19762 is documented, but not standardized, in the exact same way the “options” parameter added by this PR is documented, but not standardized

and IMO complimentary.

Complimentary to what? It is not complimentary to this PR. It is incompatible with this PR.

#19762 can also be a foundation for supporting future usability improvements like being able to declare individual arguments keyword-only (only allowed to be passed by name and not position).

This would make sense if positional use was deprecated, which it is not

It makes sense in combination with #19762, period.

kristapsk commented at 10:20 am on August 27, 2021: contributor

functional tests currently don’t have tests for skip at all

I was wrong, skip is covered by test/functional/wallet_coinbase_category.py.

kristapsk force-pushed on Aug 27, 2021

kristapsk commented at 10:37 am on August 27, 2021: contributor

Addressed review suggestions from @luke-jr.

kristapsk force-pushed on Aug 28, 2021

kristapsk commented at 6:23 pm on August 28, 2021: contributor

Removed unnecessary extra blank line inside RPCHelpMan() added by accident.

promag commented at 6:00 pm on August 29, 2021: member

NACK 3e8b4dd220b2ddf8ae7f10808d529e4b09058180, why not just append options argument? Or in #19443 just append new arguments.

kristapsk commented at 2:20 pm on August 30, 2021: contributor

@promag That was initial way it was done, to add new positional arguments, see #14898. It was @luke-jr idea to add it instead in place of existing skip and I think it makes sense. Together skip, paginatebypointer, nextpagepointer and ascending_order can be seen as analogue to MySQL’s ORDER BY x LIMIT y,z and they will be used most likely rarely, so not adding four additional positional arguments IMHO is better approach.

promag commented at 8:03 am on September 3, 2021: member

@kristapsk I think it’s mixing 2 different things: add options parameter VS allow shorter -cli calls. If we want to simplify/improve -cli usage then I think we should try @ryanofsky approach first before “hijacking” one argument to support the other on the server-side.

in src/wallet/rpcwallet.cpp:1415 in 3e8b4dd220 outdated

1411@@ -1412,7 +1412,11 @@ static RPCHelpMan listtransactions()
1412                     {"label|dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
1413                           "with the specified label, or \"*\" to disable filtering and return all transactions."},
1414                     {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
1415-                    {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
1416+                    {"options|skip", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a number instead of an object will result in {\"skip\":nnn}",

jonatack commented at 3:54 pm on September 15, 2021:

I’m not sure here, but previous reviews on questions like this and doc/developer-notes.md generally seemed to discourage mixing types / overloading:

0- Try not to overload methods on argument type. E.g. don't make `getblock(true)`
1  and `getblock("hash")` do different things.

luke-jr commented at 8:03 pm on September 15, 2021:

That’s about supported use, not deprecation compatibility…

meshcollider commented at 0:58 am on December 8, 2021:

Shouldn’t the old use be deprecated with planned removal then?

DrahtBot commented at 4:17 pm on December 2, 2021: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	promag
Concept ACK	luke-jr

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Dec 8, 2021

maflcko closed this on Jul 25, 2022

kristapsk commented at 2:24 pm on September 26, 2022: contributor

Could this be reopened? Will rebase.

glozow reopened this on Sep 26, 2022

kristapsk force-pushed on Sep 26, 2022

kristapsk commented at 8:01 pm on September 26, 2022: contributor

Rebased

DrahtBot removed the label Needs rebase on Sep 26, 2022

fanquake commented at 10:51 am on December 6, 2022: member

Note that #19762, which is mentioned in the early commentary on this PR, has now recently been merged.

maflcko commented at 1:49 pm on January 12, 2023: member

0wallet/rpc/transactions.cpp:555:5: error: no matching function for call to ‘RPCHelpMan::RPCHelpMan(<brace-enclosed initializer list>)’

kristapsk commented at 4:24 pm on January 12, 2023: contributor

@MarcoFalke I cannot reproduce that compiler error. Try make clean?

maflcko commented at 4:46 pm on January 12, 2023: member

Did you run git rebase bitcoin-core/master?

maflcko added the label Needs rebase on Jan 12, 2023

kristapsk commented at 4:57 pm on January 12, 2023: contributor

Did you run git rebase bitcoin-core/master?

Ok, seeing error after rebase. Will look at this.

DrahtBot removed the label Needs rebase on Jan 12, 2023

kristapsk force-pushed on Jan 15, 2023

kristapsk commented at 3:41 pm on January 15, 2023: contributor

Rebased

kristapsk force-pushed on Apr 12, 2023

kristapsk commented at 4:22 pm on April 12, 2023: contributor

Rebased

maflcko commented at 5:06 am on July 11, 2023: member

I think you’ll still need to rebase and fix the compile error

kristapsk force-pushed on Jul 12, 2023

DrahtBot added the label CI failed on Jul 12, 2023

in src/wallet/rpc/transactions.cpp:441 in 7701d951d9 outdated

437@@ -438,7 +438,11 @@ RPCHelpMan listtransactions()
438                     {"label|dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "If set, should be a valid label name to return only incoming transactions\n"
439                           "with the specified label, or \"*\" to disable filtering and return all transactions."},
440                     {"count", RPCArg::Type::NUM, RPCArg::Default{10}, "The number of transactions to return"},
441-                    {"skip", RPCArg::Type::NUM, RPCArg::Default{0}, "The number of transactions to skip"},
442+                    {"options|skip", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a number instead of an object will result in {\"skip\":nnn}",

maflcko commented at 6:29 am on July 13, 2023:

0wallet/rpc/transactions.cpp:441:75: error: no member named 'OMITTED_NAMED_ARG' in 'RPCArg::Optional'
1                    {"options|skip", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a number instead of an object will result in {\"skip\":nnn}",
2                                                        ~~~~~~~~~~~~~~~~~~^
3wallet/rpc/transactions.cpp:508:9: error: use of undeclared identifier 'RPCTypeCheckArgument'
4        RPCTypeCheckArgument(options, UniValue::VOBJ);
5        ^
62 errors generated.

Add universal options argument to listtransactions 3a88c2967d

kristapsk force-pushed on Jul 13, 2023

kristapsk commented at 2:20 pm on July 13, 2023: contributor

Now it builds, but backwards compatibility with passing number instead of object as a third parameter is broken, will look at it later.

maflcko commented at 8:47 am on July 21, 2023: member

Maybe mark as draft/WIP for as long as this is still WIP?

kristapsk marked this as a draft on Jul 21, 2023

maflcko commented at 5:38 pm on September 20, 2023: member

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

kristapsk commented at 6:05 pm on September 20, 2023: contributor

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

Will try to look at this. There was bug after rebase (see comment above), need to go through other code changes to understand properly what has changed and what causes the issue.

glozow commented at 11:18 am on December 20, 2023: member

Are you still working on this?

kristapsk commented at 12:59 pm on December 20, 2023: contributor

Are you still working on this?

Not at the moment. Feel free to close. Will open a new PR in future if will get back to this.

maflcko closed this on Dec 20, 2023

RPC: Add universal options argument to listtransactions #22807

Code Coverage

Reviews

Conflicts