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

Support Ws and Ipc providers #369

Merged
merged 14 commits into from
May 7, 2024
Merged

Conversation

fala13
Copy link
Contributor

@fala13 fala13 commented Mar 19, 2024

Motivation

Allow to use Ws an Ipc on par with Http RPCs.
Closes #306

Solution

Straightforward adaptation of https://www.gakonst.com/ethers-rs/providers/custom.html to include Http.
Added example tests to cover the new code.

Considerations - feedback welcome

  • The RPC type is heuristically determined depending on the rpc_url string content and defaults to Ipc if neither 'http' or 'ws' is part of the string. There could be some edge cases that this omits, leading to inability to connect.
  • Ideally we would like to reuse same provider object like proposed in [EXP] perf(decode): approx. 100x speed improvement w/ various optimizations #310, so the cost of checking the rpc_url string content and .await when trying to connect to the RPC can be accepted.
  • The name HttpOrIpcOrWs isn't very catchy but follows ethers-rs example convention. Happy to change it if better suggested.
  • No tests for Ipc added as it depends on having a local node, however the handling is same as Ws.
  • The visibility of HttpOrIpcOrWs and the new rpc::get_provider() could be restricted.

@fala13 fala13 requested a review from Jon-Becker as a code owner March 19, 2024 13:16
Copy link
Owner

@Jon-Becker Jon-Becker left a comment

Choose a reason for hiding this comment

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

looks great! once tests pass we can merge this in :)

huge thank you for contributing ❤️

T: Debug + Serialize + Send + Sync,
R: DeserializeOwned + Send,
{
// println!("request");
Copy link
Owner

Choose a reason for hiding this comment

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

nit: remove

@Jon-Becker
Copy link
Owner

will get this in today :)

@Jon-Becker
Copy link
Owner

ill fix the broken test case and include in #372, so this will be part of 0.8.0

@fala13
Copy link
Contributor Author

fala13 commented Apr 18, 2024

Hey @Jon-Becker, I'm trying to use Heimdall in prod doing decompilation and I get a lot of crashes in the - esp. the more timeout I give it - in debug there are arithmetic overflows in the vm, with arithmetic checks disabled I get vector capacity errors and so on. Are you interested in those as new issues or should I wait or 0.8 and report then? Would you like them as unit tests, or just example contract addresses? Their appearance is probabilistic, some I need multiple retries to reproduce.

I tried fixing them on my own, but I'm not familiar with this code enough (yet).

@Jon-Becker
Copy link
Owner

if they happen on nightly, please open an issue!!

you can install nightly with bifrost +nightly or bifrost -v main

@Jon-Becker
Copy link
Owner

Awesome! Looks good enough to merge, i'll fix clippy in a future commit.

Copy link
Owner

@Jon-Becker Jon-Becker left a comment

Choose a reason for hiding this comment

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

<3

@Jon-Becker Jon-Becker merged commit 70892f0 into Jon-Becker:main May 7, 2024
5 of 6 checks passed
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.

Support Ws and Ipc providers
2 participants