Is your feature request related to a problem? Please describe.
The keystore instantiation logic in keystore.py uses two parallel if/elif dispatch chains to select the correct keystore implementation based on DerivationType and KeystoreTextType. Adding a new keystore type requires modifying both instantiate_keystore() and instantiate_keystore_from_text(), violating the Open/Closed Principle. It also makes the dispatch logic hard to test in isolation.
Current code in keystore.py:
def instantiate_keystore(derivation_type: DerivationType, data: Dict[str, Any],
parent_keystore: Optional[KeyStore]=None,
row: Optional[MasterKeyRow]=None) -> KeyStore:
keystore: KeyStore
if derivation_type == DerivationType.BIP32:
keystore = BIP32_KeyStore(data, row, parent_keystore)
elif derivation_type == DerivationType.HARDWARE:
assert parent_keystore is None
keystore = app_state.device_manager.create_keystore(data, row)
elif derivation_type == DerivationType.ELECTRUM_MULTISIG:
assert parent_keystore is None
keystore = Multisig_KeyStore(data, row)
elif derivation_type == DerivationType.ELECTRUM_OLD:
assert parent_keystore is None
keystore = Old_KeyStore(data, row)
else:
raise Exception(_("unknown masterkey type {}:{}".format(
row.masterkey_id if row is not None else None, derivation_type)))
return keystoreinstantiate_keystore_from_text() has a similar 6-branch if/elif chain over KeystoreTextType.
A similar pattern exists for account types in wallet.py, where 11 isinstance() checks dispatch across ImportedAddressAccount, ImportedPrivkeyAccount, DeterministicAccount, StandardAccount, MultisigAccount, etc.
Describe the solution you'd like
Replace the if/elif chains with a registry-based approach where each keystore type registers itself and the controller looks it up by type identifier. This way:
- New keystore types can be added by registering a new handler, without modifying existing dispatch functions.
- The dispatch is testable in isolation (register a fake handler, verify it gets called).
- Unknown types produce a clear, specific error instead of a generic exception.
Example sketch:
from typing import Protocol, Dict, Optional
class KeyStoreHandler(Protocol):
@staticmethod
def handles() -> DerivationType: ...
@staticmethod
def instantiate(data: dict, row=None, parent=None) -> KeyStore: ...
class KeyStoreRegistry:
def __init__(self):
self._handlers: Dict[DerivationType, KeyStoreHandler] = {}
def register(self, handler: KeyStoreHandler) -> None:
key = handler.handles()
if key in self._handlers:
raise ValueError(f"Handler for {key} already registered")
self._handlers[key] = handler
def instantiate(self, derivation_type: DerivationType, data: dict,
row=None, parent=None) -> KeyStore:
if derivation_type not in self._handlers:
raise ValueError(f"Unsupported keystore type: {derivation_type}")
return self._handlers[derivation_type].instantiate(data, row, parent)
def has(self, derivation_type: DerivationType) -> bool:
return derivation_type in self._handlers
# Individual handlers — wrap existing classes, do not replace them
class BIP32KeyStoreHandler:
@staticmethod
def handles() -> DerivationType:
return DerivationType.BIP32
@staticmethod
def instantiate(data, row=None, parent=None) -> KeyStore:
return BIP32_KeyStore(data, row, parent)
class HardwareKeyStoreHandler:
@staticmethod
def handles() -> DerivationType:
return DerivationType.HARDWARE
@staticmethod
def instantiate(data, row=None, parent=None) -> KeyStore:
assert parent is None
return app_state.device_manager.create_keystore(data, row)
# ... same for Multisig, Old, Imported
# Registration (called once at startup)
def init_keystore_registry() -> KeyStoreRegistry:
reg = KeyStoreRegistry()
reg.register(BIP32KeyStoreHandler())
reg.register(HardwareKeyStoreHandler())
reg.register(MultisigKeyStoreHandler())
reg.register(OldKeyStoreHandler())
reg.register(ImportedKeyStoreHandler())
return regThe same pattern applies to account types in wallet.py, where a AccountRegistry could replace the 11 scattered isinstance() checks.
Describe alternatives you've considered
- Enum + dispatch dict: A simple
{DerivationType: callable} mapping would eliminate the if/elif chain without a full registry class. This is lighter weight and sufficient if no runtime extensibility is needed.
- Leave as-is: The current code works and has only 5 branches. The if/elif chain is readable. However, the same argument applies to any code that works — the issue is maintainability as new types are added.
Additional context
- The
exchange_rate.py module already uses a similar (but less structured) approach: inspect.getmembers() discovers all ExchangeBase subclasses at runtime. A registry pattern would formalize this and add get_id() / fail-fast behavior.
- This change is backward-compatible: existing keystore classes stay unchanged, the registry wraps them. No public API changes needed.
- The dispatch functions
instantiate_keystore() and instantiate_keystore_from_text() could become thin wrappers around the registry for backward compatibility during migration.
Is your feature request related to a problem? Please describe.
The keystore instantiation logic in
keystore.pyuses two parallel if/elif dispatch chains to select the correct keystore implementation based onDerivationTypeandKeystoreTextType. Adding a new keystore type requires modifying bothinstantiate_keystore()andinstantiate_keystore_from_text(), violating the Open/Closed Principle. It also makes the dispatch logic hard to test in isolation.Current code in
keystore.py:instantiate_keystore_from_text()has a similar 6-branch if/elif chain overKeystoreTextType.A similar pattern exists for account types in
wallet.py, where 11isinstance()checks dispatch acrossImportedAddressAccount,ImportedPrivkeyAccount,DeterministicAccount,StandardAccount,MultisigAccount, etc.Describe the solution you'd like
Replace the if/elif chains with a registry-based approach where each keystore type registers itself and the controller looks it up by type identifier. This way:
Example sketch:
The same pattern applies to account types in
wallet.py, where aAccountRegistrycould replace the 11 scatteredisinstance()checks.Describe alternatives you've considered
{DerivationType: callable}mapping would eliminate the if/elif chain without a full registry class. This is lighter weight and sufficient if no runtime extensibility is needed.Additional context
exchange_rate.pymodule already uses a similar (but less structured) approach:inspect.getmembers()discovers allExchangeBasesubclasses at runtime. A registry pattern would formalize this and addget_id()/ fail-fast behavior.instantiate_keystore()andinstantiate_keystore_from_text()could become thin wrappers around the registry for backward compatibility during migration.