-
Notifications
You must be signed in to change notification settings - Fork 373
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
fix: Add default ports for net.Dial
if missing in RPC URL
#2360
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2360 +/- ##
==========================================
- Coverage 54.62% 54.61% -0.02%
==========================================
Files 582 582
Lines 78401 78366 -35
==========================================
- Hits 42824 42797 -27
+ Misses 32365 32360 -5
+ Partials 3212 3209 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
Generally speaking, a custom Dialer
can be very useful for various purposes, such as mocking connections, creating middleware, and using custom transports like buffered connections or net.Pipe
for internal clients. This can help avoid the need to secure the connection from external calls.
I've used all the previously mentioned dialers with gRPC when I was working on a mobile project. It was vital for me to be able to customize the dialer.
The problem here is that we cannot actually customize the Dialer
. It's only wrapping a TCP Dialer
, which seems a bit useless. The better approach would be to let the user pass their own Dialer
and fall back on Go default Dialer
, passing the remote address naturally without wrapping it inside the Dialer
.
Co-authored-by: Manfred Touron <[email protected]>
Description
This PR fixes a bug with the
http.Client
, where the customDialContext
would fail if the specified RPC URL did not contain a port number (ex:https://rpc.gno.land
).I've added the default catch values, and removed useless error returns.
This additionally begs the question -- do we even want a custom dial context, given that we don't even use the features it provides?
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description