Using different ports for http and ws connections

[ochaloup]

I would like to use the PR as a manner to discuss a trouble when I want to use different ports for HTTP(S) and WS(S) connections.
Currently the cluster_urls can be configured as https and then the protocol is switched for wss when websocket subscription is created : https://github.com/blockworks-foundation/mango-explorer/blob/main/mango/websocketsubscription.py#L62

What I would need is having chance to define different protocol https vs. ws and/or different port for WS and HTTP connections.

I'm thinking about changes in code. (The currently proposed ones neither work fully nor are clean for being good to be merged.)

I was thinking to have instead of cluster_urls: typing.Sequence[str] a dictionary like cluster_urls: typing.Dict[str,str] where would be possible to say cluster_urls = {'https': 'https://localhost:8080', 'ws': 'ws://localhost:8181'}. But this kind of change is more difficult to place to be loaded from command line args (or maybe not and I just need to find how to do it). I took kind of side step here in way to represent the issue.

If such kind of change would be desirable then I would fix and clean the proposed code in way that will be recommended to me.
If such kind of change is not a thing that should be part of this codebase I will delete this PR.

Thank you for any feedback.

[OpinionatedGeek]

Thanks for this! As you've noticed, the creation of the websocket URL was a quick job to get it working, and it's a good idea to revisit it to make it a bit more correct/useful.

I like how you have the Client and RPCCaller entirely unaware of how the websocket URL is created/configured/derived. Neat!

I'm not sure about the name cluster_url_ws. I suppose if we were starting this now I'd like the BetterClient/1RPCCallerproperties to becluster_rpc_urlandcluster_ws_urlbut I'm not sure it's worth changingcluster_url` now. What do you think?

The biggest headache, as you say, is configuring this, especially from the command line. I don't have a great answer, but here's a possibility I'd like your opinion on:

Right now, --cluster-url takes the RPC node's URL. How about this is extended so it takes the RPC node's URL, followed (optionally) by a separator, then by the websocket URL.

It's optional, so if it's not provided the websocket URL is derived the way it currently is (which works with most RPC node providers). If it is provided, that's passed to the client instead of the derived version.

I'm not sure what to use a separator - it needs to be something that's not (usually) valid in a URL, and something not (usually) interpreted by the shell. I can't think of one right now but imagine it was %. Then you could pass:

--cluster-url https://localhost:8080

for the current behaviour, or:

--cluster-url https://localhost:8080%ws://localhost:8181

What do you think?

[ochaloup]

@OpinionatedGeek great, thanks. Based on your comments I changed the PR and now it's ready for review and potentially for merging.
I changed for cluster_rpc_url/cluster_ws_url as it sounds me cleaner.
I decided to use for argparse nargs=* and the parameter can be specified like --cluster-url http://localhost ws://localhost

The tests are passing, I tried to run with a command from bin and seems to work fine.

Just the matter of fact that the API of the methods changed. I decided to create a data transfer object that brings data about http connection url and ws connection url.

WDYT?

[ochaloup]

@OpinionatedGeek ping, how do you feel about these changes?

[OpinionatedGeek]

Sorry. I'm sure it felt like I was ignoring you.

One quick thing - you've added pyproject.toml to .gitignore and that's probably a bad thing.

I didn't know about nargs and that's neat! I'm still unsure about it in practice though. (I can be convinced...)

Let's compare an example command done two ways.

Using this PR:

show-group --cluster-url https://api.mainnet-beta.solana.com wss://api.mainnet-beta.solana.com --cluster-url https://solana-api.projectserum.com wss://solana-api.projectserum.com

Using a splitter token of '::':

show-group --cluster-url https://api.mainnet-beta.solana.com::wss://api.mainnet-beta.solana.com --cluster-url https://solana-api.projectserum.com::wss://solana-api.projectserum.com

Downside of this PR: you can have 'wrong stuff' on the command line without noticing, like:

show-group --cluster-url https://api.mainnet-beta.solana.com wss://api.mainnet-beta.solana.com NeverGonnaGiveYouUp --cluster-url https://solana-api.projectserum.com wss://solana-api.projectserum.com NeverGonnaLetYouDown

Downside of a splitter token of '::': whatever splitter token we use, it's possible it's going to appear in a valid URL. '::' is unlikely, certainly in an RPC server URL, but not impossible.

I think both downsides are nit-picky. I suppose the fundamental question I'm not sure about is: will users expect/be able to use a command-line parameter that takes two values, or will the expect/be able to use one that takes a 'compound' value. Having a parameter --X Y Z looks weird to me but I'm not sure why. Also I'm not sure --X Y::Z is better.

What do you think?

[ochaloup]

Sure, I removed the .gitignore from the set.

Originally I was thinking that it's better to use the python option for parsing parameters than having defined a artificial splitter token.
My intention was that the user will use in most cases the one parameter and if he finds a need for a different url for ws connection then he will check the doc anyway (even for the splitter option).

But I do not have strong preference. I feel from your comment that you prefer the splitter (::) I can rework the PR that way.

As I placed my points then just please let me know what you want better. I can change the code here.

[OpinionatedGeek]

Honestly I don't have a strong preference either. I think I did prefer the '::' approach but now I'm not so sure. And maybe part of my reluctance was never having used nargs before.

How about: we change the current implementation to raise an exception if there are more than 2 arguments (so it can be --cluster-url RPCSERVER or --cluster-url RPCSERVER WSSSERVER but not --cluster-url RPCSERVER WSSSERVER SOMERANDOMTHING? That would address one thing that worried me a little.

And then we go with your approach, and I'll merge it in.

(Oh, also I appear to have introduced a conflict. Sorry.)

[ochaloup]

@OpinionatedGeek sure, nice, thank you for the confidence in my approach :-) I will fix both later today. Thanks for your time looking into this.

[ochaloup]

@OpinionatedGeek +1. I pushed the changes, it's ready for review.

[ochaloup]

I updated the doc. If you consider the PR good enough for being merge then I realized there will be a self conflict with #30. I will fix that conflict then. Ideally if you can merge this PR first.

[OpinionatedGeek]

Thank you so much for this!