Query and record exchange rate using notification service #1020

annakaz · 2019-09-17T00:38:15Z

Description

To allow for a time series of the gold/dollar exchange rate, query the Exchange contract exchange rate using the notification service polling every 30 minutes, and store the result in Firebas. This allows developers to build like showing a time series of the gold/dollar exchange rate to app users.

The view calls are made by a web3 instance that uses a transaction node as a provider.

Tested

Tested locally with yarn start:local.

Not yet deployed to GCP, so storage in firebase has not been tested.

Other changes

Updated readme deploy script.

Related issues

Fixes Notification service SBAT make view calls to contracts #924

Backwards compatibility

Yes, this builds on top of notification service but does not modify what is already there.

…o-org/celo-monorepo into annakaz/ex-rate-notification-service

codecov · 2019-09-17T01:00:13Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1020   +/-   ##
=======================================
  Coverage   66.45%   66.45%           
=======================================
  Files         256      256           
  Lines        7349     7349           
  Branches      424      424           
=======================================
  Hits         4884     4884           
  Misses       2377     2377           
  Partials       88       88

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

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 bd4657d...702df1b. Read the comment docs.

jmrossy

Nice first pass! A few comments below :)

jmrossy · 2019-09-17T12:33:15Z

packages/notification-service/package.json

@@ -19,6 +19,8 @@
    "deploy": "./deploy.sh"
  },
  "dependencies": {
+    "@celo/utils": "^0.0.6-beta5",
+    "@celo/walletkit": "^0.0.14",


Seems like we shouldn't add a dependency on walletkit here given that we want to deprecate that in favor of contract kit anyway. Can you change this?

This is to be consistent with the mobile app, which uses walletkit: https://github.com/celo-org/celo-monorepo/blob/master/packages/mobile/src/exchange/actions.ts#L6
contractkit doesn't have the same getExchangeRate() function, so using contractkit would mean pasting over the code in walletkit.

I created an issue to migrate this and the mobile app away from walletkit after the exchange (or another util where this and the mobile app can share logic) is implemented in contractkit. #1026

Updated ContractKit with the needed functionality
#1083

packages/notification-service/package.json

jmrossy · 2019-09-17T12:35:26Z

packages/notification-service/src/config.ts

@@ -36,6 +36,10 @@ export const DEFAULT_LOCALE = process.env.DEFAULT_LOCALE
 export const POLLING_INTERVAL = Number(process.env.POLLING_INTERVAL) || 1000
 export const NOTIFICATIONS_TTL_MS = Number(process.env.NOTIFICATION_TTL_MS) || 3600 * 1000 * 24 * 7 // 1 week in milliseconds

+export const EXCHANGE_POLLING_INTERVAL =
+  Number(process.env.EXCHANGE_POLLING_INTERVAL) || 30 * 60 * 1000 // 30 minutes in milliseconds
+export const WEB3_PROVIDER_URL = process.env.WEB3_PROVIDER_URL || 'http://34.83.137.48:8545' // Default to integration transaction node


Nit: this ip seems likely to change as integration churns right? I don't feel strongly but I'd prefer to have it just error if it can find a web3 provider value. Or, even better, have it skip exchange rate polling.

Good idea. Updated to skip exchange polling if it can't find a web3 provider value

jmrossy · 2019-09-17T12:36:14Z

packages/notification-service/src/exchange/exchangeQuery.ts

+let web3: Web3
+export function getWeb3Instance(): Web3 {
+  if (web3) {
+    if (web3.eth.net.isListening()) {


And if not?

jmrossy · 2019-09-17T12:38:50Z

packages/notification-service/src/polling.ts

+
+export const exchangePolling = AsyncPolling(async (end) => {
+  try {
+    const web3 = await getWeb3Instance()


Nit, any reason to get web3 here given that it's in the same file as handleExchangeQuery?

jmrossy · 2019-09-17T12:39:56Z

packages/notification-service/src/polling.ts

@@ -13,3 +14,14 @@ export const notificationPolling = AsyncPolling(async (end) => {
    end()
  }
 }, POLLING_INTERVAL)
+
+export const exchangePolling = AsyncPolling(async (end) => {


I'm slightly concerned about the impact of having two things polling at the same time given that JS is single threaded, though admittedly this one doesn't run often. If you can confirm that it doesn't seem to interfere with the notification polling then I think we're good

From running locally, this doesn't seem to interfere, but I'm going to test deploying this to integration to confirm

jmrossy · 2019-09-17T12:41:23Z

packages/notification-service/test/exchange.test.ts

+  let dollarMakerRate: number
+  let goldMakerRate: number
+  it('should fetch a working web3 instance', async () => {
+    web3 = await getWeb3Instance()


Are you mocking web3 for this? if not, I recommend we do.
If you need a semi-realistic web3 mock, you can use the one in mobile/__mocks__

After moving the getExchangeRate function to ContractKit, these tests don't improve coverage, as they are directly testing a function in ContractKit. So I removed this

…o-org/celo-monorepo into annakaz/ex-rate-notification-service

annakaz · 2019-09-24T17:51:43Z

Note that since integration is having issues with view functions (running into same Error: Returned values aren't valid, did it run Out of Gas? as in wallet), this was tested with an alfajores web3 provider in the local config, then running yarn start:local.

annakaz · 2019-09-24T17:52:27Z

Added instructions to update WEB3_PROVIDER_URL to the Celo Operations Runbook.

annakaz · 2019-09-24T18:07:28Z

Note that the build is failing, but this is happening on master right now and will be fixed by #1088

* master: (61 commits) Remove locales as website is now just in English (#1050) Add MetadataURL to account struct (#1103) Allow validators to use any valid combination of gold commitments as stake (#885) Fix blockscout websocket jsonrpc url (#1096) [Wallet] Preliminary iOS support (#1098) [Wallet] Set security fee description translation in Spanish (#1097) Exclude generated in vscode file watcher setting (#1082) Update .env and .env.integration files (#1087) Allow a testnet to run without ethstats (#1085) Collect exchange rate time series using notification service (#1020) Return to preview view when Fee Education is closed (#1068) [Wallet] Pin Setup Flow v2 (#1054) Added a variable for electoral threshold (#1023) [celotool]Store .env config on GCS after deployment (#1086) Group size limit (#1035) Fix governance unit tests (#1084) Add getExchangeRate to ContractKit (#1083) [CLI]Unlock till the geth exits (#1070) Add Quorum and Refactor Governance (#430) Shuffle elected validators using block randomness (#1033) ...

annakaz added 19 commits September 12, 2019 16:37

Add some todos

ec0fd6c

Merge branch 'master' into annakaz/ex-rate-notification-service

091e92e

Update config file, create function signatures

b02a819

Add makeExhangeQuery and getWeb3Instance

86bcbd0

Add exchangeRatePair object

c18fd14

Add call to exchangePolling

c21bf0d

Create web3 instance every time polled

44256d4

Update web3 handling and add exchangeRate to firebase

3923663

Write record to firebase

6400495

Add to package.json

d641579

Update to integration provider url for testing

a5bb2f2

Get local test working

4c6d22c

Add web3 provider as a param

d757f91

Remove unused interface

1c45663

Cleanup

8bf979a

Add comments, remove unneeded BigNumber type signature

9ef79fd

Refactor functions to make unit testing possible

65fdb91

Add exchange tests

ddedead

Make exchange queries in parallel

e7d519f

annakaz requested review from cmcewen and jmrossy as code owners September 17, 2019 00:38

annakaz assigned jmrossy Sep 17, 2019

annakaz and others added 2 commits September 16, 2019 17:38

Merge branch 'master' into annakaz/ex-rate-notification-service

80a339c

Merge branch 'annakaz/ex-rate-notification-service' of github.com:cel…

c8eb924

…o-org/celo-monorepo into annakaz/ex-rate-notification-service

jmrossy reviewed Sep 17, 2019

View reviewed changes

jmrossy assigned annakaz and unassigned jmrossy Sep 17, 2019

annakaz and others added 2 commits September 17, 2019 10:28

Merge branch 'master' into annakaz/ex-rate-notification-service

93eea21

Make changes based on review

15c1df0

Merge branch 'annakaz/ex-rate-notification-service' of github.com:cel…

70ba895

…o-org/celo-monorepo into annakaz/ex-rate-notification-service

jeanregisser mentioned this pull request Sep 18, 2019

Rename notification-service to better represent its new responsibilities #1044

Closed

annakaz added 4 commits September 23, 2019 13:45

Merge branch 'master' into annakaz/ex-rate-notification-service

a9c62fc

Update nested ifs and missing web3 provider behavior

4bddc02

Remove exchange tests

2ae3fb0

Add comment about contractkit

ce4bedc

annakaz assigned jmrossy and unassigned annakaz Sep 23, 2019

ashishb closed this Sep 24, 2019

ashishb deleted the annakaz/ex-rate-notification-service branch September 24, 2019 01:21

ashishb restored the annakaz/ex-rate-notification-service branch September 24, 2019 02:15

ashishb reopened this Sep 24, 2019

jmrossy approved these changes Sep 24, 2019

View reviewed changes

annakaz and others added 3 commits September 24, 2019 09:56

Merge branch 'master' into annakaz/ex-rate-notification-service

e4eebc7

Add web3 provider urls for integration and alfajores

211c973

Merge branch 'annakaz/ex-rate-notification-service' of github.com:cel…

702df1b

…o-org/celo-monorepo into annakaz/ex-rate-notification-service

annakaz merged commit 8f2fb7f into master Sep 24, 2019

annakaz deleted the annakaz/ex-rate-notification-service branch October 15, 2019 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query and record exchange rate using notification service #1020

Query and record exchange rate using notification service #1020

annakaz commented Sep 17, 2019 •

edited

Loading

codecov bot commented Sep 17, 2019 •

edited

Loading

jmrossy left a comment

jmrossy Sep 17, 2019

annakaz Sep 17, 2019 •

edited

Loading

annakaz Sep 23, 2019

jmrossy Sep 17, 2019

annakaz Sep 17, 2019

jmrossy Sep 17, 2019

annakaz Sep 23, 2019

jmrossy Sep 17, 2019

annakaz Sep 17, 2019

jmrossy Sep 17, 2019

annakaz Sep 17, 2019

jmrossy Sep 17, 2019

annakaz Sep 23, 2019

annakaz commented Sep 24, 2019

annakaz commented Sep 24, 2019

annakaz commented Sep 24, 2019

Query and record exchange rate using notification service #1020

Query and record exchange rate using notification service #1020

Conversation

annakaz commented Sep 17, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

codecov bot commented Sep 17, 2019 • edited Loading

Codecov Report

jmrossy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakaz Sep 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

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

annakaz commented Sep 24, 2019

annakaz commented Sep 24, 2019

annakaz commented Sep 24, 2019

annakaz commented Sep 17, 2019 •

edited

Loading

codecov bot commented Sep 17, 2019 •

edited

Loading

annakaz Sep 17, 2019 •

edited

Loading