Feat/consensus call #70

Keyrxng · 2025-02-25T18:28:10Z

Resolves #60

New Method:

  public async consensusCall<TMethodReturnData = unknown>(requestPayload: RequestPayload, quorumThreshold: `0.${number}`): Promise<TMethodReturnData> {

added test case
manually qa'd

QA:

…andlerInterface

…thods

…nd eth_getCode methods

Keyrxng · 2025-02-25T18:52:58Z

@rndquu @EresDev Let me know what you guys think of this whenever you can cheers.

Through my manual QA, it ofc does not reach a consensus 100% of the time (which is good?). But at first glance in-sync:12, validated:9 looks like a better approach than just #62 alone.

Although if we consider the failure rate + the fact this is a read-op-only method I'm not sure about implementing it as a core way of validating providers and it's probably better to stay a separate method in it's own right.

I've only read about 100 pages so far Eres, give me a week to read it a couple times and I'll go heavy on the refactoring in a dedicated PR. Great recommendation, ty.

package.json

types/rpc-service.ts

…RequestAndByteCodeRacePromises for clarity

github-actions · 2025-02-26T15:35:59Z

Unlisted binaries (1)

Filename	binaries
package.json	`x`

…t and script execution

Keyrxng · 2025-02-26T16:08:34Z

Knip CI failing out of my control and I can't re-run it

0x4007 · 2025-02-26T16:11:03Z

Knip CI failing out of my control and I can't re-run it

I suppose we can try using bun instead of yarn. https://github.com/0x4007/travel-stipend/blob/f5bb2faa1b873bb1ca40ba04b97ba468b0d2cc97/.github/workflows/knip.yml

rndquu

@Keyrxng Pls update readme with the new method

types/rpc-handler.ts

…check

rndquu

Pls add a unit test for the consensusCall() method when it fails to reach the quorum
Pls update script-test-consensus.ts and add an example of using it with eth_getTransactionReceipt

types/rpc-handler.ts

rndquu · 2025-02-28T16:34:18Z

types/rpc-handler.ts

+          if (val instanceof Error) {
+            val = val.message;
+          } else {
+            val = val.hash;


Some of the RPC methods (like eth_getTransactionReceipt) don't have the hash property in the response so I don't fully understand the logic

I debated this myself a little.

I went with JSON.Stringify at first and then return returning JSON.parse but some response data is enormous so I went with hash instead thinking it was more universal than what you've pointed it out to be.

Other than JSON.Stringify I'm not sure how we can match responses with certainty across all methods/request results.

what about .hash ?? .transactionHash I feel that might be better but still a lot of methods would be excluded.

Perhaps consensusCall takes a a 3rd param which is the prop of the return data that we need to compare and then allow the user to tell us?

I've implemented .hash ?? .transactionHash for now as that covers eth_getTransactionReceipt at least.

A 3rd param sounds scalable across all eth_ methods, failing that, stringifying the results might be the best bet.

JSON.Stringify sounds good, we need a generalized solution that covers most of RPC methods, not just 2

types/rpc-handler.ts

…load

…mprove type definitions

…RPCHandler and RPCService

…erbose in consensus tests

EresDev · 2025-02-28T21:33:07Z

types/rpc-handler.ts

+  public async getFirstAvailableRpcProvider() {
+    const rpcList = [...networkRpcs[this._networkId].rpcs].filter((rpc) => rpc.url.includes("https"));
+    shuffleArray(rpcList);
+    const rpcPromises: Record<string, Promise<PromiseResult>[]> = {};
+
+    for (const rpc of rpcList) {
+      this._rpcService.createBlockRequestAndByteCodeRacePromises(this._runtimeRpcs, rpcPromises, this._rpcTimeout);
+      const results = await Promise.allSettled(rpcPromises[rpc.url] ?? []);
+      const hasPassedAllChecks = results.every((res) => res && res.status === "fulfilled" && res.value.success);
+      if (hasPassedAllChecks) {
+        return new JsonRpcProvider({ url: rpc.url, skipFetchSetup: true }, Number(this._networkId));
+      }
+    }


when this function runs for me in the unit tests, this._runtimeRpcs is always empty. This is major dependency of createBlockRequestAndByteCodeRacePromises. Because it gets empty array where it is supposed to work on, it just doesn't do anything here. Is this._runtimeRpcs also always empty for you? then remove redundant code here.

EresDev · 2025-02-28T21:41:46Z

types/rpc-service.ts

@@ -244,4 +227,47 @@ export class RPCService {
      return false;
    }
  }
+
+  createBlockRequestAndByteCodeRacePromises(runtimeRpcs: string[], rpcPromises: Record<string, Promise<PromiseResult>[]>, rpcTimeout: number) {


We discussed this in the previous PR. Do not change objects passed in arguments. rpcPromises is being passed here, just to get filled up inside this function.

Results produced by a function should be returned, and not fill up in one of its arguments.

Also, break this down into two functions. If you will do so, name of this function will improve, and you will not have to add such checks.

if (result.value.rpcMethod === "eth_getBlockByNumber") {

} else if (result.value.rpcMethod === "eth_getCode"

EresDev · 2025-02-28T21:44:42Z

types/rpc-service.ts

      return {
        rpcUrl,
        duration: performance.now() - startTime,
-        success: true,
+        success: "result" in (res?.data ?? {}) ? true : false,


maybe change to

success: !!res?.data?.result

But there is one difference, see if it works. But this line should be simplified even if my suggestion doesn't work.

EresDev · 2025-02-28T21:49:25Z

types/rpc-service.ts

-const getBlockNumberPayload = JSON.stringify({
+// this is similar to `ValidBlockData`, I didn't want to change it incase it's in other projects
+type JsonRpcResponse = { jsonrpc: string; id: number; result: string | { number: string; timestamp: string; hash?: string; transactionHash?: string } };
+export type PromiseResult<T extends JsonRpcResponse = JsonRpcResponse> = {


PromiseResult Naming needs improvement. You can use AI to suggest you better variable, types, and function names. See

EresDev · 2025-02-28T22:07:34Z

types/rpc-handler.ts

-    const rpcList = [...networkRpcs[this._networkId].rpcs];
-    shuffleArray(rpcList);
-    for (const rpc of rpcList) {
+  public async consensusCall<TMethodReturnData = unknown>(requestPayload: RequestPayload, quorumThreshold: `0.${number}`): Promise<TMethodReturnData> {


I say let's add new class Security{} and add this method consensusCall to it. Later, we will keep adding more security related stuff to this class.

RpcHandler is already a god object for me. @rndquu what do you think?

EresDev · 2025-02-28T23:16:47Z

tests/consensus.test.ts

+export const testConfig: HandlerConstructorConfig = {
+  networkId: "100",
+  autoStorage: false,
+  cacheRefreshCycles: 3,
+  networkName: null,
+  rpcTimeout: 10000,
+  runtimeRpcs: rpcUrls,
+  networkRpcs: rpcUrls.map((url) => ({ url })),
+  proxySettings: {
+    retryCount: 3,
+    retryDelay: 10,
+    logTier: "verbose",
+    logger: new PrettyLogs(),
+    strictLogs: true,
+  },
+};


this exact object is present in 3 files. Reduce duplication. Extract it to somewhere like tests/shared.ts and reuse it from here.

EresDev · 2025-02-28T23:18:45Z

package.json

@@ -67,6 +69,7 @@
    "jest-md-dashboard": "0.8.0",
    "knip": "5.27.0",
    "lint-staged": "^15.2.2",
+    "nock": "13.5.6",


nock for http mocking here? We are already using msw in text-conversation-rewards plugin and pay.ubq.fi.

EresDev · 2025-02-28T23:20:36Z

tests/consensus.test.ts

+    nock(rpcUrls[0])
+      .post("/")
+      .reply(200, {
+        jsonrpc: "2.0",
+        result: { number: "0x1b4", hash: "0x1b4" },
+        id: 1,
+      });
+
+    nock(rpcUrls[1])
+      .post("/")
+      .reply(200, {
+        jsonrpc: "2.0",
+        result: { number: "0x1b4", hash: "0x1b4" },
+        id: 1,
+      });
+
+    nock(rpcUrls[2])
+      .post("/")
+      .reply(200, {
+        jsonrpc: "2.0",
+        result: { number: "0x1b4", hash: "0x1b4" },
+        id: 1,
+      });


loop to reduce duplication

EresDev · 2025-02-28T23:48:37Z

tests/script-test-consensus.ts

+
+/**
+ * A test script to ensure that the module can be imported and used correctly
+ * This script is not meant to be run in the test suite


actually this is test and all your script-***.ts files. These are e2e tests. These are very useful. Move them to tests/e2e folder. At the end of each test, instead of console.log, use expect().

EresDev · 2025-02-28T23:50:22Z

tests/script-test-consensus.ts

+    },
+  };
+
+  const blockConsensusResponse = await handler.consensusCall(blockByNumberPayload, "0.5");


block number consensus test will not be deterministic. So, instead of block number in e2e test, maybe you can read token symbol of UBQ.

Keyrxng added 8 commits February 25, 2025 17:25

refactor: enhance RPCService structure and improve request handling

24bbc2b

feat: add getFirstAvailableRpcProvider and consensusCall methods to H…

9489b15

…andlerInterface

feat: consensusCalls

23b515c

feat: update consensusCall method to support generic return types

4de52d8

test: add tests for getFirstAvailableRpcProvider and consensusCall me…

8ed301b

…thods

feat: enhance error handling in RPCService for eth_getBlockByNumber a…

6e2c82f

…nd eth_getCode methods

feat: add test scripts for consensus and standard testing in RPCHandler

7ebea23

refactor: use hash for comparison, return response object

5010ef3

ci: bump test timeout for real rpc call tests

126c6f3

0x4007 reviewed Feb 26, 2025

View reviewed changes

package.json Show resolved Hide resolved

0x4007 reviewed Feb 26, 2025

View reviewed changes

types/rpc-service.ts Outdated Show resolved Hide resolved

Keyrxng added 2 commits February 26, 2025 15:33

refactor: rename createBlockReqAndByteCodeRacePromises to createBlock…

86127f0

…RequestAndByteCodeRacePromises for clarity

chore: update test scripts to use bun for execution

8c533d0

Keyrxng added 5 commits February 26, 2025 15:37

ci: add ignoreBinaries option to Knip config ignoring x

af20469

ci: switch testing workflow from Yarn to Bun for dependency managemen…

73e7930

…t and script execution

chore: revert bun modifications

a653ef2

chore: add nock dependency and separate consensus tests into a new file

dfed20f

chore: downgrade nock dependency to version 13.5.6

92d0c25

Keyrxng added 2 commits February 26, 2025 16:12

chore: fix comment

c2c81e6

chore: fix it again

b04974a

rndquu requested changes Feb 28, 2025

View reviewed changes

rndquu reviewed Feb 28, 2025

View reviewed changes

types/rpc-handler.ts Outdated Show resolved Hide resolved

rndquu requested a review from EresDev February 28, 2025 11:12

docs: update README with usage examples for RPCHandler and consensus …

37ea075

…check

rndquu self-requested a review February 28, 2025 16:08

rndquu requested changes Feb 28, 2025

View reviewed changes

Keyrxng added 6 commits February 28, 2025 17:05

test: enhance consensus tests with improved RPC configuration and pay…

f39fbd9

…load

refactor: remove consensus failure check from RPCHandler

8abd8ff

fix: update RPCHandler and RPCService to handle transactionHash and i…

8df5fba

…mprove type definitions

refactor: improve type definitions and enhance consensus handling in …

1847a8d

…RPCHandler and RPCService

refactor: simplify RPCHandler instantiation and update log level to v…

d0b2d7e

…erbose in consensus tests

refactor: consensusCall comment

9558484

EresDev requested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/consensus call #70

Feat/consensus call #70

Keyrxng commented Feb 25, 2025 •

edited

Loading

Keyrxng commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 26, 2025

Keyrxng commented Feb 26, 2025

0x4007 commented Feb 26, 2025

rndquu left a comment

rndquu left a comment

rndquu Feb 28, 2025

Keyrxng Feb 28, 2025 •

edited

Loading

Keyrxng Feb 28, 2025 •

edited

Loading

Keyrxng Feb 28, 2025

rndquu Feb 28, 2025

EresDev Feb 28, 2025

EresDev Feb 28, 2025 •

edited

Loading

EresDev Feb 28, 2025

EresDev Feb 28, 2025

EresDev Feb 28, 2025 •

edited

Loading

EresDev Feb 28, 2025

EresDev Feb 28, 2025

EresDev Feb 28, 2025

EresDev Feb 28, 2025 •

edited

Loading

EresDev Feb 28, 2025

Feat/consensus call #70

Are you sure you want to change the base?

Feat/consensus call #70

Conversation

Keyrxng commented Feb 25, 2025 • edited Loading

Keyrxng commented Feb 25, 2025 • edited Loading

github-actions bot commented Feb 26, 2025

Unlisted binaries (1)

Keyrxng commented Feb 26, 2025

0x4007 commented Feb 26, 2025

rndquu left a comment

Choose a reason for hiding this comment

rndquu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keyrxng Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Keyrxng Feb 28, 2025 • edited Loading

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

EresDev Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EresDev Feb 28, 2025 • edited Loading

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

EresDev Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keyrxng commented Feb 25, 2025 •

edited

Loading

Keyrxng commented Feb 25, 2025 •

edited

Loading

Keyrxng Feb 28, 2025 •

edited

Loading

Keyrxng Feb 28, 2025 •

edited

Loading

EresDev Feb 28, 2025 •

edited

Loading

EresDev Feb 28, 2025 •

edited

Loading

EresDev Feb 28, 2025 •

edited

Loading