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

[Attestation Service] get_attestations endpoint, HA support for delivery receipts #5011

Merged
merged 48 commits into from
Oct 1, 2020

Conversation

timmoreton
Copy link
Contributor

@timmoreton timmoreton commented Sep 8, 2020

Description

All delivery state is now persisted in the database. Supports HA setups with multiple Attestation Service instances.

Deliveries can be re-requested, regardless of their delivery status. It kicks off the next attempt, assuming retries are available, with the next provider.

Test attestation endpoint now accepts provider parameter to force provider to test:

tim@Tims-MacBook-Pro attestation-service % curl -H "Content-Type: application/json" -d '{ "phoneNumber":"+1415xxxxxxx","message":"hello","signature":"", "provider":"nexmo"}' localhost:3000/test_attestations

{"success":true,"identifier":"0xfc992dad242cca0dd8607aad3cab6ff4e2f9e47b0cd5c34df1ca19d7d641a373","account":"0xxxx","issuer":"0xxxx","attempt":0,"countryCode":"US","status":"Sent","salt":"9f68a7c35c9d2b04cbb39a714096d66c927132edb72f4be93dd92b919c8db47a","provider":"nexmo"}

It also randomly generates a "salt" so that tests can be stored alongside regular attestations in the database, so they can share more code and be accessible via get_attestations.

New endpoint /get_attestations to retrieve delivery info:

tim@Tims-MacBook-Pro attestation-service % curl -i -H "Content-Type: application/json"  'localhost:3000/get_attestations?account=0xxxx&issuer=0xxxx&phoneNumber=%2B1415xxxxxxx&salt=9f68a7c35c9d2b04cbb39a714096d66c927132edb72f4be93dd92b919c8db47a'
HTTP/1.1 200 OK
X-Powered-By: Express
X-Request-Id: ba5f9a0c-dc47-40dd-a6b9-df8c2b31caeb
X-RateLimit-Limit: 50
X-RateLimit-Remaining: 48
Date: Tue, 08 Sep 2020 23:01:13 GMT
X-RateLimit-Reset: 1599606076
Content-Type: application/json; charset=utf-8
Content-Length: 278
ETag: W/"116-lQIpekV90rYPlJEkNfRYo95+NcA"
Connection: keep-alive

{"success":true,"identifier":"0xfc992dad242cca0dd8607aad3cab6ff4e2f9e47b0cd5c34df1ca19d7d641a373","account":"0xxxx","issuer":"0xxxx","attempt":0,"countryCode":"US","status":"Delivered","provider":"nexmo","duration":2274}

Delivery info includes time taken to final delivery success or failure, in ms.

The celocli identity:test-attestation-service command now also uses the /get_attestations endpoint to await a delivery receipt, so that validators have another way of testing that receipts are configured correctly.

Other changes

  • new metric attestation_service_healthy for overall service health
  • If an attestation is not found on-chain, the request retries up to 10 secs to avoid the case where the full node is running behind a block or two (not if the attestation is found on-chain but is complete)
  • Parameter DB_RECORD_EXPIRY_MINS to set db record expiry, and now done on a timer, not when attestations happen
  • Fix default config's setting of MAX_DELIVERY_ATTEMPTS. Delivery attempts now striped across providers, i.e try other provider next before retry with same provider
  • Fix broken metrics attestation_requests_believed_delivered_sms and attestation_requests_failed_to_deliver_sms
  • Setting NODE_ENV=dev allows signature and other checks to be skipped (do not run in production)
  • mention TheCelo in docs

Tested

Baklava validator.

Related issues

  • Fixes #[issue number here]

Backwards compatibility

Brief explanation of why these changes are/are not backwards compatible.

Copy link
Contributor

@aslawson aslawson left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Some thoughts above, but nothing urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants