Support Ledger clear signing #1603

franciszekjob · 2025-05-12T12:52:27Z

Closes #1473

Introduced changes

Add support for clear singing in LedgerSigner. Now we're getting displayed tx details before approveing/rejecting it.

Add LedgerSigningMode
Use clear signing by default

clear_sign_example.mov

This PR contains breaking changes

franciszekjob · 2025-05-12T13:18:52Z

starknet_py/constants.py

-# Ledger constants
-STARKNET_CLA = 0x5A
-EIP_2645_PURPOSE = 0x80000A55
-EIP_2645_PATH_LENGTH = 6
-PUBLIC_KEY_RESPONSE_LENGTH = 65
-SIGNATURE_RESPONSE_LENGTH = 65
-VERSION_RESPONSE_LENGTH = 3
-


note: These variables aren't used across multiple files so let's keep them in the dedicated (ledger) one.

franciszekjob · 2025-05-12T19:14:11Z

starknet_py/tests/unit/signer/test_ledger_signer.py

-    recipient_address = (
-        0x1323CACBC02B4AAED9BB6B24D121FB712D8946376040990F2F2FA0DCF17BB5B
-    )
+    recipient_address = 0x123


note: 0x123 is better as an example address.

franciszekjob · 2025-05-13T00:32:53Z

starknet_py/tests/unit/signer/speculos_automation.json

@@ -2,10 +2,13 @@
  "version": 1,
  "rules": [
    {
-      "text": "Confirm Hash to sign",
-      "conditions": [],
+      "text": "Blind igning",


note: It's not a typo - for some reason that's how ledger displays the text

franciszekjob · 2025-05-13T00:34:05Z

starknet_py/tests/unit/signer/test_ledger_signer.py

+@pytest.mark.parametrize(
+    "transaction",
+    [
+        InvokeV3(
+            version=3,
+            signature=[],
+            nonce=1,
+            resource_bounds=MAX_RESOURCE_BOUNDS_SEPOLIA,
+            calldata=[
+                1,
+                2009894490435840142178314390393166646092438090257831307886760648929397478285,
+                232670485425082704932579856502088130646006032362877466777181098476241604910,
+                3,
+                0x123,
+                100,
+                0,
+            ],
+            sender_address=0x123,
+        ),
+        DeployAccountV3(
+            class_hash=0x123,
+            contract_address_salt=0x123,
+            constructor_calldata=[1, 2, 3],
+            version=3,
+            signature=[],
+            nonce=0,
+            resource_bounds=MAX_RESOURCE_BOUNDS_SEPOLIA,
+        ),
+    ],
+)


note: We can't use mocks for clear singing as under the hood we're converting values from tx fields to bytes, and we would get TypeError: can't concat Mock to bytes.

starknet_py/net/signer/ledger_signer.py

ddoktorski · 2025-05-15T11:45:56Z

.github/workflows/checks.yml

-  DEVNET_SHA: fc5a2753a2eedcc27eed7a4fae3ecac08c2ca1b4
-  LEDGER_APP_SHA: db93a5e33d00bd44752fdc8c07aefcffa08d91c3
-  LEDGER_APP_DEV_TOOLS_SHA: a037d42181f4bed9694246256e2c9e2a899e775c302a9c6482c81f87c28e1432
+  DEVNET_SHA: fc5a2753a2eedcc27eed7a4fae3ecac08c2ca1b4 # v0.3.0


Not related to this PR, but why devnet is not updated to the latest version?

Nothing forced us to bump it, so it wasn't urgent. Bumped it to v0.4.1.

starknet_py/net/signer/ledger_signer.py

ddoktorski · 2025-05-15T14:07:13Z

starknet_py/tests/e2e/fixtures/devnet.py

@@ -52,13 +56,20 @@ def get_start_devnet_command(devnet_port: int, fork_mode: bool = False) -> List[
            ]
        )

+    if predeclare_argent:


nit: Why not to predeclare always?

Seems like nothing prevents use from having it always predeclared. Will change it.

docs/guide/signing.rst

ddoktorski · 2025-05-15T14:33:38Z

starknet_py/net/signer/ledger_signer.py


    def sign_transaction(self, transaction: AccountTransaction) -> List[int]:
+        if self.signing_mode == LedgerSigningMode.CLEAR:
+            if isinstance(transaction, DeclareV3):
+                raise ValueError("DeclareV3 signing is not supported bye LedgerSigner")


Do you know why declare is not supported?

Not really, haven't digged into the specific reason but maybe it's related with device hardware limits (considering that declare tx includes contract class) or security?

ddoktorski · 2025-05-15T14:49:29Z

starknet_py/net/signer/ledger_signer.py

+        account_address_bytes = contract_address.to_bytes(32, byteorder="big")
+        chain_id_bytes = self.chain_id.to_bytes(32, byteorder="big")
+        nonce_bytes = tx.nonce.to_bytes(32, byteorder="big")
+        common_tx_fields = tx.get_common_fields(
+            tx_prefix=TransactionHashPrefix.DEPLOY,
+            address=contract_address,
+            chain_id=self.chain_id,
+        )
+        da_mode_hash_bytes = common_tx_fields.get_data_availability_modes().to_bytes(
+            32, byteorder="big"
+        )
+        class_hash_bytes = tx.class_hash.to_bytes(32, byteorder="big")
+        salt_bytes = tx.contract_address_salt.to_bytes(32, byteorder="big")


nit: Maybe some kind of a helper function/struct to convert V3 transactions to the same struct but with fields as bytes?

Hmm, potentially we could add a method for txs classes which would return their values as bytes, but that would require additional struct as the return type (cause returning as tuple of bytes wouldn't be very convenient). Tbh I'm not sure if adding extra code would improve current state, as now it's fairly simple and readable. Wdyt?

starknet_py/net/signer/ledger_signer.py

ddoktorski · 2025-05-15T14:57:16Z

starknet_py/net/signer/ledger_signer.py

+    if application_name == "LedgerW":
+        application_bytes = (
+            (43).to_bytes(1, byteorder="big")
+            + (206).to_bytes(1, byteorder="big")
+            + (231).to_bytes(1, byteorder="big")
+            + (219).to_bytes(1, byteorder="big")
+        )
+    else:
+        application_bytes = _string_to_4byte_hash(application_name)


Could you explain this logic?

Seems like these four integers are predefined for LedgerW and in other cases we just generate the representation.
I referred to https://github.com/starknet-io/starknet.js/blob/050616221a58545d79eece7040b4abc205c220e0/src/signer/ledgerSigner221.ts#L644

…rknet.py into 1473-update-ledger-signer

This reverts commit bafe1a0.

franciszekjob added 5 commits May 12, 2025 14:47

Update SHAs for ledger repos

ffa255f

Add blind-signing allowance script

27d2d66

Implement clear signing

4cd9c84

Add client fixture with predeclared argent accounts

8494f0d

Update tests and automation rules

f1d158a

github-actions bot assigned franciszekjob May 12, 2025

franciszekjob added 4 commits May 12, 2025 15:02

update automation rules

0d254cc

Update poetry.lock

b208ea6

Fix typechecks

844f410

Fir formatting

b7ad26c

franciszekjob commented May 12, 2025

View reviewed changes

franciszekjob added 8 commits May 12, 2025 18:54

Update docs

9022364

Refactor call encoding

ccdee97

Add docstrings for LedgerSigningMode

55e6042

Include LedgerSigningMode in docs

f90450e

Update migration guide

c94b837

Fix linting

b4d2619

Fix Build the app inside Docker container step

02bf3c4

Fix typo

6a29cea

franciszekjob commented May 12, 2025

View reviewed changes

franciszekjob added 5 commits May 12, 2025 21:14

Update port

1d6a784

Remove "Reject" automation rule

ad39434

Fix automation rules

0cd5287

Formatting

7e237e8

Update order of automation rules

95468d1

franciszekjob commented May 13, 2025

View reviewed changes

starknet_py/net/signer/ledger_signer.py Show resolved Hide resolved

franciszekjob marked this pull request as ready for review May 13, 2025 08:05

Trigger CI

80807cd

franciszekjob requested a review from ddoktorski May 13, 2025 08:52

franciszekjob added 4 commits May 13, 2025 11:57

Temporarily skip test_deploy_account_and_transfer

b31777b

Temporarily skip test_create_account_with_ledger_signer

6a285a6

Temporarily skip rest of ledger tests

c7fcf72

Fix automation rules

8164c03

franciszekjob commented May 13, 2025

View reviewed changes

starknet_py/net/signer/ledger_signer.py Show resolved Hide resolved

ddoktorski reviewed May 15, 2025

View reviewed changes

franciszekjob added 7 commits May 15, 2025 18:20

Apply code review suggestions

b001bb4

Bump devnet

d20ff65

Merge branch 'development' of https://github.com/software-mansion/sta…

a8f8a9c

…rknet.py into 1473-update-ledger-signer

Fix typo

e518de7

Always predeclare argent

bafe1a0

Revert "Always predeclare argent"

37f7dc9

This reverts commit bafe1a0.

Fix typo

a38ba8f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Ledger clear signing #1603

Support Ledger clear signing #1603

franciszekjob commented May 12, 2025 •

edited

Loading

franciszekjob May 12, 2025 •

edited

Loading

franciszekjob May 12, 2025 •

edited

Loading

franciszekjob May 13, 2025 •

edited

Loading

franciszekjob May 13, 2025

ddoktorski May 15, 2025

franciszekjob May 15, 2025

ddoktorski May 15, 2025

franciszekjob May 15, 2025 •

edited

Loading

ddoktorski May 15, 2025

franciszekjob May 15, 2025 •

edited

Loading

ddoktorski May 15, 2025

franciszekjob May 15, 2025

ddoktorski May 15, 2025

franciszekjob May 15, 2025

Support Ledger clear signing #1603

Are you sure you want to change the base?

Support Ledger clear signing #1603

Conversation

franciszekjob commented May 12, 2025 • edited Loading

Introduced changes

franciszekjob May 12, 2025 • edited Loading

Choose a reason for hiding this comment

franciszekjob May 12, 2025 • edited Loading

Choose a reason for hiding this comment

franciszekjob May 13, 2025 • edited Loading

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

franciszekjob May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob May 15, 2025 • edited Loading

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

franciszekjob commented May 12, 2025 •

edited

Loading

franciszekjob May 12, 2025 •

edited

Loading

franciszekjob May 12, 2025 •

edited

Loading

franciszekjob May 13, 2025 •

edited

Loading

franciszekjob May 15, 2025 •

edited

Loading

franciszekjob May 15, 2025 •

edited

Loading