Skip to content

Conversation

@jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Oct 11, 2024

We only set step's error, but missing the result's one.

use this tx to test it

{
  "jsonrpc": "2.0",
  "method": "debug_traceTransaction",
  "id": "1",
  "params": [
    "0x18ba7c5bcb851072612c0451377f55ac4ef83986ec8d9f87a2ce07c9a7d6371f", 
	{
      "tracer": "{ fault: function() {}, result: function(ctx, _) { return { error: !!ctx.error }; } }"
    }
  ]
}

for geth/erigon's response is:

{
  "id": "1",
  "jsonrpc": "2.0",
  "result": {
    "error": true
  }
}

currently reth's response is:

{
  "id": "1",
  "jsonrpc": "2.0",
  "result": {
    "error": false
  }
}

Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
@jsvisa jsvisa requested a review from DaniPopes October 12, 2024 15:50
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty,
one suggestion

Comment on lines 259 to 262
if let TransactTo::Call(target) = env.tx.transact_to {
to = Some(target);
}
let r#type = match env.tx.transact_to {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, did we not set this properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, declare a tmp variable to make the ctx builder more compacted. And I see your point, I'll rollback the changes.

let (db, _db_guard) = EvmDbRef::new(&state, db);

let gas_used = result.gas_used();
let mut to = None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets remove this tmp var and do env.tx.transact_to.to().copied() instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, as txkind's to address will return None for the creation tx, so also need to extract from the creation's result

https://github.com/alloy-rs/core/blob/main/crates/primitives/src/common.rs#L47-L53

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see

Signed-off-by: jsvisa <[email protected]>
Signed-off-by: jsvisa <[email protected]>
let (db, _db_guard) = EvmDbRef::new(&state, db);

let gas_used = result.gas_used();
let mut to = None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see

@mattsse mattsse merged commit d55d3c1 into paradigmxyz:main Oct 15, 2024
11 checks passed
@jsvisa jsvisa deleted the revert-error-not-set branch October 15, 2024 08:14
lwedge99 pushed a commit to sentioxyz/revm-inspectors that referenced this pull request Jan 3, 2025
We only set step's error, but missing the result's one. 


use this tx to test it

```json
{
  "jsonrpc": "2.0",
  "method": "debug_traceTransaction",
  "id": "1",
  "params": [
    "0x18ba7c5bcb851072612c0451377f55ac4ef83986ec8d9f87a2ce07c9a7d6371f", 
	{
      "tracer": "{ fault: function() {}, result: function(ctx, _) { return { error: !!ctx.error }; } }"
    }
  ]
}
```

for geth/erigon's response is:

```json
{
  "id": "1",
  "jsonrpc": "2.0",
  "result": {
    "error": true
  }
}
```

currently reth's response is:

```json
{
  "id": "1",
  "jsonrpc": "2.0",
  "result": {
    "error": false
  }
}
```

---------

Signed-off-by: jsvisa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants