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

fix: Incoming tx on activity tab when transfering funds between wallets on the same client #20973

Merged
merged 14 commits into from
Sep 22, 2023

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented Sep 20, 2023

Description

When sending a simple send transaction between two accounts of the same mnemonic on the same client, the incoming transaction is lost.

The issue failed to reproduce in production, which means it was about changes not in the latest release, v11.0.0.

The problem seems to be coming from the following function:

  _onIncomingTransactions({ added: transactions }) {
    log.debug('Detected new incoming transactions', transactions);

    const currentTransactions = this.store.getState().transactions || {};

    const incomingTransactions = transactions
      .filter((tx) => !this._hasTransactionHash(tx.hash, currentTransactions))
      .reduce((result, tx) => {
        result[tx.id] = tx;
        return result;
      }, {});

    const updatedTransactions = {
      ...currentTransactions,
      ...incomingTransactions,
    };

    this.store.updateState({ transactions: updatedTransactions });
  }

Since currentTransactions includes the outgoing simpleSend transaction with the same hash as the incoming type transaction contained in transactions, the incoming transaction gets filtered out, which means it's not available to be displayed in the activity tab.

The implemented solution is to keep both the simpleSend and incoming transaction by adding a new condition to the filter.

Manual testing steps

See original issue ticket video

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained:
    • What problem this PR is solving.
    • How this problem was solved.
    • How reviewers can test my changes.
  • I’ve indicated what issue this PR is linked to: Fixes #???
  • I’ve included tests if applicable.
  • I’ve documented any added code.
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@github-actions
Copy link
Contributor

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.

@pedronfigueiredo pedronfigueiredo force-pushed the fix/incoming-tx-filtering branch from 1e830c4 to 1974868 Compare September 20, 2023 14:40
@metamaskbot
Copy link
Collaborator

Builds ready [a99ea44]
Page Load Metrics (1907 ± 135 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1192981704923
domContentLoaded156525471907282135
load156525471907282135
domInteractive156525471907282135
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 37 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Patch coverage: 16.00% and project coverage change: -0.02% ⚠️

Comparison is base (da7dcb6) 68.38% compared to head (9f9be44) 68.36%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #20973      +/-   ##
===========================================
- Coverage    68.38%   68.36%   -0.02%     
===========================================
  Files         1006     1006              
  Lines        40243    40262      +19     
  Branches     10760    10763       +3     
===========================================
+ Hits         27518    27522       +4     
- Misses       12725    12740      +15     
Files Changed Coverage Δ
app/scripts/controllers/transactions/index.js 72.64% <ø> (ø)
...app/transaction-list/transaction-list.component.js 38.20% <16.00%> (-4.65%) ⬇️

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

@pedronfigueiredo pedronfigueiredo force-pushed the fix/incoming-tx-filtering branch 3 times, most recently from 7d70ece to 6a05f89 Compare September 21, 2023 09:18
matthewwalsh0
matthewwalsh0 previously approved these changes Sep 21, 2023
@danjm danjm added release-blocker This bug is blocking the next release release-11.1.0 Issue or pull request that will be included in release 11.1.0 labels Sep 21, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [8858b8f]
Page Load Metrics (1761 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint120183153178
domContentLoaded15852093176014268
load15852094176114469
domInteractive15852093176014268
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 44 Bytes (0.00%)
  • ui: 378 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

DDDDDanica
DDDDDanica previously approved these changes Sep 21, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [ace23e2]
Page Load Metrics (1522 ± 26 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint118164134136
domContentLoaded1434163815225426
load1434163815225426
domInteractive1434163815225426
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 44 Bytes (0.00%)
  • ui: 378 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@pedronfigueiredo pedronfigueiredo force-pushed the fix/incoming-tx-filtering branch from be9da69 to 1bb38aa Compare September 21, 2023 15:01
matthewwalsh0
matthewwalsh0 previously approved these changes Sep 21, 2023
DDDDDanica
DDDDDanica previously approved these changes Sep 21, 2023
@pedronfigueiredo pedronfigueiredo force-pushed the fix/incoming-tx-filtering branch 2 times, most recently from e87c56e to ddb326b Compare September 21, 2023 16:46
@metamaskbot
Copy link
Collaborator

Builds ready [ddb326b]
Page Load Metrics (1637 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1221981512110
domContentLoaded14631922163610551
load14631922163710550
domInteractive14631922163610551
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 44 Bytes (0.00%)
  • ui: 378 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@pedronfigueiredo pedronfigueiredo force-pushed the fix/incoming-tx-filtering branch 2 times, most recently from 0f99b9a to 55243bb Compare September 22, 2023 07:46
@metamaskbot
Copy link
Collaborator

Builds ready [55243bb]
Page Load Metrics (1927 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1292321702311
domContentLoaded16902348192615072
load16902348192715273
domInteractive16902348192515072

@pedronfigueiredo pedronfigueiredo force-pushed the fix/incoming-tx-filtering branch from 55243bb to 9f9be44 Compare September 22, 2023 08:50
@metamaskbot
Copy link
Collaborator

Builds ready [9f9be44]
Page Load Metrics (1746 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1262411653215
domContentLoaded15202091174615273
load15202091174615273
domInteractive15202091174615273
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 44 Bytes (0.00%)
  • ui: 378 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@pedronfigueiredo pedronfigueiredo merged commit 97bb2c4 into develop Sep 22, 2023
9 checks passed
@pedronfigueiredo pedronfigueiredo deleted the fix/incoming-tx-filtering branch September 22, 2023 09:36
@github-actions github-actions bot locked and limited conversation to collaborators Sep 22, 2023
@metamaskbot metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 22, 2023
@Gudahtt Gudahtt removed the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.1.0 Issue or pull request that will be included in release 11.1.0 release-blocker This bug is blocking the next release team-extension-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants