Skip to content

Drop JSONRPCRequest constructors after #21366 #21574

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 8, 2021
Merged

Drop JSONRPCRequest constructors after #21366 #21574

merged 1 commit into from
Apr 8, 2021

Conversation

ryanofsky
Copy link
Contributor

This just makes an additional simplification after #21366 replaced
util::Ref with std::any. It was originally suggested
#21366 (comment) but
delayed for a followup. It would have prevented usage bug
#21572.

@theStack
Copy link
Contributor

theStack commented Apr 2, 2021

Concept ACK

Considering the bug fixed in #21572, I think it would also make sense to deduplicate the two loops in WalletClientImpl::registerRpcs(), as their bodies contain exactly the same code. (Doesn't necessarily have to be in this PR though; if not, I'm happy to do a follow-up).

@ryanofsky
Copy link
Contributor Author

I think it would also make sense to deduplicate the two loops in WalletClientImpl::registerRpcs(), as their bodies contain exactly the same code. (Doesn't necessarily have to be in this PR though; if not, I'm happy to do a follow-up).

Thanks for reviewing and suggesting the improvement. #21467 is removing the second loop though, so best not to consolidate these.

@DrahtBot DrahtBot mentioned this pull request Apr 3, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 3, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

This was referenced Apr 3, 2021
Merged
Merged
Merged
meshcollider
meshcollider reviewed Apr 6, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

Code review ACK 2b98711

theStack
theStack approved these changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK 2b98711
Nice simplification!

promag
promag reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK 2b98711

@ryanofsky
Drop JSONRPCRequest constructors after bitcoin#21366
9044522 
This just makes an additional simplification after bitcoin#21366 replaced
util::Ref with std::any. It was originally suggested
bitcoin#21366 (comment) but
delayed for a followup. It would have prevented usage bug
bitcoin#21572.
@maflcko
Copy link
Member

maflcko commented Apr 7, 2021

I think you forgot to rebase this on 937fd4a when opening the pr. Git can't figure out that this doesn't conflict, otherwise.

@ryanofsky
Copy link
Contributor Author

I think you forgot to rebase this on 937fd4a when opening the pr. Git can't figure out that this doesn't conflict, otherwise.

Thanks! Rebased now.

Rebased 2b98711 -> 9044522 (pr/simpreq.1 -> pr/simpreq.2, compare) due to conflict with #21572

@promag
Copy link
Contributor

promag commented Apr 7, 2021

ACK 9044522, fixed conflict in src/wallet/interfaces.cpp.

@maflcko maflcko merged commit 6664211 into bitcoin:master Apr 8, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 8, 2021
Merge bitcoin#21574: Drop JSONRPCRequest constructors after bitcoin#2…
3d28753 
…1366

9044522 Drop JSONRPCRequest constructors after bitcoin#21366 (Russell Yanofsky)

Pull request description:

  This just makes an additional simplification after bitcoin#21366 replaced
  util::Ref with std::any. It was originally suggested
  bitcoin#21366 (comment) but
  delayed for a followup. It would have prevented usage bug
  bitcoin#21572.

ACKs for top commit:
  promag:
    ACK 9044522, fixed conflict in src/wallet/interfaces.cpp.

Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
@ryanofsky ryanofsky mentioned this pull request Apr 9, 2021
Draft
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 7, 2022
@ryanofsky @PiRK
Drop JSONRPCRequest constructors
5ae5480 
Summary:
This just makes an additional simplification after D11552 replaced `util::Ref` with `std::any`.

Note that we sometimes wrap the context in a `HTTPRPCRequestProcessor`instance, so some changes don't apply in bitcoind.cpp and init.cpp.

This is a backport of [[bitcoin/bitcoin#21574 | core#21574]]
Depends on D11552

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11560
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@meshcollider meshcollider meshcollider left review comments

@promag promag promag left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RPC/REST/ZMQ Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ryanofsky @theStack @DrahtBot @maflcko @promag @meshcollider