[Wallet] Fix slow activity feed tx confirmations #2290

annakaz · 2019-12-17T22:27:06Z

Description

Currently, the activity feed can be slow to update transactions in the activity from "confirming..." to "confirmed", especially when on a 3g connection. Balances update but the "confirming..." is still there, along with a duplicate "confirmed" transaction.

Now, the activity feed removes the "confirming..." transactions promptly and never renders "confirming..." and "confirmed" at the same time.

Tested

Updated jest tests
Tested on pixel 2, throttling to 3g speeds using Charles, also using normal wifi. Confirmed that transaction confirmations for send, exchange and invite come in at the same time that the balance is updated

Other changes

Created Transaction type to align Standby and confirmed (from graphQL) transactions
Updated some logging

Related issues

Fixes [Wallet] Users on 3g network SBAT get tx receipts more reliably #1741

Backwards compatibility

Yes

annakaz · 2019-12-17T22:30:34Z

packages/mobile/src/escrow/saga.ts

@@ -86,7 +86,7 @@ function* registerStandbyTransaction(id: string, value: string, address: string)
      id,
      type: TransactionTypes.ESCROW_SENT,
      status: TransactionStatus.Pending,
-      value,
+      value: +value,


Note that storing StandbyTransaction as number instead of string does not impact precision, as the confirmed transaction value is stored as a number anyway. Did not want to modify graphQL to change both to strings so as not to overlap with @jeanregisser 's local currency work

annakaz · 2019-12-17T22:31:12Z

packages/mobile/src/transactions/reducer.ts

@@ -61,6 +46,24 @@ type TransferTransactionTypes =
  | TransactionTypes.INVITE_RECEIVED
  | TransactionTypes.NETWORK_FEE

+export const isTransferType = (txType: TransactionTypes) => {


Couldn't find a cleaner way to check multiple types but lmk if you think of one

sometimes i will make an array here and then just check if the string is contained in the array

Kk I find the function cleaner than the array so will keep as is

codecov · 2019-12-17T23:17:56Z

Codecov Report

Merging #2290 into master will decrease coverage by 0.02%.
The diff coverage is 61.9%.

@@            Coverage Diff             @@
##           master    #2290      +/-   ##
==========================================
- Coverage   74.55%   74.52%   -0.03%     
==========================================
  Files         278      278              
  Lines        7777     7777              
  Branches      991      709     -282     
==========================================
- Hits         5798     5796       -2     
- Misses       1868     1869       +1     
- Partials      111      112       +1

Flag	Coverage Δ
#mobile	`74.52% <61.9%> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
packages/mobile/src/tokens/saga.ts	`91.04% <ø> (ø)`	⬆️
packages/mobile/src/escrow/saga.ts	`21.47% <ø> (ø)`	⬆️
packages/mobile/src/apollo/types.ts	`100% <ø> (ø)`	⬆️
packages/mobile/src/home/TransactionsList.tsx	`48.38% <50%> (ø)`	⬆️
...ckages/mobile/src/transactions/TransactionFeed.tsx	`83.33% <57.14%> (-1.97%)`	⬇️
packages/mobile/src/transactions/reducer.ts	`70.27% <80%> (+1.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b93d855...b691fb3. Read the comment docs.

cmcewen

woot woot woot

cmcewen · 2019-12-18T01:14:47Z

packages/mobile/src/apollo/__mockData__.ts


 export const invitedAddress = '0x1b173'

 const exchangeDollar: HomeExchangeFragment = {
-  __typename: EventTypeNames.Exchange,
-  type: 'EXCHANGE' as TransactionTypes,
+  type: 'EXCHANGE' as TransactionTypes.EXCHANGE,


Why not just use the enum's actual value? (not sure who did this originally)

I think this is a more realistic mock of what would be returned?

I think Connors point is that TransactionTypes.Exchange === 'EXCHANGE' so why hardcode the value here?
And I think the reason it was originally hard coded in the first place is that the auto-gen typing tool did it this way

cmcewen · 2019-12-18T01:17:12Z

packages/mobile/src/apollo/types.ts

@@ -1,41 +1,51 @@
 import { Query } from 'react-apollo'
-import { TransactionTypes } from 'src/transactions/reducer'
+import { CURRENCY_ENUM } from 'src/geth/consts'


I just tried it and it doesn't work right now, but i think this file originally came from running yarn run build:gen-graphql-types with the blockchain api running locally

Kk are you saying we should avoid modifying this generated file?

This file was originally made that way but most of that auto gen code has been changed over time. I've made modifications here too.

I believe we diverged away from the auto gen types because of the differing shapes of our standby tx and graphql txs

cmcewen · 2019-12-18T01:17:43Z

packages/mobile/src/transactions/ExchangeFeedItem.test.tsx

@@ -28,8 +27,7 @@ describe('ExchangeFeedItem', () => {
      <Provider store={createMockStore({})}>
        <ExchangeFeedItem
          status={TransactionStatus.Complete}
-          __typename={EventTypeNames.Exchange}
-          type={'EXCHANGE' as TransactionTypes}
+          type={'EXCHANGE' as TransactionTypes.EXCHANGE}


same enum comment

Same as above

cmcewen · 2019-12-18T01:19:43Z

packages/mobile/src/transactions/reducer.ts

@@ -61,6 +46,24 @@ type TransferTransactionTypes =
  | TransactionTypes.INVITE_RECEIVED
  | TransactionTypes.NETWORK_FEE

+export const isTransferType = (txType: TransactionTypes) => {


sometimes i will make an array here and then just check if the string is contained in the array

jmrossy

Thanks for tackling this! Can you clarify how this solves the problem exactly?

jmrossy · 2019-12-18T11:30:51Z

packages/mobile/src/apollo/types.ts

@@ -1,41 +1,51 @@
 import { Query } from 'react-apollo'
-import { TransactionTypes } from 'src/transactions/reducer'
+import { CURRENCY_ENUM } from 'src/geth/consts'


This file was originally made that way but most of that auto gen code has been changed over time. I've made modifications here too.

jmrossy · 2019-12-18T11:32:23Z

packages/mobile/src/apollo/types.ts

@@ -1,41 +1,51 @@
 import { Query } from 'react-apollo'
-import { TransactionTypes } from 'src/transactions/reducer'
+import { CURRENCY_ENUM } from 'src/geth/consts'


I believe we diverged away from the auto gen types because of the differing shapes of our standby tx and graphql txs

jmrossy · 2019-12-18T11:35:37Z

packages/mobile/src/apollo/__mockData__.ts


 export const invitedAddress = '0x1b173'

 const exchangeDollar: HomeExchangeFragment = {
-  __typename: EventTypeNames.Exchange,
-  type: 'EXCHANGE' as TransactionTypes,
+  type: 'EXCHANGE' as TransactionTypes.EXCHANGE,


I think Connors point is that TransactionTypes.Exchange === 'EXCHANGE' so why hardcode the value here?
And I think the reason it was originally hard coded in the first place is that the auto-gen typing tool did it this way

jmrossy · 2019-12-18T11:37:32Z

packages/mobile/src/apollo/__mockData__.ts

  timestamp: Date.now(),
  address: '0x423043cca38e75d7913504fedfd1dd4539cc55b3',
  comment: 'FAKE FAKE FAKE',
  hash: '01010',
 }
 const verificationReward: HomeTransferFragment = {
-  __typename: EventTypeNames.Transfer,


Why don't we need these graphQL provided __typename properties anymore? Is it cause @jeanregisser split the home feed and exchange feed queries in bc-api?

Yeah we don't need them since we know the EventType from the transaction type

jmrossy · 2019-12-18T11:38:12Z

packages/utils/src/phoneNumbers.ts

@@ -153,7 +153,9 @@ export function parsePhoneNumber(
        }
      : null
  } catch (error) {
-    console.debug(`phoneNumbers/parsePhoneNumber/Failed to parse phone number, error: ${error}`)
+    console.debug(
+      `phoneNumbers/parsePhoneNumber/Failed to parse phone number ${phoneNumberRaw}, error: ${error}`


We intentionally excluded the phone number for privacy reasons, as the logs get uploaded to firebase

annakaz · 2020-01-08T00:53:33Z

Thanks for tackling this! Can you clarify how this solves the problem exactly?

This solves the problem as the issue was that the "Confirming..." notification was not being removed once the confirmation came in. It was actually not a network issue of the transaction taking a long time to be confirmed- it was a UI issue of the PendingTransaction not going away (though it occurred more often when internet was slow, perhaps bc it took longer to query the notification feed?). After cleaning up src/transactions/TransactionFeed.tsx to treat PendingTransactions and Transactions as the same type, we know not to display the PendingTransaction if we see the matching Transaction item

* master: (139 commits) Accelerate time based parameters (#2377) Fix circle not being started (#2380) Blockscout logs patch for json format (#2268) [Wallet] Fix slow activity feed tx confirmations (#2290) Aaronmgdr/about followup (#2376) Change "not syncing" to "not synced" (#2372) [Docs] Update Contributing guide with links to good first issues (#2375) Optimize Bundles (#2352) Update blockscout for alfajores and pilot envs (#1954) Make the same hotfix proposal executable with unique salts (#2357) Fix Contribution Guildelines in the docs (#2370) Add Snapshot for each page on website (#2313) Turn on Emit (#2369) Update to latest blockchain with select issuers fix (#2362) Add docs on how to generate sms retriever hash (#2356) Aaronmgdr/about below (#1194) Switch to using native pbkdf to fix hanging on older devices (#2354) Adding a command to the docker script allowing to stop validating (#2295) Update prettier to 1.19.1 to support TypeScript 3.7 (optional chaining, nullish coalescing, etc) (#2358) Fixes needed to make slashing work (#2346) ...

This reverts commit 8ebc663. Conflicts too much with the transaction feed refactor.

annakaz added 8 commits December 17, 2019 09:58

WIP

8390328

Delete commented out code

f276c08

Put back ts ignore

019a73a

Fix lint/build errors

562c4b4

Fix comment tsignore

cc57d76

Fix types

1386888

Upate snapshots

818b4ff

Replace Event with Transaction type

10e2411

annakaz requested review from cmcewen, jmrossy and yerdua as code owners December 17, 2019 22:27

annakaz commented Dec 17, 2019

View reviewed changes

annakaz requested a review from a team December 17, 2019 22:32

annakaz assigned cmcewen Dec 17, 2019

Merge branch 'master' into annakaz/standby-tx

90bf3e2

cmcewen reviewed Dec 18, 2019

View reviewed changes

Merge branch 'master' into annakaz/standby-tx

5246ec4

cmcewen assigned annakaz and unassigned cmcewen Dec 18, 2019

annakaz assigned cmcewen and unassigned annakaz Dec 18, 2019

Merge branch 'master' into annakaz/standby-tx

7802782

jmrossy reviewed Dec 18, 2019

View reviewed changes

annakaz and others added 4 commits December 18, 2019 11:32

Merge branch 'master' into annakaz/standby-tx

516a29c

Merge branch 'master' into annakaz/standby-tx

c618606

Use enum in mockdata and remove phone number

a52e86a

Merge branch 'master' into annakaz/standby-tx

c283318

cmcewen approved these changes Jan 7, 2020

View reviewed changes

annakaz added 2 commits January 7, 2020 17:19

Merge branch 'master' into annakaz/standby-tx

76fd3f3

Merge branch 'master' into annakaz/standby-tx

b691fb3

annakaz added the automerge Have PR merge automatically when checks pass label Jan 8, 2020

celo-ci-bot-user merged commit 8ebc663 into master Jan 8, 2020

celo-ci-bot-user deleted the annakaz/standby-tx branch January 8, 2020 01:06

jeanregisser added a commit that referenced this pull request Jan 9, 2020

Revert "[Wallet] Fix slow activity feed tx confirmations (#2290)"

0d5bd2a

This reverts commit 8ebc663. Conflicts too much with the transaction feed refactor.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Fix slow activity feed tx confirmations #2290

[Wallet] Fix slow activity feed tx confirmations #2290

annakaz commented Dec 17, 2019

annakaz Dec 17, 2019 •

edited

Loading

annakaz Dec 17, 2019 •

edited

Loading

cmcewen Dec 18, 2019

annakaz Dec 18, 2019

codecov bot commented Dec 17, 2019 •

edited

Loading

cmcewen left a comment

cmcewen Dec 18, 2019

annakaz Dec 18, 2019

jmrossy Dec 18, 2019

cmcewen Dec 18, 2019

annakaz Dec 18, 2019

jmrossy Dec 18, 2019

jmrossy Dec 18, 2019

cmcewen Dec 18, 2019

annakaz Dec 18, 2019

cmcewen Dec 18, 2019

jmrossy left a comment

jmrossy Dec 18, 2019

jmrossy Dec 18, 2019

jmrossy Dec 18, 2019

jmrossy Dec 18, 2019

annakaz Dec 18, 2019 •

edited

Loading

jmrossy Dec 18, 2019

annakaz commented Jan 8, 2020

[Wallet] Fix slow activity feed tx confirmations #2290

[Wallet] Fix slow activity feed tx confirmations #2290

Conversation

annakaz commented Dec 17, 2019

Description

Tested

Other changes

Related issues

Backwards compatibility

annakaz Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

annakaz Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2019 • edited Loading

Codecov Report

cmcewen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrossy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakaz Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakaz commented Jan 8, 2020

annakaz Dec 17, 2019 •

edited

Loading

annakaz Dec 17, 2019 •

edited

Loading

codecov bot commented Dec 17, 2019 •

edited

Loading

annakaz Dec 18, 2019 •

edited

Loading