refactor: inject the inspectors into RPC probing path - #4244
Conversation
df5f6e2 to
2b5d1b2
Compare
2b5d1b2 to
83aaaea
Compare
712f054 to
788dd53
Compare
04e2103 to
5e32044
Compare
0931f52 to
1f6729a
Compare
28d6bd7 to
d8d8a48
Compare
BuildRpcClients is the seam: choosing it chooses the transport, and by extension every inspector built over it, so a caller can put the whole set on clients it controls. RpcClients reaches the real providers. Nothing uses it yet. The probe and transaction verification each build their own inspectors today, and move across separately.
Building the client that talks to a provider and building the inspector over that client are separate concerns, so they get a module each. The crate resolved credentials and nothing else, so it is renamed for what it builds now.
Free functions can only be called. As trait methods they can be implemented, so a caller can answer for the inspectors themselves rather than only for the transport beneath them. A blanket implementation over BuildRpcClients keeps both levels open: choosing the clients settles the inspectors, and a caller that needs to supply its own implements BuildInspectors directly and builds no client at all.
A caller that spans chains, as the probe does, needs a single type to hold them behind. BuildInspectors::any answers with it, or with None where no inspector exists, and its match is the only list of covered chains. Reaching that needed canonical_fingerprint to take self: with no receiver there is nothing to dispatch on, so no erased inspector could implement the trait. Every implementation already ignored the distinction.
probe_all_providers takes a BuildInspectors, so the chain dispatch it used to carry moves behind the factory and its coverage becomes one list rather than two. InspectorFactory holds the clients it builds over, so choosing the clients and choosing the inspectors stay separate decisions. The tests that were bounded by the wall clock now script an inspector under paused time, taking the timeout case from a second to microseconds. The ones that still stand up a server keep covering what a script cannot: real request shapes, auth splicing and client setup.
BuildInspectors is now the only seam: it lives in foreign-chain-inspector, and RpcInspector is a concrete enum over the real transports. The BuildRpcClients trait and RpcClientFactory had no consumers and only made the transport a detail the caller had to pick; InspectorFactory becomes a unit struct. The verify path's duplicated construction is left for a follow-up.
d8d8a48 to
9a40dfe
Compare
Pull request overview
Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
|
@claude review |
Pull request overview
Since the previous review round, the two functional blockers are resolved: Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
Our intension is to have inspectors for and probe all chains that are defined in our code base. Not supporting a chain is not the final state, I feel adding a test to check that "we are still in progress" is overkill. |
netrome
left a comment
There was a problem hiding this comment.
Just some minor considerations from my side.
| replies: Arc<Mutex<VecDeque<MockReply>>>, | ||
| calls: Arc<AtomicUsize>, |
There was a problem hiding this comment.
Nit: calls can be derived from replies if the initial length is known so we could drop the atomic here.
There was a problem hiding this comment.
I wanted to use calls here to enable the test cases to verify that "Inspector was called x times", which is sort of independent from number of MockReplies configured.
| .expect("call beyond the queued replies"); | ||
| match reply { | ||
| MockReply::Answer { delay, fingerprint } => { | ||
| tokio::time::sleep(delay).await; |
There was a problem hiding this comment.
What's the purpose of adding a delay here? This feels strange.
There was a problem hiding this comment.
It is not used in this PR directly, but I added this so we can test Fanout concurrency. Aka given a Fanout with 1 inspector taking 1 sec and another taking 3 sec, Fanout should return after 3 sec not 4 sec.
There was a problem hiding this comment.
Okay and how do you test that reliably? With tokio paused time and advance? That also reminds me, seems like we're not running these tests with paused tokio time - something we probably should do if we let tokio act as the time abstraction without controlling it ourselves.
There was a problem hiding this comment.
Yes my plan is to use tokio paused time. That is the next PR in the stack.
Closes #4043.
probe_all_providersnow takes its inspectors as a dependency via theBuildInspectorstrait. Production path uses a new structInspectorFactoryand tests can re-implement this trait and utilizeMockInspectorforeign-chain-rpc-authcrate is renamedforeign-chain-rpc-factory, since it now includes code to resolve credentials and build inspectors.SIde notes:
timeout_ofmoved ontoForeignChainConfig::timeout_duration().foreign-chain-rpc-factorywill contain all client and inspector construction methods. To restrict the scope of this PR I purposefully tried not to touch the TX verification route.