Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

generic Provider allowing relaxed json rpc handling #2527

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aathan
Copy link

@aathan aathan commented Jul 27, 2023

Motivation

Sometimes you have no choice but to interact with json rpc endpoints that don't exactly adhere to the standards. Innocuous departures from the standard should be tolerable.

Solution

I'm new to Rust, so I do not claim this implementation is ideal. From what I could tell the only way to achieve this is to modify the deserializer, and in order to do this without additional runtime state variables, I implemented this via a const bool generic type parameter on Response and Provider. A pull request I am submitting to https://github.com/foundry-rs/foundry will reference this pull request.

Because I export a type name Http which is equivalent to the old type, I believe this is not a breaking change; but that depends on what level of abstraction your source is at.

PR Checklist

  • Added Tests -- the existing tests cover the implementation except in the relaxed/loose case which I tested manually
  • Added Documentation
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm supportive of this feature, because this really comes up a lot in foundry, unfortunately, especially with new testnets.

having an option to allow malformed json rpc would be useful.

defer to @DaniPopes re changes/how to proceed

@aathan
Copy link
Author

aathan commented Jul 28, 2023

Since there's interest to accept this PR I'm putting in the time to do a couple of minor cleanups. E.g., the name of the typevar is now RELAXED and LooseHttp is RelaxedHttp.

Note that the real thrust of this change is to allow these types to be generics so that behavior can be parameterized in general. To do this RELAXED would be a more complex type, but the bones are there.

Copy link
Contributor

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would love to get this in cc @DaniPopes

@aathan anything else needs cleanup here?=

@Evalir
Copy link
Contributor

Evalir commented Aug 15, 2023

👋 checking in here

Copy link
Collaborator

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs cargo +nightly fmt.

This is a pretty big breaking change, I think this is better implemented with a run time check that is opt-in at construction of the provider, like a relaxed(bool) method.

@aathan
Copy link
Author

aathan commented Aug 16, 2023

May I ask what would break exactly? It's been a while since I wrote/submitted this but as I recall:

(a) the ResponseVisitor didn't seem like something with external dependents
(b) a pub type ResponseVisitor = GenResponseVisitor could be done if it did have external dependencies and
(c) it seemed wasteful to do runtime checks

of course, (c) is not a big deal in this case.

This change would really be helpful in that they'd allow foundry users to connect to imperfect providers. I'd like to get this merged so if you feel strongly about it and the changes really are breaking external dependents I can refactor.

aathan and others added 2 commits August 23, 2023 16:27
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@aathan
Copy link
Author

aathan commented Aug 23, 2023

cargo +nightly fmt done and pushed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants