CIP1: Attestation service #1078
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1078 +/- ##
=======================================
Coverage 66.45% 66.45%
=======================================
Files 256 256
Lines 7367 7367
Branches 427 491 +64
=======================================
Hits 4896 4896
+ Misses 2381 2379 -2
- Partials 90 92 +2
Continue to review full report at Codecov.
|
| "build": "tsc -b .", | ||
| "clean": "tsc -b . --clean", | ||
| "clean:all": "yarn clean && rm -rf src/generated", | ||
| "build:gen": "yarn --cwd ../protocol build", |
There was a problem hiding this comment.
Is this command relevant here?
|
|
||
| const phoneUtil = PhoneNumberUtil.getInstance() | ||
|
|
||
| let nexmoClient: any |
There was a problem hiding this comment.
The library unfortunately does not have any types
There was a problem hiding this comment.
You can define you own but not important for now
| } | ||
|
|
||
| export async function handleAttestationRequest(req: express.Request, res: express.Response) { | ||
| // TODO: Should parse request appropriately |
There was a problem hiding this comment.
Suggest adding error handling at this level
There was a problem hiding this comment.
Agreed, and have a separate ticket for it #1075
There was a problem hiding this comment.
It's not just about malformed requests, if you code throws an exception somewhere (like when sending with Nexmo), that will propagate all the way up and the 500 we return will leak details about the internal error
There was a problem hiding this comment.
My goal would it be to handle this more globally with that ticket. You are right that the ticket did not specify that, I edited the ticket
|
|
||
| init().catch((err) => { | ||
| console.error(`Error occurred while running server, exiting ....`) | ||
| console.error(err) |
There was a problem hiding this comment.
Nit, err can beg the 2nd param of the previous console.error call
There was a problem hiding this comment.
I personally like the error on a separate line, do you mind keeping it this way?
| export async function sendSms(phoneNumber: string, message: string) { | ||
| const countryCode = phoneUtil.getRegionCodeForNumber(phoneUtil.parse(phoneNumber)) | ||
|
|
||
| if (countryCode === undefined) { |
There was a problem hiding this comment.
Consider if (!countryCode)
There was a problem hiding this comment.
Changed it, though I'm generally a fan of comparing to undefined as ! will also match the '' 0, etc.
There was a problem hiding this comment.
right, that's really the benefit though. If getRegionCodeForNumber returns null instead of undefined, that's now covered
Description
This PR adds the minimum possible version for CIP1 celo-org/celo-proposals#1
It currently does the following:
Outstanding work that we'll want prior to betanet
Outstanding work that we should do eventually but does not need to happen right now:
Tested
Related issues