[BlockchainApi] Add ability to get exchange rates from/to cGLD or cUSD #2005

jeanregisser · 2019-12-03T16:48:23Z

Description

Add ability to get exchange rates from/to cGLD or cUSD.

It's now possible to get rates from cGLD to any other currency (cUSD, USD, MXN, etc) and vice versa.
Supports getting cGLD rate at a specific date/time (with a granularity of 30 mins as that's the current interval at which the rate is being stored)

For now the cUSD/USD and USD/cUSD rates are assumed to be 1.
Also going from cGLD to local currency (or vice versa) is currently assumed to be the same as cGLD -> cUSD -> USD -> local currency.
And similar to cUSD to local currency, but with one less step.

Tested

Added new tests.
Successfully deployed on integration: https://integration-dot-celo-testnet.appspot.com

Other changes

Use service account key for all envs. This is so blockchain-api in celo-testnet project can access the firebase db in celo-org-mobile project. This is temporary until we migrate blockchain-api to celo-org-mobile too.
Speed up jest tests by mocking contractkit.

WITHOUT mocking contractkit:

yarn test (clean cache): ~27secs
yarn test (populated cache): 23 secs

WITH mocking contractkit:

yarn test (clean cache): ~8secs
yarn test (populated cache): ~4secs

Related issues

Currency conversion in activity feed should use historical rate #834

Backwards compatibility

Yes

WITHOUT mocking contractkit: - yarn test (clean cache): ~27secs - yarn test (populated cache): 23 secs WITH mocking contractkit: - yarn test (clean cache): ~8secs - yarn test (populated cache): ~4secs

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

Similar gains as seen in #2005

…hain-api-convert-gold

deploy runs yarn from packages/blockchain-api so ends up with slightly different node_modules

…-mobile

This is so blockchain-api in celo-testnet project can access the firebase db in celo-org-mobile project This is temporary until we migrate blockchain-api to celo-org-mobile too.

codecov · 2019-12-10T14:19:25Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #2005   +/-   ##
=======================================
  Coverage   74.88%   74.88%           
=======================================
  Files         279      279           
  Lines        7797     7797           
  Branches      690      690           
=======================================
  Hits         5839     5839           
  Misses       1842     1842           
  Partials      116      116

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

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 61bfc10...a7abbe6. Read the comment docs.

annakaz

LGTM! Left some minor comments

annakaz · 2019-12-10T19:23:00Z

packages/blockchain-api/src/config.ts

+  // TODO: move project to celo-org-mobile
+  // until then, using serviceAccountKey for all envs
+  // tslint:disable-next-line: no-constant-condition
+  if (true /* DEPLOY_ENV === 'local' */) {


Remove if (true) ? Or uncomment /* DEPLOY_ENV === 'local' */?

I wanted to keep it this way so we can uncomment once we deploy it to celo-org-mobile.
Makes sense?

annakaz · 2019-12-10T19:32:02Z

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

+}
+
+export default class GoldExchangeRateAPI<TContext = any> extends DataSource {
+  // TODO(jeanregisser): memoize results


TODOs here and below for a future PR? Or left in for now?

I left them for a future PR.

annakaz · 2019-12-10T19:38:30Z

packages/blockchain-api/src/currencyConversion/CurrencyConversionAPI.test.ts

+    expect(mockGoldGetExchangeRate).toHaveBeenCalledTimes(1)
+  })
+
+  it('should retrieve rate for MXN/cGLD', async () => {


Does this test cover any additional logic from the MXN/cGLD one given the mocks?

They look really similar indeed but the idea was to cover both local to cGLD and cGLD to local.

annakaz · 2019-12-10T19:39:43Z

packages/blockchain-api/src/currencyConversion/CurrencyConversionAPI.test.ts

+    expect(mockGoldGetExchangeRate).toHaveBeenCalledTimes(0)
+  })
+
+  it('should retrieve rate for MXN/USD', async () => {


Same with this one- is there anything this test would catch that wouldn't be covered by the USD/MXN one?
Could also consider adding some tests for getConversionSteps

Yes similar to my comment above.
I decided not to test getConversionSteps directly to avoid testing the internals too much.

annakaz · 2019-12-10T19:39:58Z

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

+    const pair = `${fromCode}/${toCode}`
+
+    if (pair === 'cUSD/USD' || pair === 'USD/cUSD') {
+      // TODO: use real rates once we have the data


jmrossy

A few small comments but mostly looks good!

jmrossy · 2019-12-11T10:32:02Z

packages/blockchain-api/package.json

@@ -23,6 +23,7 @@
    "bignumber.js": "^7.2.0",
    "dotenv": "^6.1.0",
    "express": "^4.16.4",
+    "firebase-admin": "^8.8.0",


Please use the same version we're using in other monorepo packages (8.6.1)

jmrossy · 2019-12-11T10:33:20Z

packages/blockchain-api/src/config.ts

+    try {
+      const serviceAccount = require('../serviceAccountKey.json')
+      return admin.credential.cert(serviceAccount)
+    } catch {


Should we catch and log the error object here?

Yes probably, I blindly cut and pasted this from notification-service

jmrossy · 2019-12-11T10:38:09Z

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

@@ -0,0 +1 @@
+export { default as CurrencyConversionAPI } from './CurrencyConversionAPI'


Why create this index file instead of having other files import from CurrencyConversionAPI directly?

Mostly to hide the internal file organisation of currency conversion with its somewhat private internal modules (ExchangeRateAPI and GoldExchangeRateAPI).
But doesn't really matter.

…hain-api-convert-gold

* master: [Wallet] Use Charles proxy to see eth JSON rpc calls when using forno (#2204) [Wallet] Disable skip button when the user enable contact access (#2224) [Wallet] Redesigning notification lists (#1967) [Wallet] Fix crash on iOS when segment is enabled (#2222) Update documentation wrt. epoch rewards fractions (#2182) Improvement facilitating to run a full node (#2130) [BlockchainApi] Add ability to get exchange rates from/to cGLD or cUSD (#2005) Improve reliability of e2e governance test (#2208) Fix/protocol test flakyness (#2155) Fix bignumber display in CLI (#2212) Doc changes to address frequently asked questions (#2209) Upgrade TS version (#2196) Wait for only waitTime - 1 blocks (#2207) Minor baklava docs reconnection fixes (#2215) Update walletkit gateway fee to fix transactions in forno mode (#2211) Update baklava network ID in docs for 1.1 (#2214) Support more than 1 attesation bot at a time (#2192) Check sync status of attestation service (#2191) Indicate to run Twilio globally (#2193) Add Twilio and attestation bot envs (#2194)

jeanregisser added 2 commits December 3, 2019 16:50

Add ability to convert to/from cGLD or cUSD

e639642

Mock contractkit to speed up jest tests

f861a22

WITHOUT mocking contractkit: - yarn test (clean cache): ~27secs - yarn test (populated cache): 23 secs WITH mocking contractkit: - yarn test (clean cache): ~8secs - yarn test (populated cache): ~4secs

annakaz reviewed Dec 3, 2019

View reviewed changes

packages/blockchain-api/src/currencyConversion/CurrencyConversionAPI.ts Show resolved Hide resolved

packages/blockchain-api/src/currencyConversion/CurrencyConversionAPI.ts Show resolved Hide resolved

jeanregisser added 2 commits December 4, 2019 12:00

Fix lint errors

536356e

Fix test when no service account key is present

4ba55e3

jeanregisser mentioned this pull request Dec 5, 2019

[Wallet] Compatibility with exchange rate in string format #2060

Merged

jeanregisser added a commit that referenced this pull request Dec 9, 2019

Mock contractkit to speed up jest tests

ed845cc

Similar gains as seen in #2005

jeanregisser mentioned this pull request Dec 9, 2019

[NotificationService] Change exchange rate stored format (group by pair) #2132

Merged

jeanregisser added 7 commits December 9, 2019 17:16

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

a43bbe5

…hain-api-convert-gold

Compatibility with the new stored exchange rate format

a304396

Set FIREBASE_DB for all envs

301317d

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

40bfc60

…hain-api-convert-gold

Ignore test files to fix deploy

e056ab8

deploy runs yarn from packages/blockchain-api so ends up with slightly different node_modules

Use service account key for all envs until we can migrate to celo-org…

e78a8c6

…-mobile

Add encrypted service account key

941dfe2

This is so blockchain-api in celo-testnet project can access the firebase db in celo-org-mobile project This is temporary until we migrate blockchain-api to celo-org-mobile too.

Improve comments

b3ea316

jeanregisser marked this pull request as ready for review December 10, 2019 14:58

jeanregisser requested review from cmcewen and jmrossy as code owners December 10, 2019 14:58

annakaz approved these changes Dec 10, 2019

View reviewed changes

jmrossy reviewed Dec 11, 2019

View reviewed changes

jeanregisser added 4 commits December 11, 2019 11:53

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

cf0d86c

…hain-api-convert-gold

Use same firebase-admin version as the other packages

2082fd4

Also log error when admin creds fail to load

b4c3341

Remove module index for now

5630c37

jmrossy approved these changes Dec 11, 2019

View reviewed changes

jeanregisser added the automerge Have PR merge automatically when checks pass label Dec 11, 2019

jeanregisser added 2 commits December 12, 2019 10:20

Merge branch 'master' into jeanregisser/blockchain-api-convert-gold

475687b

Merge branch 'master' into jeanregisser/blockchain-api-convert-gold

a7abbe6

celo-ci-bot-user merged commit 5b84b23 into master Dec 12, 2019

celo-ci-bot-user deleted the jeanregisser/blockchain-api-convert-gold branch December 12, 2019 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BlockchainApi] Add ability to get exchange rates from/to cGLD or cUSD #2005

[BlockchainApi] Add ability to get exchange rates from/to cGLD or cUSD #2005

jeanregisser commented Dec 3, 2019 •

edited

Loading

codecov bot commented Dec 10, 2019 •

edited

Loading

annakaz left a comment

annakaz Dec 10, 2019

jeanregisser Dec 11, 2019

annakaz Dec 10, 2019

jeanregisser Dec 11, 2019

annakaz Dec 10, 2019

jeanregisser Dec 11, 2019 •

edited

Loading

annakaz Dec 10, 2019

jeanregisser Dec 11, 2019 •

edited

Loading

annakaz Dec 10, 2019

jmrossy left a comment

jmrossy Dec 11, 2019

jeanregisser Dec 11, 2019

jmrossy Dec 11, 2019

jeanregisser Dec 11, 2019

jmrossy Dec 11, 2019

jeanregisser Dec 11, 2019 •

edited

Loading

		@@ -0,0 +1 @@
		export { default as CurrencyConversionAPI } from './CurrencyConversionAPI'

[BlockchainApi] Add ability to get exchange rates from/to cGLD or cUSD #2005

[BlockchainApi] Add ability to get exchange rates from/to cGLD or cUSD #2005

Conversation

jeanregisser commented Dec 3, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

codecov bot commented Dec 10, 2019 • edited Loading

Codecov Report

annakaz 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

jeanregisser Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Dec 11, 2019 • edited Loading

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

Choose a reason for hiding this comment

jeanregisser Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

jeanregisser commented Dec 3, 2019 •

edited

Loading

codecov bot commented Dec 10, 2019 •

edited

Loading

jeanregisser Dec 11, 2019 •

edited

Loading

jeanregisser Dec 11, 2019 •

edited

Loading

jeanregisser Dec 11, 2019 •

edited

Loading