Add function_arguments_typed to return alloy Vec<DynSolType> #4

wtdcode · 2024-07-04T16:24:48Z

As title, this adds a new function function_arguments_typed to return Vec<DynSolType>, which makes it much easier to use because parsing string returned by function_arguments is really a pain.

I ran the benchmark locally, and I didn't notice obvious performance regression. I compared the new implementation against evmole-py and no errors are found.

dataset largest1k (1000 contracts, 24427 functions), evmole-rs:
  time: 1.5s
  bad:  0 functions 0.00%
  good: 24427 functions (100.00%)
  errors:

dataset random50k (50000 contracts, 1171102 functions), evmole-rs:
  time: 51.7s
  bad:  0 functions 0.00%
  good: 1171102 functions (100.00%)
  errors:

dataset vyper (780 contracts, 21244 functions), evmole-rs:
  time: 1.2s
  bad:  0 functions 0.00%
  good: 21244 functions (100.00%)
  errors:

Note the comparison is based on string so this even doesn't break compatibility.

This PR also includes a few fixes.

cdump · 2024-07-06T20:23:29Z

Thanks for PR, I like the idea and will take a look after the EthCC week (after 15 July)

cdump · 2024-07-15T07:50:39Z

rust/src/evm/vm.rs

@@ -368,9 +368,27 @@ where
                    Err(UnsupportedOpError { op }.into())
                } else {
                    let mut data: Vec<u8> = vec![0; size];
+                    let code_len = self.code.len();
+
+                    let (from_code_length, _) = if src_off + size < code_len {


Why are you returning two values from this if, but always ignoring the second one? Could it be simplified to just the 1st value?

I think a small change to the original code fits better here:

let code_len = self.code.len(); if src_off < code_len { let n = std::cmp::min(size, code_len - src_off); data[0..n].copy_from_slice(&self.code[src_off..src_off + n]); }

wdyt?

I can't remember exactly the reason because I was fixing this in a midnight to solve evmole crashing with a random unverified contract. Anyway, you are right, it can be simplified.

This one is shorter but harder to understand and maintain. I would like to leave it clearly and explicitly as current implementation so that the thing in point 1 above won't happen next time =). I can also leave a few more comments about the rationale of CODECOPY.

Let's review it again after (1) simplification; it will probably really be much easier to read and understand

I've pushed if src_off < code_len { to the master branch just to make master work while we are here. Feel free to rewrite it according to your proposal if you want.

cdump · 2024-07-15T08:06:27Z

rust/src/arguments.rs

+    function_arguments_typed(code, selector, gas_limit)
+        .into_iter()
+        .map(|t| t.sol_type_name().to_string())
+        .join(",")


I want to keep external deps as minimal as possible now, so I suggest using this variant instead of "itertools":

.map(|t| t.sol_type_name().to_string()) .collect::<Vec<String>>() .join(",")

We'll have additional allocation, but we can accept that.

okay, though I think rustc should be smart enough to optimize this.

cdump · 2024-07-15T08:07:41Z

rust/Cargo.toml

@@ -11,7 +11,10 @@ repository = "https://github.com/cdump/evmole"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [dependencies]
-ruint = "1.11.1"
+alloy = {version = "0.1", features = ["full"]}


We need only alloy-dyn-abi = {version = "0.7.7" }, not the full alloy package

itertools is not required, see the review comment below

Sure, just my bad habits when playing with the code.

cdump · 2024-07-15T08:10:19Z

rust/src/arguments.rs

            args.mark_not_bool(&path, offset);
        }

-          StepResult{op: op::AND, fa: Some(Element{label: Some(Label::Arg(Val{offset, path, add_val, and_mask: None})), ..}), sa: Some(ot), ..}


nit: I find that the old format for match looks better, but I was unable to configure rust-fmt to keep only that part untouched. So it's okay; I'll probably revert this part by hand after merging or think about another way to configure rust-fmt properly.

cdump · 2024-07-15T08:32:20Z

rust/src/arguments.rs

+    selector: &Selector,
+    gas_limit: u32,
+) -> Vec<DynSolType> {
+    trace!(


I don't like this change.
These log messages are not intended to be used by library users; they're just for me (and other developers who need to debug evmole internals).
Logging from libraries is quite a controversial issue in itself, which I think should be used very carefully and with at least providing more context about the log message origin. But in this case, I'm sure it will be better to stick with the old feature=trace and println!.

Okay. I can understand it's different taste and I'm neutral with it. Will revert to previous trace feature.

cdump · 2024-07-15T08:37:11Z

rust/src/lib.rs

@@ -3,6 +3,7 @@
 //! Accuracy and speed comparison with other tools, as well as Python and JavaScript implementations, are available on [GitHub](https://github.com/cdump/evmole/tree/master#benchmark).

 pub use arguments::function_arguments;
+pub use arguments::function_arguments_typed;


What do you think about function_arguments_alloy?

If you are okay with both variants, let me think for some time about _typed vs _alloy, just want to make sure that we will choose the best option for public API

I think function_arguments_alloy is better

cdump · 2024-07-15T08:39:57Z

rust/src/arguments.rs

        };

        match self.tinfo {
-            Some(InfoVal::Array(_)) => format!("{}[]", c),
+            Some(InfoVal::Array(_)) => {
+                if q.len() == 1 {


Maybe

vec![DynSolType::Array(Box::new( if q.len() == 1 { q[0].clone() } else { DynSolType::Tuple(q) } ))]

cdump · 2024-07-15T08:41:31Z

rust/src/arguments.rs

            })
+            .flatten()


From rust clippy:

warning: called `map(..).flatten()` on `Iterator` --> src/arguments.rs:89:14 | 89 | .map(|k| { | ______________^ 90 | | self.children 91 | | .get(&k) 92 | | .map_or(vec![DynSolType::Uint(256)], |val| val.to_alloy_type(false)) 93 | | .into_iter() 94 | | }) 95 | | .flatten() | |______________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten = note: `#[warn(clippy::map_flatten)]` on by default help: try replacing `map` with `flat_map` and remove the `.flatten()` | 89 ~ .flat_map(|k| { 90 + self.children 91 + .get(&k) 92 + .map_or(vec![DynSolType::Uint(256)], |val| val.to_alloy_type(false)) 93 + .into_iter() 94 + })

Didn't notice this, will fix.

cdump · 2024-08-24T05:31:04Z

parsing string returned by function_arguments is really a pain

Wrapping the result in () and extracting Vec<DynSolType> from results looks not so ugly:

use  alloy_dyn_abi::DynSolType;

fn main() {
    let code = hex::decode("6080604052348015600e575f80fd5b50600436106030575f3560e01c80632125b65b146034578063b69ef8a8146044575b5f80fd5b6044603f3660046046565b505050565b005b5f805f606084860312156057575f80fd5b833563ffffffff811681146069575f80fd5b925060208401356001600160a01b03811681146083575f80fd5b915060408401356001600160e01b0381168114609d575f80fd5b80915050925092509256").unwrap();

    let a = evmole::function_arguments(&code, &[0x21, 0x25, 0xb6, 0x5b], 0);
    println!("{}", a);

    if let Ok(DynSolType::Tuple(v)) = DynSolType::parse(&format!("({})", &a)) {
        println!("{:?}", v); // type = Vec<DynSolType>
    }
}

uint32,address,uint224
[Uint(32), Address, Uint(224)]

wtdcode · 2024-08-24T05:33:02Z

parsing string returned by function_arguments is really a pain

Wrapping the result in () and extracting Vec<DynSolType> from results looks not so ugly:

use  alloy_dyn_abi::DynSolType;

fn main() {
    let code = hex::decode("6080604052348015600e575f80fd5b50600436106030575f3560e01c80632125b65b146034578063b69ef8a8146044575b5f80fd5b6044603f3660046046565b505050565b005b5f805f606084860312156057575f80fd5b833563ffffffff811681146069575f80fd5b925060208401356001600160a01b03811681146083575f80fd5b915060408401356001600160e01b0381168114609d575f80fd5b80915050925092509256").unwrap();

    let a = evmole::function_arguments(&code, &[0x21, 0x25, 0xb6, 0x5b], 0);
    println!("{}", a);

    if let Ok(DynSolType::Tuple(v)) = DynSolType::parse(&format!("({})", &a)) {
        println!("{:?}", v); // type = Vec<DynSolType>
    }
}

uint32,address,uint224
[Uint(32), Address, Uint(224)]

This doesn’t work for certain types string returned by evmole.

cdump · 2024-08-24T05:49:02Z

Do you have any examples? This is probably a bug inside evmole that I need to fix independently of this PR

wtdcode · 2024-08-24T05:51:03Z

Do you have any examples? This is probably a bug inside evmole that I need to fix independently of this PR

This is generally related to the nested types, where the string returned by evmole is not accepted by alloy, though the abi is correct by my manual checking. I don't have exact example for that because this happens pretty frequently when I decoded more than 50M onchain contracts across various evm-compatible chains.

wtdcode · 2024-08-24T06:03:00Z

I have resolved the conflicts and updated according to reviews. I just forgot this PR =).

wtdcode · 2024-08-24T06:09:09Z

Also add a testcase for previous crashing bytecode.

cdump · 2024-08-24T08:07:16Z

the string returned by evmole is not accepted by alloy

I've tested it on all mainnet contracts and didn't find any errors; that's strange.

cdump · 2024-08-24T08:08:43Z

Merged to master with reverting some code-format changes: 2ee8495

wtdcode · 2024-08-24T08:12:10Z

the string returned by evmole is not accepted by alloy

I've tested it on all mainnet contracts and didn't find any errors; that's strange.

I would assume it's due to some updates, either alloy or emvole? I notice this breakage after one cargo update. But anyway, thanks for merging this =).

wtdcode · 2024-08-24T08:13:08Z

Merged to master with reverting some code-format changes: 2ee8495

You may want a custom rustfmt style? I just run cargo fmt automatically.

cdump · 2024-08-24T08:17:21Z

due to some updates, either alloy or emvole

maybe, but anyway function returning Vec is better

want a custom rustfmt style

I just don't like rustfmt's style for match cases; I find the current version more readable. For other parts, rustfmt is okay. Contributors should use it by default, and I will either roll back the match-case style, find another way to configure it, or convince myself that rustfmt's style is also acceptable.

Thanks for PR :)

Add function_arguments_typed to return alloy DynSolType

833b972

wtdcode force-pushed the typed branch from 394b8ed to 833b972 Compare July 4, 2024 16:25

wtdcode added 3 commits July 4, 2024 16:26

Fix string

9269436

Fix crash when copy out of range code

e46677d

Format

88ba561

cdump requested changes Jul 15, 2024

View reviewed changes

wtdcode added 3 commits August 24, 2024 05:55

Merge master branch and solve reviews

ce71038

Minor fixes

8a95b21

Format code

c70e039

Add test for previous crash cases

05d4368

cdump pushed a commit that referenced this pull request Aug 24, 2024

Merge #4 (function_arguments_typed)

2ee8495

cdump closed this Aug 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function_arguments_typed to return alloy Vec<DynSolType> #4

Add function_arguments_typed to return alloy Vec<DynSolType> #4

wtdcode commented Jul 4, 2024 •

edited

Loading

cdump commented Jul 6, 2024

cdump Jul 15, 2024

wtdcode Jul 16, 2024 •

edited

Loading

cdump Jul 16, 2024

cdump Jul 25, 2024

cdump Jul 15, 2024

wtdcode Jul 16, 2024

cdump Jul 15, 2024

wtdcode Jul 16, 2024

cdump Jul 15, 2024

cdump Jul 15, 2024

wtdcode Jul 16, 2024

cdump Jul 15, 2024

wtdcode Jul 16, 2024

cdump Jul 16, 2024

cdump Jul 16, 2024

cdump Jul 15, 2024

wtdcode Jul 16, 2024

cdump Jul 15, 2024

wtdcode Jul 16, 2024

cdump commented Aug 24, 2024 •

edited

Loading

wtdcode commented Aug 24, 2024

cdump commented Aug 24, 2024

wtdcode commented Aug 24, 2024

wtdcode commented Aug 24, 2024

wtdcode commented Aug 24, 2024

cdump commented Aug 24, 2024

cdump commented Aug 24, 2024

wtdcode commented Aug 24, 2024

wtdcode commented Aug 24, 2024

cdump commented Aug 24, 2024 •

edited

Loading

Add function_arguments_typed to return alloy Vec<DynSolType> #4

Add function_arguments_typed to return alloy Vec<DynSolType> #4

Conversation

wtdcode commented Jul 4, 2024 • edited Loading

cdump commented Jul 6, 2024

Choose a reason for hiding this comment

wtdcode Jul 16, 2024 • edited Loading

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

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

cdump commented Aug 24, 2024 • edited Loading

wtdcode commented Aug 24, 2024

cdump commented Aug 24, 2024

wtdcode commented Aug 24, 2024

wtdcode commented Aug 24, 2024

wtdcode commented Aug 24, 2024

cdump commented Aug 24, 2024

cdump commented Aug 24, 2024

wtdcode commented Aug 24, 2024

wtdcode commented Aug 24, 2024

cdump commented Aug 24, 2024 • edited Loading

wtdcode commented Jul 4, 2024 •

edited

Loading

wtdcode Jul 16, 2024 •

edited

Loading

cdump commented Aug 24, 2024 •

edited

Loading

cdump commented Aug 24, 2024 •

edited

Loading