Skip to content

cherrypick fix: Fix transaction metric event asset type detection (#24770) into v11.16.1#24843

Merged
dbrans merged 1 commit intoVersion-v11.16.1from
Version-v11.16.1-CPick-fix-missing-await
May 28, 2024
Merged

cherrypick fix: Fix transaction metric event asset type detection (#24770) into v11.16.1#24843
dbrans merged 1 commit intoVersion-v11.16.1from
Version-v11.16.1-CPick-fix-missing-await

Conversation

@dbrans
Copy link
Copy Markdown
Contributor

@dbrans dbrans commented May 28, 2024

🍒-pick fix: Fix transaction metric event asset type detection c1bba92 (#24770) into v11.16.1

Merge conflict in shared/modules/transaction.utils.ts (accepted incoming):

    try {
      // We don't need a balance check, so the second parameter to
      // getTokenStandardAndDetails is omitted.
<<<<<<< HEAD
      const details = getTokenStandardAndDetails(txMeta.txParams.to);
=======
      const details = await getTokenStandardAndDetails(txMeta.txParams.to);
>>>>>>> c1bba92d8c (fix: Fix transaction metric event asset type detection (#24770))
      if (details.standard) {
        return {

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label May 28, 2024
@dbrans dbrans marked this pull request as ready for review May 28, 2024 20:29
@dbrans dbrans requested a review from a team as a code owner May 28, 2024 20:29
@codecov
Copy link
Copy Markdown

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 67.42%. Comparing base (603cdcd) to head (cb98f67).

Files Patch % Lines
shared/modules/transaction.utils.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           Version-v11.16.1   #24843   +/-   ##
=================================================
  Coverage             67.42%   67.42%           
=================================================
  Files                  1265     1265           
  Lines                 49385    49385           
  Branches              12846    12846           
=================================================
  Hits                  33296    33296           
  Misses                16089    16089           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@legobeat legobeat changed the title 🍒-pick fix: Fix transaction metric event asset type detection (#24770) into v11.16.1 cherrypick fix: Fix transaction metric event asset type detection (#24770) into v11.16.1 May 28, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

We collect metric data for each phase of a transaction's lifecycle.
During one phase, we try to detect the type of asset being transacted.
This step was broken due to a missing `await`, so the asset type was
never identified. Additionally, the missing `await` meant that errors
were not being handled correctly, resulting in many junk Sentry error
events.

This seems to have been broken as part of a TypeScript refactor, in

that was added in #21330

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24770?quickstart=1)

Fixes #18300

TODO

I haven't reproduced any actual bug/missing metrics data from this yet,
but presumably we should see that and be able to test this. I discovered
this by looking into the Sentry error volume side-effect.

N/A

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] 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.

Co-authored-by: Derek Brans <dbrans@gmail.com>
@dbrans dbrans force-pushed the Version-v11.16.1-CPick-fix-missing-await branch from cb98f67 to 77a36b5 Compare May 28, 2024 22:32
@dbrans dbrans merged commit 358859a into Version-v11.16.1 May 28, 2024
@dbrans dbrans deleted the Version-v11.16.1-CPick-fix-missing-await branch May 28, 2024 23:22
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [77a36b5]
Page Load Metrics (1407 ± 522 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint782511324522
domContentLoaded13412774
load62265014071086522
domInteractive13412774

@metamaskbot metamaskbot added the release-11.16.1 Issue or pull request that will be included in release 11.16.1 label May 29, 2024
@metamaskbot
Copy link
Copy Markdown
Collaborator

No release label on PR. Adding release label release-11.16.1 on PR, as PR was cherry-picked in branch 11.16.1.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

INVALID-PR-TEMPLATE PR's body doesn't match template release-11.16.1 Issue or pull request that will be included in release 11.16.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants