Skip to content
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

[RPC] re introducing filtering args in getbalance #1831

Merged

Conversation

furszy
Copy link

@furszy furszy commented Sep 1, 2020

Have re introduced the filtering arguments that were removed when the accounts system got deprecated in getbalance. With this, getbalance RPC command will be able, again, to filter by watch-only, minimum depth and include/exclude delegated balance.

@furszy furszy self-assigned this Sep 1, 2020
@furszy furszy force-pushed the 2020_rpc_re_introduce_filtering_args branch from 70c655c to 8efb2c5 Compare September 1, 2020 23:15
@furszy furszy force-pushed the 2020_rpc_re_introduce_filtering_args branch from 8efb2c5 to 37f0cd1 Compare September 2, 2020 19:59
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the removed argument, need to lower the size in the checks accordingly (see comments inlined).

btw, styling nit: could use a more compact syntax, like

    const int paramsSize = request.params.size();
    const int nMinDepth = (paramsSize > 0 ? request.params[0].get_int() : 0);
    isminefilter filter = ISMINE_SPENDABLE | (paramsSize > 1 && request.params[1].get_bool() ? ISMINE_WATCH_ONLY : ISMINE_NO);
    filter |= (paramsSize <= 2 || request.params[2].get_bool() ? ISMINE_SPENDABLE_DELEGATED : ISMINE_NO);

    return ValueFromAmount(pwalletMain->GetAvailableBalance(filter, true, nMinDepth));

Also, the initial check for the help needs to be updated to

if (request.fHelp || 
    (request.params.size() > 4 && IsDeprecatedRPCEnabled("accounts")) || 
    (request.params.size() > 3 && !IsDeprecatedRPCEnabled("accounts")))

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
watch only, depth and includeDelegated arguments.
@furszy furszy force-pushed the 2020_rpc_re_introduce_filtering_args branch from 37f0cd1 to 35af377 Compare September 2, 2020 23:44
@furszy
Copy link
Author

furszy commented Sep 2, 2020

yeah, great review zebra.
Made all of the changes 👍 .
We will need to add some tests for this in the future.

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the need for more specific tests over getbalance RPC.
utACK 35af377

@random-zebra random-zebra added Cleanup Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes RPC Wallet labels Sep 3, 2020
@random-zebra random-zebra added this to the 5.0.0 milestone Sep 3, 2020
@furszy furszy requested review from Fuzzbawls and removed request for Fuzzbawls September 6, 2020 04:48
@furszy furszy merged commit 28509bf into PIVX-Project:master Sep 7, 2020
@random-zebra random-zebra modified the milestones: 5.0.0, 4.3.0 Sep 10, 2020
@Fuzzbawls Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Sep 27, 2020
@furszy furszy deleted the 2020_rpc_re_introduce_filtering_args branch November 29, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants