Skip to content

Conversation

@rohan-agarwal-coinbase
Copy link
Contributor

@rohan-agarwal-coinbase rohan-agarwal-coinbase commented Feb 11, 2025

Summary

  • Added src/actions/ which contains our sendUserOperation and waitForUserOperation functions
  • Added src/wallets/ which contains types.ts for Wallet/Signer related types, createSmartWallet, and toSmartWallet functions
  • Added src/types/ which will contain general type definitions. Addedchain.ts
  • Added src/utils/ for common utilities. Added chain.ts with a helper function, and wait.ts which is a reusable function to poll and optionally transform API responses into expected types
  • For types, I made SmartWallet fully pure by having a separated "connected" Smart Wallet type called NetworkScopedSmartWallet. This provides flexibility to use SmartWallet as a multi-network wallet by passing in a chainId/paymasterUrl on requests, or by passing in that information once and not on subsequent requests. I also introduced signer as a more generic owner of the SmartWallet, and made createSmartWallet take in a Signer instead. LocalAccount is fully compatible with this type so viem can easily be used without any transformation (see test script)
  • For network related information, we now use chainId instead of networkId which is more standard. I introduced a Network abstraction though which encapsulates other related concepts like networkId and could be extended to include protocolFamily as well.
  • The other really big API design change is that I'm not wrapping underlying OpenAPI models. There are explicit response types with only the relevant information. It's always easier to add in more concepts later and this feels better to me than simply exposing everything.
  • Updated package.json, jest config, tsconfig for new code conventions around test files

Notes to reviewers

  • I've intentionally kept src/coinbase/coinbase.ts and src/coinbase/types.ts to follow old naming convention patterns. Since these are internal, we can address this separately later.
  • Ignore the changes in src/tests, this was all formatting-related and are unrelated to this PR

Testing

  • E2E tests succeed, filed a ticket and commented out flaky tests
  • Added unit tests which succeed

@cb-heimdall
Copy link

cb-heimdall commented Feb 11, 2025

🟡 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

@rohan-agarwal-coinbase rohan-agarwal-coinbase changed the title SmartWallet support SmartWallet support (functional) Feb 11, 2025
Copy link
Contributor

@marcin-cb marcin-cb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments, nothing blocking 😎

@yuga-cb
Copy link
Contributor

yuga-cb commented Feb 11, 2025

My 2c on the questions:

Do we like the structure of having new folders like src/coinbase/wallets, src/coinbase/actions? Is src/wallets and src/actions better? I put it under coinbase since everything currently lives there.

src/wallets, src/actions is better, but doesn't have to be in this PR

Do we like src/coinbase/utils or src/coinbase/util? I think plural mirrors the other folders better.

+1 to plural

Do we like getSmartWallet.ts or fetchSmartWallet.ts or connectSmartWallet.ts? I'm leaning to getSmartWallet.ts or fetchSmartWallet.ts in this functional paradigm.

My preference is fetch

Does the strategy for evolving these types look generally good in src/coinbase/wallets/types.ts? See the commented code.

See my question on the motivation behind functional vs. class-based

I removed the reload function on user operation since I don't think it's actually all that useful, any concerns? Can always add this later. It might be better to have a getUserOperation.ts instead.

We'd want to mirror what we do with xxSmartWallet? So get/get or fetch/fetch

For wait(), should it modify the UserOperation in place, or should it return a new object? Our current SDK behavior is to modify in place, but in a functional paradigm it feels more appropriate to create a new object to me. See testNewSmartWallet.ts line 49

+1 to new object; mutations no-bueno in functional

For useNetwork(), should this return a new object or mutate in place? It's currently mutating. If we want to be strict on functional paradigm and keep things pure, it should return a new object, but this feels like an appropriate use case for mutating for DevEx.

Perhaps wallet.forNetwork(), and return a different object?

What about calling useNetwork switchNetwork instead? Recommendation from @0xRAG and Carson!

Could work

@rohan-agarwal-coinbase rohan-agarwal-coinbase changed the base branch from master to v0.19.0 February 21, 2025 02:17
@rohan-agarwal-coinbase rohan-agarwal-coinbase changed the base branch from v0.19.0 to master February 21, 2025 02:18
@rohan-agarwal-coinbase rohan-agarwal-coinbase changed the base branch from master to v0.19.0 February 21, 2025 02:19
@rohan-agarwal-coinbase rohan-agarwal-coinbase merged commit 6c55607 into v0.19.0 Feb 21, 2025
6 checks passed
@rohan-agarwal-coinbase rohan-agarwal-coinbase deleted the rohan/CDP-221 branch February 21, 2025 17:08
rohan-agarwal-coinbase added a commit that referenced this pull request Feb 21, 2025
* feat: expose fee recipient and forwarded fee recipient addresses on validator and pending_claimable_balance from staking context api

* add eurc & cbbtc to Coinbase assets

* SmartWallet support (#376)

* update changelog

* Prepping v0.19.0 release (#387)

---------

Co-authored-by: Rohit Durvasula <rohit.durvasula@coinbase.com>
Co-authored-by: Rohit Durvasula <88731568+drohit-cb@users.noreply.github.com>
Co-authored-by: Milan Saxena <milan.saxena@coinbase.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

10 participants