RPC Client V2 - TODOs

[bkontur]

Relates to: paritytech/polkadot-sdk#4494

1. https://github.com/paritytech/polkadot-sdk/pull/4494/files#diff-6f85287d47d35ae153cf13830b47d293d811605b322de8b7ff53a94bf96c11f1R37

// TODO (https://github.com/paritytech/parity-bridges-common/issues/2133): remove `Environment` trait
// after test client is implemented
#[async_trait]
impl<C: Chain, T: crate::client::Client<C>> Environment<C> for T {
	async fn header_id_by_hash(&self, hash: HashOf<C>) -> Result<HeaderIdOf<C>, Error> {
		self.header_by_hash(hash).await.map(|h| HeaderId(*h.number(), hash))
	}
}

• Q: remove transaction_tracker::Environment and replace by trait Client<C> directly?

---

2. https://github.com/paritytech/polkadot-sdk/pull/4494/files#diff-6f85287d47d35ae153cf13830b47d293d811605b322de8b7ff53a94bf96c11f1R34-R35

        // TODO (https://github.com/paritytech/parity-bridges-common/issues/2133): remove me after
	// test client is implemented
	/// Converts self into tracker with different environment.
	pub fn switch_environment<NewE: Environment<C>>(

• CachingClient replaces RpcClient backend by itself for TransactionTracker https://github.com/paritytech/polkadot-sdk/pull/4494/files#diff-27a6843672484e5bb95531315d54d18b81bee9af8de8c43cffb3d4dcaf40f4b4R438-R441
• Q: switch_environment rename to switch_client? or just remove that TODO?

---

3. ✅ relay_utils::relay_loop::Client for CachingClient - this is not used, I removed it and everything works...

// TODO (https://github.com/paritytech/parity-bridges-common/issues/2133): this must be implemented for T: Client<C>
#[async_trait]
impl<C: Chain, B: Client<C>> relay_utils::relay_loop::Client for CachingClient<C, B> {
	type Error = Error;

	async fn reconnect(&mut self) -> Result<()> {
		<Self as Client<C>>::reconnect(self).await
	}
}

• Q: what is the rationale here? remove reconnect from trait Client and/or extend it e.g.: trait Client: + relay_utils::relay_loop::Client?
  • this could mess up lots of code, because of &mut for relay_utils::relay_loop::Client::reconnect(&mut) vs Client::reconnect(&self)
• remove based on comment

---

4. Continuation of: Traitify Substrate client #2129 - is this still actual?
   What is left for future PRs:

• make the relay code generic over underlying client type. Right now we are using CachingClient<RpcClient> everywhere, but instead we shall be able to use anything that implements Client trait;
  • Q: what does this mean? Do we want to replace pub type DefaultClient<C> = relay_substrate_client::RpcWithCachingClient<C>; with just trait Client<C>?
• introduce TestClient;
• use TestClient in tests where right now we have (or must have) "environment" traits;
  • Q: do we need to still create TestClient with configurable TestClientData e.g. similar what we have for https://github.com/paritytech/polkadot-sdk/blob/master/bridges/relays/messages/src/message_lane_loop.rs#L663-L664
• more cleanup - e.g. errors with context.

[svyatonik]

1. Yes, the idea was to replace it directly with TestClient, because test client should have all means to provide required headers by hashes (and hence no need to mock the header_id_by_hash call). But I had some issues with properly implementing TestClient, so it was not done :/
2. That's a consequence of (1) - once we have the TestClient, we will remove Environment and this fn will become obsolete
3. Let's remove it - if it works, then w have probably already removed something that was using that impl :) No need to remove Client::reconnect - it should still be available
4. Yes, the idea was to remove DefaultClient and make everything generic, because IIRC some relays are still non-generic. But given Remove complex, on-demand and batch relayers code #3002, it should be inactual - we'll only loops that are generic once we remove on-demand and complex relays. Re TestClient - as I've saide before, it isn't trivial to impl it, because e.g. when someone calls get_raw_storage, we are unable to understand what stands behind that storage key in low-level Client, so we need to propagate this knowledge (what we are requesting, what we are calling) to the low level code (TestClient) and match there. See Tests for on-demand relays #2413 (comment) for details. But as I said, with on-demand and complex relays removal, TestClient importance becomes low (althought it'll be still helpful for some tests) - remaining relay loops have their own 'clients' and have a tests suite.

[svyatonik]

Errors with context - I think it has already been implemented in some way in client v2 - now at least error message is descriptive - so it isn't just Failed to decode value, it is something like Failed to read storage key 0xagagagagagagagag at Rococo Bridge Hub: Failed to decode value