Page MenuHomePhabricator

[chronik] check max number of parameters for Electrum commands
ClosedPublic

Authored by PiRK on Tue, Dec 17, 14:16.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABCe0dd041c75da: [chronik] check max number of parameters for Electrum commands
Summary

Fulcrum does this.

$ echo '{"jsonrpc": "2.0", "method": "server.ping", "params": ["04012200"], "id": "test"}' | nc fulcrum.pepipierre.fr 50001
{"error":{"code":-32602,"message":"Expected at most 0 parameters for server.ping, got 1 instead"},"id":"test","jsonrpc":"2.0"}
$ echo '{"jsonrpc": "2.0", "method": "blockchain.transaction.get_height", "params": [], "id": "test"}' | nc fulcrum.pepipierre.fr 50001
{"error":{"code":-32602,"message":"Expected at least 1 parameter for blockchain.transaction.get_height, got 0 instead"},"id":"test","jsonrpc":"2.0"}
$ echo '{"jsonrpc": "2.0", "method": "blockchain.transaction.get_height", "id": "test"}' | nc fulcrum.pepipierre.fr 50001
{"error":{"code":-32602,"message":"Missing required params"},"id":"test","jsonrpc":"2.0"}
$ echo '{"jsonrpc": "2.0", "method": "blockchain.transaction.get", "id": "test", "params": [1, 2, 3]}' | nc fulcrum.pepipierre.fr 50001
{"error":{"code":-32602,"message":"Expected at most 2 parameters for blockchain.transaction.get, got 3 instead"},"id":"test","jsonrpc":"2.0"}

Note that we cannot match exactly fulcrum's error messages because:

  • I don't want to repeat the method name in the code when calling the macro
  • RPCError::InvalidParams(...) expects a string literal / static string, so we cannot provide the actual number of passed params to the error message
  • the get_param macro already provides a better error message when we don't provide at least the min number of params: {"code": -32602, "message": "Missing mandatory 'txid' parameter"}

Ref T3598

Test Plan

ninja check-functional

Event Timeline

PiRK requested review of this revision.Tue, Dec 17, 14:16

Note that I also tried a form for the macro that check both min and max check_number_of_params!(params, 1, 2); but then I ran into a compiler warning when I added check_number_of_params!(params, 0, 0); in the server.ping handler

warning: comparison is useless due to type limits
   --> chronik/chronik-http/src/electrum.rs:288:24
    |
288 |                     if obj.len() < $min_num_params {
    |                        ^^^^^^^^^
...
307 |         check_number_of_params!(params, 0, 0);
    |         ------------------------------------- in this macro invocation
    |
    = note: this warning originates in the macro `check_number_of_params` (in Nightly builds, run with -Z macro-backtrace for more info)
PiRK retitled this revision from [chronik] check max number of parameters to [chronik] check max number of parameters for Electrum commands.Tue, Dec 17, 16:08
Fabien requested changes to this revision.Tue, Dec 17, 16:28
Fabien added a subscriber: Fabien.
Fabien added inline comments.
chronik/chronik-http/src/electrum.rs
264–271 ↗(On Diff #51644)

A bit better imo, and it works with 0 parameter (instead of 0 parameterS)

test/functional/chronik_electrum_basic.py
39 ↗(On Diff #51644)
This revision now requires changes to proceed.Tue, Dec 17, 16:28
chronik/chronik-http/src/electrum.rs
264–271 ↗(On Diff #51644)

But 0 needs a plural for some reason.
https://english.stackexchange.com/questions/38293/why-is-zero-followed-by-a-plural-noun
But ok with using the if instead of match

Fabien added inline comments.
chronik/chronik-http/src/electrum.rs
264–271 ↗(On Diff #51644)

TIL

This revision is now accepted and ready to land.Tue, Dec 17, 20:03