fix: process incoming requests in order #4638

ganchoradkov · 2024-06-21T11:25:54Z

Description

Updated intake logic of incoming relay messages so they are processed by Sign Client one by one instead of all at the same time. The old approach was resulting in conflicts when messages were dependent on each other such as session_update adding new chain to the session and session_eventinstructing the client to switch to that new chain. In the above case, the session_event must be processed after the session_update has finished the update, else the client has no knowledge of that new chain.

Additionally, implemented relayMessageCache that stores any messages received while the client is still initializing and the messages are processed after init has finished. This allows implementing apps to setup all event listeners and not miss events

Type of change

Chore (non-breaking change that addresses non-functional tasks, maintenance, or code quality improvements)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Draft PR (breaking/non-breaking change which needs more work for having a proper functionality [Mark this PR as ready to review only when completely ready])
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

dogfooding

Checklist

I have performed a self-review of my own code
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

Please include any additional information that may be useful for the reviewer.

…eleased while client is initializing

Cali93

Are all tests passing ? Should we add unhappy path cases ?

Cali93 · 2024-06-24T10:09:38Z

packages/sign-client/src/controllers/engine.ts

@@ -1028,6 +1029,23 @@ export class Engine extends IEngine {
    return formatMessage(request, iss);
  };

+  public processRelayMessageCache: IEngine["processRelayMessageCache"] = () => {
+    // process the relay messages cache in the next tick to allow event listeners to be registered by the implementing app
+    setTimeout(async () => {


Not sure if it's a good idea to mix setTimeout with an async operation.
Unless I misunderstood, I'd rather isolate the setTimeout and do something along the lines of

Suggested change

setTimeout(async () => {

await sleep(50)

if (this.relayMessageCache.length === 0) return;

while (this.relayMessageCache.length > 0) {

why do we need that small timeout?

the goal is to process the message after the finish initializing. This allows to setup listeners after the client.init()

yes, but why 50 and not other number? Is 50 enough in a RN environment?

it's arbitrary number. We can change it to some other value if you have suggestion

Shouldn't we use a flag like client.initialized then ?

Cali93 · 2024-06-24T10:13:19Z

packages/sign-client/src/controllers/engine.ts

-        });
+    this.client.core.relayer.on(RELAYER_EVENTS.message, (event: RelayerTypes.MessageEvent) => {
+      // capture any messages that arrive before the client is initialized so we can process them after initialization is complete
+      if (!this.initialized || this.relayMessageCache.length > 0) {


Is it a cache or a queue ? Why are we pushing the events in the cache instead of processing them?

we put all messages received during init, and process them after init has finished. Otherwise it can lead to missed events by the implementing app

packages/sign-client/src/controllers/engine.ts

ganchoradkov · 2024-06-28T10:47:42Z

Are all tests passing ? Should we add unhappy path cases ?

Not sure how to slow down the client and force the behaviour thus has been manually dogfooded. Will tinker @Cali93

Cali93

LGTM

Cali93 · 2024-07-02T12:17:56Z

packages/sign-client/src/controllers/engine.ts

@@ -1028,6 +1029,23 @@ export class Engine extends IEngine {
    return formatMessage(request, iss);
  };

+  public processRelayMessageCache: IEngine["processRelayMessageCache"] = () => {
+    // process the relay messages cache in the next tick to allow event listeners to be registered by the implementing app
+    setTimeout(async () => {


Shouldn't we use a flag like client.initialized then ?

ganchoradkov added 3 commits June 20, 2024 10:27

fix: handles incoming requests one by one

620801a

feat: implements relay message cache for any messages that might be r…

3444a27

…eleased while client is initializing

refactor: cleanup

6f46d28

ganchoradkov requested review from bkrem, Cali93 and lukaisailovic June 21, 2024 11:25

arein added the accepted label Jun 21, 2024

chore: rm logs

3b9a19f

ganchoradkov marked this pull request as draft June 21, 2024 11:27

ganchoradkov marked this pull request as ready for review June 24, 2024 07:40

glitch-txs approved these changes Jun 24, 2024

View reviewed changes

Cali93 reviewed Jun 24, 2024

View reviewed changes

chore: rm redundant await

661bf00

ganchoradkov requested review from ignaciosantise and Cali93 June 28, 2024 10:46

Cali93 approved these changes Jul 2, 2024

View reviewed changes

ganchoradkov merged commit 4f27d4e into v2.0 Jul 2, 2024
9 checks passed

ganchoradkov deleted the fix/process-incoming-requests-in-order branch July 2, 2024 12:35

ganchoradkov mentioned this pull request Jul 17, 2024

chore: prep for 2.14.0 release #5195

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: process incoming requests in order #4638

fix: process incoming requests in order #4638

ganchoradkov commented Jun 21, 2024 •

edited by skibitsky

Loading

Cali93 left a comment

Cali93 Jun 24, 2024

ignaciosantise Jun 24, 2024

ganchoradkov Jun 28, 2024

ignaciosantise Jul 1, 2024

ganchoradkov Jul 2, 2024 •

edited

Loading

Cali93 Jul 2, 2024

Cali93 Jun 24, 2024

ganchoradkov Jun 28, 2024

ganchoradkov commented Jun 28, 2024 •

edited

Loading

Cali93 left a comment

Cali93 Jul 2, 2024

-    setTimeout(async () => {
+   await sleep(50)
+   if (this.relayMessageCache.length === 0) return;
+   while (this.relayMessageCache.length > 0) {

fix: process incoming requests in order #4638

fix: process incoming requests in order #4638

Conversation

ganchoradkov commented Jun 21, 2024 • edited by skibitsky Loading

Description

Type of change

How has this been tested?

Checklist

Additional Information (Optional)

Cali93 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

ganchoradkov Jul 2, 2024 • 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

ganchoradkov commented Jun 28, 2024 • edited Loading

Cali93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganchoradkov commented Jun 21, 2024 •

edited by skibitsky

Loading

ganchoradkov Jul 2, 2024 •

edited

Loading

ganchoradkov commented Jun 28, 2024 •

edited

Loading