Skip to content

MV3: add retry logic to actions#15337

Merged
jpuri merged 43 commits intodevelopfrom
mv3_action_retry
Sep 5, 2022
Merged

MV3: add retry logic to actions#15337
jpuri merged 43 commits intodevelopfrom
mv3_action_retry

Conversation

@jpuri
Copy link
Copy Markdown
Contributor

@jpuri jpuri commented Jul 26, 2022 •
edited
Loading

Fixes: #15535

Add retry logic to actions

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 26, 2022

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.

jpuri
jpuri commented Jul 26, 2022
View reviewed changes
jpuri
jpuri commented Jul 26, 2022
View reviewed changes
jpuri
jpuri commented Jul 26, 2022
View reviewed changes
@danjm
Copy link
Copy Markdown
Contributor

danjm commented Jul 26, 2022

Progresses #14853

digiwand
digiwand reviewed Jul 27, 2022
View reviewed changes
@jpuri jpuri force-pushed the mv3_action_retry branch 2 times, most recently from 5770f82 to a880ead Compare August 3, 2022 10:02
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 3, 2022

Builds ready [f392964]
Page Load Metrics (1702 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint85140107157
domContentLoaded15411978167710752
load1541197817029948
domInteractive15411978167710752

highlights:

storybook

digiwand
digiwand reviewed Aug 3, 2022
View reviewed changes
naugtur
naugtur reviewed Aug 4, 2022
View reviewed changes
naugtur
naugtur reviewed Aug 4, 2022
View reviewed changes
naugtur
naugtur reviewed Aug 4, 2022
View reviewed changes
naugtur
naugtur reviewed Aug 4, 2022
View reviewed changes
naugtur
naugtur reviewed Aug 4, 2022
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 4, 2022

Builds ready [4d41431]
Page Load Metrics (1831 ± 266 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint822751154823
domContentLoaded149735031808538258
load149735031831554266
domInteractive149735031808537258

highlights:

storybook

@jpuri jpuri force-pushed the mv3_action_retry branch from dbcf8f2 to 07b928d Compare August 5, 2022 11:16
naugtur
naugtur reviewed Aug 8, 2022
View reviewed changes
naugtur
naugtur reviewed Aug 8, 2022
View reviewed changes
ui/store/action-queue/index.test.js Outdated
// connectionStream: {
// readable: true,
// },
// backgroundFunction: async () => {
Copy link
Copy Markdown
Contributor

@naugtur naugtur Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was supposed to be a callback style function - it's being wrapped in pify afterall.
Hours spent here: 2 :D

naugtur
naugtur reviewed Aug 8, 2022
View reviewed changes
@jpuri
Copy link
Copy Markdown
Contributor Author

jpuri commented Aug 8, 2022

Currently when clearing the queue we run action asynchronously, but I am thinking may be we should do it synchronous as they are in sequence of user interaction. Consider user updating gas params on transaction twice - we need to ensure that they run in sequence.

jpuri
jpuri commented Aug 8, 2022
View reviewed changes
jpuri
jpuri commented Aug 8, 2022
View reviewed changes
jpuri
jpuri commented Aug 8, 2022
View reviewed changes
@naugtur
Copy link
Copy Markdown
Contributor

naugtur commented Aug 9, 2022

Currently when clearing the queue we run action asynchronously, but I am thinking may be we should do it synchronous as they are in sequence of user interaction. Consider user updating gas params on transaction twice - we need to ensure that they run in sequence.

I thought executing them quickly was a requirement. This makes implementation much nicer, may even help eliminate the need for the splice.

@danjm danjm mentioned this pull request Aug 9, 2022
Closed
jpuri
jpuri commented Aug 9, 2022
View reviewed changes
jpuri
jpuri commented Aug 9, 2022
View reviewed changes
jpuri
jpuri commented Aug 9, 2022
View reviewed changes
BelfordZ
BelfordZ reviewed Aug 11, 2022
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 23, 2022

Builds ready [0c19e3e]
Page Load Metrics (1828 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint931821232411
domContentLoaded16202367179918086
load16212367182817785
domInteractive16202367179918086

highlights:

storybook

@brad-decker
Copy link
Copy Markdown
Contributor

brad-decker commented Aug 23, 2022

Is this a feature branch? Is the idea here that all those idempotent prs are gonna collect here? Could we add the PR template here and fill it out with as much detail as possible

digiwand
digiwand reviewed Aug 23, 2022
View reviewed changes
@jpuri
Copy link
Copy Markdown
Contributor Author

jpuri commented Aug 23, 2022

Is this a feature branch? Is the idea here that all those idempotent prs are gonna collect here? Could we add the PR template here and fill it out with as much detail as possible

Hey @brad-decker : the intend is not to make it feature branch, this is ready for merge. Other PRs are based on this as they need these changes to work.

@jpuri @digiwand
Update ui/store/action-queue/index.js
8161b91 
Co-authored-by: Ariella Vu <20778143+digiwand@users.noreply.github.com>
segun
segun reviewed Aug 23, 2022
View reviewed changes
.storybook/preview.js
import { Router } from 'react-router-dom';
import { createBrowserHistory } from 'history';
import { _setBackgroundConnection } from '../ui/store/actions';
import { _setBackgroundConnection } from '../ui/store/action-queue';
Copy link
Copy Markdown
Contributor

@segun segun Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: my brain sees underscore before a variable/method and it thinks private variable/method.

Copy link
Copy Markdown
Contributor Author

@jpuri jpuri Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid changing existing method in this PR.

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 23, 2022

Builds ready [8161b91]
Page Load Metrics (1735 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint97177119188
domContentLoaded1573197717099345
load15742104173511756
domInteractive1573197717099345

highlights:

storybook

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 24, 2022

Builds ready [b67442c]
Page Load Metrics (1724 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint102153116147
domContentLoaded1587194716948943
load1591194717249546
domInteractive1587194716948943

highlights:

storybook

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 24, 2022

Builds ready [678f4af]
Page Load Metrics (1878 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint993151334421
domContentLoaded16102127185212359
load16892127187812058
domInteractive16102127185212359

highlights:

storybook

naugtur
naugtur reviewed Aug 25, 2022
View reviewed changes
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 25, 2022

Builds ready [85c7075]
Page Load Metrics (1715 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint901685191343165
domContentLoaded1629184916965928
load1639187217156029
domInteractive1629184916965928

highlights:

storybook

jpuri and others added 2 commits August 25, 2022 16:45
@jpuri @naugtur
Update ui/store/action-queue/index.js
825205b 
Co-authored-by: Zbyszek Tenerowicz <naugtur@gmail.com>
@jpuri
fix
650a94c
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 25, 2022

Builds ready [650a94c]
Page Load Metrics (1786 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint91154116157
domContentLoaded16072015176812359
load16072107178613063
domInteractive16072015176812359

highlights:

storybook

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Aug 26, 2022

Builds ready [a17b32a]
Page Load Metrics (1865 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1002951334119
domContentLoaded16632228184514369
load16632262186514469
domInteractive16632228184514369

highlights:

storybook

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Sep 5, 2022

Builds ready [cebe284]
Page Load Metrics (1639 ± 30 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint882621123517
domContentLoaded1501177016227134
load1556177016396330
domInteractive1501177016227134

highlights:

storybook

darkwing
darkwing approved these changes Sep 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The only thing I saw that I questioned was the big comment block in action-queue/index.js ; do we need that?

This is great!

@jpuri jpuri merged commit 99ed42b into develop Sep 5, 2022
@jpuri jpuri deleted the mv3_action_retry branch September 5, 2022 14:55
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@naugtur naugtur naugtur approved these changes

@garrettbear garrettbear Awaiting requested review from garrettbear garrettbear is a code owner automatically assigned from MetaMask/extension-devs

+6 more reviewers

@segun segun segun left review comments

@BelfordZ BelfordZ BelfordZ left review comments

@digiwand digiwand digiwand left review comments

@Ellajoke Ellajoke Ellajoke left review comments

@darkwing darkwing darkwing approved these changes

@Russell4115 Russell4115 Russell4115 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR - P1 identifies PRs deemed priority for Extension team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add retry logic to actions on client side