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

Support blacklisting country codes for the attestation service #1738

Merged
merged 3 commits into from
Nov 17, 2019

Conversation

nambrot
Copy link
Contributor

@nambrot nambrot commented Nov 15, 2019

Description

The primary objective of this PR is to support #1446, to allow validators to blacklist locales they do not want to serve, primarily for cost reasons if some locales are too prohibitive for them to serve.

Tested

  • Against namoffchainreveal

Other changes

Related issues

Backwards compatibility

  • It is not backwards compatible with deployed attestation services, but since those are non-existent this should be fine

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1738    +/-   ##
========================================
  Coverage   74.26%   74.26%            
========================================
  Files         277      277            
  Lines        7617     7617            
  Branches      953      669   -284     
========================================
  Hits         5657     5657            
  Misses       1845     1845            
  Partials      115      115
Flag Coverage Δ
#mobile 74.26% <ø> (ø) ⬆️

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 598e2cb...2ac611d. Read the comment docs.

packages/attestation-service/README.md Outdated Show resolved Hide resolved

- `DATABASE_URL` - The URL under which your database is accessible, currently supported are `postgres://`, `mysql://` and `sqlite://`
- `CELO_PROVIDER` - The URL under which a celo blockchain node is reachable, i.e. something like `https://integration-forno.celo-testnet.org`
- `ACCOUNT_ADDRESS` - The address of the account on the `Accounts` smart contract
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get this from contract kit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see how the phrasing is confusing. It's not the address of the Accounts smart contract, but the account address of the validator account on the Accounts smart contract. Not sure what a better name/description would be

packages/attestation-service/README.md Outdated Show resolved Hide resolved
packages/attestation-service/src/attestation.ts Outdated Show resolved Hide resolved
packages/attestation-service/src/db.ts Show resolved Hide resolved
packages/attestation-service/src/sms/nexmo.ts Show resolved Hide resolved
packages/utils/src/address.ts Outdated Show resolved Hide resolved
@jmrossy jmrossy assigned nambrot and unassigned jmrossy Nov 15, 2019
@nambrot nambrot assigned jmrossy and unassigned nambrot Nov 15, 2019
@nambrot nambrot added the automerge Have PR merge automatically when checks pass label Nov 17, 2019
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.

Validators SBAT whitelist/blacklist locales by text provider they dont want to serve
3 participants