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
refactor: Throw exception on invalid Univalue pushes over silent ignore #25551
The head ref may contain hidden characters: "2207-json-push-\u{1F3AC}"
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
Would be good to add coverage for it with few BOOST_CHECK_THROW
(one per changed method).
Thanks, done |
…f VNULL This should not change behavior and makes the code consistent with other places.
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code ACK fa277cd
Non-blocking cosmetic nit:
Could probably unify all the throw calls with something like this:
static void ThrowIfNotEqual(UniValue::VType type, UniValue::VType expected_type)
{
if (type != expected_type) throw std::runtime_error{"JSON value is not an " + std::string(uvTypeName(type)) + " as expected"};
}
Then just call ThrowIfNotEqual(typ, VARR)
or ThrowIfNotEqual(typ, VOBJ)
at the beginning of each function.
Thanks, will do in a non-refactoring follow-up |
@@ -85,6 +85,16 @@ BOOST_AUTO_TEST_CASE(univalue_constructor) | |||
BOOST_CHECK_EQUAL(v9.getValStr(), "zappa"); | |||
} | |||
|
|||
BOOST_AUTO_TEST_CASE(univalue_push_throw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only noticed on rebase these tests aren't called. Addressed in 25617
@@ -1633,7 +1633,7 @@ RPCHelpMan walletcreatefundedpsbt() | |||
}, true | |||
); | |||
|
|||
UniValue options = request.params[3]; | |||
UniValue options{request.params[3].isNull() ? UniValue::VOBJ : request.params[3]}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this well-defined, valid C++? At least in normal C, the ?:
operator requires both results to be the same type...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be, though it likely involves the copy-constructor. Pretty sure I copied it from the other places where this is used consistently.
…shes over silent ignore fa277cd univalue: Throw exception on invalid pushes over silent ignore (MacroFake) ccccc17 refactor: Default options in walletcreatefundedpsbt to VOBJ instead of VNULL (MacroFake) Pull request description: The return value of the `push*` helpers is never used, but important to determine if the operation was successful. One way to fix this would be to add the "nodiscard" attribute. However, this would make the code (and this diff) overly verbose for no reason. So fix it by removing the never used return value. Also, fail verbosely in case of a programming mistake. ACKs for top commit: furszy: code ACK fa277cd Tree-SHA512: ef212a5bf5ae6bbad20acc4dafa3715521e81544185988d1eab724f440e4864a27e686aff51d5bc51b3017892c2eb8e577bcb8f37e8ddbaa0d8833bb622f2f9c
The return value of the
push*
helpers is never used, but important to determine if the operation was successful. One way to fix this would be to add the "nodiscard" attribute. However, this would make the code (and this diff) overly verbose for no reason.So fix it by removing the never used return value. Also, fail verbosely in case of a programming mistake.