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

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

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Tue, Dec 17, 14:16

Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Dec 17, 14:16

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

Harbormaster completed remote builds in B31695: Diff 51643.Tue, Dec 17, 14:16

line length nit

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)

Harbormaster completed remote builds in B31696: Diff 51644.Tue, Dec 17, 14:47

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

PiRK added inline comments.Tue, Dec 17, 17:11

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