-
Notifications
You must be signed in to change notification settings - Fork 747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ledger signer 211 #1246
base: develop
Are you sure you want to change the base?
Ledger signer 211 #1246
Conversation
txDetails: V2InvocationsSignerDetails, | ||
calls: Call[] | ||
): Promise<{ hash: bigint; signature: Signature }> { | ||
// APDU 0 for path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is APDU ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a Ledger wording, about a batch of bytes exchanged with a Nano.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of nitpicks but in general LGTM!
We would need to refactor this soon when v1 tx gets deprecated with RPC 0.8
@@ -354,12 +355,25 @@ describe('Ethereum signer', () => { | |||
describe('Ledger Signer', () => { | |||
// signature of Ledger can't be tested automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is some way with virtual ledger like
https://developers.ledger.com/docs/ledger-live/discover/integration/wallet-api/simulator/introduction
@@ -82,3 +82,7 @@ export const SNIP9_V1_INTERFACE_ID = | |||
'0x68cfd18b92d1907b8ba3cc324900277f5a3622099431ea85dd8089255e4181'; | |||
export const SNIP9_V2_INTERFACE_ID = | |||
'0x1d1144bb2138366ff28d8e9ab57456b1d332ac42196230c3a602003c89872'; | |||
|
|||
// Ledger signer | |||
export const HARDENING_BYTE = Number('0x80'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick, Can we store this as a static constant
export const HARDENING_BYTE = Number('0x80'); | |
// 0x80 | |
export const HARDENING_BYTE = 128; |
|
||
// Ledger signer | ||
export const HARDENING_BYTE = Number('0x80'); | ||
export const HARDENING_4BYTES = BigInt('0x80000000'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick , Same
export const HARDENING_4BYTES = BigInt('0x80000000'); | |
// 0x80000000 | |
export const HARDENING_4BYTES = 2147483648n |
@@ -24,17 +24,33 @@ export function hashDAMode(nonceDAMode: BigNumberish, feeDAMode: BigNumberish) { | |||
return (BigInt(nonceDAMode) << DATA_AVAILABILITY_MODE_BITS) + BigInt(feeDAMode); | |||
} | |||
|
|||
export function hashFeeField(tip: BigNumberish, bounds: ResourceBounds) { | |||
const L1Bound = | |||
export function encodeResourceBoundsL1(bounds: ResourceBounds): bigint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add desc, and note what this encoding is for
Ledger informed me that they are preparing a new version, including a human readable account deployment . |
Motivation and Resolution
Ledger has released a new version 2.1.1 of the Starknet APP.
It includes improvements, including a comprehensive display of the details of V1 & V3 transactions.
Starknet.js is now able to use APP v1.1.1 & v2.1.1.
Usage related changes
Important
No breaking changes for code using Ledger APP v1.1.1 (just no comprehensive display, only blind signatures)
No change of the code :
LedgerSigner
andLedgerSigner111
classes are equivalent.They can also use the v1.1.1 signer ; they will only not have access to the display of transaction details.
Development related changes
LedgerSigner
andgetLedgerPathBuffer
are still there, and are related to v1.1.1 APP.They are now also named
LedgerSigner111
andgetLedgerPathBuffer111
.LedgerSigner211
andgetLedgerPathBuffer211
have been created to handle v2.1.1.As several functions are common for both signers, the
LedgerSigner211
is an extension of theLedgerSigner111
.Some maths included in
hashFeeField
(utils/hash/transactionHash/v3.ts) are necessary, and have been isolated in exportable functions.The new signer has been tested successfully in Node ( https://github.com/PhilippeR26/starknet.js-workshop-typescript/blob/main/src/scripts/ledgerNano/6.testLedgerAccount211.ts ) and in a demo DAPP ( https://github.com/PhilippeR26/Starknet-Ledger-Wallet )
Checklist: