Aligning RPC API with Bitcoin Core conventions

[JeremyRand]

The Namecoin Core RPC API for name operations has a bunch of inconsistencies with what Bitcoin Core does. Some of these inconsistencies are due to Vince doing weird things, others are due to Namecoin Core adding some features before Bitcoin Core did so. Other parts of the API are confusing for historical reasons (though not specifically in conflict with Bitcoin Core).

Some notable examples of API issues:

• "Method naming: use consecutive lower-case names such as getrawtransaction and submitblock."
  • name_checkdb, name_firstupdate, name_history, name_list, name_new, name_pending, name_scan, name_show, and name_update don't follow this.
• Method naming: Bitcoin Core RPC methods tend to be named in imperative form, e.g. abandontransaction.
  • I don't see any formal guideline in Bitcoin Core's developer notes for this, maybe we should ask them to document it?
  • The name_ methods don't follow this.
  • Suggested new names: registername (registration and pre-registration are more intuitive than firstupdate and new), getnamehistory (consistent with getmempoolancestors), listnames (consistent with listtransactions), preregistername (ibid), getmempoolname (consistent with getmempoolentry), scannameset (consistent with scantxoutset), getname (consistent with getrawtransaction/gettransaction), and updatename.
• "Argument and field naming: ... use snake case fee_delta (and not, e.g. feedelta or camel case feeDelta)."
  • allowExisting, allowExpired, byHash, destAddress, minConf, maxConf, nameEncoding, sendCoins, and valueEncoding don't follow this.
• Bitcoin Core supports name-based arguments now.
  • options does the same thing, but worse.
• "Try to make the RPC response a JSON object."
  • name_checkdb, name_firstupdate, name_history, name_list, name_new, name_pending, name_scan, name_update, and queuerawtransaction don't follow this.
• rand argument
  • Should be renamed to salt, since this value is typically not random anymore.
• allowExisting vs allow_active
  • allow_active is probably more precise.
• tx argument
  • Should be renamed to txid, since tx could be confused with rawtx

Would there be any objections to updating the API to fix these issues (leaving the existing API methods in place to avoid disrupting consumers of the current API)?

[domob1812]

My main objection would be added complexity if we keep the old names around and just "add" new ones.

[JeremyRand]

My main objection would be added complexity if we keep the old names around and just "add" new ones.

@domob1812 I was figuring we could add the new methods, mark the old ones as deprecated, then a release or two later we could mark the old ones as invisible (I think Bitcoin Core supports invisible RPC methods that don't show up in help?), and then after another 1-2 releases we could remove the old methods.

If you strongly prefer a faster schedule to remove the old methods, I might be amenable to that.

[domob1812]

No, I definitely do not prefer to remove the old methods quickly, because that will break everyone out there. And I also don't like having the methods duplicated, as that will be quite a bit of extra complexity in the code (not specifically "complex" code, but just bloat that needs to be updated whenever upstream Bitcoin updates the RPC doc code again and all that).

To be honest, my impression is that either way is "bad" and not worth it just to change the naming convention. I'd prefer to keep the names and arguments as they are.

[JeremyRand]

So, I'm not sure if this has been reaching you, but we're getting a lot of unhappy support requests from users who find the current RPC API to be confusing / bad UX due to its dissimilarity to Bitcoin Core. Some of these complaints are from funders. So I think it is worth fixing.

For the specific concern about having duplicate methods, I can think of two workarounds:

1. Have the old method function simply act as a shim that calls the new method function.
2. I believe Bitcoin Core's RPC system supports method aliases, i.e. the same function has two names, and only one of them shows up in the help.

Option 2 seems like it would be better for avoiding the bloat that needs to be kept in sync with upstream. I've never actually tried to use method aliases, but I don't mind looking into how it works.

Thoughts?

[domob1812]

I am not an expert about Bitcoin's RPC code, either - but if method aliases work that way, then I have no objection to adding them. However, another concern would then be the rename of method arguments, which entails with it the full RPC doc code (which is self-verifying and thus needs to be set correctly or it will break).

I assumed that we would want to leave the old method as-is, and add the new method with changed argument names, too. And in that case, we could use neither option 1 nor 2, as we would (at the least) have to duplicate the RPC doc logic.