Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor incoming tx controller #10639

Merged
merged 1 commit into from
Mar 19, 2021
Merged

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented Mar 12, 2021

Rationale

As a means of more deeply understanding what the incoming transaction controller does I set out to document its methods with types and descriptions. In so doing I found a number of cases where I felt like the readability of the controller could be improved. This should be a non-functional refactor. Please take care in reading the changes to the test file to see if I have altered the desired outcomes in any functional way.

This will require a migration as I renamed incomingTxLastFetchedBlocksByNetwork to incomingTxLastFetchedBlockByChainId. Although the value is an object storing values that are blocks, each chainId only has one block. Although I've changed it from network to chainId, this shouldn't result in functional changes except where user's are using custom networks with chainIds that match default chains.

Migration now included, and the case above doesn't exist because we were mapping chainId to provider type before, so custom network with chainId 0x1 would have had its last fetched block stored in the mainnet key

@metamaskbot
Copy link
Collaborator

Builds ready [adb61ce]
Page Load Metrics (593 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45766084
domContentLoaded34578759112258
load34678759312259
domInteractive34578659112258

@brad-decker brad-decker marked this pull request as ready for review March 12, 2021 19:29
@brad-decker brad-decker requested a review from a team as a code owner March 12, 2021 19:29
@brad-decker brad-decker added the DO-NOT-MERGE Pull requests that should not be merged label Mar 12, 2021
@brad-decker brad-decker force-pushed the refactor-incoming-tx-controller branch from adb61ce to 534a292 Compare March 12, 2021 21:38
@metamaskbot
Copy link
Collaborator

Builds ready [534a292]
Page Load Metrics (644 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint50706163
domContentLoaded4498176426832
load4508196446833
domInteractive4488176426832

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

This looks great!

Although I've changed it from network to chainId, this shouldn't result in functional changes except where user's are using custom networks with chainIds that match default chains.

I think even for those users it won't be a functional change, as we were using the chainId and mapping it back to a type (using CHAIN_ID_TO_NETWORK_ID_MAP). So it was effectively grouping by chainId already, even though it was keyed by the network identifier. Unless I missed something.

shared/constants/transaction.js Outdated Show resolved Hide resolved
shared/constants/transaction.js Outdated Show resolved Hide resolved
@brad-decker
Copy link
Contributor Author

I think even for those users it won't be a functional change, as we were using the chainId and mapping it back to a type (using CHAIN_ID_TO_NETWORK_ID_MAP). So it was effectively grouping by chainId already, even though it was keyed by the network identifier. Unless I missed something.

No, that's correct. This should be an entirely non-functional change.

@metamaskbot
Copy link
Collaborator

Builds ready [9b87f73]
Page Load Metrics (580 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43805684
domContentLoaded3517625789545
load3587635809445
domInteractive3517625779545

@brad-decker brad-decker removed the DO-NOT-MERGE Pull requests that should not be merged label Mar 12, 2021
@metamaskbot
Copy link
Collaborator

Builds ready [fd4f5ee]
Page Load Metrics (593 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint4610658126
domContentLoaded3997285927335
load4007295937335
domInteractive3997275927335

@brad-decker brad-decker force-pushed the refactor-incoming-tx-controller branch from fd4f5ee to dfba6c0 Compare March 12, 2021 22:50
@metamaskbot
Copy link
Collaborator

Builds ready [dfba6c0]
Page Load Metrics (594 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45805894
domContentLoaded3757135929345
load3807135949345
domInteractive3747125929345

Gudahtt
Gudahtt previously approved these changes Mar 15, 2021
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@brad-decker
Copy link
Contributor Author

@Gudahtt thanks for review and approval! I'm going to hold this and change my migration number to avoid stealing two migration numbers from @darkwing 😬. going to bump the number up by one and wait for #10593 to land.,

@metamaskbot
Copy link
Collaborator

Builds ready [ae29cd0]
Page Load Metrics (643 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint46725873
domContentLoaded4728076417435
load4738086437435
domInteractive4718076417435

@brad-decker brad-decker force-pushed the refactor-incoming-tx-controller branch from ae29cd0 to a24b030 Compare March 17, 2021 16:50
@metamaskbot
Copy link
Collaborator

Builds ready [a24b030]
Page Load Metrics (608 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint44745694
domContentLoaded5418316076130
load5428326086129
domInteractive5408316066130

@brad-decker brad-decker force-pushed the refactor-incoming-tx-controller branch from a24b030 to fe5944e Compare March 19, 2021 17:44
@metamaskbot
Copy link
Collaborator

Builds ready [fe5944e]
Page Load Metrics (689 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint4511160147
domContentLoaded55694368711656
load55894468911656
domInteractive55694268711656

@brad-decker
Copy link
Contributor Author

@Gudahtt could use another look at this, only thing that has changed is the migration number (and test relocation due to my other PR)

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@brad-decker brad-decker merged commit a81629e into develop Mar 19, 2021
@brad-decker brad-decker deleted the refactor-incoming-tx-controller branch March 19, 2021 21:54
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants