Page MenuHomePhabricator

Merge #11050: Avoid treating null RPC arguments different from missing arguments
AcceptedPublic

Authored by nakihito on Wed, Sep 4, 21:17.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky)
fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky)
e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky)
e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky)

Pull request description:

This is a followup to #10783.

- The first commit doesn't change behavior at all, just simplifies code.
- The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors.
- The third commit updates developer notes after the cleanup.
- The forth commit does some additional code cleanup in `getbalance`.

Followup changes that should happen in future PRs:

- [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133850525
- [ ] Add braces around if statements. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133851133
- [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. https://github.com/bitcoin/bitcoin/pull/11050#discussion_r133829303

Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9

Backport of Core PR11050
https://github.com/bitcoin/bitcoin/pull/11050/

Depends on D3464

Test Plan
make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR11050
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7335
Build 12713: Bitcoin ABC Teamcity Staging
Build 12712: arc lint + arc unit

Event Timeline

nakihito created this revision.Wed, Sep 4, 21:17
Owners added a reviewer: Restricted Owners Package.Wed, Sep 4, 21:17
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Sep 4, 21:17
nakihito added a comment.Wed, Sep 4, 21:36

rpc/blockchain.cpp changes were made here: D954
rpc/mining.cpp changes were excluded because we did not backport much of Core's fee code.
Changes to rpc/misc.cpp that were excluded were to outdated logging code.
signrawtransaction() was removed after being deprecated: D3978

src/wallet/rpcwallet.cpp
3009 ↗(On Diff #11089)

The next block of code is different from the original PR due to an out of order backport: D3420.

Fabien requested changes to this revision.Thu, Sep 5, 06:54

The changes to rpc/misc.cpp are missing.
It's not from an outdated code, but applies to the logging RPC backport that you did (stack starting from D3464). You need to rebase this diff on top of it.
The setaccount change is missing from rpcwallet.cpp (alias for setlabel).

src/wallet/rpcwallet.cpp
993 ↗(On Diff #11089)

request.params[1] => minconf

998 ↗(On Diff #11089)

No need to split the ifs here.

2120 ↗(On Diff #11089)

No need to split the ifs here.

This revision now requires changes to proceed.Thu, Sep 5, 06:54
nakihito updated this revision to Diff 11110.Thu, Sep 5, 19:15

Corrected nits, rebased off D3464, and added changes to logging().

nakihito edited the summary of this revision. (Show Details)Thu, Sep 5, 19:16
nakihito added a parent revision: D3464: [rpc] Add logging RPC.
nakihito added inline comments.Thu, Sep 5, 19:20
src/wallet/rpcwallet.cpp
349

Changes to setlabel() (setaccount() in the orinigal PR) where made in D1986.

Fabien accepted this revision.Fri, Sep 6, 07:17
This revision is now accepted and ready to land.Fri, Sep 6, 07:17