-
Notifications
You must be signed in to change notification settings - Fork 399
Removed duplicate solving_data for transaction funding RPCs #1471
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
Removed duplicate solving_data for transaction funding RPCs #1471
Conversation
delta1
left a comment
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.
ACK e7921a0
Running tests locally
e7921a0 to
a9da179
Compare
|
re-based to master |
delta1
left a comment
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.
ACK a9da179
Ran tests locally
| uint32_t psbt_version = 2; | ||
| if (!request.params[6].isNull()) { | ||
| if (!request.params[5].isNull()) { | ||
| psbt_version = request.params[6].get_int(); |
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.
Hey @tomt1664 just spotted this, should be params[5] here? Presumably I've fixme'd out some test code that would have caught this
psbt_version = request.params[5].get_int();
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, you are right - should be params[5].
As afar as I can see there is no test (including fixmes) that check this.
I will update and add a test case to catch this.
solving_dataremoved as explicit arguments forwalletcreatefundedpsbtandfundrawtransactionwhere the argument was being ignored.solving_dataremains part of theoptionsexplicit argument.Fixes: #1470