feat: improve smoldot integration #374

kratico · 2022-11-09T20:38:34Z

Resolves #376

Description

Improve Smoldot integration for relay chains by

capturing Smoldot errors
mapping specific .send requests to specific .send calls

Smoldot parachain integration will be tacked in an upcoming PR

kratico · 2022-11-09T20:39:57Z

rpc/provider/smoldot.test.ts

+  sanitizeOps: false,
+  sanitizeResources: false,


Smoldot seems to not clear a few timers started in

at start_timer (https://deno.land/x/[email protected]/instance/bindings-smoldot-light.js:107:17)

kratico · 2022-11-10T16:14:52Z

rpc/client.ts

+      const pendingCall = this.pendingCalls[egressMessageId];
+      if (!pendingCall) {
+        console.log({ e });
+        // TODO: pipe error to listeners and message the likely cause,
+        //       a duplicate client.
+        throw new Error();
+      }


I think this check can be removed.
It happens when the provider is being discarded and an inflight/pending response is emitted.
In that scenario, I assume that we don't care about the response so we can ignore it.

Thoughts @harrysolovay?

Which lines specifically?

if (!pendingCall) { console.log({ e }); // TODO: pipe error to listeners and message the likely cause, // a duplicate client. throw new Error(); }

Gotcha. Removing sounds good.

rpc/provider/errors.ts

harrysolovay · 2022-11-10T16:29:40Z

rpc/provider/smoldot.ts

+        try {
+          conn = await connection(chainSpec, listener);
+        } catch (error) {
+          listener(new ProviderHandlerError(error as SmoldotHandlerErrorData));


Awesome solution!

harrysolovay · 2022-11-10T16:36:06Z

rpc/client.ts

+      const pendingCall = this.pendingCalls[egressMessageId];
+      if (!pendingCall) {
+        console.log({ e });
+        // TODO: pipe error to listeners and message the likely cause,
+        //       a duplicate client.
+        throw new Error();
+      }


Which lines specifically?

rpc/client.ts

harrysolovay · 2022-11-10T16:42:31Z

rpc/provider/smoldot.ts

+  | CrashError
+  | JsonRpcDisabledError
+  | AddChainError;
+type SmoldotCloseErrorData = AlreadyDestroyedError | CrashError;


rpc/provider/smoldot.ts

wirednkod · 2022-11-10T17:32:29Z

rpc/provider/smoldot.ts

@@ -67,27 +92,24 @@ async function connection(
  }
  let conn = connections.get(chainSpec);
  if (!conn) {
-    // TODO: try catch this and send through handler within a `ProviderHandlerError`
    const inner = await client.addChain({ chainSpec });


for parachain support, all you need to do is to pass in the potentialRelayChains all the chains inner that you collect during the addChain.
Meaning something like:

Suggested change

const inner = await client.addChain({ chainSpec });

const innerMap: Chain[] = [];

connections.forEach((value: SmoldotProviderConnection) => {

innerMap.push(value.inner);

});

const inner = await client.addChain({ chainSpec, potentialRelayChains: innerMap });

As described yesterday, smoldot will understand which parachain should go to which chain etc etc.

EDIT: Tested the solution and It works - This is the testing code

thx @wirednkod
The parachain support will be done in a separate PR

@harrysolovay we need to decide 2 things

1st when connecting to a parachain, I believe that we need to pass the parachain spec and the potential relay chain spec

2nd smoldot connection clean up, given that it takes time to sync the light client, connecting/disconnecting to the same chain will make it slow, so perhaps we can keep the synced state without closing the connections

Thouths?

thx @wirednkod The parachain support will be done in a separate PR

#378

kratico · 2022-11-11T14:31:08Z

rpc/provider/smoldot.ts

+    let inner: Chain;
+    if (typeof chainSpec === "string") {
+      inner = await client.addChain({ chainSpec });
+    } else {
+      const [parachainSpec, relayChainSpec] = chainSpec;
+      const relayChainConnection = await client.addChain({ chainSpec: relayChainSpec });
+      inner = await client.addChain({
+        chainSpec: parachainSpec,
+        potentialRelayChains: [relayChainConnection],
+      });
+    }


@harrysolovay @wirednkod WDYT?

Looks fantastic!

Giving my 2 cents here, I think this will strict the user always to start relay chain and parachain at the same spot of the application... IMO these steps should be separated (add relay chain, add parachain) with the same function

thanks for the input @wirednkod
For now, the relay/parachain start will be on demand on the first .send invocation.

rpc/provider/smoldot.ts

Co-authored-by: Harry Solovay <[email protected]>

kratico commented Nov 9, 2022

View reviewed changes

kratico force-pushed the feat/smoldot-integration branch 2 times, most recently from f43a9b4 to 2b14816 Compare November 10, 2022 13:49

kratico commented Nov 10, 2022

View reviewed changes

kratico marked this pull request as ready for review November 10, 2022 16:22

kratico requested a review from harrysolovay as a code owner November 10, 2022 16:22

harrysolovay suggested changes Nov 10, 2022

View reviewed changes

wirednkod reviewed Nov 10, 2022

View reviewed changes

kratico requested a review from harrysolovay November 10, 2022 19:03

wirednkod mentioned this pull request Nov 10, 2022

feat: improve smoldot integration (parachain) #378

Closed

kratico force-pushed the feat/smoldot-integration branch from 9b5b629 to b527680 Compare November 11, 2022 14:20

kratico commented Nov 11, 2022

View reviewed changes

rpc/provider/smoldot.ts Outdated Show resolved Hide resolved

kratico and others added 6 commits November 11, 2022 13:21

feat: improve smoldot integration

6abb9e5

feat: map provider.send errors

ef0d5fd

feat: handle smoldot errors

69c14f4

feat: make ProviderSendError.egressMessage non optional

eff66b1

feat: update smoldot connection options

b2cb196

feat: add smoldot props

e262b4f

Co-authored-by: Harry Solovay <[email protected]>

kratico force-pushed the feat/smoldot-integration branch from b527680 to e262b4f Compare November 11, 2022 16:30

kratico added 3 commits November 11, 2022 13:35

fix: smoldot test props

91c6d6e

chore: update smoldot cpu limit to .25

11cce07

feat: remove extra pendingCalls checks

0c4c694

harrysolovay approved these changes Nov 11, 2022

View reviewed changes

kratico merged commit cc121e3 into main Nov 11, 2022

kratico deleted the feat/smoldot-integration branch November 11, 2022 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve smoldot integration #374

feat: improve smoldot integration #374

kratico commented Nov 9, 2022 •

edited

Loading

kratico Nov 9, 2022 •

edited

Loading

kratico Nov 10, 2022

harrysolovay Nov 10, 2022

kratico Nov 10, 2022

harrysolovay Nov 11, 2022

harrysolovay Nov 10, 2022

harrysolovay Nov 10, 2022

harrysolovay Nov 10, 2022

wirednkod Nov 10, 2022 •

edited

Loading

kratico Nov 10, 2022

kratico Nov 10, 2022

wirednkod Nov 10, 2022

kratico Nov 11, 2022

harrysolovay Nov 11, 2022

wirednkod Nov 11, 2022

kratico Nov 11, 2022

-    const inner = await client.addChain({ chainSpec });
+    const innerMap: Chain[] = [];
+    connections.forEach((value: SmoldotProviderConnection) => {
+      innerMap.push(value.inner);
+    });
+    const inner = await client.addChain({ chainSpec, potentialRelayChains: innerMap });

feat: improve smoldot integration #374

feat: improve smoldot integration #374

Conversation

kratico commented Nov 9, 2022 • edited Loading

Description

kratico Nov 9, 2022 • 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

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

wirednkod Nov 10, 2022 • 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

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

kratico commented Nov 9, 2022 •

edited

Loading

kratico Nov 9, 2022 •

edited

Loading

wirednkod Nov 10, 2022 •

edited

Loading