feat: implement conversion of optimism deposit tx from alloy to reth #8763

zobront · 2024-06-11T21:14:01Z

The alloy_compat feature in reth/primitives contains conversion functions from Alloy types to Reth types. When converting transactions, the Optimism deposit tx type is currently unimplemented. This PR contains an implementation.

Questions:

How do you all like to handle tests for functions like this? I've tested manually in another crate, but to add a test directly to the alloy_compat.rs file would require importing a lot of machinery. Is there a better way to handle it?
The errors for this function are imported from Alloy, and the enum currently doesn't contain some errors I need. I've stubbed it out with inaccurate errors. I assume best process to submit PR to Alloy to add those errors, then update them here before merging?

mattsse

can we add a test using:

{
  "blockHash": "0xef664d656f841b5ad6a2b527b963f1eb48b97d7889d742f6cbff6950388e24cd",
  "blockNumber": "0x73a78fd",
  "depositReceiptVersion": "0x1",
  "from": "0x36bde71c97b33cc4729cf772ae268934f7ab70b2",
  "gas": "0xc27a8",
  "gasPrice": "0x0",
  "hash": "0x0bf1845c5d7a82ec92365d5027f7310793d53004f3c86aa80965c67bf7e7dc80",
  "input": "0xd764ad0b000100000000000000000000000000000000000000000000000000000001cf5400000000000000000000000099c9fc46f92e8a1c0dec1b1747d010903e884be100000000000000000000000042000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007a12000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000e40166a07a0000000000000000000000000994206dfe8de6ec6920ff4d779b0d950605fb53000000000000000000000000d533a949740bb3306d119cc777fa900ba034cd52000000000000000000000000ca74f404e0c7bfa35b13b511097df966d5a65597000000000000000000000000ca74f404e0c7bfa35b13b511097df966d5a65597000000000000000000000000000000000000000000000216614199391dbba2ba00000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
  "mint": "0x0",
  "nonce": "0x74060",
  "r": "0x0",
  "s": "0x0",
  "sourceHash": "0x074adb22f2e6ed9bdd31c52eefc1f050e5db56eb85056450bccd79a6649520b3",
  "to": "0x4200000000000000000000000000000000000007",
  "transactionIndex": "0x1",
  "type": "0x7e",
  "v": "0x0",
  "value": "0x0"
}

or pick any other deposit tx: https://optimistic.etherscan.io/address/0x4200000000000000000000000000000000000007

mattsse · 2024-06-11T22:04:16Z

crates/primitives/src/alloy_compat.rs

+use crate::{
+    transaction::TxDeposit,
+    constants::OP_SYSTEM_TX_FROM_ADDR,
+};


it'd like to use full paths and not feature gate imports

mattsse · 2024-06-11T22:05:25Z

crates/primitives/src/alloy_compat.rs

-            Some(TxType::Deposit) => todo!(),
+            Some(TxType::Deposit) => {
+                Ok(Self::Deposit(TxDeposit {
+                    // TODO: Uses the wrong error here, need alloy ConversionError enum updated with new errors for this.


do you want to add a Custom(String) variant to the alloy error?

i think best to just add ConversionError::MissingSourceHash, ConversionError::InvalidSourceHash, and ConversionError::InvalidMintValue for consistency.

mattsse · 2024-06-11T22:22:05Z

crates/primitives/src/alloy_compat.rs

+            Some(TxType::Deposit) => {
+                Ok(Self::Deposit(TxDeposit {
+                    // TODO: Uses the wrong error here, need alloy ConversionError enum updated with new errors for this.
+                    source_hash: match &tx.other["sourceHash"] {


otherfields has get_deserialized::<String>(key) which we should use combined with Option::transpose

thanks for the tip. what's the reasoning to use Option::transpose since i'm going to have to unwrap to the String anyways in order to parse into B256?

i would think ideal would be something like:

source_hash: tx.other.get_deserialized::<String>("sourceHash") .ok_or(ConversionError::MissingSourceHash)? .map_err(|_| ConversionError::InvalidSourceHash)? .parse() .map_err(|_| ConversionError::InvalidSourceHash)?,

mattsse · 2024-06-11T22:22:21Z

crates/primitives/src/alloy_compat.rs

+                        None => TxKind::Create,
+                    },
+                    mint: match &tx.other["mint"] {
+                        serde_json::Value::String(mint) => mint.parse::<u128>().ok().and_then(|num| if num == 0 { None } else { Some(num) }),


this will fail because mint is U128

is there a convenient way to convert U128 to u128?

are you sure mint is U128?

i see that is here: https://github.com/alloy-rs/alloy/blob/ed8a6baa2b1c404030a388633e9b7ee47825ed52/crates/rpc-types-eth/src/transaction/optimism.rs#L16

but i'm not sure if that struct is actually used. when i read the alloy block transactions, it's all strings and doesn't match the fields of that struct:

other: OtherFields {"depositReceiptVersion": String("0x1"), "mint": String("0x0"), "sourceHash": String("0x3a20754de0b2ef572fac9ca91ec3b182e61101c66d666897e8539e8b57b9a40c")}

i tried writing a test assuming it's a string and it works fine, see latest commit.

mattsse · 2024-06-12T15:01:27Z

crates/primitives/res/txs/opt_deposit_tx_mint.json

these should be used in the test as raw string let input: &str = r#...#; not extra file

mattsse · 2024-06-12T15:01:51Z

crates/primitives/src/alloy_compat.rs

+                    mint: tx.other.get_deserialized::<String>("mint")
+                        .ok_or(ConversionError::MissingBlobVersionedHashes)?
+                        .map_err(|_| ConversionError::MissingMaxFeePerBlobGas)?
+                        .parse::<u128>()
+                        .ok() // TODO: This converts failure to None. Better to error instead?


this get be get_deserialized::

zobront · 2024-06-12T16:21:03Z

@mattsse I think this should be good to go, pending the Alloy errors (which we're discussing here: alloy-rs/alloy#875).

Thank you for your help with this. Please let me know if you have any other feedback — otherwise will ping you when errors are updated for final approval.

zobront · 2024-06-14T01:47:07Z

@mattsse Alloy error update was merged (alloy-rs/alloy#875) so updated this accordingly. It's ready to go but will need an Alloy version bump before it works.

Is best practice just to let it wait until that time, or is there another way to handle it?

DaniPopes · 2024-06-14T09:35:18Z

crates/primitives/src/alloy_compat.rs

+                    mint: Option::transpose(tx.other.get_deserialized::<alloy_primitives::U128>("mint"))
+                        .map_err(|_| ConversionError::Custom(String::from("MissingMintValue")))?
+                        .and_then(|num| {
+                            if num.to::<u128>() == 0 {


.map(|num| num.to::<u128>()) .filter(|num| num > 0)

DaniPopes · 2024-06-14T09:35:27Z

crates/primitives/src/alloy_compat.rs

+                        .ok_or(ConversionError::Custom(String::from("MissingSourceHash")))?
+                        .map_err(|_| ConversionError::Custom(String::from("MissingSourceHash")))?
+                        .parse()
+                        .map_err(|_| ConversionError::Custom(String::from("InvalidSourceHash")))?,


use .to_string()

DaniPopes · 2024-06-14T09:36:33Z

crates/primitives/src/alloy_compat.rs

+                    source_hash: tx
+                        .other
+                        .get_deserialized::<String>("sourceHash")
+                        .ok_or(ConversionError::Custom(String::from("MissingSourceHash")))?


@DaniPopes Thanks for the notes. Made all changes and pushed commit, but I'm curious to learn from this one. Why the preference for ok_or_else vs ok_or here?

String::from allocates, and ok_or is eager which means the allocation is performed regardless of whether the Option is Some or None.

See Option::ok_or

Arguments passed to ok_or are eagerly evaluated; if you are passing the result of a function call, it is recommended to use ok_or_else, which is lazily evaluated.

zobront added 14 commits June 7, 2024 22:27

zkvm skeleton, errors everywhere

360ace5

convert op deposit tx from alloy to reth

383e89f

compilation fixes

adde7da

unpack serde values

75711b1

serde json wrangling

27fe42b

errors

26e5f2c

hmm

2df4d04

hack test

751ded4

import b256

f5fce8d

borrow other field

7ff6403

clean up error handling

8d1b327

clean up error handling

a932b69

unchanged alloy errors

9c7d77d

only existing errors

3ab7eb4

zobront requested review from onbjerg, gakonst, DaniPopes and Rjected as code owners June 11, 2024 21:14

zobront added 4 commits June 11, 2024 16:16

rm zkvm work

b92c8ad

restore cargo toml

882f69d

exact toml match

8b71756

imports to opt feature flag

bdf02ee

mattsse requested changes Jun 11, 2024

View reviewed changes

zobront mentioned this pull request Jun 12, 2024

add custom conversion error to handle additional situations (such as optimism deposit tx) alloy-rs/alloy#875

Merged

3 tasks

zobront added 2 commits June 11, 2024 23:21

fixes and tests

9d1221e

assume mint value is a string

d8fd919

mattsse requested changes Jun 12, 2024

View reviewed changes

zobront added 2 commits June 12, 2024 10:52

input in tests & fix mint deserialization

7a38595

mint deserialization, tests passing

7a1dd57

chore: touchups

95d7a60

mattsse approved these changes Jun 12, 2024

View reviewed changes

mattsse enabled auto-merge June 12, 2024 17:01

mattsse mentioned this pull request Jun 12, 2024

Use proper conversion errors for OP tx variant #8777

Closed

clean up

37d51c4

auto-merge was automatically disabled June 14, 2024 01:45
Head branch was pushed to by a user without write access

DaniPopes reviewed Jun 14, 2024

View reviewed changes

dani changes

a7df6bd

emhane requested a review from DaniPopes June 17, 2024 09:28

emhane added the A-op-reth Related to Optimism and op-reth label Jun 17, 2024

Merge branch 'main' into alloy-compat-opt

07dc21f

mattsse approved these changes Jun 17, 2024

View reviewed changes

mattsse enabled auto-merge June 17, 2024 12:06

mattsse added 2 commits June 18, 2024 14:10

Merge branch 'main' into alloy-compat-opt

4bfa1a8

Merge branch 'main' into alloy-compat-opt

21b2355

mattsse added this pull request to the merge queue Jun 18, 2024

Merged via the queue into paradigmxyz:main with commit d786b45 Jun 18, 2024
30 checks passed

DaniPopes mentioned this pull request Jun 18, 2024

chore: simplify optimism tx compat #8925

Merged

zobront deleted the alloy-compat-opt branch June 18, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement conversion of optimism deposit tx from alloy to reth #8763

feat: implement conversion of optimism deposit tx from alloy to reth #8763

zobront commented Jun 11, 2024 •

edited

Loading

mattsse left a comment

mattsse Jun 11, 2024

zobront Jun 12, 2024

mattsse Jun 11, 2024

zobront Jun 12, 2024

mattsse Jun 11, 2024

zobront Jun 12, 2024

mattsse Jun 11, 2024

zobront Jun 12, 2024

zobront Jun 12, 2024

mattsse Jun 12, 2024

mattsse Jun 12, 2024

zobront commented Jun 12, 2024

zobront commented Jun 14, 2024

DaniPopes Jun 14, 2024

DaniPopes Jun 14, 2024

DaniPopes Jun 14, 2024

zobront Jun 14, 2024

DaniPopes Jun 18, 2024

feat: implement conversion of optimism deposit tx from alloy to reth #8763

feat: implement conversion of optimism deposit tx from alloy to reth #8763

Conversation

zobront commented Jun 11, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zobront commented Jun 12, 2024

zobront commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zobront commented Jun 11, 2024 •

edited

Loading