-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
fix: ledger vrs values #27966
base: feat/9544
Are you sure you want to change the base?
fix: ledger vrs values #27966
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
return Transaction.fromSerializedTx(txBuffer, { | ||
const VALID_TYPES = [1, 2]; | ||
const txType = VALID_TYPES.includes(rawTx[0]) ? rawTx[0] : null; | ||
const rlpData = txType === null ? txBuffer : txBuffer.slice(1, tx.length); |
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.
Interesting, so why are we removing the transaction type of supported transactions?
What would happen here with a pre-EIP-1559 transaction, or a non-supported transaction type?
@@ -173,8 +173,14 @@ export class FakeLedgerBridge extends FakeKeyringBridge { | |||
// common, | |||
// }).sign(Buffer.from(KNOWN_PRIVATE_KEYS[0], 'hex')); | |||
|
|||
//removing r, s, v values from the unsigned tx |
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.
Nit: Maybe worth explaining why we're doing this. e.g. that Ledger uses v
to communicate the chain ID, but we're removing it because these values are not a valid signature at this point.
Also, thoughts on setting chainId
based on v
rather than hard-coding it? It would make the test more realistic (e.g. we would want this to fail if v
isn't 1337
, because a real Ledger signature would fail in that circumstance as well.)
Description
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist