feat: EVM chains RPC probing logic - #4049
Conversation
baefb63 to
c1c7b25
Compare
haiyuechen-nearone
changed the title
Pull request overviewWires the startup network-fingerprint probe for the six EVM chains served by Changes:
Reviewed changesPer-file summary
FindingsNo blocking issues. Non-blocking (nits, follow-ups, suggestions):
✅ Approved |
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
A few nits, nothing blocking - though I did a relatively quick review
| /// Every EVM chain the probe covers: what `eth_chainId` answers on mainnet, and the decimal | ||
| /// chain id an operator configures. | ||
| const EVM_MAINNETS: [(ForeignChain, &str, &str); 6] = [ | ||
| (ForeignChain::Abstract, "0xab5", "2741"), | ||
| (ForeignChain::Arbitrum, "0xa4b1", "42161"), | ||
| (ForeignChain::Base, "0x2105", "8453"), | ||
| (ForeignChain::Bnb, "0x38", "56"), | ||
| (ForeignChain::HyperEvm, "0x3e7", "999"), | ||
| (ForeignChain::Polygon, "0x89", "137"), | ||
| ]; |
There was a problem hiding this comment.
I find this hard to read. Could you add a struct instead of using nested tuples here? This would remove the need for the comment. I see how this is used in tests, but it would be nice with a struct like:
struct EvmMainnetFixture {
chain: ForeignChain,
expected: &'static str,
answered: &'static str,
}There are probably better field names here.
Also, the relation between "expected" and "answered" seems to be a hex to decimal conversion. Shouldn't this be handled by logic instead of duplicating the same information in the setup?
| None => Self(within_cap), | ||
| Some(_) => { | ||
| let kept: String = within_cap.chars().take(KEPT_CHARS).collect(); | ||
| Self(kept + Self::CUT_SHORT_MARKER) |
There was a problem hiding this comment.
Adding strings with + is a bit unconventional. I'd find using format! more readable here.
| Self(kept + Self::CUT_SHORT_MARKER) | |
| Self(format!{"{kept}{Self::CUT_SHORT_MARKER}"}) |
There was a problem hiding this comment.
961f76d to
9058369
Compare
One eth_chainId call covers Abstract, Arbitrum, Base, BNB, HyperEVM and Polygon, since EvmInspector serves all of them. Chain ids are compared in decimal, the form they are published and configured in, while the RPC answers a hex quantity.
`NetworkFingerprint::new` crops, so the probe's `bounded` helper goes and no inspector can forget the cap. Adds the live chain id check to each EVM manual test, alongside Starknet's.
`MAX_CHARS` and the truncation marker are associated constants, so the test reads the same values the cap is built from. Shares `NO_PARAMS` between the inspectors.
`network_fingerprint` goes through `canonical_fingerprint`, the function the operator's configured value already goes through. Documents what a report shows for an answer that is no fingerprint or is too long, and keeps `U256` out of the crate's public surface.
Also corrects the length of Bitcoin's genesis hash in the cap's doc, and states that the cap must clear every fingerprint, since values are compared after the cut.
9058369 to
6585e55
Compare
netrome
left a comment
netrome
left a comment
There was a problem hiding this comment.
Thank you for updating 🙏
| struct EvmMainnet { | ||
| chain: ForeignChain, | ||
| chain_id: u64, | ||
| } | ||
|
|
||
| impl EvmMainnet { | ||
| /// The form an operator configures. | ||
| fn expected(&self) -> String { | ||
| self.chain_id.to_string() | ||
| } | ||
|
|
||
| /// The `0xXXX` hex quantity an RPC provider answers to an `eth_chainId` request. | ||
| fn answered(&self) -> String { | ||
| format!("{:#x}", self.chain_id) | ||
| } | ||
| } |
3 participants
Closes #4096. Second slice of the identity probing series, after #4013
EvmInspectoralready serves all six chains, soeth_chainIdcovers Abstract, Arbitrum, Base, BNB, HyperEVM and Polygon at once.Notes for review
Chain ids are compared in decimal, the form they are published and configured in, while
eth_chainIdanswers a0xhex quantity. Parsed asU256, since EIP-155 permits ids wider than au64. Text that is neither reads back unchanged, so the report shows what the provider actually claimed.The
NetworkFingerprintlength cap moved intoNetworkFingerprint::new. Replaces the probe'sboundedhelper function. An answer past the cap ends in_TRUNCATED.The injection of time and the RPC client (Inject the foreign chain probe's dependencies and report why setup failed #4043) is deliberately not folded in: I plan to refactor after all chains are wired up for startup probing.