LIT-3394 - Improve connect() performance

[MaximusHaximus]

Description

Improves connect() performance by parallelizing some operations performed during connection to the LIT network.

• Solution leverages the existing contractContextfunctionality to maintain backwards compatibility with consumers who use that API
• Fixed config.rpcUrl being optional string, but also a union with null -- this conflicted with the definition of same in the contract SDK and it made no sense to default an optional parameter to null anyway 🤦 :)
• Moved verification of the litNetwork to the constructor rather than it being nestled inside of setConfig() where it would explode the first time someone actually connected with the client after configuring it with an invalid network
• Removed setNewConfig, which was an internal method that would leave the core / client in an inconsistent state if ever called outside of the connect() flow.
• Standardized initialization so that we no longer have different looking branches that actually behave identically for all networks. It turns out setConfig() was much less complex in practice than it looked at first glance.
• Note that this appears to remove the 'using custom bootstrapUrls' logic -- but this functionality was already not functional; we always overrode config.bootstrapUrls with values returned from the chain or the lit-general-worker during connect() processing.

Type of change

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] 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?

• [x] Ran local tests against Habanero and Cayenn

Checklist:

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

[MaximusHaximus]

Looks good, one question i have is if we want to reuse the staking contract instance in the static contract-sdk calls. This in theory will save us some cycles in new object creation but curious your thoughts.

I looked into that, actually -- but it looks like the only time actual Ethers contract instances are maintained is inside use the non-static version of the contract SDK instance objects -- the instance does take in a contractContext object, and creates actual ethers.js Contract instances and keeps them in scope.

I actually considered just changing core to use an instance of the contract SDK instead for this reason, because that's actually the behaviour we want here -- but it would mean that core would then keep copies of all contract instances in scope and all it actually needs is the StakingContract, and the instance logic has a bunch of other complexity in it that seemed superfluous for the usage we need in core

I wanted to keep respecting the existing rpcUrl and contractContext logic, which is entangled inside of the getValidators getStakingContract and getMinNodeCount methods.

I thought about in-lining the logic from those methods into lit-core to ensure we only ever created one-and-only-one contract instance per core instance, but getValidators is actually a fair amount of code and I didn't want to duplicate it.

I think maybe the cleanest way to handle this would be to allow actual contract instances to be passed in LitContractContext instead of just the LitContract definitions of those Contract instances -- then update any method that takes a LitContractContext to use the Contract instance provided rather than creating its own internally if they exist.

I didn't undertake this as part of the PR because the creation of the extra contract objects was 'cheap' (I couldn't benchmark any difference at all in performance with 1 instance vs. having the methods create extras, though there is obviously some temporary memory usage overhead), and it seemed like a bit of a rabbit-hole to go down given our desire to ship the connect enhancements promptly. WDYT?

[joshLong145]

Looks good, one question i have is if we want to reuse the staking contract instance in the static contract-sdk calls. This in theory will save us some cycles in new object creation but curious your thoughts.

I looked into that, actually -- but it looks like the only time actual Ethers contract instances are maintained is inside use the non-static version of the contract SDK instance objects -- the instance does take in a contractContext object, and creates actual ethers.js Contract instances and keeps them in scope.

I actually considered just changing core to use an instance of the contract SDK instead for this reason, because that's actually the behaviour we want here -- but it would mean that core would then keep copies of all contract instances in scope and all it actually needs is the StakingContract, and the instance logic has a bunch of other complexity in it that seemed superfluous for the usage we need in core

I wanted to keep respecting the existing rpcUrl and contractContext logic, which is entangled inside of the getValidators getStakingContract and getMinNodeCount methods.

I thought about in-lining the logic from those methods into lit-core to ensure we only ever created one-and-only-one contract instance per core instance, but getValidators is actually a fair amount of code and I didn't want to duplicate it.

I think maybe the cleanest way to handle this would be to allow actual contract instances to be passed in LitContractContext instead of just the LitContract definitions of those Contract instances -- then update any method that takes a LitContractContext to use the Contract instance provided rather than creating its own internally if they exist.

I didn't undertake this as part of the PR because the creation of the extra contract objects was 'cheap' (I couldn't benchmark any difference at all in performance with 1 instance vs. having the methods create extras, though there is obviously some temporary memory usage overhead), and it seemed like a bit of a rabbit-hole to go down given our desire to ship the connect enhancements promptly. WDYT?

This makes sense, and yes I want to move the static methods out to lit-core helpers as we only need the staking contract and small abi which we can just use static strings for to avoid having to bundle the entire staking contract abi into LitCore I agree doing this is not in scope of this pr. There is a task for this in Linear already so we will address it when that task is pulled from the backlog.

[Ansonhkg]

has this been tested with localchain & Shiva?

[joshLong145]

has this been tested with localchain & Shiva?

I pulled the branch and ran tests on manzano to see how the connection speeds where with sev attestation happening and with the added concurrency I can reproduce the described speed improvements.