-
Notifications
You must be signed in to change notification settings - Fork 29
feat: Support polkadot EVM clients #389
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
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new Polkadot Westend network configuration file is added alongside infrastructure improvements to the HTTP transport layer. The HTTP client gains TLS and HTTP/2 support through additional builder method calls, while keeping existing logic intact. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes The changes are straightforward: a new configuration file following an established pattern and builder-method additions to enable HTTP/2 and TLS without logic modifications. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/config/networks/polkadot_westend.json(1 hunks)src/services/blockchain/transports/http.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test
- GitHub Check: msrv
- GitHub Check: clippy
- GitHub Check: Redirect rules - openzeppelin-monitor
- GitHub Check: Header rules - openzeppelin-monitor
- GitHub Check: Pages changed - openzeppelin-monitor
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
examples/config/networks/polkadot_westend.json (1)
10-10: RPC endpoint is accessible and functional.The endpoint responds correctly to JSON-RPC requests with valid results (HTTP 200, valid chain ID response).
| .use_rustls_tls() | ||
| .http2_prior_knowledge() |
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.
🧩 Analysis chain
HTTP/2 prior knowledge may break existing endpoints.
The http2_prior_knowledge() setting forces all connections to use HTTP/2 without protocol negotiation, which will cause failures on RPC endpoints that only support HTTP/1.1. Since this change affects the global HTTP client used by all networks (not just Polkadot), it risks breaking existing integrations.
Consider making HTTP/2 support configurable per network rather than enforcing it globally. For example, add a field like "requires_http2": true to the network configuration.
Verify that all currently configured networks support HTTP/2:
Additionally, consider testing HTTP/2 support for each endpoint or search the web for documentation on their protocol requirements:
🏁 Script executed:
#!/bin/bash
# Find all network configuration files
echo "=== Network configuration files ==="
fd -e json . examples/config/networks/
echo -e "\n=== Extracting RPC URLs from network configs ==="
fd -e json . examples/config/networks/ --exec jq -r '.name as $name | .rpc_urls[]? | select(.url.type == "plain") | "\($name): \(.url.value)"'Length of output: 2955
🌐 Web query:
Do Ethereum RPC endpoints typically support HTTP/2 prior knowledge mode?
💡 Result:
Short answer: No — public Ethereum RPC endpoints typically do not support HTTP/2 "prior knowledge" (h2c). They are served over HTTPS (so HTTP/2 if available is negotiated via TLS/ALPN as "h2") or over plain HTTP/1.1; h2c prior‑knowledge over cleartext TCP is uncommon for public RPC providers. [1][2]
Notes/sources:
- Public RPC providers (e.g., Infura) expose JSON‑RPC over HTTPS/WSS (TLS + ALPN for h2), not h2c prior knowledge. [1]
- The HTTP/2 spec distinguishes h2 (over TLS/ALPN) from h2c (cleartext) and documents the prior‑knowledge mechanism for h2c — which is rarely used in public APIs. [2]
- Practical HTTP client/server docs explain h2c prior‑knowledge is mainly for controlled/internal environments and not broadly deployed on public HTTPS APIs. [3]
Sources:
[1] Infura — Ethereum API (HTTPS/WSS endpoints).
[2] RFC 9113 / RFC 7540 — HTTP/2 (sections on h2, h2c, and prior knowledge).
[3] httpcore / HTTP/2 documentation (notes on h2c and prior-knowledge usage).
HTTP/2 prior knowledge forces all networks to use HTTP/2 without fallback—this will break all existing RPC endpoints.
The HTTP/2 specification explicitly marks "prior knowledge" negotiation as intended for non-secure HTTP connections only. HTTP/2 over TLS requires Application-Layer Protocol Negotiation (ALPN) to negotiate whether both client and server support HTTP/2. Public servers don't support HTTP/2 prior knowledge—only ALPN.
All 31+ configured RPC networks use HTTPS endpoints that negotiate HTTP/2 via ALPN. Applying http2_prior_knowledge() globally will cause connection failures on every network that doesn't support prior knowledge mode (which is all of them). This is a critical breaking change that affects not just Polkadot but all blockchain integrations.
Solution: Make HTTP/2 support configurable per network. Either:
- Add
"http2_required": trueto network config to opt-in selectively - Skip
http2_prior_knowledge()entirely and rely on automatic ALPN negotiation when supported
🤖 Prompt for AI Agents
In src/services/blockchain/transports/http.rs around lines 81-82, the call to
.http2_prior_knowledge() forces HTTP/2 prior-knowledge for all networks
(breaking HTTPS/ALPN negotiation); remove that global call and make HTTP/2
prior-knowledge opt-in per network: read a new per-network config flag (eg.
http2_required or http2_prior_knowledge) and only call .http2_prior_knowledge()
when that flag is true, otherwise omit it and rely on TLS ALPN (or simply drop
http2_prior_knowledge() entirely if you prefer ALPN-only). Ensure the builder
path still compiles when the call is omitted and add the config flag to network
config schema and defaults.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
=======================================
- Coverage 96.6% 96.6% -0.1%
=======================================
Files 76 76
Lines 26631 26630 -1
=======================================
- Hits 25740 25739 -1
Misses 891 891
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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, thanks
Summary
A new Polkadot Westend network configuration file is added alongside infrastructure improvements to the HTTP transport layer. The HTTP client gains TLS and HTTP/2 support through additional builder method calls, while keeping existing logic intact.
Testing Process
Checklist
Summary by CodeRabbit