Smart Backend Wallets #709

d4mr · 2024-10-07T00:34:58Z

PR-Codex overview

This PR introduces enhancements to wallet management, including support for smart wallets, new wallet types, and updated database schemas. It also improves caching mechanisms and adds tests for new features.

Detailed summary

Added entrypointAddress and accountFactoryAddress to wallet schemas.
Introduced smart wallet types: smart:aws-kms, smart:gcp-kms, and smart:local.
Updated database migrations for new fields.
Enhanced wallet creation functions to support smart wallets.
Implemented caching for chain capabilities.
Added tests for smart backend wallets.
Deprecated some existing wallet functions and marked them for removal.

The following files were skipped due to too many changes: src/utils/cache/getWallet.ts, src/db/wallets/getWalletDetails.ts, src/utils/account.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

d4mr · 2024-10-07T00:41:05Z

src/server/utils/wallets/getLocalWallet.ts


+  const json = walletDetails.encryptedJson;
+
  const wallet = await Wallet.fromEncryptedJson(


this can be moved over to using decryptJsonWallet from legacyLocalCrypto. The interface isn't as clunky here though, so we can let it be too.

joaquim-verges · 2024-10-07T02:46:50Z

src/server/utils/wallets/createSmartWallet.ts

+  accountFactoryAddress?: Address;
+}) => {
+  const smartAccount = smartWallet({
+    chain: defineChain(1),


I assume you're only using this to predict the smart wallet address?

The danger here is that it requires the account factory to be there on that chain.

I think for smart accounts it's better to recreate the smart wallet at runtime based on the chain coming in the request.

So you would create a normal backend wallet and just give it a smart flag in the db (which you kinda do already with the factory address) then on every request, compute the smart wallet based on the chain in the request. If factory not available on that chain = error.

I think for smart accounts it's better to recreate the smart wallet at runtime based on the chain coming in the request.

That's the intention here, did I miss something in the implementation?

then on every request, compute the smart wallet based on the chain in the request

This would be confusing. We must ensure smart wallet has same account address on every chain for UX. I assumed this was already always the case, so we are storing the predicted account address.

If factory not available on that chain = error.

Exactly. Right now all the inner interfaces support overriding accountFactoryAddress, but I have not exposed it to the endpoint. We throw an error if:

user provided accountFactory does not have exist

OR user did not provide accountFactoryAddress and thirdweb default factory does not exist

src/db/wallets/createWalletDetails.ts

arcoraven · 2024-10-07T06:24:32Z

src/server/utils/wallets/createAwsKmsWallet.ts

+export const createAndStoreAwsKmsWallet = async ({
  label,
  ...overrides
 }: CreateAwsKmsWalletParams): Promise<string> => {
+  const { awsKmsArn, params } = await createAwsKmsWallet(overrides);
+
+  return importAwsKmsWallet({


I think our verbs are getting confusing. Let's use fewer unique verbs but add details (even if longer var/fn names) for clarity.

create

import

store

👇

createAwsKmsKey

createAwsKmsWallet or createAwsKmsWalletDetails or createAwsKmsWalletToDb

This function can remain createAwsKmsWallet since that makes sense

arcoraven · 2024-10-07T06:27:58Z

src/server/utils/wallets/createGcpKmsWallet.ts

+ * If any required parameter cannot be resolved from either the configuration or the overrides, an error is thrown.
+ */
+export const createGcpKmsWallet = async (
+  partialParams: Partial<GcpKmsWalletParams>,


Keep naming consistent with the above AWS KMS flow (change either of them).

above: input is named params, here it's partialParams

above: the fetched params is named awsKmsParams, here it's params

src/server/utils/wallets/createLocalWallet.ts

arcoraven · 2024-10-07T06:43:14Z

src/server/utils/wallets/createSmartWallet.ts

+import { getAwsKmsAccount } from "./getAwsKmsAccount";
+import { getGcpKmsAccount } from "./getGcpKmsAccount";
+
+const createSmartWallet = async ({


Same comment here on naming. Is this "creating a smart wallet" or simply "getting" one? Is there a clearer verb here?

Agreed this is confusing, I like your generate suggestion. It's still a little unclear because for AWS and GCP, generate would still have side effect (interactions with AWS and GCP).

arcoraven · 2024-10-07T06:46:17Z

src/server/utils/wallets/getLocalWallet.ts

+/**
+ * @deprecated
+ * DEPRECATED: Use getLocalWalletAccount instead
+ */


There's only one place this is called:

// src/utils/cache/getWallet.ts:116 wallet = await getLocalWallet({ chainId, walletAddress });

Could we resolve this wallet from the response from getLocalWalletAccount()?

we can't because there's no way to expose the privateKey from the Account that we get from getLocalWallet. lmk if you think it's worth exposing it separately.

arcoraven · 2024-10-07T06:49:39Z

src/server/utils/wallets/legacyLocalCrypto.ts

+  version: number;
+}
+
+export async function toEncryptedJson({


This seems way too low level to maintain within Engine. Does this exist in thirdweb or underlying SDKs?

this replaces the LocalStorage class implementation that we needed to do, which had dependencies on v4 SDK and ethers. We could alternatively use the ethers code directly, but I wanted to remove all ethers dependencies.

arcoraven · 2024-10-07T06:55:36Z

src/utils/account.ts

+    case WalletType.smartGcpKms:
+    case WalletType.smartLocal: {
+      if (!walletDetails.accountSignerAddress) {
+        throw createCustomError(


Just throw new Error here if you expect a 5xx response anyway. We should minimize status-code specific logic in helper or reused functions since it's a bad pattern.

I saw line 38 so assumed it was okay to throw HTTP error here. Completely agree though, it's a bad pattern

if (!walletDetails) { throw createCustomError( `No configured wallet found with address ${from}`, StatusCodes.BAD_REQUEST, "BAD_REQUEST", ); }

arcoraven · 2024-10-07T06:56:53Z

src/utils/account.ts

+      const adminAccount = await getAccount({
+        chainId,
+        from: getAddress(walletDetails.accountSignerAddress),
+      });
+
+      const unconnectedSmartWallet = smartWallet({
+        chain: await getChain(chainId),
+        sponsorGas: true,
+        factoryAddress: walletDetails.accountFactoryAddress ?? undefined,
+      });
+
+      return await unconnectedSmartWallet.connect({
+        client: thirdwebClient,
+        personalAccount: adminAccount,
+      });
+    }


Doesn't the createSmartWallet() helper function do all this already?

arcoraven · 2024-10-07T21:27:28Z

src/server/utils/wallets/legacyLocalCrypto.ts

+  privateKey: string;
+  address: string;
+}> {
+  console.log(encryptedJson);


remove logging

- add sbw flow for v4 sdk getWallet

- Refactor walletDetailsToAccount function and add caching for admin accounts

socket-security · 2024-10-17T18:03:04Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@types/[email protected]	None	`+1`	386 kB	types
npm/[email protected]	eval, filesystem Transitive: environment, network, shell, unsafe	`+71`	295 MB	egoist

View full report↗︎

arcoraven · 2024-10-19T00:34:45Z

src/db/wallets/createWalletDetails.ts

+      awsKmsSecretAccessKey: string; // will be encrypted and stored, pass plaintext to this function
+      awsKmsAccessKeyId: string;


Won't making these fields non-optional be a breaking change? We could ofc update the dashboard, but it'll impact anyone in the slim chance they're automating KMS backend wallet creation.

We decided to always denormalize and store credentials in the DB row, regardless of it being an override or coming from config. Users don't have to pass it in, the route fills in this from the config.

arcoraven · 2024-10-19T00:50:19Z

src/db/wallets/createWalletDetails.ts

+  // we will never reach here
+  // this helps typescript understand that this function will always return


No need for these comments IMO. Should be clear. And if it's not, just make the thrown error message more clear.

arcoraven · 2024-10-19T00:55:13Z

src/prisma/schema.prisma

  gcpApplicationCredentialEmail      String? @map("gcpApplicationCredentialEmail") /// if not available, default to: Configuration.gcpApplicationCredentialEmail
  gcpApplicationCredentialPrivateKey String? @map("gcpApplicationCredentialPrivateKey") /// if not available, default to: Configuration.gcpApplicationCredentialPrivateKey
+  // Smart Backend Wallet
+  accountSignerAddress               String? @map("accountSignerAddress") /// this, and either local, aws or gcp encryptedJson, are required for smart wallet


Nit: This naming confused me because account made me think this is the same as smart account address, esp when next to accountFactoryAddress. I think these names are clearer:

signerAddress

adminAddress

the WalletDetails schema is very denormalized, so in the interest of maximum clarity, I think having an account prefix is better here.

Between signer and admin, I think signer is better. It leaves us with more flexibility later to allow importing SBW if we want.

arcoraven · 2024-10-19T01:10:23Z

src/db/wallets/getWalletDetails.ts

+  try {
+    return walletDetailsSchema.parse(walletDetails, {
+      errorMap: (issue) => {
+        const fieldName = issue.path.join(".");
+        return {
+          message: `${fieldName} is necessary for wallet ${address} of type ${walletDetails.type}, but not found in wallet details or configuration`,
+        };
+      },
+    });
+  } catch (e) {
+    if (e instanceof z.ZodError) {
+      throw new WalletDetailsError(
+        e.errors.map((error) => error.message).join(", "),
+      );
+    }
+    throw e;
+  }


This zod parsing + prettifying error thing is nice, but definitely belongs in a util where we can re-use it. Not-blocking, but something to eventually abstract so we have a nicer interface here like

return prettyZodParse(walletDetails, walletDetailsSchema)

Then it's also very unit testable.

Agreed. Also unrelated, but I think we should move from a util mindset to a lib mindset. util has us thinking about one-off things that we can store to reuse later. lib would require us to be more intentional about categorizing and co-locating relevant functionality, so it's easier to discover/reuse later. We can start incrementally in this direction with a parse lib and error lib

arcoraven · 2024-10-19T01:20:33Z

src/server/utils/wallets/createLocalWallet.ts

+
+  return {
+    account,
+    // these exact values are stored for backwards compatibility


If only data is read and the others aren't read anymore, shouldn't it be safe to only store data now?

arcoraven · 2024-10-19T02:37:21Z

src/utils/cache/getWallet.ts

+    });
+  } catch (e) {
+    if (e instanceof WalletDetailsError) {
+      throw createCustomError(e.message, 400, "BAD_REQUEST");


Nit: Use StatusCodes everywhere for consistency (it'll be helpful if we're searching for where a certain status code is emitted). Or if you don't like em we should just switch off them entirely and use raw numbers.

arcoraven · 2024-10-19T02:38:28Z

src/utils/cache/getWallet.ts

+
+      const splitArn = splitAwsKmsArn(walletDetails.awsKmsArn);
+
+      const personalWallet = new AwsKmsWallet({


Same nit: this is the adminWallet or signerWallet (pick one), so keep naming consistent.

arcoraven · 2024-10-19T02:53:23Z

src/worker/tasks/sendTransactionWorker.ts

+      const account = await getAccount({
+        chainId: transaction.chainId,
+        from: transaction.from,
+      });
+
+      resultTransaction = await _sendTransaction(job, transaction, account);


It didn't seem like we needed to change this: getAccount can still be called inside _sendTransaction. If there's not a good reason to, let's revert this change.

arcoraven · 2024-10-19T02:54:33Z

src/worker/tasks/sendTransactionWorker.ts

+      // this can either be a regular backend wallet userop or a smart backend wallet userop
+      const accountAddress = transaction.accountAddress;
+      assert(accountAddress, "Invalid userOp parameters: accountAddress");
+
+      let adminAccount: Account | undefined;
+
+      try {
+        adminAccount = await getSmartBackendWalletAdminAccount({
+          accountAddress,
+          chainId: transaction.chainId,
+        });
+      } catch {
+        // do nothing, this might still be a regular backend wallet userop
+      }
+
+      try {
+        adminAccount = await getAccount({
+          chainId: transaction.chainId,
+          from: transaction.from,
+        });
+      } catch {
+        job.log(
+          `Failed to get admin account for userOp: ${stringify(transaction)}`,
+        );
+      }
+
+      if (!adminAccount) {
+        resultTransaction = {
+          ...transaction,
+          status: "errored",
+          errorMessage: "Failed to get admin account",
+        };
+      } else {
+        resultTransaction = await _sendUserOp(job, transaction, adminAccount);
+      }


Can all this just be moved inside _sendUserOp? Not sure if it needs to be out here, and it keeps all userOp logic inside the _sendUserOp function.

arcoraven · 2024-10-19T02:56:20Z

src/worker/tasks/sendTransactionWorker.ts

+          from: transaction.from,
+        });
+      } catch {
+        job.log(


Don't need this added logging. When this job errors, the .on("failed") listener at the bottom of this file already job.log's all errors with the error message.

arcoraven · 2024-10-23T06:03:25Z

sdk/src/services/BackendWalletService.ts

        requestBody: {
            message: string;
            isBytes?: boolean;
+            chainId?: number;


Note from our convo: can you check if signing on different chains result in the same output. If so, we could default to a testnet (Sepolia?).

arcoraven · 2024-10-23T06:31:25Z

src/server/utils/wallets/createLocalWallet.ts

 * Does not store the wallet in the database
 */
-export const createLocalWallet = async () => {
+export const createRandomLocalWallet = async () => {


Nit: "random" sounds unclear to me (especially since the other analogous KMS ones don't have it). createLocalWallet was fine, or generateLocalWallet?

arcoraven · 2024-10-23T06:35:40Z

src/utils/account.ts

-        sponsorGas: true,
-        factoryAddress: walletDetails.accountFactoryAddress ?? undefined,
+      const connectedWallet = await getConnectedSmartWallet({
+        adminAccount: signerAccount,


Nit: Saw you used signerAccount here (AWS) but adminAccount below (GCP)

github-actions · 2024-10-31T00:57:25Z

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this PR will be closed in 3 days.

arcoraven · 2024-11-02T03:20:35Z

src/lib/chain/chain-capabilities.ts

+  enabled: boolean;
+}>;
+
+// Create cache with 2048 entries and 5 minute TTL


Comment is wrong

arcoraven · 2024-11-02T03:23:23Z

src/lib/cache/swr.ts

+      this.dedupedFetch(key, fetchFn).catch(() => {
+        // Silence background revalidation errors
+      });


So this catch makes it if any error occurs while fetching, the old data continues to be used.

Let's at least log a warning here. If this errors silently we won't know if the cache is stuck in a stale state.

smart backend wallet creation + initial flow

68c21b6

d4mr requested a review from arcoraven October 7, 2024 00:35

d4mr commented Oct 7, 2024

View reviewed changes

joaquim-verges reviewed Oct 7, 2024

View reviewed changes

arcoraven reviewed Oct 7, 2024

View reviewed changes

d4mr added 8 commits October 11, 2024 19:02

Merge branch 'main' into pb/sbw

05491fe

refactor wallet detail decryption

082b54a

- add sbw flow for v4 sdk getWallet

change names

1b87760

Merge branch 'main' into pb/sbw

fc12095

Refactor import statement for TransactionReceipt in sdk.ts

aaa5691

Refactor biome.json to ignore the "sdk" directory

dbc567f

Fix smart backend wallet functionality for both v4 and v5 SDK

941e55d

- Refactor walletDetailsToAccount function and add caching for admin accounts

use v6 factory default

51bd785

arcoraven reviewed Oct 19, 2024

View reviewed changes

d4mr added 7 commits October 23, 2024 02:24

consistent naming

17245a1

remove redundant comment

7bb1b80

entrypoint updates

a6ece2b

chainId in sign + thirdweb client in e2e tests

8984844

worker refactors

f4ec6f9

more tests

550dc14

consistent naming

3256832

arcoraven approved these changes Oct 23, 2024

View reviewed changes

github-actions bot added the no-pr-activity label Oct 31, 2024

d4mr added 4 commits November 2, 2024 02:40

Merge branch 'main' into pb/sbw

38b46c6

conditionally require chainId for signing smart account messsages

3a47e77

naming consistencies

a3957ba

reject transaction if unsupported chain

ef64f4c

github-actions bot removed the no-pr-activity label Nov 2, 2024

arcoraven approved these changes Nov 2, 2024

View reviewed changes

d4mr added 3 commits November 4, 2024 07:48

fix userop tests

b5503b9

fix: only transform QueuedTransaction if sbw

b64dff4

Refactor SWRCache to log errors during cache revalidation

c1986ef

d4mr merged commit 94afe24 into main Nov 4, 2024
5 checks passed

d4mr deleted the pb/sbw branch November 4, 2024 02:28


		const json = walletDetails.encryptedJson;

		const wallet = await Wallet.fromEncryptedJson(

		awsKmsSecretAccessKey: string; // will be encrypted and stored, pass plaintext to this function
		awsKmsAccessKeyId: string;

		// we will never reach here
		// this helps typescript understand that this function will always return


		const splitArn = splitAwsKmsArn(walletDetails.awsKmsArn);

		const personalWallet = new AwsKmsWallet({

Smart Backend Wallets #709

Smart Backend Wallets #709

Uh oh!

Conversation

d4mr commented Oct 7, 2024 • edited by pr-codex bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR-Codex overview

Detailed summary

Uh oh!

d4mr Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d4mr Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

socket-security bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d4mr Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

d4mr commented Oct 7, 2024 •

edited by pr-codex bot

Loading

d4mr Oct 7, 2024 •

edited

Loading

d4mr Oct 7, 2024 •

edited

Loading

socket-security bot commented Oct 17, 2024 •

edited

Loading

d4mr Oct 21, 2024 •

edited

Loading