feat: register, change and verify email addresses #148
Conversation
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
|
||
| export type RegisterEmailRequestHeader = { | ||
| ['Safe-Wallet-Signature']: string | ||
| ['Safe-Wallet-Signature-Timestamp']: string |
There was a problem hiding this comment.
In the API spec, this is of type number
There was a problem hiding this comment.
Headers are always passed as strings, that's why I set the type for headers to always be Record<string ,string> Otherwise we have to map over the values and call toString()
There was a problem hiding this comment.
So in web-core we would send Date.now().toString() as the header parameter. Will this be handled correctly by the controller in CGW? @fmrsabino
There was a problem hiding this comment.
So in web-core we would send
Date.now().toString()as the header parameter. Will this be handled correctly by the controller in CGW? @fmrsabino
This is totally fine. We validate if it's a number and do proper conversion 👍
https://github.com/safe-global/safe-client-gateway/blob/d1d59c40226973bb64f481b0656ce61feefef46d/src/routes/email/guards/timestamp.guard.ts#L16-L18
There was a problem hiding this comment.
Looks good so far!
I suggest to also add a deleteEmail function since we need that on the wallet side as well (API Spec)
For future iterations we will need a way to unsubscribe from notifications too but I would leave it as out-of-scope here.
src/index.ts
Outdated
| /** | ||
| * Gets the registered email address of the signer | ||
| * | ||
| * @param chainId | ||
| * @param safeAddress | ||
| * @param signerAddress address of the owner of the Safe | ||
| * | ||
| * @returns email address and verified flag | ||
| */ |
There was a problem hiding this comment.
Since it's mentioned in the other authenticated endpoints. This one also requires a signature: email-retrieval-{chainId}-{safe}-{signer}-{timestamp}
src/index.ts
Outdated
| /** | ||
| * Delete a registered email address for the signer | ||
| * | ||
| * @param chainId | ||
| * @param safeAddress | ||
| * @param signerAddress | ||
| * @param headers | ||
| */ |
There was a problem hiding this comment.
Same as my comment above. Signature would be email-delete-{chainId}-{safe}-{signer}-{timestamp}
| @@ -70,6 +78,13 @@ export interface PostEndpoint extends Endpoint { | |||
| } | |||
| } | |||
|
|
|||
| export interface PutEndpoint extends Endpoint { | |||
| put: { | |||
| parameters: PostParams | null | |||
There was a problem hiding this comment.
What does PostParams represent under the PutEndpoint?
There was a problem hiding this comment.
I did not duplicate the types here as they are identical.
The Params represent what kind of parameters the endpoints accept:
In both cases
- body params
- header params
- query params
| headers: AuthorizationEmailRequestHeader | ||
| } | ||
| responses: { | ||
| 200: { |
| responses: { | ||
| 200: { | ||
| schema: void | ||
| } | ||
| 202: { | ||
| schema: void | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this only include successful responses? If so then only 202 should be expected. There are client related error codes returned under other conditions though.
src/types/api.ts
Outdated
| responses: { | ||
| 202: { | ||
| schema: void | ||
| } | ||
| 200: { | ||
| schema: void | ||
| } | ||
| 429: unknown | ||
| 409: unknown | ||
| } |
There was a problem hiding this comment.
FYI: After safe-global/safe-client-gateway#1181 is merged, only 202 will be returned.
| 200: { | ||
| schema: void | ||
| } |
There was a problem hiding this comment.
This status code is not expected to be returned on this endpoint.
There was a problem hiding this comment.
Currently our types enforce a 200 response code this. We should look into how to make them more flexible.
Probably the success-return type could be expressed through generics instead of using the 200 code return type.
|
|
||
| export type RegisterEmailRequestHeader = { | ||
| ['Safe-Wallet-Signature']: string | ||
| ['Safe-Wallet-Signature-Timestamp']: string |
There was a problem hiding this comment.
So in web-core we would send
Date.now().toString()as the header parameter. Will this be handled correctly by the controller in CGW? @fmrsabino
This is totally fine. We validate if it's a number and do proper conversion 👍
https://github.com/safe-global/safe-client-gateway/blob/d1d59c40226973bb64f481b0656ce61feefef46d/src/routes/email/guards/timestamp.guard.ts#L16-L18
New Endpoints
Other changes
Links