Skip to content

feat: Resimulate transactions for every 3 seconds on focused MM window #29878

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

Merged
merged 14 commits into from
Mar 20, 2025

Conversation

OGPoyraz
Copy link
Member

@OGPoyraz OGPoyraz commented Jan 23, 2025

Description

This PR aims to implement re-simulation (updating simulationData on transactionMeta) of transaction type of confirmations on every 3 seconds if notification window is focused.

Core PR that updates simulation data on active transactions: MetaMask/core#5189

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3922

Manual testing steps

See original task description.

Screenshots/Recordings

Resimulation.mov

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@metamaskbot metamaskbot added the team-confirmations Push issues to confirmations team label Jan 23, 2025
Copy link

socket-security bot commented Jan 23, 2025

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@OGPoyraz OGPoyraz marked this pull request as ready for review January 24, 2025 12:24
@OGPoyraz OGPoyraz requested a review from a team as a code owner January 24, 2025 12:24
OGPoyraz added a commit to MetaMask/core that referenced this pull request Feb 13, 2025
## Explanation

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->

This PR adds `ResimulateHelper`, which focuses on
`transactionMeta.isActive` property and re-simulates transaction
depending on that value.

In order to capsulate re-simulation logic, this PR also relocates other
utility functions under the new created `ResimulationHelper.ts` file.

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Fixes #12345
* Related to #67890
-->

Fixes: MetaMask/MetaMask-planning#3922
Extension PR: MetaMask/metamask-extension#29878 

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/transaction-controller`

- **ADDED**: Adds ability of re-simulating transaction depending on the
`isActive` property on `transactionMeta`
   - `isActive` property is expected to set by client.
- Re-simulation of transactions will occur every 3 seconds if `isActive`
is `true`.
- Adds `setTransactionActive` function to update the `isActive` property
on `transactionMeta`.

## Checklist

- [X] I've updated the test suite for new or updated code as appropriate
- [X] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [X] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [X] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

---------

Co-authored-by: Elliot Winkler <[email protected]>
@OGPoyraz OGPoyraz force-pushed the 3922-re-simulate-transactions-for-every-new-block branch from 4642c4d to ef97961 Compare March 11, 2025 07:29
@OGPoyraz OGPoyraz changed the title feat: Resimulate transactions for every new block feat: Resimulate transactions for every 3 seconds on focused MM window Mar 11, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [cef1628]
Page Load Metrics (1900 ± 107 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26025701805409196
domContentLoaded164124921858210101
load166826081900223107
domInteractive27443253
backgroundConnect13128453316
firstReactRender157430199
getState65721189
initialActions00000
loadScripts12151987140717986
setupStore865262110
uiStartup190630102202293141
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 52 Bytes (0.00%)
  • ui: 2.54 KiB (0.03%)
  • common: 123 Bytes (0.00%)

@OGPoyraz OGPoyraz force-pushed the 3922-re-simulate-transactions-for-every-new-block branch from cef1628 to ddcad08 Compare March 17, 2025 10:58
@metamaskbot
Copy link
Collaborator

Builds ready [af73098]
Page Load Metrics (3125 ± 1431 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31015859280730871482
domContentLoaded171415757296029691426
load172915869312529791431
domInteractive2367210314670
backgroundConnect131093180244117
firstReactRender16191574321
getState51035116228109
initialActions01000
loadScripts127213277227725431221
setupStore84975910550
uiStartup193716046412435171689
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 52 Bytes (0.00%)
  • ui: 2.54 KiB (0.03%)
  • common: 123 Bytes (0.00%)

Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

logic looks good. suggestion to rename some methods and variables to improve readability through more explicit naming

type === TransactionType.tokenMethodTransferFrom ||
type === TransactionType.tokenMethodSafeTransferFrom
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion] we could use an O(1) lookup rather than the conditional O(n)

const FOCUSABLE_TYPES: Set<TransactionType> = new Set([
  TransactionType.contractInteraction,
  TransactionType.deployContract,
  TransactionType.simpleSend,
  TransactionType.smart,
  TransactionType.tokenMethodTransfer,
  TransactionType.tokenMethodTransferFrom,
  TransactionType.tokenMethodSafeTransferFrom,
]);

then we could update

const shouldBeMarked = shouldSetFocusedForType(type as TransactionType);

to

const shouldBeMarked = FOCUSABLE_TYPES.has(type); 

Copy link
Member Author

Choose a reason for hiding this comment

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

There were initially other logics inside shouldSetFocusedForType but they all removed, Set definitely make sense now, done.

const { id, type } = currentConfirmation ?? {};
const isWindowFocused = useWindowFocus();
const dispatch = useDispatch();
const [focusedConfirmation, setFocusedConfirmation] = useState<string | null>(
Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion] rename focusedConfirmation → focusedConfirmationId for more context and improved readability

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, done

);

useEffect(() => {
const shouldBeMarked = shouldSetFocusedForType(type as TransactionType);
Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion] maybe renaming shouldBeMarked → isFocusable.

I think "should" in shouldBeMarked implies that it will be set. However, the state is only conditionally set. The reason I suggested removing "Marked" is to avoid introducing synonyms. There is room for interpretation using Marked. We can be more explicit by choosing e.g. isFocusable, canSetFocus, canSetTransactionFocus

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@OGPoyraz OGPoyraz force-pushed the 3922-re-simulate-transactions-for-every-new-block branch from af73098 to 2dd406e Compare March 20, 2025 06:42
@OGPoyraz OGPoyraz force-pushed the 3922-re-simulate-transactions-for-every-new-block branch from 2dd406e to 66dda95 Compare March 20, 2025 06:44
@metamaskbot
Copy link
Collaborator

Builds ready [66dda95]
Page Load Metrics (3289 ± 1145 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint35713262289625561227
domContentLoaded146413123282424591181
load153713173328923841145
domInteractive2688310518689
backgroundConnect651018448318153
firstReactRender28326966933
getState1368522319996
initialActions00000
loadScripts105312689219724711187
setupStore1236512211756
uiStartup171313419566332131543
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 52 Bytes (0.00%)
  • ui: 2.52 KiB (0.03%)
  • common: 123 Bytes (0.00%)

Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

Lgtm!

@OGPoyraz OGPoyraz added this pull request to the merge queue Mar 20, 2025
Merged via the queue into main with commit 87946e4 Mar 20, 2025
76 checks passed
@OGPoyraz OGPoyraz deleted the 3922-re-simulate-transactions-for-every-new-block branch March 20, 2025 09:32
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2025
@metamaskbot metamaskbot added the release-12.16.0 Issue or pull request that will be included in release 12.16.0 label Mar 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.16.0 Issue or pull request that will be included in release 12.16.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants