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

Use contractkit in notification service #1118

Merged
merged 22 commits into from
Sep 30, 2019
Merged

Conversation

annakaz
Copy link
Contributor

@annakaz annakaz commented Sep 26, 2019

Description

Use contractkit in notification service to query the exchange rate. This replaces walletkit is being deprecated

Tested

Locally with yarn start:local

Issues

Addresses #225

Other changes

Use contractkit to get token addresses instead of hardcoding.
Update to readme with instructions to get the IP address of a web3 provider

@cmcewen cmcewen added the automerge Have PR merge automatically when checks pass label Sep 27, 2019
@cmcewen cmcewen removed the automerge Have PR merge automatically when checks pass label Sep 27, 2019
@codecov
Copy link

codecov bot commented Sep 27, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1118   +/-   ##
=======================================
  Coverage   66.59%   66.59%           
=======================================
  Files         257      257           
  Lines        7394     7394           
  Branches      430      430           
=======================================
  Hits         4924     4924           
  Misses       2375     2375           
  Partials       95       95
Flag Coverage Δ
#mobile 66.59% <ø> (ø) ⬆️

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 ecc3d1b...523eadd. Read the comment docs.

@cmcewen
Copy link
Contributor

cmcewen commented Sep 27, 2019

@annakaz @jmrossy had to make some changes and publish some things to get this working - please take a look

Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below.
Another possible issue, why does this add a yarn.lock in the notification service package?

service: integration
instance_class: F4
automatic_scaling:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this when I first created the service and found that automatic scaling would always eventually kill the service since it wasn't receiving any inbound network requests. Seems like app engine is kind of hard wired to run web servers and not well suited to other kinds of apps...
It's possible this issue has since been fixed but please test that before changing this.
Also worth it to double check that we have alerting for when the service goes down

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the version that I deployed has been running for a while - do you remember when it would get killed? https://console.cloud.google.com/appengine/instances?project=celo-org-mobile&instancesTablesize=20&versionId=20190926t191656

It also looks like we have an uptime check https://app.google.stackdriver.com/uptime/efe3d6cc3966ae961e398dd5b73d2094?project=celo-testnet

Does this seem okay to you then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use automatic scaling if min and max instances are both set to 1?

@jmrossy jmrossy removed their assignment Sep 27, 2019
@cmcewen
Copy link
Contributor

cmcewen commented Sep 27, 2019

@jmrossy the yarn.lock is actually a symlink to the root one. @nityas figured out to add it to the website which we have been using for a while - it means we actually pin deps to the right version 😄

@annakaz annakaz removed their assignment Sep 27, 2019
@annakaz
Copy link
Contributor Author

annakaz commented Sep 27, 2019

Can't approve through github since it's my own PR

Approving (assuming we test that app engine does not kill the service after deploying this)

@annakaz
Copy link
Contributor Author

annakaz commented Sep 27, 2019

This may have caused the stackdriver alert with logs disappearing

If we no longer expect logs on standard env, can you update the alert policy here?
https://app.google.stackdriver.com/policies/edit/14455258600176610656?project=celo-testnet

@jmrossy jmrossy dismissed their stale review September 30, 2019 17:18

Service handed off

@annakaz annakaz changed the title Use contractkit instead of walletkit for exchange querying Use contractkit in notification service Sep 30, 2019
@annakaz annakaz added the automerge Have PR merge automatically when checks pass label Sep 30, 2019
@annakaz annakaz assigned annakaz and unassigned cmcewen Sep 30, 2019
@cmcewen cmcewen merged commit d36c24f into master Sep 30, 2019
@cmcewen cmcewen deleted the annakaz/to-contractkit branch September 30, 2019 22:10
lodash "^4.17.14"
web3-utils "1.0.0-beta.37"

"@celo/utils@^0.0.6-beta5":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmcewen @annakaz
It's merged, but check this. yarn.lock now has 2 version of contractkit and utils... when should be none.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very strange

aaronmgdr added a commit that referenced this pull request Oct 1, 2019
* master: (31 commits)
  Upgrade to Node 10 (#1148)
  [faucet] Add custom metrics (#1143)
  Add IdentityMetadata to Contractkit (#1126)
  [Wallet] Local currency v1.1 (#1137)
  Add attestation-service deploy (#1128)
  [Wallet] A few docs and build cleanup (#1138)
  [CircleCI]Add comment on how to fix lint checks (#1134)
  2019-09-30 integration deployment (#1149)
  Update web3 provider to new integration url (#1151)
  [celotool]Add fast mode to celotool invite (#1135)
  Revert "Feature/909 proxy delegatecall" (#1146)
  Use contractkit in notification service (#1118)
  Feature/909 proxy delegatecall (#1003)
  integration deployment for 2019-09-29 (#1139)
  Add instructions for npm publication to tag commit (#1117)
  Client Logs Data Flow script update (#1055)
  Deploying latest proxy code in genesis (#1122)
  Enable floating promises check everywhere (fix issues) (#1115)
  [cli] Solution for build error contractkit on Linux 19.04 distro (#960)
  [Wallet] Merge back changes made for mx pilot (#1113)
  ...

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Oct 1, 2019
* master: (35 commits)
  [Wallet] Network fee in transaction feed (#1145)
  New About Page Cover (#905)
  Upgrade to Node 10 (#1148)
  [faucet] Add custom metrics (#1143)
  Add IdentityMetadata to Contractkit (#1126)
  [Wallet] Local currency v1.1 (#1137)
  Add attestation-service deploy (#1128)
  [Wallet] A few docs and build cleanup (#1138)
  [CircleCI]Add comment on how to fix lint checks (#1134)
  2019-09-30 integration deployment (#1149)
  Update web3 provider to new integration url (#1151)
  [celotool]Add fast mode to celotool invite (#1135)
  Revert "Feature/909 proxy delegatecall" (#1146)
  Use contractkit in notification service (#1118)
  Feature/909 proxy delegatecall (#1003)
  integration deployment for 2019-09-29 (#1139)
  Add instructions for npm publication to tag commit (#1117)
  Client Logs Data Flow script update (#1055)
  Deploying latest proxy code in genesis (#1122)
  Enable floating promises check everywhere (fix issues) (#1115)
  ...

# Conflicts:
#	packages/web/src/about/About.tsx
#	packages/web/src/about/images/index.ts
#	packages/web/static/locales/en/about.json
@annakaz annakaz restored the annakaz/to-contractkit branch October 16, 2019 18:34
@annakaz annakaz deleted the annakaz/to-contractkit branch October 16, 2019 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants