SmartWallet support (object-oriented structure) #374

rohan-agarwal-coinbase · 2025-02-05T22:54:59Z

APIs for review:
Everything in the new SmartWallet class, smart_wallet.ts

create()
connect() - alternatives considered were fetch to align with existing Wallet. We like connect cause it follows what Gnosis SAFE does: https://docs.safe.global/reference-sdk-protocol-kit/initialization/connect
use() - alternatives considered were switch() or making it part of the connect() interface. Functionally it's different than connect which is responsible for fetching the smart wallet and validating the local account is the owner, which is why a separate function felt more appropriate.

We have to invent something because we have no concept of a provider like in ethers, or client like in viem. We could also consider specifying network as part of the Coinbase initialization. That step involves API initialization and doesn't exactly feel appropriate either, but it's closer to a viem DevEx.

getAddress() - alternatives considered were getSmartWalletAddress() or getContractAddress()
sendUserOperation() - alternatives considered were execute(), invoke(), executeBatch(). My issue with sendUserOperation is that while it is low-level function that's similar to viem's sendUserOperation, it's not exactly the same thing since we take a simpler request type as input and orchestrate through our backend.

The new toLocalAccount utility, to_local_account.ts

toLocalAccount() - alternatives we're considering are createViemAccount (that's what Privy calls it), and createAccount (that's what Turnkey calls it). I kind of like createViemAccount the most

See test_smart_wallet.ts and test_viem_account.ts to see how these new APIs are used in practice.

cb-heimdall · 2025-02-05T22:55:03Z

🟡 Heimdall Review Status

Requirement Status More Info

Reviews

🟡 0/1

Denominator calculation

Show calculation

1 if user is bot 0

1 if user is external 0

From .codeflow.yml 1

Additional review requirements

Show calculation

Max	0

0

From CODEOWNERS 0

Global minimum 0

Max 1

1

1 if commit is unverified 0

Sum 1

This stops wrapping the signature client-side as this is handled server-side during user operation orchestration.

yuga-cb · 2025-02-07T19:36:52Z

src/coinbase/smart_wallet.ts

+
+export class SmartWallet {
+  private model: SmartWalletModel;
+  private account: LocalAccount;


Should we call this "signer" instead, to make it clearer what it does?

imo because this i private account is fine but not a strong opinion

yeah I went with account based on this Viem FAQ: https://viem.sh/docs/faq#why-use-the-terms-wallet--account-instead-of-signer

yuga-cb · 2025-02-07T19:41:03Z

src/coinbase/smart_wallet.ts

+    return smartWallet;
+  }
+
+  public static async connect({smartWalletAddress, account}: {smartWalletAddress: string, account: LocalAccount}): Promise<SmartWallet> {


It's an interesting experience reading these APIs without documentation - because without the JSDoc, I can't immediately grok what they would do.

I understand there are schelling points with Gnosis Safe, but I personally haven't used it, so I don't have a point of comparison.

Seems to me that, in a vacuum, this method should be called fetch. Just my 2c

imo connect makes sense, you're not really fetching anything (well you are literally calling an api...), the job to be done here is get a SmartWallet object from the users perspective, fetching is an implementation detail. Would vote connect for convention

yuga-cb · 2025-02-07T19:41:38Z

src/coinbase/smart_wallet.ts

+    return smartWallet;
+  }
+
+  public async use({networkId}: {networkId: NetworkIdentifier}) {


Similar comment here, although it is more parseable. Weirdly, I would probably name this method connect in a vacuum (i.e. the SmartWallet is connecting to a network).

Imo explicit is better, useNetwork. We're not actually connecting to anything locally, we're setting the network of operation

yuga-cb · 2025-02-07T19:43:33Z

src/coinbase/user_operation.ts

+   * Private constructor to prevent direct instantiation outside of the factory methods.
+   *
+   * @ignore
+   * @param userOperationModel - The UserOperation model.


Missing param for smartWalletAddress

yuga-cb · 2025-02-07T19:45:30Z

src/coinbase/accounts/to_local_account.ts

+  return `${payloadSignature.getSignature()}` as Hex;
+}
+
+export async function signTransaction<


Export these too in index?

erikreppel · 2025-02-08T02:05:41Z

src/coinbase/accounts/to_local_account.ts

+import { WalletAddress } from "../address/wallet_address";
+import { hashMessage } from "viem";
+
+export function toLocalAccount(address: WalletAddress): LocalAccount {


JSDoc on all exported functions please!

erikreppel · 2025-02-08T02:06:10Z

src/coinbase/accounts/to_local_account.ts

@@ -0,0 +1,83 @@
+import { hashTypedData, keccak256, serializeTransaction } from "viem";


lets start file names using proper TS conventions in this PR

erikreppel · 2025-02-08T02:07:24Z

src/coinbase/smart_wallet.ts

+
+export class SmartWallet {
+  private model: SmartWalletModel;
+  private account: LocalAccount;


imo because this i private account is fine but not a strong opinion

erikreppel · 2025-02-08T02:08:36Z

src/coinbase/smart_wallet.ts

+    this.account = account;
+  }
+
+  public static async create({account}: {account: LocalAccount}): Promise<SmartWallet> {


I agree with the lint, JSDoc on all public and exported function

erikreppel · 2025-02-08T02:12:40Z

src/coinbase/smart_wallet.ts

+    return smartWallet;
+  }
+
+  public static async connect({smartWalletAddress, account}: {smartWalletAddress: string, account: LocalAccount}): Promise<SmartWallet> {


imo connect makes sense, you're not really fetching anything (well you are literally calling an api...), the job to be done here is get a SmartWallet object from the users perspective, fetching is an implementation detail. Would vote connect for convention

erikreppel · 2025-02-08T02:13:49Z

src/coinbase/smart_wallet.ts

+    return smartWallet;
+  }
+
+  public async use({networkId}: {networkId: NetworkIdentifier}) {


Imo explicit is better, useNetwork. We're not actually connecting to anything locally, we're setting the network of operation

erikreppel · 2025-02-08T02:14:16Z

src/coinbase/smart_wallet.ts

+    return this.model.address;
+  }
+
+  public async sendUserOperation<T extends readonly unknown[]>(


erikreppel · 2025-02-08T02:17:29Z

src/coinbase/types.ts

+  createSmartWallet: (
+    createSmartWalletRequest?: CreateSmartWalletRequest,
+    options?: RawAxiosRequestConfig,
+  ) => AxiosPromise<SmartWalletModel>;
+
+  /*
+    Get the smart wallet by address
+
+    @param smartWalletAddress - The address of the smart wallet to fetch.
+    @param options - Override http request option.
+    @throws {APIError} If the request fails.
+  */
+  getSmartWallet: (
+    smartWalletAddress: string,
+    options?: RawAxiosRequestConfig,
+  ) => AxiosPromise<SmartWalletModel>;


Does these stutter when constructed?

const smartWalletClient = SmartWalletAPIClient() smartWalletClient.createSmartWallet()

I guess it depends on the construction

erikreppel · 2025-02-08T02:19:35Z

src/coinbase/types.ts

+/**
+ * SmartWalletAPI client type definition.
+ */
+export type SmartWalletAPIClient = {


SmartWalletAPIClient feels a little long to me, maybe just SmartWalletClient or SmartWallet if this is the entrypoint for devs to broadcast transactions might make sense

erikreppel · 2025-02-08T02:19:48Z

src/coinbase/types.ts

 };

+/**
+ * SmartWalletAPI client type definition.


should match the type name

erikreppel · 2025-02-08T02:20:20Z

src/coinbase/types.ts

+  createSmartWallet: (
+    createSmartWalletRequest?: CreateSmartWalletRequest,
+    options?: RawAxiosRequestConfig,
+  ) => AxiosPromise<SmartWalletModel>;


Should this be typed as AxiosPromise or Promise

erikreppel · 2025-02-08T02:20:59Z

src/coinbase/types.ts

+   *
+   * @class
+   * @param createdSmartWalletRequest - The smart wallet creation request.
+   * @param options - Axios request options.


I don't love exposing dependency libs in our docs / api

erikreppel · 2025-02-08T02:21:17Z

src/coinbase/types.ts

+    networkId: string,
+    createUserOperationRequest: CreateUserOperationRequest,
+    options?: RawAxiosRequestConfig,
+  ) => AxiosPromise<UserOperationModel>;


same as above on Axios, and same below

erikreppel · 2025-02-08T02:23:31Z

src/coinbase/user_operation.ts

+ * A representation of a UserOperation, which calls a smart contract method
+ * onchain.
+ */
+export class UserOperation {


this feels very ruby to me, all functions other than wait and reload are just a property on model or address. Might be worth just using functions.

WIP

02a8bf0

changes so far

c8d290e

rohan-agarwal-coinbase changed the title ~~WIP~~ SmartWallet support WIP Feb 5, 2025

rohan-agarwal-coinbase and others added 20 commits February 5, 2025 17:58

remove

c0ebe94

network

b2442a0

Cleanup

9cdc74d

Initial user operation class

bbf7d5a

Fix

3a23b1c

index

ea1ee75

Update code to compile

8fdefaa

Rename

815ac06

Test smart wallet

67c2b37

Fix

8389547

Fix

a54ed82

Fix

ebb837d

Encode value as a string, wait for faucet tx, and use network

3813f94

chore: Stop wrapping signature client-side

4f37a8c

This stops wrapping the signature client-side as this is handled server-side during user operation orchestration.

CLeanup

89dc554

toLocalAccount

d70c3ce

Changes

2fe9b82

test

cebedc9

Fix

a95a10b

Rename file

e472398

yuga-cb reviewed Feb 7, 2025

View reviewed changes

erikreppel reviewed Feb 8, 2025

View reviewed changes

Changes

6bc30ac

rohan-agarwal-coinbase mentioned this pull request Feb 11, 2025

SmartWallet support #376

Merged

rohan-agarwal-coinbase changed the title ~~SmartWallet support WIP~~ SmartWallet support (object-oriented structure) Feb 11, 2025

		@@ -0,0 +1,83 @@
		import { hashTypedData, keccak256, serializeTransaction } from "viem";

SmartWallet support (object-oriented structure) #374

Are you sure you want to change the base?

SmartWallet support (object-oriented structure) #374

Uh oh!

Conversation

rohan-agarwal-coinbase commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cb-heimdall commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟡 Heimdall Review Status

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

rohan-agarwal-coinbase commented Feb 5, 2025 •

edited

Loading

cb-heimdall commented Feb 5, 2025 •

edited

Loading