Conversation
🟡 Heimdall Review Status
|
marcin-cb
left a comment
There was a problem hiding this comment.
Left some minor comments, nothing blocking 😎
|
My 2c on the questions:
src/wallets, src/actions is better, but doesn't have to be in this PR
+1 to plural
My preference is fetch
See my question on the motivation behind functional vs. class-based
We'd want to mirror what we do with xxSmartWallet? So get/get or fetch/fetch
+1 to new object; mutations no-bueno in functional
Perhaps wallet.forNetwork(), and return a different object?
Could work |
* 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>
Summary
src/actions/which contains oursendUserOperationandwaitForUserOperationfunctionssrc/wallets/which containstypes.tsfor Wallet/Signer related types,createSmartWallet, andtoSmartWalletfunctionssrc/types/which will contain general type definitions. Addedchain.tssrc/utils/for common utilities. Addedchain.tswith a helper function, andwait.tswhich is a reusable function to poll and optionally transform API responses into expected typesSmartWalletfully pure by having a separated "connected" Smart Wallet type calledNetworkScopedSmartWallet. This provides flexibility to useSmartWalletas 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 introducedsigneras a more generic owner of theSmartWallet, and made createSmartWallet take in aSignerinstead.LocalAccountis fully compatible with this type so viem can easily be used without any transformation (see test script)networkrelated information, we now usechainIdinstead ofnetworkIdwhich is more standard. I introduced aNetworkabstraction though which encapsulates other related concepts likenetworkIdand could be extended to includeprotocolFamilyas well.Notes to reviewers
src/coinbase/coinbase.tsandsrc/coinbase/types.tsto follow old naming convention patterns. Since these are internal, we can address this separately later.src/tests, this was all formatting-related and are unrelated to this PRTesting