Change how `IFederationApi` works.

[dpc]

The IFederationApi doesn't scale well (we have to keep making unimplemented!() in every mock implementations), and I'm quite convinced it's not the right interface to put there. And it's up to the caller to know both the call meaning and the implementation (like types to be serialized, but also who to call and when exactly ("strategy")).

It seems to me that what we really need is:

pub trait IFederationApi {
    async fn request_member_raw<R>(
        &self,
        peer_id: PeerId,
        method: &str,
        params: &[serde_json::Value],
    ) -> Result<serde_json::Value>;

    async fn request_union(&self, method: &str, params: &[serde_json::Value]) -> Vec<serde_json::Value> {
       /* default impl using Self::request_member_raw and union strategy */
     }
    async fn request_current_consensus(&self, method: &str, params: &[serde_json::Value]) -> Vec<serde_json::Value> {
       /* default impl using Self::request_member_raw and current consensus strategy */
    }
    /* ... */
}

+/- some tweaks and helper functions to auto-convert stuff to/from serde_json::Value

The mocking will have to happen on string level (intercept whole request_member_raw and match method etc.)

This way picking the strategy, dealing with serialization types and any other logic is entirely on the caller of IFederationApi, while the implementation takes care of delivering the requests, and possibly faking responses.

Feedback appreciated.

[dpc]

@elsirion @jkitman :+1: / :-1: :question:

[jkitman]

That could make sense...the API needs to be modularized as well, maybe this would help?

[dpc]

That could make sense...the API needs to be modularized as well, maybe this would help?

After this there will be no need to modularize as we do not list each individual call anymore.

[elsirion]

Each client module Foo could define a trait FooFedimintAPI[^1] and impl it for any dyn IFederationAPI, e.g.

impl LnFederationAPI for &dyn IFederationAPI { // alternatively for T: IFederationAPI
    async fn get_contract(id: ContractID) -> Contract {
        self.request_current_consensus("/ln/contract", id).await
    }
}

[^1]: will be slightly different now with instance IDs, maybe needs some wrapper type that contains the instance id and a ref to the general API client

[dpc]

Edit:

Ah, OK, sorry, I didn't read carefully. Yes, the types modules can wrap the DynFederationApi however they desire to avoid mistakes.

I don't know if another trait is not overdoing it (struct LnFederationApi(DynFederationApi) would do just fine, I guess), but whatever works, so if there's a reason for traits or combination of wrappers and traits, we'll do that.

Previous comment

We really don't need to play play with a per-module-kind apis. At the end of the day everything goes through a same calling convention which is stringly. I know that in Rust stringly makes you :vomiting_face: , but such is the nature of the network APIs, and we should just accept it and embrace the benefits (it being a general purpose/abstract without any extra effort).

The caller knows:

1. what is the call and with what arguments
2. what strategy it requires (which peer to call an when)
3. which instance id they are referring to

The caller can use constants, enums, functions, whatever to avoid mistakes with 1. - which is the biggest issue with stringly apis.

We can add extension traits / extra methods with default impls to make calling the right consensus strategy short and easy if the caller has one of common needs.

The job of this interface is really to give us a single call where we can intercept / stub the responses, validate some stuff etc. and switch the underlying delivery mechanism.

The job of the trait implementation is to just deliver the call and return the response.