Skip to content

Accept optional server.version arguments per the protocol - #1328

Open
pucedoteth wants to merge 1 commit into
romanz:masterfrom
pucedoteth:fix-server-version-optional-args
Open

Accept optional server.version arguments per the protocol#1328
pucedoteth wants to merge 1 commit into
romanz:masterfrom
pucedoteth:fix-server-version-optional-args

Conversation

@pucedoteth

Copy link
Copy Markdown

The Electrum protocol declares both server.version arguments as optional:

server.version(client_name="", protocol_version="1.4")

Params::Version held a required (String, VersionRequest), so anything short of two arguments failed to deserialize and the request was rejected with invalid params before version() ran. On master:

Params::parse("server.version", [])          -> Err   // failed to parse []
Params::parse("server.version", ["client"])  -> Err   // one-arg form rejected

This mirrors the fix in #1325 for blockchain.transaction.id_from_pos: VersionArgs is an untagged arity enum, the same shape already used by TxGetArgs, TxFromPosArgs and BroadcastArgs. An omitted protocol_version defaults to "1.4" as the protocol specifies, rather than to PROTOCOL_VERSION, so the negotiation still runs through check_between and would correctly reject a client that relies on the default if the server's supported version ever moves past 1.4.

Tests follow the pattern of test_tx_from_pos_args / test_params_id_from_pos: one covering the arities VersionArgs accepts (and that malformed params are still rejected), one covering the dispatcher. Both fail on master with failed to parse [].

cargo test --lib passes (11 tests), cargo fmt --all --check is clean, and cargo clippy --all-targets -- -D warnings reports nothing.

Out of scope, noted in case you want it: a request that omits the params member entirely arrives as Value::Null rather than [], which this does not accept. That affects every method, not just server.version, so it seemed better kept separate.


Disclosure: written with AI assistance; the analysis, fix and tests were verified locally.

Both `client_name` and `protocol_version` are optional in the Electrum
protocol, defaulting to "" and "1.4". `Params::Version` required a
two-element tuple, so `server.version` with no arguments or with only
`client_name` was rejected as invalid params before the handler ran.

Use the same untagged-arity enum the other optional-argument methods
use, and default the omitted `protocol_version` to "1.4".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant