feat(wallet): Auto-create Coinbase wallet if not found#2120
feat(wallet): Auto-create Coinbase wallet if not found#2120phuongdao03 wants to merge 2 commits intoOpenMind:mainfrom
Conversation
Implements automatic wallet creation when COINBASE_WALLET_ID is not set. This resolves the TODO(Kyle) comment in the codebase. Changes: - Add _get_or_create_wallet() method with priority-based wallet retrieval: 1. Try COINBASE_WALLET_ID environment variable first 2. Load from saved seed file if exists 3. Create new wallet and persist seed for future use - Add wallet persistence via seed file (~/.om1/coinbase_wallet_seed.json) - Saves wallet_id, seed, and network_id - Sets restrictive permissions (0600) for security - Add new config options: - network_id: Configure network (base-sepolia, base-mainnet, etc.) - wallet_seed_file: Custom path for wallet seed storage - Improve error handling and logging throughout This enables users to start using Coinbase wallet integration without manually creating a wallet first. The wallet is automatically persisted so subsequent runs use the same wallet.
There was a problem hiding this comment.
Pull request overview
This PR implements automatic wallet creation for the Coinbase wallet integration, resolving a TODO in the codebase. Previously, users had to manually create a wallet and set the COINBASE_WALLET_ID environment variable before using the integration.
Changes:
- Implements priority-based wallet retrieval: environment variable → saved seed file → new wallet creation
- Adds wallet seed persistence to
~/.om1/coinbase_wallet_seed.jsonwith restrictive file permissions (0o600) - Adds new configuration options:
network_idandwallet_seed_filefor customizing wallet creation and seed storage
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wallet = Wallet.fetch(wallet_id) | ||
|
|
||
| # Load seed for signing capabilities | ||
| if seed: | ||
| wallet.load_seed(self.wallet_seed_file) | ||
| logging.info(f"Loaded wallet with signing capabilities: {wallet.id}") | ||
| else: | ||
| logging.info(f"Loaded wallet (view-only): {wallet.id}") | ||
|
|
||
| return wallet | ||
| except Exception as e: | ||
| logging.warning(f"Could not load wallet from seed file: {e}") | ||
| return None |
There was a problem hiding this comment.
When loading from a seed file, if the wallet no longer exists on the Coinbase server (e.g., it was deleted), the Wallet.fetch(wallet_id) call will fail and the method returns None. This causes the system to create a new wallet in _create_new_wallet(), losing the original wallet's seed data. Consider using a method like Wallet.import_data() to restore the wallet from the seed data, or at minimum preserve the seed file when creation of a new wallet occurs to avoid data loss.
| asset_id: str = Field(default="eth", description="Asset ID to query") | ||
| network_id: str = Field( | ||
| default="base-sepolia", | ||
| description="Network ID for wallet creation (e.g., base-sepolia, base-mainnet)", |
There was a problem hiding this comment.
The documentation for network_id field mentions example values like "base-sepolia" and "base-mainnet", but doesn't provide guidance on what other valid values exist or how users can discover them. Consider adding a reference to the CDP SDK documentation or listing all supported network IDs to help users configure this correctly.
| description="Network ID for wallet creation (e.g., base-sepolia, base-mainnet)", | |
| description=( | |
| "Network ID for wallet creation (e.g., base-sepolia, base-mainnet). " | |
| "Refer to the CDP SDK documentation for the full list of supported network IDs." | |
| ), |
| if not API_KEY or not API_SECRET: | ||
| logging.error( | ||
| "COINBASE_API_KEY or COINBASE_API_SECRET environment variable is not set" | ||
| ) | ||
| else: | ||
| Cdp.configure(API_KEY, API_SECRET) | ||
|
|
||
| try: | ||
| # fetch wallet data | ||
| if not self.COINBASE_WALLET_ID: | ||
| raise ValueError("COINBASE_WALLET_ID environment variable is not set") | ||
| self.wallet = None | ||
| self.balance = 0.0 | ||
| self.balance_previous = 0.0 | ||
| return |
There was a problem hiding this comment.
The early return when API keys are missing will break the existing test test_initialization_without_api_keys_does_not_call_configure (tests/inputs/plugins/test_wallet_coinbase.py:98-124). The test expects the wallet to still be initialized when API keys are missing, but this code now returns early with wallet set to None. This changes the behavior from the original implementation.
| def _get_or_create_wallet(self) -> Optional[Wallet]: | ||
| """ | ||
| Get existing wallet or create a new one if not found. | ||
|
|
||
| Priority: | ||
| 1. Use COINBASE_WALLET_ID environment variable if set | ||
| 2. Load wallet from saved seed file if exists | ||
| 3. Create new wallet and save seed for future use | ||
|
|
||
| Returns | ||
| ------- | ||
| Optional[Wallet] | ||
| The wallet instance, or None if creation/fetch failed. | ||
| """ | ||
| # Priority 1: Try to fetch wallet using environment variable | ||
| if self.COINBASE_WALLET_ID: | ||
| try: | ||
| wallet = Wallet.fetch(self.COINBASE_WALLET_ID) | ||
| logging.info(f"Fetched existing wallet: {wallet.id}") | ||
|
|
||
| # Try to load seed if available for signing capabilities | ||
| self._load_wallet_seed(wallet) | ||
| return wallet | ||
| except Exception as e: | ||
| logging.warning( | ||
| f"Could not fetch wallet with ID {self.COINBASE_WALLET_ID}: {e}" | ||
| ) | ||
|
|
||
| # Priority 2: Try to load wallet from saved seed file | ||
| wallet = self._load_wallet_from_seed_file() | ||
| if wallet: | ||
| return wallet | ||
|
|
||
| # Priority 3: Create new wallet | ||
| return self._create_new_wallet() | ||
|
|
||
| def _load_wallet_from_seed_file(self) -> Optional[Wallet]: | ||
| """ | ||
| Load wallet from saved seed file. | ||
|
|
||
| Returns | ||
| ------- | ||
| Optional[Wallet] | ||
| The wallet instance if loaded successfully, None otherwise. | ||
| """ | ||
| try: | ||
| if not os.path.exists(self.wallet_seed_file): | ||
| return None | ||
|
|
||
| with open(self.wallet_seed_file, "r") as f: | ||
| wallet_data = json.load(f) | ||
|
|
||
| wallet_id = wallet_data.get("wallet_id") | ||
| seed = wallet_data.get("seed") | ||
|
|
||
| if not wallet_id: | ||
| logging.warning("Wallet seed file exists but missing wallet_id") | ||
| return None | ||
|
|
||
| wallet = Wallet.fetch(wallet_id) | ||
|
|
||
| # Load seed for signing capabilities | ||
| if seed: | ||
| wallet.load_seed(self.wallet_seed_file) | ||
| logging.info(f"Loaded wallet with signing capabilities: {wallet.id}") | ||
| else: | ||
| logging.info(f"Loaded wallet (view-only): {wallet.id}") | ||
|
|
||
| return wallet | ||
| except Exception as e: | ||
| logging.warning(f"Could not load wallet from seed file: {e}") | ||
| return None | ||
|
|
||
| def _load_wallet_seed(self, wallet: Wallet) -> None: | ||
| """ | ||
| Attempt to load seed for an existing wallet to enable signing. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| wallet : Wallet | ||
| The wallet to load seed for. | ||
| """ | ||
| try: | ||
| if os.path.exists(self.wallet_seed_file): | ||
| with open(self.wallet_seed_file, "r") as f: | ||
| wallet_data = json.load(f) | ||
|
|
||
| if wallet_data.get("wallet_id") == wallet.id and wallet_data.get( | ||
| "seed" | ||
| ): | ||
| wallet.load_seed(self.wallet_seed_file) | ||
| logging.info(f"Loaded seed for wallet: {wallet.id}") | ||
| except Exception as e: | ||
| logging.debug(f"Could not load seed for wallet: {e}") | ||
|
|
||
| def _create_new_wallet(self) -> Optional[Wallet]: | ||
| """ | ||
| Create a new wallet and save its seed for future use. | ||
|
|
||
| Returns | ||
| ------- | ||
| Optional[Wallet] | ||
| The newly created wallet, or None if creation failed. | ||
| """ | ||
| try: | ||
| logging.info(f"Creating new wallet on network: {self.network_id}") | ||
| wallet = Wallet.create(network_id=self.network_id) | ||
|
|
||
| # Save wallet data for future use | ||
| self._save_wallet_seed(wallet) | ||
|
|
||
| logging.info(f"Created new wallet: {wallet.id}") | ||
| logging.info( | ||
| f"Default address: {wallet.default_address.address_id if wallet.default_address else 'N/A'}" | ||
| ) | ||
|
|
||
| # Set environment variable for current session | ||
| os.environ["COINBASE_WALLET_ID"] = wallet.id | ||
|
|
||
| return wallet | ||
| except Exception as e: | ||
| logging.error(f"Failed to create new wallet: {e}") | ||
| return None | ||
|
|
||
| def _save_wallet_seed(self, wallet: Wallet) -> None: | ||
| """ | ||
| Save wallet seed to file for future use. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| wallet : Wallet | ||
| The wallet to save. | ||
| """ | ||
| try: | ||
| # Ensure directory exists | ||
| seed_dir = os.path.dirname(self.wallet_seed_file) | ||
| if seed_dir: | ||
| Path(seed_dir).mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Export and save wallet data | ||
| wallet_data = wallet.export_data() | ||
| save_data = { | ||
| "wallet_id": wallet_data.wallet_id, | ||
| "seed": wallet_data.seed, | ||
| "network_id": wallet_data.network_id, | ||
| } | ||
|
|
||
| with open(self.wallet_seed_file, "w") as f: | ||
| json.dump(save_data, f, indent=2) | ||
|
|
||
| # Set restrictive permissions (owner read/write only) | ||
| os.chmod(self.wallet_seed_file, 0o600) | ||
|
|
||
| logging.info(f"Wallet seed saved to: {self.wallet_seed_file}") | ||
| except Exception as e: | ||
| logging.error(f"Failed to save wallet seed: {e}") |
There was a problem hiding this comment.
The new wallet creation and seed file persistence functionality lacks test coverage. The following scenarios should be tested:
- Creating a new wallet when no wallet ID or seed file exists
- Loading a wallet from a seed file when COINBASE_WALLET_ID is not set
- Saving wallet seed data with correct permissions (0o600)
- Handling cases where seed file exists but is malformed
- Fallback behavior when wallet creation fails
- The priority order: env var -> seed file -> create new
Since other plugins in tests/inputs/plugins/ have comprehensive test coverage, this new functionality should follow the same pattern.
|
|
||
| # Load seed for signing capabilities | ||
| if seed: | ||
| wallet.load_seed(self.wallet_seed_file) |
There was a problem hiding this comment.
The usage of wallet.load_seed(self.wallet_seed_file) appears incorrect. According to the CDP SDK patterns, load_seed likely expects the actual seed data or a wallet data object, not a file path. The code already reads the JSON file and extracts wallet_data, but then passes the file path string instead of using the extracted seed data. Consider using the Wallet.import_data() method or passing the actual seed/wallet_data object to properly restore signing capabilities.
| if wallet_data.get("wallet_id") == wallet.id and wallet_data.get( | ||
| "seed" | ||
| ): | ||
| wallet.load_seed(self.wallet_seed_file) |
There was a problem hiding this comment.
The usage of wallet.load_seed(self.wallet_seed_file) appears incorrect. According to the CDP SDK patterns, load_seed likely expects the actual seed data or a wallet data object, not a file path. The code already reads the JSON file and extracts wallet_data, but then passes the file path string instead of using the extracted seed data. Consider using the Wallet.import_data() method or passing the actual seed/wallet_data object to properly restore signing capabilities.
Addresses all review comments from GitHub Copilot:
1. Use Wallet.import_data instead of deprecated load_seed
- Replaced load_seed() with Wallet.import_data(WalletData(...))
- Uses WalletData class for proper wallet restoration
2. Handle wallet deleted from server
- Added _recreate_wallet_from_seed() method
- When import_data fails, attempts to recreate wallet with same seed
- Preserves wallet addresses derived from original seed
3. Fix seed_dir edge case
- Added comment explaining os.path.dirname returns empty string
for files in current directory
- Current directory always exists, so this is acceptable
4. Improve error handling when seed save fails
- _save_wallet_seed now returns bool to indicate success
- Logs CRITICAL warning with wallet ID when save fails
- Allows users to manually save wallet ID
5. Add comprehensive documentation for network_id
- Lists supported networks (base-sepolia, base-mainnet, etc.)
- Includes link to CDP SDK documentation
6. Add unit tests for new functionality
- Test wallet creation when no ID set
- Test loading from seed file
- Test malformed seed file handling
- Test wallet recreation when deleted from server
- Test priority order (env var > seed file > create new)
- Test custom network_id configuration
1 participant
Problem
Resolves TODO(Kyle) in
src/inputs/plugins/wallet_coinbase.py:Currently, users must manually create a Coinbase wallet and set
COINBASE_WALLET_IDenvironment variable before using the wallet integration. This creates friction for new users.Solution
Implement automatic wallet creation with persistence:
Priority-based wallet retrieval:
COINBASE_WALLET_IDenvironment variableWallet persistence:
~/.om1/coinbase_wallet_seed.jsonNew configuration options:
network_id: Configure target network (default: base-sepolia)wallet_seed_file: Custom path for seed storageTesting
python -m py_compileSecurity Considerations