Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make plebbit options more similar to plebbit-js plebbit options #6

Open
estebanabaroa opened this issue Apr 18, 2023 · 10 comments
Open

Comments

@estebanabaroa
Copy link
Member

The current API is:

FLAGS
  --ipfsApiPort=<value>      (required) [default: 32429] Specify the API port of the ipfs node to listen on
  --ipfsGatewayPort=<value>  (required) [default: 32430] Specify the gateway port of the ipfs node to listen on
  --plebbitApiPort=<value>   (required) [default: 32431] Specify Plebbit API port to listen on
  --plebbitDataPath=<value>  (required) [default: /home/runner/.local/share/plebbit] Path to plebbit data path where

Wouldn't it be better if the API was more consistent with plebbit-js. ie:

  --ipfsGatewayUrl
  --ipfsHttpClientOptions
  --pubsubHttpClientOptions
  --dataPath

I would make them singuar, but allow passing more than 1. e.g.:

plebbit --ipfsGatewayUrl https://ipfs.io --ipfsGatewayUrl https://cloudflare-ipfs.com

For chainProviders we could handle multiple URLs with:

plebbit --chainProviders.eth.url site.com --chainProviders.eth.url site2.com

I'm not sure how we would handle ipfsHttpClientOptions as options, maybe we dont allow it, or maybe we do:

plebbit --ipfsHttpClientOptions.0.url https://site1.com --ipfsHttpClientOptions.1.url https://site2.com

or

plebbit --ipfsHttpClientOptions[0].url https://site1.com --ipfsHttpClientOptions[1].url https://site2.com
@Rinse12
Copy link
Member

Rinse12 commented Nov 11, 2023

Are you referring to the command plebbit daemon? The assumption is the daemon will boot up its own IPFS node and the args ipfsApiPort and ipfsGatewayPort will be used to set the ports the IPFS API and gateway will listen on. It's not for connecting to other IPFS APIs or gateways.

There should be a way to set chainProviders for the daemon though. Maybe something like plebbit daemon --chainProviders.eth[0].url https://ethrpc.com

--plebbitDataPath and plebbitApiPort should be changed to --dataPath and --plebbitRpcApiPort

@Rinse12
Copy link
Member

Rinse12 commented Nov 5, 2024

  • imo all plebbit-js options should go under plebbitOptions. We change --plebbitDataPath to --plebbitOptions.plebbitDataPath, and allow other plebbit options like chainProviders to be set like --plebbitOptions.chainProviders.eth.url https://ethgateway.com.

  • Replace port flags with url flags, instead of --ipfsApiPort 12345, it would be --ipfsApiUrl http://localhost:12345. That would allow users to set a remote ipfs node url instead of having to boot up a new ipfs node, which the current implementation forces.

We should allow setting array values via flags without having to declare the flag multiple times, ie plebbit daemon --plebbitOptions.ipfsGatewayUrls 'http://gateway1.com' 'https://gateway2.com'

@estebanabaroa
Copy link
Member Author

estebanabaroa commented Nov 5, 2024

We should allow setting array values via flags without having to declare the flag multiple times, ie plebbit daemon --plebbitOptions.ipfsGatewayUrls 'http://gateway1.com' 'https://gateway2.com'

I like having plebbitOptions.something instead of just something but I don't think an array can be done. I dont know any cli programs that do this. they only allow multiple options when it's the last argument, for example rm file1 file2 file3. I dont know any CLI program that lets you do command --flag something something something --otherflag something something something, they usually force you to repeat them like command --flag something --flag something --flag something, and I think repeating is fine. IMO we dont care that it's long to write, people will only set it up once and probably prepare the command in a text file or something.

@estebanabaroa
Copy link
Member Author

  • Replace port flags with url flags, instead of --ipfsApiPort 12345, it would be --ipfsApiUrl http://localhost:12345. That would allow users to set a remote ipfs node url instead of having to boot up a new ipfs node, which the current implementation forces.

well isnt ipfsApiPort used by the plebbit-cli process to launch kubo? if you launch kubo, you're always on localhost. if you want to use a remote kubo node, why not just set --plebbitOptions.ipfsHttpClientsOption "http://remotekubonode"?

@Rinse12
Copy link
Member

Rinse12 commented Nov 6, 2024

if you launch kubo, you're always on localhost.

A user may want kubo to listen on 0.0.0.0 or other network interfaces (like docker) instead of 127.0.0.1 like it does by default now. imo --ipfsApiUrl should be flexible to accommodate as many setups as possible.

if you want to use a remote kubo node, why not just set --plebbitOptions.ipfsHttpClientsOption "http://remotekubonode/"?

I agree we should be doing that. Although if a user provides both --plebbitOptions.ipfsHttpClientsOptions and --ipfsApiUrl it should throw. So I guess --ipfsApiUrl will be an optional value that defaults to http://localhost:5001 when plebbitOptions.ipfsHttpClientsOptions is undefined. Same thing for --ipfsGatewayPort

  • for --plebbitApiPort, it should be changed to --plebbitRpcApiUrl with ws://localhost:9138 as a default, for the same reasons we changed --ipfsApiPort above. --plebbitRpcApiUrl should always be required, and --plebbitOptions.plebbitRpcClientOptions should be forbidden because there's no point in starting a local daemon just to connect to another daemon

@estebanabaroa
Copy link
Member Author

A user may want kubo to listen on 0.0.0.0 or other network interfaces (like docker) instead of 127.0.0.1 like it does by default now. imo --ipfsApiUrl should be flexible to accommodate as many setups as possible.

true I didnt think about that, since kubo supports putting a full IP in the config, we should also support it so that the settings are just passed from plebbit-cli to kubo directly

@estebanabaroa
Copy link
Member Author

estebanabaroa commented Nov 6, 2024

Same thing for --ipfsGatewayPort

we should change it to --ipfsGatewayUrl since kubo also supports setting a full URL

@estebanabaroa
Copy link
Member Author

if a user provides both --plebbitOptions.ipfsHttpClientsOptions and --ipfsApiUrl it should throw. So I guess --ipfsApiUrl will be an optional value that defaults to http://localhost:5001 when plebbitOptions.ipfsHttpClientsOptions is undefined.

seems fine

@estebanabaroa
Copy link
Member Author

  • for --plebbitApiPort, it should be changed to --plebbitRpcApiUrl with ws://localhost:9138 as a default, for the same reasons we changed --ipfsApiPort above. ``

we can't use RpcApi imo, it has to be API or RPC, not both. and since we already called it plebbit rpc everywhere, and that it's a websocket json rpc, not a rest api like kubo, we should probably call it plebbitRpcUrl, and since rpc-websockets accepts a "host" param, we could use pass the plebbitRpcUrl to the host field, though this could be buggy, like it might block requests that use name.com as "host" even though the user meant to accept them, so maybe we should not use the "host" param yet.

@Rinse12
Copy link
Member

Rinse12 commented Nov 13, 2024

true I didnt think about that, since kubo supports putting a full IP in the config, we should also support it so that the settings are just passed from plebbit-cli to kubo directly

Actually maybe we should have it include ip version as well. Because you do have to specify it in kubo config. This is how I'm setting API url:

await _spawnAsync(log, ipfsExePath, ["config", "Addresses.API", `/ip4/127.0.0.1/tcp/${apiPortNumber}`], { env, hideWindows: true });

IMO it should default to /ip4/127.0.0.1/tcp/5001.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants