DAPP action replay improvements #16250
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [40793dd]
Page Load Metrics (2468 ± 52 ms)
|
jpuri
force-pushed
the
dapp_action_replay_improvements
branch
from
e7ad374 to
3d8a9ad
Compare
Builds ready [c4723f9]
Page Load Metrics (2590 ± 121 ms)
|
app/scripts/contentscript.js
Outdated
| @@ -261,10 +261,14 @@ const setupPageStreams = () => { | |||
| pageChannel = pageMux.createStream(PROVIDER); | |||
| }; | |||
|
|
|||
| // The field below is used to ensure that replay is done only once for each restart. | |||
| let REPLAY_ENABLED = false; | |||
There was a problem hiding this comment.
It took me a little bit of time to wrap my head around this logic. Now I see that REPLAY_ENABLED, in other words, means something like canSendExtensionConnectedMessage
In the case that the listener receiving the message expands on the logic, this comment and variable name could become outdated. REPLAY_ENABLED works. I'm just mentioning this here in case we want to take a preventative and potentially more scalable approach by renaming this
There was a problem hiding this comment.
I'm back with a follow-up thought...
Essentially, this might just be me, but the naming here isn't sitting with me well. It seems to go from retry -> connect -> retry.
In this PR, REPLAY_ENABLED is used to send the METAMASK_EXTENSION_STREAM_CONNECT message which is then mapped to a message for retryOnMessage
https://github.com/MetaMask/providers/blob/main/src/StreamProvider.ts
this._jsonRpcConnection = createStreamMiddleware({
retryOnMessage: 'METAMASK_EXTENSION_STREAM_CONNECT',
});
what if we align the naming across the code to something like:
CAN_SEND_RETRY_ACTIONS_ON_CONNECT_MSG
METAMASK_EXTENSION_RETRY_ACTIONS_ON_CONNECT
retryActionsOnConnectMessage
?
There was a problem hiding this comment.
+1
I'd rename:
METAMASK_EXTENSION_STREAM_CONNECT => METAMASK_EXTENSION_CONNECT_CAN_RETRY
REPLAY_ENABLED => METAMASK_EXTENSION_CONNECT_SENT
retryOnMessage => retryActionsOnConnectMessage
There was a problem hiding this comment.
+1
I did renamed first 2 suggestions, changing the third is hard as it is used in 2 different repos.
Builds ready [1864752]
Page Load Metrics (2345 ± 72 ms)
|
Builds ready [164d9f1]
Page Load Metrics (2395 ± 58 ms)
|
Builds ready [f98ce46]
Page Load Metrics (2135 ± 102 ms)
|
There was a problem hiding this comment.
LGTM!
as suggested above, updating retryOnMessage => retryActionsOnConnectMessage in the other 2 repos you mentioned would be a nice-to-have; not a blocker here though
planning to be readded in follow-up PR: #16250
Builds ready [1dc318f]
Page Load Metrics (2332 ± 124 ms)
|
Builds ready [e5cbbbe]
Page Load Metrics (2088 ± 76 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
… dapp_action_replay_improvements
Builds ready [ab819c7]
Page Load Metrics (2262 ± 111 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| const setupExtensionStreams = () => { | ||
| METAMASK_EXTENSION_CONNECT_SENT = true; |
There was a problem hiding this comment.
nit: maybe METAMASK_EXTENSION_CONNECT_SENT -> METAMASK_EXTENSION_CAN_SEND_RETRY? thoughts?
cc: @naugtur
There was a problem hiding this comment.
Agreed that calling this "connect sent" isn't ideal, because it doesn't really represent whether the extension has connected. After we send the retry command, it remains true that the extension has connected, but this is false.
Maybe instead of "can send retry", it should be "should send retry"? Or "send retry when ready"
OR maybe it would be simpler to flip this around. Use this boolean to track when the retry has already been sent, call it "METAMASK_EXTENSION_RETRY_SENT", and set it to false here. Then we can set to true in extensionStreamMessageListener
There was a problem hiding this comment.
How about we call it what it is - a semaphore.
METAMASK_EXTENSION_CONNECT_SEMAPHORE
There was a problem hiding this comment.
Code looks good to me but I agree with @Gudahtt 's comment about variable naming
This comment was marked as resolved.
This comment was marked as resolved.
Builds ready [44acf02]
Page Load Metrics (2444 ± 132 ms)
Bundle size diffs
|
Builds ready [50e2322]
Page Load Metrics (2159 ± 114 ms)
Bundle size diffs
|
|
I am merging this PR, I am working on a following PR where I will address re-naming suggestion. |
Fixes: #16174
The PR improves DAPP action replay. As the background is loaded and its state is completely initialised a message is sent to inpage provider to replay actions.
To be merged after this PR is merged: #16075