feat: register, change and verify email addresses #148

schmanu · 2024-02-19T15:26:28Z

New Endpoints

register email address for signer
change email address for signer
re-send verification code
verify email address

Other changes

Some of the new endpoints require params in header fields. Therefore this PR adds header params to the api operations.

Links

Spec

github-actions · 2024-02-19T15:27:19Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

src/index.ts

usame-algan · 2024-02-20T10:28:32Z

src/types/emails.ts

+
+export type RegisterEmailRequestHeader = {
+  ['Safe-Wallet-Signature']: string
+  ['Safe-Wallet-Signature-Timestamp']: string


In the API spec, this is of type number

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()

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

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

usame-algan

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.

fmrsabino · 2024-02-21T13:10:58Z

src/index.ts

+/**
+ * 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
+ */


Since it's mentioned in the other authenticated endpoints. This one also requires a signature: email-retrieval-{chainId}-{safe}-{signer}-{timestamp}

fmrsabino · 2024-02-21T13:12:24Z

src/index.ts

+/**
+ * Delete a registered email address for the signer
+ *
+ * @param chainId
+ * @param safeAddress
+ * @param signerAddress
+ * @param headers
+ */


Same as my comment above. Signature would be email-delete-{chainId}-{safe}-{signer}-{timestamp}

fmrsabino · 2024-02-21T13:14:24Z

src/types/api.ts


+export interface PutEndpoint extends Endpoint {
+  put: {
+    parameters: PostParams | null


What does PostParams represent under the PutEndpoint?

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

fmrsabino · 2024-02-21T13:17:31Z

src/types/api.ts

+      headers: AuthorizationEmailRequestHeader
+    }
+    responses: {
+      200: {


Here it should be 201.

fmrsabino · 2024-02-21T13:21:20Z

src/types/api.ts

+    responses: {
+      200: {
+        schema: void
+      }
+      202: {
+        schema: void
+      }
+    }


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.

fmrsabino · 2024-02-21T13:24:59Z

src/types/api.ts

+    responses: {
+      202: {
+        schema: void
+      }
+      200: {
+        schema: void
+      }
+      429: unknown
+      409: unknown
+    }


FYI: After safe-global/safe-client-gateway#1181 is merged, only 202 will be returned.

fmrsabino · 2024-02-21T13:25:46Z

src/types/api.ts

+      200: {
+        schema: void
+      }


This status code is not expected to be returned on this endpoint.

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.

fmrsabino · 2024-02-21T13:28:11Z

src/types/emails.ts

+
+export type RegisterEmailRequestHeader = {
+  ['Safe-Wallet-Signature']: string
+  ['Safe-Wallet-Signature-Timestamp']: string


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

schmanu added 2 commits February 19, 2024 16:23

feat: register, change and verify email addresses

046298c

chore: bump version

9bfbf4d

schmanu requested a review from fmrsabino February 19, 2024 15:27

schmanu added 2 commits February 19, 2024 16:46

fix: use PUT method, fix tests

89429d1

tests: fix typo in test expectation

f421aa0

schmanu requested a review from iamacook February 19, 2024 15:58

usame-algan reviewed Feb 20, 2024

View reviewed changes

src/index.ts Show resolved Hide resolved

usame-algan reviewed Feb 20, 2024

View reviewed changes

feat: getRegisteredEmail endpoint

000faa2

usame-algan reviewed Feb 20, 2024

View reviewed changes

feat: Add deleteRegisteredEmail endpoint

b465f2f

usame-algan approved these changes Feb 21, 2024

View reviewed changes

fmrsabino reviewed Feb 21, 2024

View reviewed changes

usame-algan added 2 commits February 21, 2024 16:39

fix: Update return status code, add signed message format to endpoints

be5fb7d

chore: Bump version to v3.17.0

5b8e3cf

usame-algan merged commit e7f7d64 into main Feb 22, 2024

usame-algan deleted the emails branch February 22, 2024 08:28

usame-algan mentioned this pull request Feb 22, 2024

fix: Pass headers for GET and DELETE requests #149

Merged

Uh oh!

feat: register, change and verify email addresses #148

feat: register, change and verify email addresses #148

Uh oh!

Conversation

schmanu commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Endpoints

Other changes

Links

Uh oh!

github-actions bot commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usame-algan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

schmanu commented Feb 19, 2024 •

edited

Loading

github-actions bot commented Feb 19, 2024 •

edited

Loading