Change function signature for Client constructor

[djmbritt]

I've been playing around with the code a little bit, and I think it would be useful to change the function signature for the Client constructor.

There are two issues, first, we have two parameters for the constructor function:

• network: Network
• clientOpts: ClientOpts

We should integrate network in to ClientOpts, then we'll have:

export interface ClientOpts {
    network?: Network;
    ipfsCacheDurationInMs?: number | null;
    fetchProvider?: FetchProviderOptions;
    cacheImplementation?: Cache;
}

Second, when instantiating the constructor, you need to pass in which network you need to connect to.
I think for testing purposes, it should be chosen automatically for the user.
On one hand, I think it's useful to connect to mainnet right away, on the other hand, I like the pattern where the user needs to explicitly opt in to connect to the mainnet.
I'm not sure yet which would be better.

But then we'll end up with:

const client = createClient()

Which will automatically connect to mainnet or testnet.

[Jeffrieh]

Hi David!

Some good points, I created it to have a clear seperation between network and client options. I think these two should stay seperate for now, and i think users should always explicitly provide the network to the client. Automatically choosing a default chain might lead to confusion, especially because there might be multiple testnets on a single chain, and later on the sdk might become operational on multiple chains.

The ClientOpts is meant for some optional configuration for the client, things like a custom cache implementation, cache duration, auto connecting, and other optional things that might come up later down the line.

[djmbritt]

I've been writing tests using the createClient({ network: jungle }) function, and I've gotten used to it.
I've come around to it and I think you're right, the user should be explicit about it every time, to which network they are connecting to.

[djmbritt]

One other issue I did want to bring up is that I noticed that @wharfkit has a very similar type to what we are using for the Network type.

I was wondering if we could switch over to using it.

So we have this interface:

export interface Network {
	name: string;
	explorerUrl: string;
	eosRpcUrl: string;
	eosChainId: string;

	config: NetworkConfig;
}

And there is this interface for something similar in @wharfkit/common:

interface ChainDefinitionArgs {
	id: Checksum256Type;
	url: string;
	logo?: LogoType;
	explorer?: ExplorerDefinitionType;
	accountDataType?: typeof API.v1.AccountObject;
}

If we use the ChainDefinitionArgs, we can shorten the creation of the Session.
We incorporate it into the SDK, we tweak the configuration to use ChainDefinitionArgs, implement or inherit it so that we can use add the config: NetworkConfig into the interface.
And we should be able to initiate the session like this:

// Retrieve env vars
const { chain, permission, actor, privateKey } = destructureEnv(testEnvNetwork);

// Create client
const client = createClient({ chain });

// Set up wallet with privatekey
const walletPlugin = new WalletPluginPrivateKey(privateKey);

// Set up session with wallet and chain
const session = new Session({ actor, permission, walletPlugin, chain });

The only thing that still bugs me about this is that we have to specify the chain twice, once in the createClient function, and once more in the Session constructor.

[Jeffrieh]

hmm right, it indeed looks a bit redundant to pass the network to both the session and the client, perhaps we can extend our createClient function to either accept a network, or a session. So for use cases where we dont want to use sessions we can just pass in the network, but if we establish a session, we can just pass this straight into the createClient

[Jeffrieh]

as for the types, i think it's fine for now to tightly couple it to wharkit's version of chains/networks, initially i tried to keep it a bit more generic but let's go ahead and extend the ChainDefinitionArgs if that makes the setup a bit easier.

[djmbritt]

hmm right, it indeed looks a bit redundant to pass the network to both the session and the client, perhaps we can extend our createClient function to either accept a network, or a session. So for use cases where we dont want to use sessions we can just pass in the network, but if we establish a session, we can just pass this straight into the createClient

I do think it's still important to be able to create a Client without passing in a session.
Especially to be able to do generic things such as retrieving data from the chain.

[djmbritt]

This has been done, looking a lot better, closing for now.