Big fan of this rewrite, makes it pretty extensible without too much code duplication. Nice!
Building on your new approach, the non-default parameters could potentially be handled with pointers, to avoid unnecessary copy operations. For example, I think these 2 lines: https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/wallet/rpc/spend.cpp#L274-L275
can be rewritten as:
if (auto comment{self.Arg<const std::string*>(2)}; comment && !(comment->empty())) {
mapValue["comment"] = *comment;
}
Not necessarily shorter, but much more idiomatic imo? I've implemented it like this - can probably be cleaned up quite a bit more:
<details>
<summary>git diff on fa5468dcf0109d11ddaeb4b3591e6e04b4ea6125</summary>
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 0db66ec1b1..1f7bdac609 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -636,27 +636,47 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
return ret;
}
-static const UniValue& ArgOrDefault(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
+static const UniValue* ArgOrDefault(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
{
CHECK_NONFATAL(i < params.size());
const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
- if (!arg.isNull()) return arg;
+ if (!arg.isNull()) return &arg;
const RPCArg::Fallback& fallback{params.at(i).m_fallback};
- CHECK_NONFATAL(std::holds_alternative<RPCArg::Default>(fallback));
- return std::get<RPCArg::Default>(fallback);
+ if (std::holds_alternative<RPCArg::Default>(fallback)) {
+ return &(std::get<RPCArg::Default>(fallback));
+ }
+ return nullptr;
}
-#define TMPL_INST(ret_type, get_arg, get_type) \
- template <> \
- ret_type RPCHelpMan::get_arg(size_t i) const \
- { \
- return ArgOrDefault(m_args, m_req, i).get_type(); \
- } \
+#define TMPL_INST(ret_type, get_arg, get_type) \
+ template <> \
+ ret_type RPCHelpMan::get_arg(size_t i) const \
+ { \
+ return CHECK_NONFATAL(ArgOrDefault(m_args, m_req, i))->get_type(); \
+ } \
+ void force_semicolon()
+
+#define TMPL_INST_PTR(ret_type, get_arg, get_type) \
+ template <> \
+ std::shared_ptr<ret_type> RPCHelpMan::get_arg(size_t i) const \
+ { \
+ auto result {ArgOrDefault(m_args, m_req, i)}; \
+ if (!result) return nullptr; \
+ auto&& ref = result->get_type(); \
+ if constexpr (std::is_lvalue_reference_v<decltype(ref)>) { \
+ return std::shared_ptr<ret_type>(&ref, [](ret_type*) {}); \
+ } else { \
+ return std::make_shared<ret_type>(std::move(ref)); \
+ } \
+ } \
void force_semicolon()
+
+
TMPL_INST(int, ArgValue<int>, getInt<int>);
TMPL_INST(uint64_t, ArgValue<uint64_t>, getInt<uint64_t>);
TMPL_INST(const std::string&, ArgRef<std::string>, get_str);
+TMPL_INST_PTR(const std::string, ArgPtr<const std::string>, get_str);
bool RPCHelpMan::IsValidNumArgs(size_t num_args) const
{
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 0ef019a780..563a619208 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -18,6 +18,7 @@
#include <util/check.h>
#include <string>
+#include <type_traits>
#include <variant>
#include <vector>
@@ -392,6 +393,8 @@ public:
{
if constexpr (std::is_integral_v<R>) {
return ArgValue<R>(i);
+ } else if constexpr (std::is_pointer_v<R>) {
+ return ArgPtr<std::remove_pointer_t<R>>(i);
} else {
return ArgRef<R>(i);
}
@@ -417,6 +420,8 @@ private:
R ArgValue(size_t i) const;
template <typename R>
const R& ArgRef(size_t i) const;
+ template <typename R>
+ std::shared_ptr<R> ArgPtr(size_t i) const;
};
/**
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index b695d4bed3..420807bf1d 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -271,8 +271,9 @@ RPCHelpMan sendtoaddress()
// Wallet comments
mapValue_t mapValue;
- if (!request.params[2].isNull() && !request.params[2].get_str().empty())
- mapValue["comment"] = request.params[2].get_str();
+ if (auto comment{self.Arg<const std::string*>(2)}; comment && !(comment->empty())) {
+ mapValue["comment"] = *comment;
+ }
if (!request.params[3].isNull() && !request.params[3].get_str().empty())
mapValue["to"] = request.params[3].get_str();
</details>
I'm not married to this approach, and it doesn't need to be added in this PR either. But I think being able to use a consistent interface for Arg for all types of parameters would be nice.
