[LIT-2333] Add e2e test for initializing multiple clients #331

cairomassimo · 2024-01-29T11:15:32Z

Description

This PR enables the use of multiple instances of LitNodeClient, in order to connect to different networks in the same application, at the same time.

This was achieved by removing the global variables litConfig, litNodeClient, logger and logManager and their references.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added an e2e test which calls .connect() on multiple clients.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

This commit removes the following global variables: logManager, logger, litConfig, litNodeClient

CLAassistant · 2024-01-29T11:15:38Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

joshLong145

I see you removed methods which we where using in many packages. Can you outline the implications and behavior changes that are introduced with these utilities gone? #331 will also add many of these methods. back when merged. There does not seem to be a path forward for the above mentioned pr with the current implementation.
While removing the implementations which utilize the global state does make a path forward, I am unsure what this pr recommends as a migration from the previous implementations.

joshLong145 · 2024-01-29T14:34:56Z

e2e-nodejs/group-connection/test-connection-multi-network.mjs

+    if (!client.ready) {
+      return fail('client not ready');
+    }
+    if (client.config.litNetwork !== network) {


Could we add a check on globalThis to assert on the new global namespacing?

I didn't implement the global namespacing, I think it's a risky approach (see also #331 (comment)). There could be a use case for global namespaces I'm not aware of yet: in that case please let me know.

joshLong145 · 2024-01-29T14:35:36Z

packages/lit-auth-client/src/lib/lit-auth-client.ts

@@ -60,7 +60,6 @@ export class LitAuthClient {
   */
  constructor(options?: LitAuthClientOptions) {
    this.providers = new Map();
-    bootstrapLogManager('auth-client');


why did we remove this line?

joshLong145 · 2024-01-29T14:36:46Z

packages/misc/src/lib/misc.ts

 import { version } from '@lit-protocol/constants';

-const logBuffer: Array<Array<any>> = [];
+const defaultLogger = LogManager.Instance.get('lit-sdk');


will this be a global logger? I would like to keep the package names as categories if possible. Open to discussion however.
Also why did you remove the buffer which allows us to collect logs when we are not in a connected state?

joshLong145 · 2024-01-29T14:37:43Z

packages/misc/src/lib/misc.ts

-  if (!globalThis) {
-    // there is no globalThis, just print the log
-    console.log(...args);
-    return;
-  }
-
-  // check if config is loaded yet
-  if (!globalThis?.litConfig) {
-    // config isn't loaded yet, push into buffer
-    logBuffer.push(args);
-    return;
-  }
-
-  if (globalThis?.litConfig?.debug !== true) {
-    return;
-  }
-  // config is loaded, and debug is true
-
-  // if there are there are logs in buffer, print them first and empty the buffer.
-  while (logBuffer.length > 0) {
-    const log = logBuffer.shift() ?? '';
-    globalThis?.logger &&
-      globalThis.logManager.get(globalThis.logger.category).error(...log);
-  }
-
-  globalThis?.logger &&
-    globalThis.logManager.get(globalThis.logger.category).error(...args);


Why did we remove all of these utilities?

This reverts commit 14f35f0.

cairomassimo · 2024-01-30T13:48:55Z

@joshLong145 I updated the PR so that it only adds the e2e test for connecting to multiple networks. The test passes with the code as is.
We discussed about the changes on the logger in a call, but overall I think it makes sense to not apply those changes in this PR anyway.

To be clear, with the code as it is, creating multiple clients does mess up the global state (globalThis.litConfig and globalThis.litNodeClient get replaced). However:

this is the current behavior anyway, so the PR is not introducing a regression here,
I think we should recommend the SDK consumers to not rely on the global state anyway, rather than introducing a namespacing convention, espcially since they have to instantiate the clients themselves anyway.

There is one more check to do: making sure that there are no conflicts with the usage of localStorage when instantiating more than one client. I couldn't find any, but it's better that someone with more experience with the codebase has a look at this specifically.

joshLong145 · 2024-01-31T15:57:34Z

@cairomassimo can we close this pr since we no longer need to make code changes?

cairomassimo · 2024-02-01T09:34:31Z

@cairomassimo can we close this pr since we no longer need to make code changes?

@joshLong145 It's not much, but we can merge it so we have the new e2e test case? I'm making it "ready for review" just in case

Ansonhkg

I'm ok with having this for our e2e tests

* make capacityDelegationAuthSig optional (#337) * Ref/run sev att check on mainnets (#339) * add sev snp att check by default on main nets * fmt * [LIT-2333] Add e2e test for initializing multiple clients (#331) * Remove globals for loggers and client This commit removes the following global variables: logManager, logger, litConfig, litNodeClient * Test e2e parallel connections to multiple networks * Revert "Remove globals for loggers and client" This reverts commit 14f35f0. * Cleanup/logger (#332) * aggregate logs under categories. allow for cross category tracing * autogen * fmt * change log persistence to use request id's as keys for lookup * fmt * fmt * fix logger creation * fmt * fmt * Update/test token (#336) * update token symbol * fmt * fmt * fmt * fix: rm console.log (#342) * Fix logging tests in 3.1.2 staging (#343) * fix logging tests * fix category in test * Published version: 3.1.2 --------- Signed-off-by: Anson <[email protected]> Co-authored-by: Bean <[email protected]> Co-authored-by: Massimo Cairo <[email protected]>

cairomassimo added 2 commits January 29, 2024 12:21

Remove globals for loggers and client

14f35f0

This commit removes the following global variables: logManager, logger, litConfig, litNodeClient

Test e2e parallel connections to multiple networks

ef931a2

cairomassimo marked this pull request as ready for review January 29, 2024 11:16

cairomassimo requested review from joshLong145 and Ansonhkg as code owners January 29, 2024 11:16

joshLong145 suggested changes Jan 29, 2024

View reviewed changes

cairomassimo marked this pull request as draft January 29, 2024 16:40

Revert "Remove globals for loggers and client"

06f166c

This reverts commit 14f35f0.

cairomassimo changed the title ~~[LIT-2333] Add support for initializing multiple clients~~ [LIT-2333] Add e2e test for initializing multiple clients Jan 30, 2024

cairomassimo requested a review from joshLong145 January 30, 2024 13:49

cairomassimo marked this pull request as ready for review February 1, 2024 09:34

Ansonhkg approved these changes Feb 1, 2024

View reviewed changes

Ansonhkg changed the base branch from master to staging/2024-02-06 February 2, 2024 12:41

Ansonhkg merged commit 7fbb90e into staging/2024-02-06 Feb 2, 2024
1 of 4 checks passed

Ansonhkg deleted the feature/lit-2333-add-support-for-initializing-global-lit-variables-under branch February 2, 2024 12:41

Ansonhkg mentioned this pull request Feb 2, 2024

Publish version 3.1.2 #340

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LIT-2333] Add e2e test for initializing multiple clients #331

[LIT-2333] Add e2e test for initializing multiple clients #331

cairomassimo commented Jan 29, 2024 •

edited

Loading

CLAassistant commented Jan 29, 2024

joshLong145 left a comment •

edited

Loading

joshLong145 Jan 29, 2024

cairomassimo Jan 30, 2024

joshLong145 Jan 29, 2024

joshLong145 Jan 29, 2024 •

edited

Loading

joshLong145 Jan 29, 2024

cairomassimo commented Jan 30, 2024

joshLong145 commented Jan 31, 2024

cairomassimo commented Feb 1, 2024

Ansonhkg left a comment

[LIT-2333] Add e2e test for initializing multiple clients #331

[LIT-2333] Add e2e test for initializing multiple clients #331

Conversation

cairomassimo commented Jan 29, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

CLAassistant commented Jan 29, 2024

joshLong145 left a comment • edited Loading

Choose a reason for hiding this comment

joshLong145 Jan 29, 2024

Choose a reason for hiding this comment

cairomassimo Jan 30, 2024

Choose a reason for hiding this comment

joshLong145 Jan 29, 2024

Choose a reason for hiding this comment

joshLong145 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

joshLong145 Jan 29, 2024

Choose a reason for hiding this comment

cairomassimo commented Jan 30, 2024

joshLong145 commented Jan 31, 2024

cairomassimo commented Feb 1, 2024

Ansonhkg left a comment

Choose a reason for hiding this comment

cairomassimo commented Jan 29, 2024 •

edited

Loading

joshLong145 left a comment •

edited

Loading

joshLong145 Jan 29, 2024 •

edited

Loading