scs / substrate-api-client

Library for connecting to substrate API over WebSockets
Apache License 2.0
260 stars 122 forks source link

Support http endpoints #115

Closed chevdor closed 2 years ago

chevdor commented 3 years ago

I would be nice being able to connect via http as well :)

trevor-crypto commented 3 years ago

@clangenb continuing from the comments in #121... I'd like to implement that Client trait, but it seems a little coupled with the Api because of the subscriber model, which wouldn't exist in an HTTP RPC.

fn's such as subscribe_events, subscribe_finalized_heads, etc should be moved out of Api and to WsRpc (new websocket rpc) ? Perhaps I can also try conditional compilation to include those functions, which will be necessary anyway for excluding the ws crate

clangenb commented 3 years ago

Ah yes, for http it would make sense to separate this.

I would go for a different approach, which I had planned for anyhow. I would like to separate the Api's methods as I have done it here in some extension traits. https://github.com/scs/substraTEE-worker/pull/268

Hence, we can extract the subscriber methods to a Subscriptions trait (maybe improve naming). If we go that path, I think it might make sense to feature gate http-client and ws-client, as most users will not need both anyhow. Subscriptions would be only available in with the ws-client feature.

trevor-crypto commented 3 years ago

@clangenb ok great. I will put up a PR soon and work through any feedback that you have.

I think it might make sense to feature gate http-client and ws-client

For my use case it's important that the http client is generic (so I can implement them in my own project with various backends), so I will try to feature-gate ws-client only if that's ok

trevor-crypto commented 3 years ago

Another thing I am trying to figure out is how to add the Client to the Api:

pub struct Api<P, C>
where
    P: Pair,
    MultiSignature: From<P::Signature>,
    C: RpcClient,
{
    pub url: String,
    pub signer: Option<P>,
    pub genesis_hash: Hash,
    pub metadata: Metadata,
    pub runtime_version: RuntimeVersion,
    client: C,
}

Doing it like this will change the signature of Api::new() which is sort of undesirable, since everyone using the library will need to update their code. Please let me know if you have any ideas

clangenb commented 3 years ago

I think this will be unpreventable. Usually, you could work around this with the builder pattern, i.e.,

Api::new().with_client(client); but as the client is mandatory for the Api to be usable, this does not make sense for me. But for me, it is okay if we change the signature, we will just bump the version.

Just as a sidenode, something that I would do differently today: I would remove the constraints on P while initializing. If you only fetch storage you don't need a signer and for all these methods you could just have: Api<(), Client>. (But you don't need to change that if you don't want.)

trevor-crypto commented 3 years ago

@clangenb Thanks for the tips.

I would remove the constraints on P while initializing.

This is actually much better for my use case, so I have done that as well. I will try to get a draft pull request up, but need to finish some work first.

Side question: do you know specifics around why substrate-api-client won't work for Polkadot/Westend? I saw it on the README but I couldn't understand why.

clangenb commented 3 years ago

No hurry.

Side question: do you know specifics around why substrate-api-client won't work for Polkadot/Westend? I saw it on the README but I couldn't understand why.

Hmm, I did not know this at all. I actually believe this is an outdated statement.

@brenzi would probably know this.

brenzi commented 3 years ago

By now it should work if nailing cargo to the right commit on substrate. I think the remark is outdated

trevor-crypto commented 3 years ago

Just confirming, yes, it does work with the most recent Polkadot. Funnily enough, the official crate (substrate-subxt) isn't working with the latest version due to metadata being old!

clangenb commented 2 years ago

If you supply a custom client, we do support http requests already: https://github.com/scs/substrate-api-client/blob/66d9734d7a88ec475514062a6b3499230de990a4/src/std/mod.rs#L57

lucgerrits commented 2 years ago

So in the end, does the library support HTTP endpoints ?

clangenb commented 2 years ago

We do not provide an http client in this library yet, but you can provide your own client that implements the RPCClient, this may very well be an http-client.