Deposits: verify correct data processing in x/rollup processL1UserDeposits()

[NiloCK]

I suspect (85% confidence) the unmarshal of deposit tx data in processL1UserDepositTxs is not recovering the data it actually wants.

Specifically:

monomer/x/rollup/keeper/deposits.go

Line 59 in cff062f

var tx ethtypes.Transaction

tx is a vanilla Ethereum transaction. We access its To and Value fields and treat them as if they were the specified parameters of the deposit.

monomer/x/rollup/keeper/deposits.go

Lines 73 to 79 in cff062f

	to := tx.To()
	// if the receipient is nil, it means the tx is creating a contract which we don't support, so return an error.
	// see https://github.com/ethereum-optimism/op-geth/blob/v1.101301.0-rc.2/core/state_processor.go#L154
	if to == nil {
	ctx.Logger().Error("Contract creation txs are not supported", "index", i)
	return nil, types.WrapError(types.ErrInvalidL1Txs, "Contract creation txs are not supported, index:%d", i)
	}

monomer/x/rollup/keeper/deposits.go

Line 81 in cff062f

mintAmount := sdkmath.NewIntFromBigInt(tx.Value())

... but they are not. Here, To will be the address of the L1OptimismPortal contract. Value will be the eth value of the depositing tx, which is distinct from the minting order of the deposit.

Instead, we want the values from the encoded calldata of that tx.

[NiloCK]

@AnkushinDaniil You mentioned you were working on this.

I wasn't able to assign you, so I self-assigned to prevent someone else from picking up. Feel free to ping me on a review.

[natebeauregard]

I investigated this issue, identified some errors in the x/rollup deposit flow, and enumerated some next steps for @AnkushinDaniil to take a look at. Let me know if you want to sync up to pair on this or if you want any additional information from the investigation.

Background

The op-node uses the opaqueData emitted by the OptimismPortal contract to build a user deposit tx before monomer picks it up and forwards it to the x/rollup module.

Deposit txs contain Mint and Value fields and they're meant to be used for different purposes.

The Mint field is set by the amount of ETH a user sent to the OptimismPortal contract on L1 and is meant to represent the newly created ETH minted to the deposit tx's From address on L2. For example, op-geth handles the deposit tx processing in state_transition.go and mints the expected deposit tx Mint amount directly to the sender's L2 balance.

The optional Value field specified in a deposit tx is expected to be a value transferred from the sender's L2 ETH balance (From) to the target address's L2 ETH balance (To). This transfer is expected to take place after any ETH is minted to the sender.

Errors in current x/rollup user deposit flow

Currently, monomer uses the Value field instead of the Mint field to mint L2 ETH and mints to the deposit tx's To address rather than the sender address (From). This is incorrect as any mint value can be specified in the deposit tx (see #183), even if the user doesn't have the funds on L1.

Also, there is no current implementation for transferring L2 ETH from the L2 sender address (From) to the L2 target adress (To).

Expected x/rollup user deposit flow changes to implement

(Feel free to split these changes up into separate PRs if that makes sense)

• Use the Mint field instead of the Value field and the From address instead of the To address whenever minting ETH to an L2 user
  • To get the From address you'll probably need to use something like types.NewEIP155Signer to get the sender address of the tx
• Add support for transferring L2 ETH specified in the Value field from the sender address (From) to the target address (To). This should occur after minting takes place.
  • We'll want to be mindful of error cases here. If ETH is successfully minted but the Value transfer errors out do we want to revert the entire deposit and permanently lock the user's funds on L1 or should we only revert the transfer? It's probably best to look into how op-geth handles this case and do the same thing