Add support for ledger #195

moisses89 · 2023-02-11T21:20:32Z

Description

Closes #269

add load_ledger_cli_owners to add owner accounts from ledger
add ledger_manager to manage connection and communication with ledger using ledgereth library
ledger_manager also includes sign_eipt712 to sign domain_hash and message_hash without enable blind signature
add hw_exceptions to handle exceptions from signing devices
add ledger_manager tests

coveralls · 2023-02-11T21:32:28Z

Pull Request Test Coverage Report for Build 4153242346

35 of 75 (46.67%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.4%) to 52.229%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
safe_cli/prompt_parser.py	4	5	80.0%
safe_cli/operators/hw_accounts/ledger_account.py	19	35	54.29%
safe_cli/operators/safe_operator.py	12	35	34.29%

Totals
Change from base Build 4046181829:	-0.4%
Covered Lines:	738
Relevant Lines:	1413

💛 - Coveralls

vzctl · 2023-03-22T11:30:15Z

Hi! Thanks for this PR - amazing work!

We (in Aurora Labs) have tested load_ledger_cli_owners function but unfortunately it only loads one owner at a time (with multiple ledgers/owners connected simultaneously).

Is it a known limitation? Is there a possible workaround?

moisses89 · 2023-03-23T11:31:01Z

Hi! Thanks for this PR - amazing work!

We (in Aurora Labs) have tested load_ledger_cli_owners function but unfortunately it only loads one owner at a time (with multiple ledgers/owners connected simultaneously).

Is it a known limitation? Is there a possible workaround?

Good point, I should test it, for now I was only testing one ledger.
I think that could be some limitations related with "ledger driver" and SO integration, but it is something interesting to test.

coveralls · 2023-10-20T09:50:53Z

coverage: 92.284% (+0.4%) from 91.927% when pulling 91a7a9d on add_ledger_owner into 09e8507 on master.

safe_cli/operators/hw_accounts/ledger_account.py

Uxio0 · 2023-10-20T10:21:01Z

safe_cli/operators/hw_accounts/ledger_account.py

+            "signTransaction is deprecated in favor of sign_transaction",
+            category=DeprecationWarning,
+        )
+        pass


Suggested change

pass

return self.sign_transaction(transaction_dict)

safe_cli/operators/safe_operator.py

requirements.txt

* add tests

Refactor ledger sign_eip712 to be used in other places

moisses89 · 2023-10-26T16:14:41Z

Hi! Thanks for this PR - amazing work!

We (in Aurora Labs) have tested load_ledger_cli_owners function but unfortunately it only loads one owner at a time (with multiple ledgers/owners connected simultaneously).

Is it a known limitation? Is there a possible workaround?

@vzctl with this new approach you can load more than one owner from ledger but you can only connect one ledger device.

Uxio0

Really nice, some small suggestions

README.md

requirements.txt

safe_cli/operators/hw_accounts/hw_exceptions.py

safe_cli/operators/hw_accounts/ledger_manager.py

Uxio0 · 2023-10-27T15:00:27Z

safe_cli/operators/safe_operator.py

@@ -325,6 +335,34 @@ def load_cli_owners(self, keys: List[str]):
            except ValueError:
                print_formatted_text(HTML(f"<ansired>Cannot load key={key}</ansired>"))

+    def load_ledger_cli_owners(self, legacy_account: bool = False):


Return type

Maybe for next iteration we should allow people to use a custom derivation path like:

> load_ledger_cli_owners DERIVATION_PATH

Uxio0 · 2023-10-27T15:04:41Z

safe_cli/utils.py

+    question: str, number_options: int, default_option: int = 0
+) -> bool:
+    if "PYTEST_CURRENT_TEST" in os.environ:
+        return 0  # Ignore confirmations when running tests


Suggested change

return 0 # Ignore confirmations when running tests

return default_option # Ignore confirmations when running tests

Uxio0 · 2023-10-27T15:05:46Z

safe_cli/utils.py

+    if "PYTEST_CURRENT_TEST" in os.environ:
+        return 0  # Ignore confirmations when running tests
+    choices = f" [0-{number_options}] default {default_option}:"
+    reply = str(input(question + choices)).lower().strip() or str(default_option)


Use get_input so you can easily mock the function for testing. Please add a test for this function

Uxio0 · 2023-10-27T15:07:07Z

safe_cli/utils.py

+        return 0  # Ignore confirmations when running tests
+    choices = f" [0-{number_options}] default {default_option}:"
+    reply = str(input(question + choices)).lower().strip() or str(default_option)
+    option = int(reply)


What happens if reply cannot be converted to a number?

Uxio0 · 2023-10-27T15:07:23Z

setup.py

        "tabulate>=0.8",
    ],
+    extras_require={"ledger": ["ledgereth==0.9.0"]},


Co-authored-by: Uxío <Uxio0@users.noreply.github.com>

Uxio0 · 2023-10-30T09:35:44Z

safe_cli/operators/hw_accounts/exceptions.py

@@ -7,25 +7,25 @@
    LedgerNotFound,
 )

-from safe_cli.operators.safe_operator import HwDeviceException
+from safe_cli.operators.exceptions import HardwareWalletException


Nice, using a exception file we prevent a posible circular dependency

JagoFigueroa · 2023-10-30T12:49:09Z

Hola team! Small detail, the command available on the readme for installing ledger support is not the correct one, should be:

pip install '.[ledger]'

Cheers!

Uxio0 · 2023-10-30T13:44:51Z

Hola team! Small detail, the command available on the readme for installing ledger support is not the correct one, should be:

pip install '.[ledger]'

Thanks, but that command is only for local development and using the last version. On the README we only have the command to use the last official release 😉

JagoFigueroa · 2023-10-31T10:15:37Z

Checking delegation! I see that in order to add or remove a delegate to a safe we need to be on tx-service mode and those commands look to be executed directly (something not possible to do from a ledger at this point).

tx-service > 0x6D04edC44F7C88faa670683036edC2F6FC10b86e > show_cli_owners
Account 0x454a478d2482C1d251F149424053d4485aDD3dC5 loaded
Not default sender set 
tx-service > 0x6D04edC44F7C88faa670683036edC2F6FC10b86e > add_delegate 0x357742b4799b3481dDbed8203CA06bFbe9Ca056E test 0x454
a478d2482C1d251F149424053d4485aDD3dC5
Account 0x454a478d2482C1d251F149424053d4485aDD3dC5 is not loaded
tx-service > 0x6D04edC44F7C88faa670683036edC2F6FC10b86e >

Here is me trying to do so from a ladger. My guess is that it fails as expected but maybe it is worth it to create a separate improvement to change that owner not loaded message as that is not the case for something referencing that it is not possible to add / remove delegates from a ledger?

Account 0x454a478d2482C1d251F149424053d4485aDD3dC5 is not loaded

(or something more generic for when we implement trezor too).

Cheers!

JagoFigueroa · 2023-10-31T10:55:53Z

Other than that Ledger is working nicely for me on the CLI 😄

moisses89 · 2023-10-31T13:13:20Z

@JagoFigueroa thanks for your review :)
As the delegates issue that you mentioned is not something critical I created a separate task to implement it. #290

moisses89 force-pushed the add_ledger_owner branch from 7f598e8 to 7eadc69 Compare February 11, 2023 21:24

moisses89 force-pushed the add_ledger_owner branch from e79fadd to 4296029 Compare April 21, 2023 16:54

Uxio0 force-pushed the add_ledger_owner branch from 4296029 to 893a298 Compare May 9, 2023 09:28

moisses89 force-pushed the add_ledger_owner branch from 893a298 to 869b410 Compare October 20, 2023 09:47

Uxio0 reviewed Oct 20, 2023

View reviewed changes

safe_cli/operators/hw_accounts/ledger_account.py Outdated Show resolved Hide resolved

Uxio0 reviewed Oct 20, 2023

View reviewed changes

safe_cli/operators/safe_operator.py Outdated Show resolved Hide resolved

Uxio0 reviewed Oct 20, 2023

View reviewed changes

requirements.txt Show resolved Hide resolved

moisses89 force-pushed the add_ledger_owner branch from 6d7963a to 369fe15 Compare October 23, 2023 17:27

moisses89 and others added 7 commits October 24, 2023 10:28

Add ledger wallet class

38ed638

Add load_ledger_cli_owners function

d3404b1

Add sign with ledger

5e08783

Add ledger-eth-lib

bad9e9f

Fix lint issues

adc4fc4

Add test_load_ledger_cli_owners

8692959

Add sign_transaction

0dc3df9

moisses89 force-pushed the add_ledger_owner branch from 369fe15 to 723daba Compare October 24, 2023 08:29

Add ledger manager

ca57e20

moisses89 force-pushed the add_ledger_owner branch from 723daba to ca57e20 Compare October 24, 2023 08:45

moisses89 added 3 commits October 25, 2023 21:23

Add HwException

c54110e

* add tests

Add delete_account

e0ca8e8

Refactor ledger sign_eip712 to be used in other places

Add submit signature with ledger

0e7a964

moisses89 force-pushed the add_ledger_owner branch from e5facc4 to 208c487 Compare October 26, 2023 13:17

Add ledger module documentation

386d54d

moisses89 force-pushed the add_ledger_owner branch from 208c487 to 386d54d Compare October 26, 2023 13:19

Fix typo

cb2e5a9

Add load_leger_cli legacy-accounts option

7755051

moisses89 marked this pull request as ready for review October 26, 2023 13:45

moisses89 requested a review from Uxio0 October 26, 2023 13:50

Uxio0 requested changes Oct 27, 2023

View reviewed changes

moisses89 and others added 4 commits October 30, 2023 09:39

Update README.md

ecb0706

Co-authored-by: Uxío <Uxio0@users.noreply.github.com>

Update requirements.txt

9ff417f

Co-authored-by: Uxío <Uxio0@users.noreply.github.com>

Refactor exceptions

56b79fe

Add load_ledger_manager function

01e9b22

Uxio0 reviewed Oct 30, 2023

View reviewed changes

Uxio0 approved these changes Oct 30, 2023

View reviewed changes

Add test_choose_option_question

91a7a9d

moisses89 force-pushed the add_ledger_owner branch from 8973866 to 91a7a9d Compare October 30, 2023 09:52

moisses89 merged commit 319991a into master Oct 30, 2023
13 checks passed

moisses89 deleted the add_ledger_owner branch October 30, 2023 10:03

github-actions bot locked and limited conversation to collaborators Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ledger #195

Add support for ledger #195

moisses89 commented Feb 11, 2023 •

edited

coveralls commented Feb 11, 2023 •

edited

vzctl commented Mar 22, 2023

moisses89 commented Mar 23, 2023

coveralls commented Oct 20, 2023 •

edited

Uxio0 Oct 20, 2023

moisses89 commented Oct 26, 2023

Uxio0 left a comment

Uxio0 Oct 27, 2023

Uxio0 Oct 27, 2023

Uxio0 Oct 27, 2023

Uxio0 Oct 27, 2023

Uxio0 Oct 27, 2023

Uxio0 Oct 27, 2023

Uxio0 Oct 30, 2023

JagoFigueroa commented Oct 30, 2023

Uxio0 commented Oct 30, 2023

JagoFigueroa commented Oct 31, 2023

JagoFigueroa commented Oct 31, 2023

moisses89 commented Oct 31, 2023

	return 0 # Ignore confirmations when running tests
	return default_option # Ignore confirmations when running tests

Add support for ledger #195

Add support for ledger #195

Conversation

moisses89 commented Feb 11, 2023 • edited

Description

coveralls commented Feb 11, 2023 • edited

Pull Request Test Coverage Report for Build 4153242346

💛 - Coveralls

vzctl commented Mar 22, 2023

moisses89 commented Mar 23, 2023

coveralls commented Oct 20, 2023 • edited

Choose a reason for hiding this comment

moisses89 commented Oct 26, 2023

Uxio0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JagoFigueroa commented Oct 30, 2023

Uxio0 commented Oct 30, 2023

JagoFigueroa commented Oct 31, 2023

JagoFigueroa commented Oct 31, 2023

moisses89 commented Oct 31, 2023

moisses89 commented Feb 11, 2023 •

edited

coveralls commented Feb 11, 2023 •

edited

coveralls commented Oct 20, 2023 •

edited