SmartWallet support #105
SmartWallet support #105
Conversation
🟡 Heimdall Review Status
|
| self._cdp_client: CdpApiClient = cdp_client | ||
| self._wallets: WalletsApi | None = None | ||
| self._smart_wallets: SmartWalletsApi | None = None | ||
| self._webhooks: WebhooksApi | None = None | ||
| self._addresses: AddressesApi | None = None | ||
| self._external_addresses: ExternalAddressesApi | None = None |
There was a problem hiding this comment.
general python nit, if you're doing a getter pattern using .__variable = is preferable if possible as it makes the variables private, seeing .smart_wallet and ._smart_wallet can be confusing
| @@ -88,7 +89,8 @@ def from_dict(cls, obj: Optional[Dict[str, Any]]) -> Optional[Self]: | |||
| return cls.model_validate(obj) | |||
|
|
|||
| _obj = cls.model_validate({ | |||
| "calls": [Call.from_dict(_item) for _item in obj["calls"]] if obj.get("calls") is not None else None | |||
| "calls": [Call.from_dict(_item) for _item in obj["calls"]] if obj.get("calls") is not None else None, | |||
| "paymaster_url": obj.get("paymaster_url") | |||
There was a problem hiding this comment.
pythonic nit, can be
[Call.from_dict(_item) for _item in obj["calls"]] if "calls" in obj else None,There was a problem hiding this comment.
ah this is auto-generated openapi code. we can see if this gets cleaned up in newer versions though, or try to configure the rules to generate these files differently
| value: Wei | None = Field(None, description="Amount of native currency to send") | ||
| abi: list[dict[str, Any]] = Field(..., description="Contract ABI specification") | ||
| function_name: str = Field(..., description="Name of the function to call") | ||
| args: list[Any] = Field(..., description="Arguments to pass to the function") |
There was a problem hiding this comment.
very very optional nit, but I think you could type args here with list[int | str | ...etc]
| # Only SmartWallet related chains are listed here right now | ||
| SupportedChainId = Literal[8453, 84532] | ||
|
|
||
| _CHAIN_ID_TO_NETWORK_ID: dict[SupportedChainId, NetworkIdentifier] = { |
There was a problem hiding this comment.
imo this should either be no _ or fully private __
| _chain_id: SupportedChainId | ||
| _network_id: NetworkIdentifier |
There was a problem hiding this comment.
why _ prefixes? I guess we aren't having users construct these but still is odd to me
| def __init__( | ||
| self, | ||
| smart_wallet_address: str, | ||
| account: BaseAccount, | ||
| chain_id: int, | ||
| paymaster_url: str | None = None, | ||
| ) -> None: | ||
| """Initialize the NetworkScopedSmartWallet. |
There was a problem hiding this comment.
How do users construct this? Do they construct this?
| self._smart_wallet_address = smart_wallet_address | ||
| self._owners = [account] |
| @classmethod | ||
| def create( | ||
| cls, | ||
| account: BaseAccount, |
There was a problem hiding this comment.
I'm still not in love with BaseAccount given when the overloaded meaning
There was a problem hiding this comment.
ah yeah this is coming from from eth_account.signers.base import BaseAccount now, not something we've invented.
I can rename it at the import layer but fundamentally this is a 3rd party class named with Base as the prefix
| NetworkScopedSmartWallet: A network-scoped version of the wallet | ||
|
|
||
| """ | ||
| from cdp.network_scoped_smart_wallet import NetworkScopedSmartWallet |
There was a problem hiding this comment.
Generally don't love local imports unless good reason like import cycle (if import cycle that might be a smell our package structure is a little off)
There was a problem hiding this comment.
yeah there's an import cycle here. how do you handle this situation where
class A subclasses class B, and there's a method in class B to construct an instance of class A? that's what is causing the import cycle
| ) | ||
|
|
||
|
|
||
| def test_smart_wallet_operations(): |
There was a problem hiding this comment.
this test doesn't seem to actually fail in a way pytest recognizes, should add asserts
There was a problem hiding this comment.
oh yeah this is just a test script, not actually unit tests. this won't get checked in - just for reference
Related PR for Node SDK: coinbase/coinbase-sdk-nodejs#376
Summary
SmartWalletclass andNetworkScopedSmartWalletsubclass (not using composition here like typescript, inheritance feels more appropriate). I'm usingBaseAccountfrom the eth_accounts library as theownerfor the wallet.EVMCalltypes for encoded calldata as well as ABI function data. I made this a separate file since we'll use these same types when we improve our existing wallet to support direct calldata and client-side encoding.UserOperationclass which will handle the APIs to interact with creating a user operation, signing it using the base account, and broadcasting it.Testing
Notes to reviewers
Questions
to_smart_walletfunction in the SmartWallet class. Based on feedback, I made it standalone instead of a static function sinceSmartWallet.to_smart_walletstutters. This naming matches Node SDK - do we want to change any thing about the naming of it though to better suit python best practices?