fix: ledger vrs values #27966
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.
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.
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