Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly re-enables HTTPS support for alloy-provider by adding the reqwest-default-tls feature flag at the workspace level. It also introduces an integration test to verify that HTTPS connections to an RPC endpoint are successful. My review identifies a potential reliability issue with the new test, which has been kept as it does not contradict any existing rules.
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
Not worth blocking over my question
| let provider = Web3::new_from_url("https://rpc.mevblocker.io"); | ||
| let response = provider.provider.get_block(BlockId::latest()).await; | ||
|
|
||
| if let Err(err) = response { |
There was a problem hiding this comment.
Is there no more specific matching?
There was a problem hiding this comment.
Yes, and no. When printing the error you get this:
Transport(Custom("batch call failed: Transport(Custom(reqwest::Error { kind: Request, url: \"https://rpc.mevblocker.io/\", source: hyper_util::client::legacy::Error(Connect, ConnectError(\"invalid URL, scheme is not http\")) }))"))
We could try to match down to the ConnectError but it seems to be the case that the connection could fail for other reasons than https support. Also the fact that the error variant identifier contains legacy is not very confidence inspiring.
Given that this check should only kick in when mevblocker is down I think this should be sufficient.
There was a problem hiding this comment.
Matching on transport was good enough tbh, but for what it is, this works just fine and once it breaks again we'll review
4 participants
Description
When replacing the
alloydependency with its sub-depdencies I let compilation errors guide me which features need to be enabled. However, this was not enough sincehttpssupport needs to be enabled via a feature and if that's missing we'll only encounter runtime errors but not compile time errors.reqwestwill throw errors likeinvalid URL, scheme is not http.Changes
Enabled
httpson the workspace level to avoid feature unification causing us to misshttpssupport in future edge casesAdded a unit test to ensure that
httpsrequests succeed.How to test
new unit test