[Wallet] Historical currency conversions in the transaction feed #2446

jeanregisser · 2020-01-15T14:57:24Z

Description

This PR enables historical currency conversions in the transaction feed.

Main highlights:

Implemented the updated schema in blockchain-api which allows querying transactions only for a specific token (cUSD on the home screen, cGLD on the exchange screen), retrieving
amounts in local currency, and later on will make it easy to paginate the data as proposed in https://docs.google.com/document/d/1ryfz7Gd0bk9QQHDBCWyaBy2AeOui63hgp-HdFtyBTyc/edit
Fixed the broken gql-gen tool and made GraphQL related code type safe again by using graphql-codegen for the client. (we should use it for blockchain-api too)
Started moving displayed money amounts into the CurrencyDisplay component. This component takes care of doing the actual currency conversions when needed. It will evolve as we move more of the existing code to it. It already simplified the feed items components.
Refactored code to use the same TransactionsList component for both the home and exchange screens and added tests for it.

We should probably run graphql-codegen as part of our React Native dev startup script to spot any type issues with the blockchain-api we're connecting to. I'll address that in a separate PR.

In the ExchangeFeedItem fragment, the amount field might feel redundant as it's either the makerAmount or the takerAmount with the addition of the sign (+/-), which we could derive client side. My main goal was to make the interface right first and optimize later when needed.

The TransactionsList.text.tsx test outputs console logs about invalid/missing fragments, this is caused by the Apollo MockProvider context missing details about the interface type we're using. I tried to pass the right fragmentMatcher like we do in the normal client (see https://www.apollographql.com/docs/react/data/fragments/#fragments-on-unions-and-interfaces)
but it then failed to return any data to the querying component.
In short, the test works as expected but outputs a lot of misleading noise.

Finally, this PR looks a bit big, but there are a lot of simple renames and snapshots changes, so hopefully shouldn't be too hard to review 🙏

Tested

Confirmed that converted local amounts in the transaction feed now use historical rates.
Added new tests.

Other changes

Currency conversion API now outputs a string instead of a number to prevent rounding issues.
The amounts in the transactions API are now string as well for the same reason stated above.
Memoized gold exchange rates queries (per requests).
Fixed a redux state update that was done in a non immutable way.
I had a problem with the test suite when running with --runInBand like in the CI and it was caused by the apollo query component not being unmounted after the test.
I upgraded react-native-testing-library to be able to cleanup mounted components after testing. See https://github.com/callstack/react-native-testing-library/blob/master/docs/API.md#cleanup
I decided to make that behavior global for all tests as I feel it's the right thing to do for us.
See b99484a
I just had to change one test which relied on a component being mounted between test.
See 6a90310
Let me know if you have any concern with this.

Related issues

Backwards compatibility

Yes.

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

…cal amounts

…ction-feed-refactor

i1skn · 2020-01-15T15:09:09Z

packages/blockchain-api/src/blockscout.ts

+        // Find the event related to the queried token
+        const tokenEvent = [inEvent, outEvent].find((event) => event.tokenSymbol === args.token)
+        if (tokenEvent) {
+          const timestamp = new BigNumber(inEvent.timeStamp).toNumber()


would parseInt do the job here?

We generally opt for BigNum over parseInt

Did not know that. Is the reason because BigNumber is more universal and can handle different input formats compare to parseInt?

packages/blockchain-api/src/currencyConversion/GoldExchangeRateAPI.ts

packages/mobile/src/send/TransferConfirmationCard.tsx

packages/mobile/src/transactions/TransactionFeed.tsx

packages/mobile/src/transactions/TransactionsList.tsx

i1skn · 2020-01-15T17:00:04Z

Great work!!! See a couple of non-blocker comments I've left :)

This fixes the TransactionsList test preventing the test suite to stop cleanly as the polling query was still mounted and doing its work.

codecov · 2020-01-16T11:59:49Z

Codecov Report

Merging #2446 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #2446    +/-   ##
========================================
  Coverage   74.79%   74.79%            
========================================
  Files         281      281            
  Lines        7922     7922            
  Branches      724     1013   +289     
========================================
  Hits         5925     5925            
  Misses       1894     1894            
  Partials      103      103

Flag	Coverage Δ
#mobile	`74.79% <ø> (ø)`	⬆️

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 7e0ec79...5b12586. Read the comment docs.

i1skn

Thanks for the tremendous improvement 🚢 !

…ork/blockscout/blob/130a4084eaf2c0db6ede18266b6f02e6a9230163/apps/block_scout_web/lib/block_scout_web/schema/scalars.ex#L11-L16

…ction-feed-refactor

jmrossy

Overall really nice work! Some comments below but nothing major

jmrossy · 2020-01-16T21:52:42Z

packages/blockchain-api/src/blockscout.ts

@@ -214,6 +218,107 @@ export class BlockscoutAPI extends RESTDataSource {
    )
    return rewards.sort((a, b) => b.timestamp - a.timestamp)
  }
+
+  async getTransactions(args: TransactionArgs) {


Can you leave a comment or rename this to clarify how it differs from the existing getTokenTransactions and getFeedEvents methods?

Yes! getFeedEvents was kept for backward compatibility, it is intended to be removed at some point.

Ok. Up to you then if you want to remove it now or later. Please leave TODOs explaining this

jmrossy · 2020-01-16T21:57:23Z

packages/blockchain-api/src/blockscout.ts

+      txHashToEventTransactions.set(tx.hash, currentTX)
+    }
+
+    await this.ensureTokenAddresses()


Not a new change in this PR but we should consider reworking this bit or at least leave a comment. This design of forcing users of token addresses to first call this method to ensure they're set is problematic. Easy to mess up. Would be better if the token addresses had getters that fetched them when null. cc @annakaz

Also I suspect this call is slowing down queries given a new data source is instantiated for each request.

jmrossy · 2020-01-16T21:58:18Z

packages/blockchain-api/src/blockscout.ts

+
+    await this.ensureTokenAddresses()
+    // Generate final events
+    txHashToEventTransactions.forEach((transactions: BlockscoutTransaction[], txhash: string) => {


Can we DRY this up with the getFeedEvents method above? Or is one being deprecated?

yes getFeedEvents is being deprecated.

jmrossy · 2020-01-16T22:03:58Z

packages/blockchain-api/src/schema.ts

      return dataSources.blockscoutAPI.getFeedEvents(args)
    },
    rewards: async (_source: any, args: EventArgs, { dataSources }: Context) => {
      return dataSources.blockscoutAPI.getFeedRewards(args)
    },
+    transactions: async (_source: any, args: TransactionArgs, context: Context) => {


My problem with transaction is that it's too vague. It could be anything from an ERC20 token transfer to a verification request to a smart contract deployment. Is there a more descriptive name we could use?

Makes sense, tokenTransactions feels like the right name then, what do you think?

Sounds good

jmrossy · 2020-01-16T22:07:46Z

packages/mobile/src/apollo/types.ts

-  comment: string
-  status?: TransactionStatus
-  // TODO: fee needs to be added here
+export type Event = Exchange | Transfer


I guess you kept the old Event interface for backwards compatibility with older app versions? But the next time we ship the app, we'll be resetting the network too and forcing users to update. We could actually remove all that.

Indeed! Let's remove it then.

I'll remove the deprecated code after this PR is merged to prevent breaking things unnecessarily given blockchain-api has to be redeployed.

jmrossy · 2020-01-16T22:15:22Z

packages/mobile/src/firebase/notifications.ts

-        value: divideByWei(transferNotification.value),
-        currency: resolveCurrency(transferNotification.currency),
+        amount: {
+          amount: divideByWei(transferNotification.value).toString(),


Another common source of confusion is whether or not a value is in wei or not. You don't need to fix in this PR but would be cool if we had a wei to type that and avoid potential mistakes.

Yep, I will definitely continue thinking about this as we improve currencies handling.

packages/mobile/src/redux/migrations.test.ts

packages/mobile/src/transactions/ExchangeFeedItem.tsx

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

packages/mobile/src/transactions/TransactionsList.tsx

…ction-feed-refactor

* master: [Wallet] Historical currency conversions in the transaction feed (#2446) CLI relock fix (#2464) Update copyright year + inline button (#2468) # Conflicts: # yarn.lock

* master: (25 commits) Add react-testing-utils + Fix Analytics (#2437) collect coverage on web (#2415) Add callouts to serve text messages in regions (#2458) [Wallet] Historical currency conversions in the transaction feed (#2446) CLI relock fix (#2464) Update copyright year + inline button (#2468) Voting bot for stake off (#2327) Change order of profile info (#2454) [Wallet] Fix type check regression for components wrapped by our custom `withTranslation` (#2457) Unfreeze rewards by default (#2452) Baklava and baklavastaging deploys (#2421) Fix coin colors (#2441) Make governance CLI more usable (#2428) Slashing params for stake off phase 2 (#2418) [Wallet] Rollback zeroSync toggle in case it was not successful (#2434) [Wallet] Cleanup unused StateProps references (#2439) Add unit tests and cli checks for validator hotfix interaction (#2340) Add proper Spanish translations (#2427) Catch exceptions during polling (#2432) Add unfreeze-contracts command to celotool (#2433) ... # Conflicts: # packages/web/src/brandkit/common/MobileMenu.test.tsx # yarn.lock

* master: Add react-testing-utils + Fix Analytics (#2437) collect coverage on web (#2415) Add callouts to serve text messages in regions (#2458) [Wallet] Historical currency conversions in the transaction feed (#2446) CLI relock fix (#2464) Update copyright year + inline button (#2468) Voting bot for stake off (#2327) Change order of profile info (#2454) [Wallet] Fix type check regression for components wrapped by our custom `withTranslation` (#2457) Unfreeze rewards by default (#2452) # Conflicts: # packages/web/src/analytics/analytics.test.ts # packages/web/src/brandkit/common/MobileMenu.test.tsx # yarn.lock

* master: add celo.org/about#contributors id (#2488) [Wallet] Fix integration build firebase db url on Android (#2495) Fixes governance CLI bugs encountered while running election contract upgrade (#2482) Add validator:signed-blocks command and fix validator:status bug (#2456) Bump @celo/celocli version to 0.0.34 (#2420) Fix Logo + backers number change (#2453) Improve election efficiency by short circuiting (#2475) Page for Experience / BrandKit / Composition (#2462) Configure the time to wait for text messages (#2450) Add react-testing-utils + Fix Analytics (#2437) collect coverage on web (#2415) Add callouts to serve text messages in regions (#2458) [Wallet] Historical currency conversions in the transaction feed (#2446) CLI relock fix (#2464) Update copyright year + inline button (#2468) Voting bot for stake off (#2327)

* master: (72 commits) [Wallet] Fix incorrect empty state on the gold tab (#2510) Added ingress resource for celostats DNS (#2480) Fix celotool testnet deploy (#2501) [Wallet] Navigate to wallet home after payment request (#2500) Require Consent for tracking globally (#2489) new academic papers + minor fix to web events (#2502) [Wallet] Fetch tobin tax for exchanges (#1492) add celo.org/about#contributors id (#2488) [Wallet] Fix integration build firebase db url on Android (#2495) Fixes governance CLI bugs encountered while running election contract upgrade (#2482) Add validator:signed-blocks command and fix validator:status bug (#2456) Bump @celo/celocli version to 0.0.34 (#2420) Fix Logo + backers number change (#2453) Improve election efficiency by short circuiting (#2475) Page for Experience / BrandKit / Composition (#2462) Configure the time to wait for text messages (#2450) Add react-testing-utils + Fix Analytics (#2437) collect coverage on web (#2415) Add callouts to serve text messages in regions (#2458) [Wallet] Historical currency conversions in the transaction feed (#2446) ... # Conflicts: # .circleci/config.yml # packages/web/src/brandkit/common/Page.tsx # packages/web/src/logos/LogoLightBg.tsx # packages/web/static/locales/en/brand.json

jeanregisser added 15 commits January 9, 2020 16:07

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

0d5bd2a

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

[WiP] New schema to support historical rates in the transaction feed

59e5e07

[WiP] Fix types and restore functionalities (except standby transaction)

3a1b6c0

Restore better log message

34f7dd2

[WiP] Map standby transactions to the new types for rendering with lo…

9c5ffed

…cal amounts

[WiP] Use MoneyAmount type in the exchange confirmation card

a7a6bdc

[WiP] Use MoneyAmount type in the transfer confirmation card

578411d

[WiP] Make CurrencyDisplay handle more currency display use cases

91d6f03

[WiP] Simplify ExchangeFeedItem by using CurrencyDisplay

508daa6

Fix non immutable redux state update

649a8cb

Memoize gold exchange rates requests (per request)

1054437

Add test for TransactionsList

29631b7

Merge remote-tracking branch 'origin/master' into jeanregisser/transa…

a5e2c77

…ction-feed-refactor

Cleanup code

1c4d4e7

Fix tests

13cb2ea

jeanregisser requested review from cmcewen and jmrossy as code owners January 15, 2020 14:57

jeanregisser requested review from annakaz, i1skn and martinvol January 15, 2020 15:06

i1skn reviewed Jan 15, 2020

View reviewed changes

jeanregisser added 4 commits January 16, 2020 11:26

Address review comments

061e9e8

Upgrade react-native-testing-library to be able to cleanup mounted trees

b99484a

This fixes the TransactionsList test preventing the test suite to stop cleanly as the polling query was still mounted and doing its work.

Fix InviteReview test now that components are unmounted after each test

6a90310

Remove noisy debug logs now that we do conversions for each transaction

781920b

i1skn approved these changes Jan 16, 2020

View reviewed changes

jeanregisser added 2 commits January 16, 2020 14:19

Use milliseconds timestamp

b76b097

Add test for blockscout getTransactions

188f15a

jeanregisser added 6 commits January 16, 2020 14:19

Hide currency symbol in the exchange item details

9df1ed5

Add depreciation notice

3d6f7b3

Add description for custom scalars

6ab93d7

Better description for Address courtesy of https://github.com/poanetw…

907d8d4

…ork/blockscout/blob/130a4084eaf2c0db6ede18266b6f02e6a9230163/apps/block_scout_web/lib/block_scout_web/schema/scalars.ex#L11-L16

Remove unneeded prop

2b3ff62

Merge remote-tracking branch 'origin/master' into jeanregisser/transa…

7249f5a

…ction-feed-refactor

jmrossy reviewed Jan 16, 2020

View reviewed changes

jeanregisser added 4 commits January 17, 2020 14:22

Address some comments

5d12679

Merge remote-tracking branch 'origin/master' into jeanregisser/transa…

79af67f

…ction-feed-refactor

Rename MoneyAmount field amount-> value

87b2e39

Rename transactions -> tokenTransactions

202e93e

jeanregisser assigned jmrossy Jan 20, 2020

jmrossy approved these changes Jan 20, 2020

View reviewed changes

jeanregisser unassigned jmrossy Jan 20, 2020

jeanregisser added the automerge Have PR merge automatically when checks pass label Jan 20, 2020

bulldozer-deployment-for-celo[bot] and others added 3 commits January 20, 2020 17:23

Merge refs/heads/master into jeanregisser/transaction-feed-refactor

99c79fb

Merge refs/heads/master into jeanregisser/transaction-feed-refactor

7e0ec79

Merge branch 'master' into jeanregisser/transaction-feed-refactor

5b12586

celo-ci-bot-user merged commit d558461 into master Jan 21, 2020

celo-ci-bot-user deleted the jeanregisser/transaction-feed-refactor branch January 21, 2020 14:29

This was referenced Jan 23, 2020

[BlockchainApi] Delete code related to the now deprecated events query #2509

Closed

[Wallet] Fix incorrect empty state on the gold tab #2510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Historical currency conversions in the transaction feed #2446

[Wallet] Historical currency conversions in the transaction feed #2446

jeanregisser commented Jan 15, 2020 •

edited

Loading

i1skn Jan 15, 2020

jmrossy Jan 15, 2020

i1skn Jan 16, 2020

jmrossy Jan 17, 2020

i1skn commented Jan 15, 2020

codecov bot commented Jan 16, 2020 •

edited

Loading

i1skn left a comment •

edited

Loading

jmrossy left a comment

jmrossy Jan 16, 2020

jeanregisser Jan 17, 2020

jmrossy Jan 17, 2020

jmrossy Jan 16, 2020

jeanregisser Jan 17, 2020 •

edited

Loading

jmrossy Jan 16, 2020

jeanregisser Jan 17, 2020

jmrossy Jan 16, 2020

jeanregisser Jan 17, 2020

jmrossy Jan 17, 2020

jeanregisser Jan 20, 2020

jmrossy Jan 16, 2020

jeanregisser Jan 17, 2020

jeanregisser Jan 20, 2020

jmrossy Jan 16, 2020

jeanregisser Jan 17, 2020 •

edited

Loading

[Wallet] Historical currency conversions in the transaction feed #2446

[Wallet] Historical currency conversions in the transaction feed #2446

Conversation

jeanregisser commented Jan 15, 2020 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

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

i1skn commented Jan 15, 2020

codecov bot commented Jan 16, 2020 • edited Loading

Codecov Report

i1skn left a comment • edited Loading

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

jeanregisser Jan 17, 2020 • edited Loading

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

jeanregisser Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

jeanregisser commented Jan 15, 2020 •

edited

Loading

codecov bot commented Jan 16, 2020 •

edited

Loading

i1skn left a comment •

edited

Loading

jeanregisser Jan 17, 2020 •

edited

Loading

jeanregisser Jan 17, 2020 •

edited

Loading