Safe integration for profile updates #201

pheuberger · 2024-12-05T18:23:10Z

This PR adds support for using a Safe to make profile updates (display name and avatar image).

When the user sends a POST request to /v1/users/<safe-address> with the correct payload for Safe signature flows the user controller will write a signature request to the signature_requests table.

The signers now need to asynchronously complete the signatures in the Safe app. When the threshold is met, a POST to /v1/signature-requests/process will now queue up all pending signature requests and verify the signatures behind the scenes. The request is non-blocking as it might take a long time to complete.

Each signature request will now compute the message hash for the data the user initially submitted, get the signatures for the specific message from the Safe API and validate them on-chain.

If the validation completes successfully, the signature request will be marked executed and the requested profile change will be written to the database.
If the validation fails, the signature request will stay in status pending and retried when the endpoint is called again.

This PR also exposes signature requests via GraphQL and attaches all profile change signature requests to a user entity.

Example query:

query UserQuery {
  signatureRequests(
    where: {status: {eq: PENDING}, purpose: {eq: UPDATE_USER_DATA}}
  ) {
    data {
      safe_address
      message_hash
      message
      status
    }
  }
  users {
    data {
      display_name
      signature_requests {
        message
        status
      }
    }
  }
}

I couldn't write tests for SignatureRequestProcessor due to type-graphql errors that pop up on tests. We need to fix those first and then I can ship tests for this and the controller classes as a fast-follow.

The user controller had a lot of repetetive code. This patch dry's this code up quite a bit and makes the high-level intent more apparent.

src/lib/safe-signature-verification/SafeSignatureVerifier.ts

src/lib/users/EOAUpsertStrategy.ts

Jipperism · 2024-12-05T19:56:24Z

src/lib/users/UserUpsertStrategy.ts

+  address: string,
+  request: MultisigUpsertRequest | EOAUpsertRequest,
+): UserUpsertStrategy {
+  if ("signature" in request) {


It's probably fine here, as there are not many other types of UpsertRequest that I can think of that might be added in the future. But generally in cases like this I'd lean towards a discriminated union pattern to distinguish between the different options. Example here in case you're not familiar.

Yep, great idea, Jip! I like them too but somehow I didn't think of using them here. Another upside is that it makes the request type more explicit for the caller.

src/services/SupabaseDataService.ts.orig

Jipperism · 2024-12-05T20:08:52Z

src/utils/getRpcUrl.ts

@@ -119,7 +119,6 @@ const fallBackProvider = (chainId: number) => {
 };

 /* Returns a PublicClient instance for the configured network. */
-// @ts-expect-error viem typings


I noticed in the commit messages that this is supposed to not throw an error. It might have been that it was only doing that in Intellij editors. It might also be that this got fixed while upgrading typescript. The original message was about the nested types being too many levels deep, iirc.

I removed it and my editor is fine and the build is working as expected. If you're still using Intellij, can you check if this is okay on your end?

Without this patch the user controller only allows updating the user data directly via EOA. Calling it with a multisig like the Safe would fail as the smart wallet isn't able to create a signature. In this instance the API needs to track signature requests, so that the user data can be written once the signature threshold is met. The need to track these signature requests arises from the fact that you cannot delete signatures from the Safe once they were "used". They will stick around forever and it is up to us to track which signature corresponds to which change, if it is discarded or implemented, etc. The patch introduces a new folder: src/lib/users. The reason is that the project is currently structured by implementation details, so I wanted to keep the user controller in with the rest of the controllers but have a place to store this business logic.

Without this patch we can only write signature requests, but not fetch them. This patch implements the required resolvers and also exposes any signature requests that are registered for a particular user.

Without this patch the user has no way of canceling signature requests. This patch introduces a status field. It has 3 possible values: PENDING, CANCELED and EXECUTED. The controller allows canceling the signature request only when it's in status PENDING and the caller authenticates as one of the owners of the Safe.

Without this patch we're missing a way for the API to execute the signature requests and commit the data to record if the signature threshold is met. This patch adds the request POST /signature-requests/process which reads all **pending** signature requests from the data DB, checks if the confirmations meet the signature threshold and if so writes the attached username and avatar changes to the users table. To stay within the rate limit of 5 requests per second required by the Safe API I implemented a rate limited queue and worker that process them concurrently. The request doesn't require authentication, so it could be invoked by a CRON job, as well as the user when they log in or specifically request an update.

This directive didn't have any effect.

In case somebody is wondering why we have duplicate type definitions in our code base.

pheuberger · 2024-12-06T11:14:48Z

@Jipperism I addressed your feedback. Feel free to leave more comments or close the conversations above 👌

bitbeckers

Congrats on the first PR!

As we discussed, we'll live with two patterns for now and will align during a sync meeting.

Most comments I've made are on logging and type inference/forcing which can be mitigated with helper methods like getAddress.

bitbeckers · 2024-12-09T09:10:26Z

src/graphql/schemas/inputs/signatureRequestInput.ts

+  message_hash?: StringSearchOptions;
+
+  @Field(() => StringSearchOptions, { nullable: true })
+  created_at?: StringSearchOptions;


My preference would be a single format for datetime. We basically stay as close to the on-chain data as possible, which would make this a timestamp in second. The datatime used in the other tables also shouldn't be there. It's terrible DX to need to figure out which timestamp you're getting.

bitbeckers · 2024-12-09T09:14:43Z

src/commands/CommandFactory.ts

+        request.chain_id,
+      );
+    default:
+      return new NoopCommand();


My assumption here would be that a request with an unknown purpose is an error. Is there a reason to not juss throw here?

bitbeckers · 2024-12-09T09:16:18Z

src/commands/UserUpsertCommand.ts

+      safeMessage.message.message,
+    );
+    if (!message.success) {
+      console.log("Unexpected message format", message.error);


console.error? also for Sentry

bitbeckers · 2024-12-09T09:21:58Z

src/commands/UserUpsertCommand.ts

+    const safeMessage = await this.safeApiKit.getMessage(this.messageHash);
+
+    if (!isTypedMessage(safeMessage.message)) {
+      throw new Error("Unexpected message type: not EIP712TypedData");


there's no log here, but there is one a few lines lower. How will the error be caught? Will it be logged there?

bitbeckers · 2024-12-09T09:22:36Z

src/commands/UserUpsertCommand.ts

+
+    const verifier = new UserUpsertSignatureVerifier(
+      Number(signatureRequest.chain_id),
+      this.safeAddress as `0x${string}`,


you can use getAddress from viem here to verify and not force the type

bitbeckers · 2024-12-09T09:43:46Z

src/controllers/UserController.ts

      };
    }
+    console.log("error", error);


console.error, if we need to log the error

bitbeckers · 2024-12-09T09:48:16Z

src/graphql/schemas/resolvers/userResolver.ts

+      },
+    };
+
+    const response = await this.supabaseDataService.getSignatureRequests(args);


As this inherits the BaseResolver, I think the pattern should match the fetching pattern as you see in the hypercertResolver. In other works, the command would be return await this.getSignatureRequests(args)

bitbeckers · 2024-12-09T09:50:49Z

src/graphql/schemas/typeDefs/signatureRequestTypeDefs.ts

+  @Field(() => Int, {
+    description: "The chain ID of the signature request",
+  })
+  chain_id?: number;


chain_id on chain is uint256, which we should match. This implies bigint | string | number, like in the hypercertTypedefs

bitbeckers · 2024-12-09T09:51:15Z

src/graphql/schemas/typeDefs/signatureRequestTypeDefs.ts

+  @Field({
+    description: "When the signature request was created",
+  })
+  created_at?: string;


See earlier comment on harmonising the datatypes, which means we have a seconds timestamp.

bitbeckers · 2024-12-09T09:52:09Z

src/lib/safe-signature-verification/SafeSignatureVerifier.ts

+      throw new Error(`Unsupported chain ID: ${chainId}`);
+    }
+    this.chainId = chainId;
+    this.safeAddress = safeAddress;


verify if address is valid address using getAddress or isAddress. getAddress returns with the appropriate casing for consistency

refactor: dry up user controller

8ab5b70

The user controller had a lot of repetetive code. This patch dry's this code up quite a bit and makes the high-level intent more apparent.

pheuberger requested review from bitbeckers and Jipperism December 5, 2024 18:25

pheuberger force-pushed the safe-integration-profile branch from 2d5d75f to 6e50961 Compare December 5, 2024 18:30

Jipperism reviewed Dec 5, 2024

View reviewed changes

pheuberger force-pushed the safe-integration-profile branch from 6e50961 to 848f8ca Compare December 6, 2024 10:52

pheuberger added 5 commits December 6, 2024 13:06

feat: expose signature requests over GraphQL

cf86566

Without this patch we can only write signature requests, but not fetch them. This patch implements the required resolvers and also exposes any signature requests that are registered for a particular user.

chore: remove unnecessary compiler directive

2e71dc5

This directive didn't have any effect.

chore: add comment about tsoa incompatibility w/ zod

ba00037

In case somebody is wondering why we have duplicate type definitions in our code base.

pheuberger force-pushed the safe-integration-profile branch from 848f8ca to ba00037 Compare December 6, 2024 11:06

bitbeckers requested changes Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe integration for profile updates #201

Safe integration for profile updates #201

pheuberger commented Dec 5, 2024

Jipperism Dec 5, 2024

pheuberger Dec 6, 2024

Jipperism Dec 5, 2024

pheuberger Dec 6, 2024

pheuberger commented Dec 6, 2024

bitbeckers left a comment

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

bitbeckers Dec 9, 2024

Safe integration for profile updates #201

Are you sure you want to change the base?

Safe integration for profile updates #201

Conversation

pheuberger commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pheuberger commented Dec 6, 2024

bitbeckers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment