Implement Ledger #231
Implement Ledger #231
Conversation
nesitor
force-pushed
the
andres-feature-implement_ledger_wallet
branch
from
422e9b0 to
ef69187
Compare
Solution: Implement Ledger use on SDK to allow using them.
1yam
force-pushed
the
andres-feature-implement_ledger_wallet
branch
from
ef69187 to
9b8d8b6
Compare
|
Failed to retrieve llama text: POST 503: 503 Service UnavailableNo server is available to handle this request. |
src/aleph/sdk/conf.py
Outdated
| address: Optional[str] = None | ||
|
|
||
| model_config = SettingsConfigDict(use_enum_values=True) | ||
| # model_config = SettingsConfigDict(use_enum_values=True) |
There was a problem hiding this comment.
Not sure, that change was from @nesitor
| def __init__(self, account: LedgerAccount, device: Dongle): | ||
| def __init__( | ||
| self, account: LedgerAccount, device: Dongle, chain: Optional[Chain] = None | ||
| ): |
There was a problem hiding this comment.
For some reason you're not calling the __init__ method of ETHAccount (the base class), why? You also re-specify a bunch of constants like CHAIN or CURVE for no clear reason.
There was a problem hiding this comment.
The init method of EthAccount expects a private_key to initialize the account.
In our case, since we are interacting with a hardware wallet (Ledger), we don’t have or need access to the private key — the Ledger device handles signing internally.
The goal here is to preserve the interface and behavior of EthAccount so that this Ledger-based account can be used seamlessly like any other account within the SDK, without requiring code changes elsewhere.
As a result, the base class init isn’t called, since it would require a private_key we cannot provide.
And for CHAIN and CURVE aren’t needed, I’ll remove them.
There was a problem hiding this comment.
Okay but is it even reusing any method of ETHAccount? If it does, just create a base class for the methods used in both Ledger / non-Ledger accounts and inherit that from both then.
| def __init__(self, account: LedgerAccount, device: Dongle): | ||
| def __init__( | ||
| self, account: LedgerAccount, device: Dongle, chain: Optional[Chain] = None | ||
| ): |
There was a problem hiding this comment.
Okay but is it even reusing any method of ETHAccount? If it does, just create a base class for the methods used in both Ledger / non-Ledger accounts and inherit that from both then.
| def get_accounts( | ||
| device: Optional[Dongle] = None, count: int = 5 | ||
| ) -> List[LedgerAccount]: | ||
| """Initialize an aleph.im account from a LedgerHQ device from |
| account_type: Optional[Type[AccountFromPrivateKey]] = None, | ||
| chain: Optional[Chain] = None, | ||
| ) -> AccountFromPrivateKey: | ||
| ) -> Union[AccountFromPrivateKey, LedgerETHAccount]: |
There was a problem hiding this comment.
Should be a base class instead
|
|
||
| path: Path | ||
| path: Optional[Path] = None | ||
| type: Optional[AccountType] = AccountType.INTERNAL |
|
|
||
| class AccountType(str, Enum): | ||
| INTERNAL: str = "internal" | ||
| EXTERNAL: str = "external" |
There was a problem hiding this comment.
Internal/External is not really representative. Use Hot/Cold, Imported / Hardware or something like that.
Problem: Ledger wallet users cannot use Aleph to send transactions.
Solution: Implement Ledger use on SDK to allow using them.