Add missing depositTxWithNonce case to MarshalJSON / SourceHash / Mint

[mdehoog]

Description

#55 added nonce support for deposit txs to UnmarshalJSON. However, we never added support for MarshalJSON, which means marshalling depositTxWithNonce values results in mostly null values.

Also added the depositTxWithNonce case to the Transaction.SourceHash and Transaction.Mint functions.

Tests

Tested locally.

[mdehoog]

there are also other places we don't account for depositTxWithNonce, like

op-geth/core/types/transaction.go

Lines 339 to 356 in d5a9661

	// SourceHash returns the hash that uniquely identifies the source of the deposit tx,
	// e.g. a user deposit event, or a L1 info deposit included in a specific L2 block height.
	// Non-deposit transactions return a zeroed hash.
	func (tx *Transaction) SourceHash() common.Hash {
	if dep, ok := tx.inner.(*DepositTx); ok {
	return dep.SourceHash
	}
	return common.Hash{}
	}
	// Mint returns the ETH to mint in the deposit tx.
	// This returns nil if there is nothing to mint, or if this is not a deposit tx.
	func (tx *Transaction) Mint() *big.Int {
	if dep, ok := tx.inner.(*DepositTx); ok {
	return dep.Mint
	}
	return nil
	}

. Not sure if we care about these, but they do make these fields inaccessible when the txs are retrieved from the API.

[mdehoog]

Decided to add these 2 cases as well.

[ajsutton]

When this was first implemented, it was only ever used (and only ever intended to be used) as part of unmarshalling. It was there to allow ethclient to expose the extra nonce field returned by deposit transactions. I don't think there should ever be depositTxWtihNonce instances created in other places, but things may have changed.

Where are you seeing depositTxWithNonce be serialised to JSON?

[mdehoog]

@ajsutton at one point I was passing a types.Transactions to a JSON RPC method, and when it was deserialized on the server side the deposit tx was mostly empty. Tracked it down to this issue. I've since switched to RLP encoding so isn't biting me anymore, but I don't see a reason we shouldn't fix.

Here's a clearer example. Working curl:

> curl -d '{"id":0,"jsonrpc":"2.0","method":"eth_getTransactionByHash","params":["0xd997de74d8811229534c858983b5374bc5a9e93628c6ade2ef25618a64031a9d"]}' -H "Content-Type: application/json" https://mainnet.base.org | jq

{
  "jsonrpc": "2.0",
  "result": {
    "blockHash": "0x46229f33f7edae5fee95599ba5a3f8925e146bf0651cb0dec4441f53889f4431",
    "blockNumber": "0x13db7d9",
    "depositReceiptVersion": "0x1",
    "from": "0x6ce6e24c7469aae61548c15c235389ac8f1ea10a",
    "gas": "0x186a0",
    "gasPrice": "0x0",
    "hash": "0xd997de74d8811229534c858983b5374bc5a9e93628c6ade2ef25618a64031a9d",
    "input": "0x",
    "mint": "0x4e28e2290f0000",
    "nonce": "0x799",
    "r": "0x0",
    "s": "0x0",
    "sourceHash": "0x9eb95bf237e38791cb4f83959620a2e2fe09da07d5613677ccf97bf56f29aba1",
    "to": "0x6ce6e24c7469aae61548c15c235389ac8f1ea10a",
    "transactionIndex": "0x1",
    "type": "0x7e",
    "v": "0x0",
    "value": "0x4e28e2290f0000"
  },
  "id": 0
}

Broken golang:

package main

import (
	"context"
	"fmt"

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

func main() {
	ctx := context.Background()
	client, _ := ethclient.DialContext(ctx, "https://mainnet.base.org")
	tx, _, _ := client.TransactionByHash(ctx, common.HexToHash("0xd997de74d8811229534c858983b5374bc5a9e93628c6ade2ef25618a64031a9d"))
	fmt.Println(tx.Mint())
	fmt.Println(tx.SourceHash())
}

Output:

<nil>
0x0000000000000000000000000000000000000000000000000000000000000000

[ajsutton]

I suspect we've broken something along the way. My understanding (might be wrong - it's going back a fair while) is that we should never have an instance of depositTxWithNonce on the server side - only the client side.

The SourceHash and Mint not being updated is a bug and I can understand those. We shouldn't ever need MarshalJSON for depositTxWithNonce based on my understanding. I'm not against adding it if it's useful, just wanting to make sure there isn't something bigger wrong that I'm missing.

It sounds like somewhere along the way you were getting a depositTxWithNonce on the server side? Was that because something was fetched with ethclient initially and then passed to the RPC server or was that coming from geth's internal directly?

[mdehoog]

Was that because something was fetched with ethclient initially and then passed to the RPC server

Yes, exactly this.

Sorry, I should have given the example usage. I have a service running in an enclave that accepts transactions as input, see here: https://github.com/mdehoog/op-enclave/blob/c6c89e7a171ed2b883fb6488686a9026ef856b05/enclave/server.go#L236-L238

I've since switched to RLP (see base-org/op-enclave@3b7ae3f), partly as a workaround for this issue.

[ajsutton]

Cool, that makes sense to me and settles my nerves. Definitely makes sense to support serialising to JSON so client users can take the tx they got from the API and re-serialize it. Thanks for bearing with me to understand it all.