RPC: Add universal options argument to listtransactions #22807

kristapsk · 2021-08-26T17:06:09Z

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 · 2021-08-26T17:26:23Z

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).

luke-jr · 2021-08-26T21:25:40Z

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).

luke-jr

utACK, a few minor nits

src/wallet/rpcwallet.cpp

ryanofsky · 2021-08-26T21:59:57Z

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 · 2021-08-27T10:20:53Z

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 · 2021-08-27T10:37:38Z

Addressed review suggestions from @luke-jr.

kristapsk · 2021-08-28T18:23:29Z

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

promag

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

kristapsk · 2021-08-30T14:20:06Z

@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 · 2021-09-03T08:03:45Z

@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.

jonatack · 2021-09-15T15:54:00Z

src/wallet/rpcwallet.cpp

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

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

That's about supported use, not deprecation compatibility...

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

DrahtBot · 2021-12-02T16:17:28Z

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.

kristapsk · 2022-09-26T14:24:40Z

Could this be reopened? Will rebase.

kristapsk · 2022-09-26T20:01:41Z

Rebased

fanquake · 2022-12-06T10:51:00Z

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

maflcko · 2023-01-12T13:49:52Z

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

kristapsk · 2023-01-12T16:24:06Z

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

maflcko · 2023-01-12T16:46:06Z

Did you run git rebase bitcoin-core/master?

kristapsk · 2023-01-12T16:57:21Z

Did you run git rebase bitcoin-core/master?

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

kristapsk · 2023-01-15T15:41:09Z

Rebased

kristapsk · 2023-04-12T16:22:13Z

Rebased

maflcko · 2023-07-11T05:06:19Z

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

maflcko · 2023-07-13T06:29:38Z

src/wallet/rpc/transactions.cpp

wallet/rpc/transactions.cpp:441:75: error: no member named 'OMITTED_NAMED_ARG' in 'RPCArg::Optional' {"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}", ~~~~~~~~~~~~~~~~~~^ wallet/rpc/transactions.cpp:508:9: error: use of undeclared identifier 'RPCTypeCheckArgument' RPCTypeCheckArgument(options, UniValue::VOBJ); ^ 2 errors generated.

kristapsk · 2023-07-13T14:20:25Z

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

maflcko · 2023-07-21T08:47:12Z

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

maflcko · 2023-09-20T17:38:48Z

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

kristapsk · 2023-09-20T18:05:03Z

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 · 2023-12-20T11:18:13Z

Are you still working on this?

kristapsk · 2023-12-20T12:59:43Z

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.

DrahtBot added RPC/REST/ZMQ Wallet labels Aug 26, 2021

kristapsk mentioned this pull request Aug 26, 2021

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

Closed

luke-jr approved these changes Aug 26, 2021

View reviewed changes

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

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

kristapsk force-pushed the listtransactions-options branch from aab6389 to c2c53d9 Compare August 27, 2021 10:36

kristapsk mentioned this pull request Aug 27, 2021

rpc: Add option to list transactions from oldest to newest in listtransactions RPC command #22775

Closed

kristapsk force-pushed the listtransactions-options branch from c2c53d9 to 3e8b4dd Compare August 28, 2021 18:22

promag reviewed Aug 29, 2021

View reviewed changes

jonatack reviewed Sep 15, 2021

View reviewed changes

DrahtBot mentioned this pull request Dec 2, 2021

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

DrahtBot mentioned this pull request Dec 4, 2021

Split up rpcwallet #23667

Merged

DrahtBot added the Needs rebase label Dec 8, 2021

maflcko closed this Jul 25, 2022

glozow reopened this Sep 26, 2022

kristapsk force-pushed the listtransactions-options branch from 3e8b4dd to 29d9743 Compare September 26, 2022 19:57

DrahtBot removed the Needs rebase label Sep 26, 2022

maflcko added the Needs rebase label Jan 12, 2023

DrahtBot removed the Needs rebase label Jan 12, 2023

kristapsk force-pushed the listtransactions-options branch from 29d9743 to c739834 Compare January 15, 2023 15:40

kristapsk force-pushed the listtransactions-options branch from c739834 to 500010d Compare April 12, 2023 16:21

kristapsk force-pushed the listtransactions-options branch from 500010d to 7701d95 Compare July 12, 2023 20:57

DrahtBot added the CI failed label Jul 12, 2023

maflcko reviewed Jul 13, 2023

View reviewed changes

Add universal options argument to listtransactions

3a88c29

kristapsk force-pushed the listtransactions-options branch from 7701d95 to 3a88c29 Compare July 13, 2023 13:30

kristapsk marked this pull request as draft July 21, 2023 09:34

maflcko closed this Dec 20, 2023

bitcoin locked and limited conversation to collaborators Dec 19, 2024

RPC: Add universal options argument to listtransactions #22807

RPC: Add universal options argument to listtransactions #22807

Uh oh!

Conversation

kristapsk commented Aug 26, 2021

Uh oh!

ryanofsky commented Aug 26, 2021

Uh oh!

luke-jr commented Aug 26, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Aug 26, 2021

Uh oh!

kristapsk commented Aug 27, 2021

Uh oh!

kristapsk commented Aug 27, 2021

Uh oh!

kristapsk commented Aug 28, 2021

Uh oh!

promag left a comment • 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 Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 3, 2021

Uh oh!

jonatack Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jr Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

meshcollider Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

kristapsk commented Sep 26, 2022

Uh oh!

kristapsk commented Sep 26, 2022

Uh oh!

fanquake commented Dec 6, 2022

Uh oh!

maflcko commented Jan 12, 2023

Uh oh!

kristapsk commented Jan 12, 2023

Uh oh!

maflcko commented Jan 12, 2023

Uh oh!

kristapsk commented Jan 12, 2023

Uh oh!

kristapsk commented Jan 15, 2023

Uh oh!

kristapsk commented Apr 12, 2023

Uh oh!

maflcko commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Jul 13, 2023

Uh oh!

maflcko commented Jul 21, 2023

Uh oh!

maflcko commented Sep 20, 2023

Uh oh!

kristapsk commented Sep 20, 2023

Uh oh!

glozow commented Dec 20, 2023

promag left a comment •

edited

Loading

kristapsk commented Aug 30, 2021 •

edited

Loading

DrahtBot commented Dec 2, 2021 •

edited

Loading

maflcko commented Jul 11, 2023 •

edited

Loading