BREAKING CHANGE: make getLatestBlockhash async

[Ansonhkg]

Description

"so i was thinking about how we get the poller out of the JS SDK. the main issue is that litNodeClient.getLatestBlockhash(); is not async. if we make it async, then we can remove the poller, and just get the latest block hash when they call that function. this is a breaking change, but def worth it IMO"

Type of change

• 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

• yarn test:e2e:nodejs --filter=test-latest-blockhash.mjs

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

[DashKash54]

We also want to remove the poller? I don't see it being removed?
Also is the purpose of this PR to always pull the latest blockhash from the nodes before making a request?

[glitch003]

this makes sense to me and looks good. i think we should add caching? if the block hash is less than 30s old, no need to talk to the nodes and get the latest? probably a good idea to prevent developers that like, accidentally call getLatestBlockhash() on every react re-render from hitting the nodes more than necessary.

[glitch003]

approving pending resolution of open questions

[MaximusHaximus]

This is re-implementing a lot of existing logic from lit-core, where we do this exact process already during the connect() flow. In fact, functionally it is the same, as it handshakes with all nodes, and even rebuilds the connectedNodes, but in its current state it leaves connectedNodes in an inconsistent state for the duration of this process -- while we're waiting on handshakes to return, the set will be mutated 'in place' - calling connect() no longer does that as of LIT-2560 -- and this could possibly run multiple concurrent handshakes if the method is called multiple times close together, which connect() will no longer do.

1. Can we move this code to live in core, since it is managing the same state as core does anyway?
2. Can we avoid re-implementing 'get most common value' and error handling/logging, by instead adding the method core that calls this.connect() and then returns this.latestBlockhash?

• We're already updating the latestBlockhash property during connect() execution anyway, using another implementation that already gets the 'most common string' in the returned values from the nodes
• We already have error handling and logging on fetching the blockhash from each node and an error is thrown if we fail to get the blockhash from any node

This method in lit-node-client can then be delegated to the core implementation.
When we find a better place to source latestBlockhash from, we can replace the connect() usage in lit-core to be lighter-weight.

[Ansonhkg]

This is re-implementing a lot of existing logic from lit-core, where we do this exact process already during the connect() flow. In fact, functionally it is the same, as it handshakes with all nodes, and even rebuilds the connectedNodes, but in its current state it leaves connectedNodes in an inconsistent state for the duration of this process -- while we're waiting on handshakes to return, the set will be mutated 'in place' - calling connect() no longer does that as of LIT-2560 -- and this could possibly run multiple concurrent handshakes if the method is called multiple times close together, which connect() will no longer do.

1. Can we move this code to live in core, since it is managing the same state as core does anyway?
2. Can we avoid re-implementing 'get most common value' and error handling/logging, by instead adding the method core that calls this.connect() and then returns this.latestBlockhash?

• We're already updating the latestBlockhash property during connect() execution anyway, using another implementation that already gets the 'most common string' in the returned values from the nodes
• We already have error handling and logging on fetching the blockhash from each node and an error is thrown if we fail to get the blockhash from any node

This method in lit-node-client can then be delegated to the core implementation. When we find a better place to source latestBlockhash from, we can replace the connect() usage in lit-core to be lighter-weight.

Great points! Made relevant changes :)

[MaximusHaximus]

Lookin' good -- thanks for adding a test for it. Just a few things in-line.

[Ansonhkg]

Merge conflicts occurred after another PR was rebased and merged to master

[MaximusHaximus]

👍

[MaximusHaximus]

@Ansonhkg I assume we want this to land in this week's release w/ other breaking changes -- LMK as soon as the conflicts are resolved and I'll re-add approved review so you can get it merged :). It sure will be nice when we get to the point that we're not modifying package.json in our feature PRs anymore :)

[joshLong145]

@MaximusHaximus conflicts are resolved can you re review?