-
Notifications
You must be signed in to change notification settings - Fork 38
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(providers): add connect_builtin example #34
Conversation
This would fail ci due to the IPC path not being present. |
does this work locally? it's possible that the file is created somewhere else, I think we want this as a getter on the anvil instance as well |
prefer the opposite. reasoning: node-bindings is nearly dep-free. we want provider to depend on it, not vice versa |
It works if the file exists but would ultimately fail with |
If you have an example that cannot be ran (see #31) you can update the filter applied in the CI here: examples/.github/workflows/test.yml Lines 44 to 55 in 60d14df
examples/.github/workflows/integration.yml Lines 25 to 37 in 60d14df
|
|
||
// Instantiate a HTTP transport provider by passing the http endpoint url | ||
let http_provider = | ||
RootProvider::<Ethereum, BoxTransport>::connect_builtin(http.as_str()).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a general comment and a personal opinion but I'm not a fan of the connect_builtin
name. I don't think it is particularly clear to users and sounds like an internal method rather than the equivalent of a default
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly beyond the scope of the intended PR but I would also prefer to see the parameter being strongly typed (Url
-like) instead of a &str
examples/providers/Cargo.toml
Outdated
alloy-network = { git = "https://github.com/alloy-rs/alloy", rev = "68952c0" } | ||
|
||
alloy-transport = { git = "https://github.com/alloy-rs/alloy", rev = "68952c0" } | ||
alloy-transport-ipc = { git = "https://github.com/alloy-rs/alloy", rev = "68952c0" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these required and not accessible on the alloy
namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler doesn't the recognize them as the same type when used with alloy-provider
(w/o the namespace). Ref: #26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, then this should be resolved once that upstream PR has been merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, pending CI filter
examples-providers
.connect_builtin
examples for all three builtin transport type. HTTP, WS and IPC.