Move IsDeprecatedRPCEnabled to rpc/util, rm redundant rpcEnableDeprecated #27322
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
|
lgtm Maybe explain that this moves it from |
jonatack
force-pushed
the
2023-03-fix-IsDeprecatedRPCEnabled-build-errors
branch
from
c852aef to
409758a
Compare
|
PR description and commit message updated per #27322 (comment) (thanks @MarcoFalke). |
jonatack
force-pushed
the
2023-03-fix-IsDeprecatedRPCEnabled-build-errors
branch
from
409758a to
8afe44e
Compare
|
Actually, on a second though. Any reason why you can't just call |
Good question. Looking at it, maybe we can remove it? Per this comment in |
jonatack
changed the title
…ed from wallet CI builds confusingly fail with "undefined reference to IsDeprecatedRPCEnabled in libbitcoin_wallet" compiler errors when this function is called from wallet RPCs. Fix this by moving IsDeprecatedRPCEnabled() from rpc/server to rpc/util, thereby moving it from node to common to be picked up by libbitcoin_wallet (see doc/design/librairies.md and src/Makefile.am).
jonatack
force-pushed
the
2023-03-fix-IsDeprecatedRPCEnabled-build-errors
branch
from
7125e2f to
4580a1c
Compare
Per the comment in src/interfaces/chain.h::L107, it looks like we want to remove these, and this function doesn't seem constrained by the HTTP port requirement: //! * The handleRpc, registerRpcs, rpcEnableDeprecated methods and other RPC //! methods can go away if wallets listen for HTTP requests on their own //! ports instead of registering to handle requests on the node HTTP port. Further, rpcEnableDeprecated() is unneeded if IsDeprecatedRPCEnabled() is in common rather than node and thereby callable from the wallet, and it is confusingly redundant to keep it.
jonatack
force-pushed
the
2023-03-fix-IsDeprecatedRPCEnabled-build-errors
branch
from
4580a1c to
9ed9b50
Compare
jonatack
changed the title
|
Rebased 😃 |
|
Is gArgs guaranteed to be identical in different processes? If not, it would be good to explain why this behavior change is intentional. If yes, it would be good to explain as well. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Needs a rebase, if still relevant, and the question above also needs addressing. |
|
Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened. |
6 participants
CI builds currently fail when
IsDeprecatedRPCEnabled()is called from wallet RPCs with "Undefined reference to IsDeprecatedRPCEnabled in libbitcoin_wallet".Fix the issue by moving
IsDeprecatedRPCEnabled()fromrpc/servertorpc/util, thereby moving it fromnodetocommonto be picked up by libbitcoin_wallet (seedoc/design/librairies.mdandsrc/Makefile.am).This move allows us to drop the redundant
rpcEnableDeprecated()method. Per the comment insrc/interfaces/chain.h::L107from #15639 we would like to remove these, and this function doesn't seem constrained by the mentioned HTTP port requirement, nor needed ifIsDeprecatedRPCEnabledis incommonrather thannode.This allows having only one call for this functionality and avoids developer confusion and head-scratching.
If it is preferred to keep all of them, this pull could alternatively document the use of these different methods for future developers doing a deprecation.