fix DepositTransaction::rlp() to match op-geth

[noot]

Motivation

The RLP encoding for Optimism DepositTransactions did not match the op-geth implementation. Deposit transactions created and encoded by ethers-rs were previously not being accepted by op-geth due to this.

Solution

Fix DepositTransaction::rlp() to match op-geth DepositTx::encode(). The types are now encoded to match op-geth: https://github.com/ethereum-optimism/op-geth/blob/optimism/core/types/deposit_tx.go#L29

I generated the expected Go output as follows (place this in op-geth core/types/deposit_tx_test.go):

package types

import (
	"bytes"
	"encoding/hex"
	"math/big"
	"testing"

	"github.com/ethereum/go-ethereum/common"
)

func TestEncodeDepositTx(t *testing.T) {
	to := common.HexToAddress("0xa0Ee7A142d267C1f36714E4a8F75612F20a79720")
	depositTx := &DepositTx{
		SourceHash:          common.HexToHash("0x7113be8bbb6ff4bb99fae05639cf76cdecf5a1afbc033b9a01d8bb16b00b9a80"),
		From:                to,
		To:                  &to,
		Mint:                big.NewInt(10000000000000000),
		Value:               big.NewInt(10000000000000000),
		Gas:                 21000,
		IsSystemTransaction: false,
		Data:                nil,
                // for the with_data test 
                // Data:                []byte("nootwashere"),
	}
	bytes := &bytes.Buffer{}
	depositTx.encode(bytes)
	t.Log(hex.EncodeToString(bytes.Bytes()))
}

Note: the existing test test_rlp_encode_deposited_tx is probably not exactly correct? Should I remove/update it?

PR Checklist

• Added Tests
• Added Documentation
• Breaking changes

[mattsse]

I feel like the entire optimism integration is a bit horrible...

hmm, I wonder why these fields are even options.

https://github.com/ethereum-optimism/optimism/blob/65ec61dde94ffa93342728d324fecf474d228e1f/specs/deposits.md#l1-attributes-deposited-transaction

but if all of them are set, then this change doesn't have an effect right, because rlp_opt simply calls append, right?

and it looks like the decode function is current because it expects all the fields

so I guess the real question is, should this not even accept options

[noot]

@mattsse good question, I don't think source_hash and is_system_tx should be Options since they're not in the Go impl. Not sure about mint (the Go impl has it tagged as rlp:"nil" - let me test that)

but if all of them are set, then this change doesn't have an effect right, because rlp_opt simply calls append, right?

the result i'm getting (before the change) is:
f863a07113be8bbb6ff4bb99fae05639cf76cdecf5a1afbc033b9a01d8bb16b00b9a80d594a0ee7a142d267c1f36714e4a8f75612f20a7972094a0ee7a142d267c1f36714e4a8f75612f20a79720872386f26fc10000c8872386f26fc10000c38252088080

compared to the go result of:
f860a07113be8bbb6ff4bb99fae05639cf76cdecf5a1afbc033b9a01d8bb16b00b9a8094a0ee7a142d267c1f36714e4a8f75612f20a7972094a0ee7a142d267c1f36714e4a8f75612f20a79720872386f26fc10000872386f26fc100008252088080

this seems strange, because you're right, it should just be encoding the value inside the option. i'll investigate more

[noot]

update, the problems were

• self.tx.from (it's an option, but needs to be encoded as not an option)
• self.tx.value (it's an option, but needs to be encoded as not an option)
• self.tx.gas (it's an Option<U256> but needs to be encoded as u64)

I left source_hash and is_system_tx as unwrap_or_default because they shouldn't be encoded as None ever imo. (maybe we should change those to not be options)

[mattsse]

I see, I think another problem is Data, which looks like a non optional []byte which is always encoded?

and iirc an empty []byte is encoded as an rlp header, but maybe that's what rlp.append("") does

[noot]

yep i also changed data to use unwrap_or_default, which matches the go encoding when you set it to nil, so i think that's ok - i'm guessing go rlp-encodes []byte(nil) and []byte{} the same?

[mattsse]

I left source_hash and is_system_tx as unwrap_or_default because they shouldn't be encoded as None ever imo.

yeah, they are required:

https://github.com/anton-rs/op-reth/blob/clabby/fixes/crates/primitives/src/transaction/optimism.rs#L12

(maybe we should change those to not be options)

agree

[mattsse]

this looks correct, since all required fields are now encoded.

I think we should consider making them non optional.

pending @DaniPopes

[mattsse]

ah yes, we need to unwrap the from field because deposit txs don't have a signature...

[mattsse]

I believe rlp_opt was correct here because it falls back to rlp.append("") which is equivalent to rlp.append(&[])

but this is better

[mattsse]

same here, rlp_opt would fallback to an empty string and 0 is also encoded as empty string code I believe.

but this is better.

[noot]

@mattsse i went ahead and updated source_hash and is_system_tx to not be options, let me know if you want me to leave it or change back

[mattsse]

I think that's the right decision

[noot]

@mattsse what's a good move for fixing the serde_txpool_content test? it appears to me that the existing json-string in that test should only be using if #[cfg(not(feature = "optimism"))], i can make another json string for if the optimism feature is enabled (or just throw a #[cfg(not(feature = "optimism"))] on the whole test?)

[mattsse]

@noot sorry, for late reply,

we can remove this test entirely, no longer needed since we're transitioning away from these types for alloy migration anyway

[noot]

@mattsse np, test removed!

[mattsse]

fyi @merklefruit