feat!: dont batch queries #65

samsiegart · 2023-12-08T08:03:22Z

We've already switched from using the batch RPC API to JSON REST API in #55. But, we were still using a Promise.all() with the new interface, effectively batching those queries together. This PR makes each vstorage path poll asynchronously on separate timers.

Importantly, this changes the error handling to be more manageable. Instead of having a separate error handler for each watchLatest, it just uses a single onError callback given to chainStorageWatcher constructor. The new JSON API doesn't include an optional error message+code in response objects--it's limited to just http-level error codes, and in the case of missing vstorage data, an empty value in the response. This allows us to use axios to automatically retry on 500-level errors a couple times before invoking onError.

This updated error handling allows for clients to more easily manage faulty RPC nodes. Instead of adding an error handler to every place watchLatest is used, and use the result to decipher whether the vstorage data is missing or the RPC node is failing, it can just use the single onError callback on the chainStorageWatcher, and trust that the chainStorageWatcher already retried a couple times. This gives the client a reliable way to know when to notify the user of an RPC outage and/or prompt them to select a different node (assuming a node selector is available).

For queryOnce, it also no longer batches with other requests, and uses axios to retry before throwing an error, more similar to a regular fetch request.

Added some test cases for the updated error handling.

turadg

Thanks for the detailed explanation in the PR body

turadg · 2023-12-08T14:07:27Z

packages/rpc/src/chainStorageWatcher.ts

+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    } catch (e: any) {


let's do our best to avoid lint suppressions. isn't it unknown?

Suggested change

// eslint-disable-next-line @typescript-eslint/no-explicit-any

} catch (e: any) {

} catch (e: unknown) {

Then onError reports that it takes only an Error. If that's the case, then here we should check that it's really an Error.

Oh yea that works, done.

turadg · 2023-12-08T14:10:28Z

packages/rpc/src/chainStorageWatcher.ts

-      isQueryInProgress = false;
-      queueNextQuery();
+    const { value, blockHeight } = response;
+    const lastValue = latestValueCache.get(pathKey);


use of the lastValue tuple is a little confusing. In fact, there's a bug in using the [0] (identifier) for both comparisons.

To avoid that please destructure instead.

Suggested change

const lastValue = latestValueCache.get(pathKey);

// 'identifier' is block height because this is a *data* query

const [ latestValueHeight, latestValue ] = latestValueCache.get(pathKey) || [];

Alternately, make a getLatestDataValue() helper that returns { value, blockHeight } like the response does. Then the condition could be like:

if (response.blockHeight === cached.blockHeight || (response.blockHeight === undefined && response.value === cached.value))

I just made a type for LatestValueIdentifier with a doc comment to hopefully clarify, and used the array destructuring you suggested

packages/rpc/src/chainStorageWatcher.ts

turadg · 2023-12-08T14:19:43Z

packages/rpc/src/chainStorageWatcher.ts


  // Assumes argument is an unserialized presence.
  const presenceToSlot = (o: unknown) => marshaller.toCapData(o).slots[0];

-  const queryBoardAux = <T>(boardObjects: unknown[]) => {
+  const queryBoardAux = (boardObjects: unknown[]) => {


why lose the generic?

Just added back, but I had to make vstorageQuery generic as well

turadg · 2023-12-08T14:20:27Z

packages/rpc/src/vstorageQuery.ts

+  node: string,
+  unmarshal: FromCapData<string>,
+  path: [AgoricChainStoragePathKind, string],
+) => {


Suggested change

) => {

): { blockHeight?: number, value: unknown } => {

Done (except had to make value a generic and the return value a Promise)

turadg · 2023-12-08T14:23:13Z

packages/rpc/test/chainStorageWatcher.test.ts

@@ -1,11 +1,12 @@
 /* eslint-disable no-use-before-define */
 /* eslint-disable import/extensions */
 import { expect, it, describe, beforeEach, vi, afterEach } from 'vitest';
+// eslint-disable-next-line import/no-extraneous-dependencies


test is part of devDependencies so the lint rules should allow it. You may need to tell the eslintconfig that test allows devDependencies: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-extraneous-dependencies.md#options

Seems like it works now, I think my vscode linter was just deceiving me or not picking up the package.json changes

turadg · 2023-12-08T14:24:11Z

packages/rpc/test/chainStorageWatcher.test.ts

+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  new Promise<any[]>(res => {


does this work?

Suggested change

// eslint-disable-next-line @typescript-eslint/no-explicit-any

new Promise<any[]>(res => {

new Promise<unknown[]>(res => {

not a blocker since this is a test

Hm now it does, just changed. Before I was getting some issue with 'axios-mock-adapter' expecting Promise<any>

turadg · 2023-12-11T21:31:30Z

packages/rpc/src/chainStorageWatcher.ts

+      if (e instanceof Error) {
+        onError && onError(e);
+      }


consider one conditional expression,

Suggested change

if (e instanceof Error) {

onError && onError(e);

}

if (onError && e instanceof Error) {

onError(e);

}

turadg · 2023-12-11T21:32:20Z

packages/rpc/src/chainStorageWatcher.ts

+      latestValue &&
+      (blockHeight === latestValueIdentifier ||
+        // Blockheight is undefined so fallback to using the stringified value
+        // as the identifier, as is the case for `children` queries.


helpful comment

turadg · 2023-12-11T21:34:50Z

packages/rpc/src/vstorageQuery.ts

+export const vstorageQuery = async <T>(
+  node: string,
+  unmarshal: FromCapData<string>,
+  path: [AgoricChainStoragePathKind, string],


consider naming these so they're clearer below

Suggested change

path: [AgoricChainStoragePathKind, string],

[pathKind, path]: [AgoricChainStoragePathKind, string],

turadg · 2023-12-11T21:36:07Z

packages/rpc/src/vstorageQuery.ts

+    const latestValue =
+      typeof data.values !== 'undefined'
+        ? parseIfJSON(data.values[data.values.length - 1])
+        : parseIfJSON(data.value);


superfluous negation makes it harder to read

Suggested change

const latestValue =

typeof data.values !== 'undefined'

? parseIfJSON(data.values[data.values.length - 1])

: parseIfJSON(data.value);

const latestValue =

typeof data.values === 'undefined'

? parseIfJSON(data.value);

: parseIfJSON(data.values[data.values.length - 1])

ditto below

Done here and below

samsiegart requested a review from turadg December 8, 2023 08:05

turadg requested changes Dec 8, 2023

View reviewed changes

samsiegart requested a review from turadg December 11, 2023 18:28

samsiegart force-pushed the remove-batch-queries branch from 4a1e77a to dc138a6 Compare December 11, 2023 19:05

turadg approved these changes Dec 11, 2023

View reviewed changes

feat!: dont batch queries

56246cd

samsiegart force-pushed the remove-batch-queries branch from dc138a6 to 56246cd Compare December 14, 2023 04:12

samsiegart enabled auto-merge (rebase) December 14, 2023 04:12

samsiegart merged commit ebfc261 into main Dec 14, 2023
1 check passed

samsiegart deleted the remove-batch-queries branch December 14, 2023 04:13

samsiegart mentioned this pull request Dec 14, 2023

feat(web-components): surface wallet connection rpc errors #66

Merged

Jorge-Lopes mentioned this pull request Apr 2, 2024

KREAd frontend should remove legacy queries Jorge-Lopes/KREAd#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: dont batch queries #65

feat!: dont batch queries #65

samsiegart commented Dec 8, 2023

turadg left a comment

turadg Dec 8, 2023

samsiegart Dec 11, 2023

turadg Dec 8, 2023

samsiegart Dec 11, 2023

turadg Dec 8, 2023

samsiegart Dec 11, 2023

turadg Dec 8, 2023

samsiegart Dec 11, 2023

turadg Dec 8, 2023

samsiegart Dec 11, 2023

turadg Dec 8, 2023

samsiegart Dec 11, 2023

turadg Dec 11, 2023

samsiegart Dec 14, 2023

turadg Dec 11, 2023

turadg Dec 11, 2023

samsiegart Dec 14, 2023

turadg Dec 11, 2023

samsiegart Dec 14, 2023

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		} catch (e: any) {

	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	} catch (e: any) {
	} catch (e: unknown) {

	const lastValue = latestValueCache.get(pathKey);
	// 'identifier' is block height because this is a data query
	const [ latestValueHeight, latestValue ] = latestValueCache.get(pathKey) \|\| [];

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		new Promise<any[]>(res => {

	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	new Promise<any[]>(res => {
	new Promise<unknown[]>(res => {

	path: [AgoricChainStoragePathKind, string],
	[pathKind, path]: [AgoricChainStoragePathKind, string],

feat!: dont batch queries #65

feat!: dont batch queries #65

Conversation

samsiegart commented Dec 8, 2023

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment