fix: Prevent runtime exceptions when chains aren't configured #13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CRITICAL FIX: Integration tests were deleting user's actual config files! Tests were calling getAllWallets(), getAllSafes(), etc. and removing ALL items from production config without isolation. This is now impossible. **Safety Guards Added:** - All storage classes (ConfigStore, WalletStorageService, SafeAccountStorage, TransactionStore) now require temp directories in test mode - Will throw error if attempting to use production config during tests - Created test-safe storage helper (createTestStorage()) for proper isolation **Integration Tests Fixed:** - wallet.test.ts: Now uses isolated storage - account.test.ts: Now uses isolated storage - transaction.test.ts: Now uses isolated storage - integration-full-workflow.test.ts: Now uses isolated storage **New E2E Test Coverage (49 new tests):** - e2e-wallet-commands.test.ts (8 tests): Wallet CLI commands - e2e-config-commands.test.ts (8 tests): Config CLI commands - e2e-account-commands.test.ts (14 tests): Account CLI commands - e2e-tx-commands.test.ts (19 tests): Transaction CLI commands **ESLint Configuration:** - Silenced 'any' warnings in test files - Kept 'any' as error in production code **Documentation:** - AGENTS.md: Added critical safety warnings and safe test patterns - Explained the bug, the fix, and mandatory safety rules **Test Results:** - 751/755 tests passing - User config files are now protected - All linting passing (0 errors, 0 warnings) - TypeScript compiling with 0 errors This prevents the catastrophic data loss bug where tests would delete encrypted wallet keys, Safe configurations, and transaction history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The persistence tests were creating new storage instances without passing the cwd option, which failed with the new safety guards. Now they correctly pass the same test directory to verify data persists across instances. All 755 tests now passing ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Removed duplicate runtime validation code from all storage classes. Protection now comes from: - Tests using isolated storage (createTestStorage()) - AGENTS.md documentation warnings - Not runtime checks This eliminates code duplication while maintaining safety through proper test isolation and documentation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Extract isTempDirectory() function to ensure consistent path validation across all safety checks. Now consistently checks tmpdir(), /tmp (Unix), and \Temp (Windows) in all locations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Previously, account and tx commands would crash with runtime exceptions when chains were removed from configuration or not yet initialized. This was caused by getShortNameFromChainId() throwing errors when a chain wasn't found. Changes: - Modified getShortNameFromChainId() to return fallback format (chain:123) instead of throwing by default - Added optional throwOnMissing parameter for strict validation scenarios - Updated getChainIdFromShortName() to parse fallback format - Added 8 new integration tests for empty chains scenarios - Updated unit tests to cover new fallback behavior Now when chains aren't configured, addresses display as "chain:1:0x..." instead of crashing, allowing users to still view and manage Safes and transactions even with missing chain configuration. Tests: - All 702 unit tests pass - All 8 new integration tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR prevents runtime exceptions when chains are removed from configuration or not yet initialized by implementing graceful fallback behavior in chain ID resolution functions.
Key Changes:
- Modified
getShortNameFromChainId()to return fallback format (chain:123) instead of throwing exceptions - Updated
getChainIdFromShortName()to parse the fallback format - Added comprehensive test coverage for empty chains scenarios
- Refactored integration tests to use isolated storage directories
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/eip3770.ts | Added fallback format support to prevent crashes when chains aren't configured |
| src/tests/unit/utils/eip3770.test.ts | Updated unit tests to verify fallback behavior and optional throwing |
| src/tests/integration/empty-chains.test.ts | Added 8 new integration tests for empty chains scenarios |
| src/tests/integration/wallet.test.ts | Refactored to use isolated test storage |
| src/tests/integration/transaction.test.ts | Refactored to use isolated test storage |
| src/tests/integration/integration-full-workflow.test.ts | Refactored to use isolated test storage |
| src/tests/integration/e2e-wallet-commands.test.ts | New E2E tests for wallet commands |
| src/tests/integration/e2e-tx-commands.test.ts | New E2E tests for transaction commands |
| src/tests/integration/e2e-config-commands.test.ts | New E2E tests for config commands |
| src/tests/integration/e2e-account-commands.test.ts | New E2E tests for account commands |
| src/tests/integration/account.test.ts | Refactored to use isolated test storage |
| src/tests/helpers/test-storage.ts | New test helper for safe isolated storage |
| src/storage/wallet-store.ts | Added cwd option support for test isolation |
| src/storage/transaction-store.ts | Added cwd option support for test isolation |
| src/storage/safe-store.ts | Added cwd option support for test isolation |
| src/storage/config-store.ts | Added cwd option support for test isolation |
| eslint.config.js | Silenced no-explicit-any warnings in test files |
| AGENTS.md | Expanded development guide with safety warnings and best practices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| beforeEach(() => { | ||
| testStorage = createTestStorage('empty-chains-test') | ||
| configStore = new ConfigStore({ cwd: testStorage.configDir, projectName: 'test-empty-chains' }) | ||
| safeStorage = new SafeAccountStorage({ cwd: testStorage.dataDir }) |
There was a problem hiding this comment.
Inconsistent directory usage: configStore uses testStorage.configDir while safeStorage uses testStorage.dataDir. The ConfigStore constructor should use configDir and SafeAccountStorage should also use configDir for consistency with other test files, or there should be a comment explaining why different directories are used here.
Suggested change
| safeStorage = new SafeAccountStorage({ cwd: testStorage.dataDir }) | |
| safeStorage = new SafeAccountStorage({ cwd: testStorage.configDir }) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Previously, account and tx commands would crash with runtime exceptions when chains were removed from configuration or not yet initialized. This was caused by
getShortNameFromChainId()throwing errors when a chain wasn't found.Changes
getShortNameFromChainId()to return fallback format (chain:123) instead of throwing by defaultthrowOnMissingparameter for strict validation scenariosgetChainIdFromShortName()to parse fallback formatImpact
Now when chains aren't configured, addresses display as
chain:1:0x...instead of crashing, allowing users to still view and manage Safes and transactions even with missing chain configuration.Test Results
Test plan
npm test- all tests passnpm run build- build succeeds🤖 Generated with Claude Code