Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rpc): ignore non-standard "jsonrpc = 1.0" in lightwalletd requests #173

Merged
merged 3 commits into from
Feb 21, 2022

Conversation

teor2345
Copy link

@teor2345 teor2345 commented Feb 21, 2022

Motivation

lightwalletd sends a "jsonrpc: 1.0" field in its RPC requests. But this field was only added in JSON-RPC 2.0, so jsonrpc_core rejects it.

Specifications

jsonrpc field:
http://www.simple-is-better.org/rpc/#differences-between-1-0-and-2-0

Valid jsonrpc_core versions:
https://docs.rs/jsonrpc-core/18.0.0/jsonrpc_core/types/request/struct.MethodCall.html#structfield.jsonrpc
https://docs.rs/jsonrpc-core/18.0.0/jsonrpc_core/types/version/enum.Version.html

Solution

  • delete the non-standard fields before parsing the request

Based on ZcashFoundation#3589

Review

I'd like both @oxarbitrage and @jvff to have a look at this fix.

This fix doesn't have tests yet, but I've tested it manually.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

  • add tests with a lightwalletd instance

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (issue3140@97c6a8e). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             issue3140     #173   +/-   ##
============================================
  Coverage             ?   79.81%           
============================================
  Files                ?      279           
  Lines                ?    32489           
  Branches             ?        0           
============================================
  Hits                 ?    25932           
  Misses               ?     6557           
  Partials             ?        0           

@teor2345
Copy link
Author

The Windows failure is unrelated, it seems to be a port conflict on port 57775.
(We expect these to happen sometimes in the tests.)

@oxarbitrage oxarbitrage merged commit 89ab602 into oxarbitrage:issue3140 Feb 21, 2022
@oxarbitrage oxarbitrage deleted the issue3140-compat branch February 21, 2022 18:30
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.

4 participants