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

feat: windows ipc provider (named pipe) #1976

Merged
merged 20 commits into from Jan 3, 2023

Conversation

DaniPopes
Copy link
Collaborator

@DaniPopes DaniPopes commented Dec 27, 2022

Motivation

Fixes #1974, cc @CodeFunta

Solution

  • Add a custom tokio::net::windows::named_pipe::NamedPipeClient wrapper to be compatible with UnixStream
    • might look intimidating but it's just re-implementing the same traits and the split method, only real implementation is in connect
  • Bump server thread stack size from 64kb to 256kb

Successfully tested after a bump in thread stack size

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

@DaniPopes
Copy link
Collaborator Author

DaniPopes commented Dec 27, 2022

P.S.: CI needs quite the revamp, it's currently only covering Windows with no features and Linux with either all or no features;
@mattsse maybe just copy it over from Foundry?

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.

thanks for picking this up.

before we merge this, it'd be great to have additional ipc tests in anvil first to test the provider.

@DaniPopes
Copy link
Collaborator Author

thanks for picking this up.

before we merge this, it'd be great to have additional ipc tests in anvil first to test the provider.

the tests are the same as websockets, do you want me to change from geth to anvil?

@mattsse
Copy link
Collaborator

mattsse commented Dec 27, 2022

we currently only do ipc e2e testing on unix for anvil

https://github.com/foundry-rs/foundry/blob/6ecebad62014ac46513099f03e8c0b80f5d9d4b6/anvil/tests/it/main.rs#L10-L11

with this we can remove the cfg and it should also work on windows, so would be great to test this first via a PR with patched ethers

@DaniPopes
Copy link
Collaborator Author

Oh I see what you mean now, on it

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.

foundry tests lgtm.

could use the windows stream as is or replicate the Windows stream implementation, both are essentially the same.

ethers-providers/Cargo.toml Outdated Show resolved Hide resolved
ethers-providers/src/transports/ipc.rs Show resolved Hide resolved
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.

lgtm,

cross platform ipc support 🔥

@gakonst gakonst merged commit 4fd742f into gakonst:master Jan 3, 2023
@gakonst
Copy link
Owner

gakonst commented Jan 3, 2023

nice work :)

Rjected pushed a commit to Rjected/ethers-rs that referenced this pull request Jan 4, 2023
* fmt: imports

* fix: ipc tests

* fmt

* chore: move ws macros

* chore: gate ipc to unix family

* chore: make tokio optional

* feat: initial named_pipe

* feat: windows ipc

* chore: update Provider

* chore: clippy

* chore: use Path instead of OsStr

* chore: clippy

* fix: docs

* lf

* lf

* test: better subscription tests

* docs

* fix: ipc doctest

* chore: make winapi optional

* fix: optional tokio
@CodeFunta
Copy link

Thank you team!

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.

New "named_pipe" provider for intercommunication with Nethermind client on Windows
4 participants