accounts/usbwallet: support dynamic tx #30180
Conversation
| @@ -353,7 +367,9 @@ func (w *ledgerDriver) ledgerSign(derivationPath []uint32, tx *types.Transaction | |||
| // Chunk size selection to mitigate an underlying RLP deserialization issue on the ledger app. | |||
| // https://github.com/LedgerHQ/app-ethereum/issues/409 | |||
| chunk := 255 | |||
| for ; len(payload)%chunk <= ledgerEip155Size; chunk-- { | |||
| if tx.Type() == types.LegacyTxType { | |||
There was a problem hiding this comment.
I am not sure if this is need for DynamicTx. Does this bug exist in DynamicTx too or it only exist in EIP155?
There was a problem hiding this comment.
The workaround can be left in place for all tx types. It's just a way of modifying the transfer so it doesn't trigger a certain bug in the firmware.
There was a problem hiding this comment.
@fjl this doesn't seems to work for other tx. This could very well be due to the way typed tx are formed.
if tx.Type() == types.DynamicFeeTxType {
if txrlp, err = rlp.EncodeToBytes([]interface{}{chainID, tx.Nonce(), tx.GasTipCap(), tx.GasFeeCap(), tx.Gas(), tx.To(), tx.Value(), tx.Data(), tx.AccessList()}); err != nil {
return common.Address{}, nil, err
}
// append type to transaction
txrlp = append([]byte{tx.Type()}, txrlp...)
}
I am not sure how to handle this bug here.
Ledger throws error
transaction type not supported
There was a problem hiding this comment.
So, I was going to ask you to show some snippet from ledger-docs or something, to demonstrate that they do accept this format, where you prepend the type into the payload like this.
But, seeing as the ledger throws transaction type not supported -- doesn't this mean that this PR is moot, and that the ledger (at least with the software version you are using) does not support signing non-legacy transactions?
There was a problem hiding this comment.
@holiman So the error transaction type not supported appears when I apply for ; len(payload)%chunk <= ledgerEip155Size; chunk-- logic for non legacy tx. If I don't do it - Ledger signing works. I have tested with my Ledger nano S. Let me find relevant ledger code
There was a problem hiding this comment.
Here you go @holiman https://github.com/LedgerHQ/app-ethereum/blob/develop/src/ethUstream.c#L300 - from what I understand it supports it.
shrimalmadhur
changed the title
|
I am also not sure why the ci test is failing. doesn't seem related? How can I rerun it? |
|
Hey @gballet I saw you assigned this task to yourself. Wondering when you can take a look at it? |
|
@gballet was assigned because he has a Ledger to test this with. |
Yeah I also have a few, for testing, so I can also give this a spin |
awesome! thanks. let me know how your testing goes. |
Fixes #30173