Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eth/tracers: fill the creationMethod in flatCall #30539

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Oct 2, 2024

rename creationMethod to createType, and fill it with the correct value.

@jsvisa jsvisa requested a review from s1na as a code owner October 2, 2024 23:53
@s1na
Copy link
Contributor

s1na commented Oct 9, 2024

Can you please expand on this? Is it not part of the parity tracing response? shouldn't we in that case fill the value rather than remove it?

@s1na
Copy link
Contributor

s1na commented Oct 14, 2024

I have consulted @ziogaschr who wrote the tracer. I think removing CreationMethod is not the way to go. We should instead try to fill the value. This is what he wrote:

So in here

From: &input.From,
, we have to add CreationMethod: the actual type, which is vm.CREATE or vm.CREATE2

@ziogaschr
Copy link
Contributor

I am sorry for missing this detail, while converting this tracer from the old JS tracers to the newer Go native ones.
I can make a PR if you like.

@jsvisa
Copy link
Contributor Author

jsvisa commented Oct 14, 2024

We should instead try to fill the value.

@s1na sorry for the inconvenience, I think I'll buy it.

@jsvisa jsvisa changed the title eth/tracers: rm the flatCall's notused CreationMethod eth/tracers: fill the creationMethod in flatCall Oct 14, 2024
@jsvisa
Copy link
Contributor Author

jsvisa commented Oct 14, 2024

Filled it with vm.opcode, kindly ask @s1na @ziogaschr to take a look.

@jsvisa
Copy link
Contributor Author

jsvisa commented Oct 14, 2024

BTW, as we use callType for the different call types, how about use the name createType instead of creationMethod?

@ziogaschr
Copy link
Contributor

BTW, as we use callType for the different call types, how about use the name createType instead of creationMethod?

Thanks @jsvisa. That's indeed a nice idea.
Probably it's preferable to stick with creationMethod for the JSON output, in order to not break compatibility with clients that know how to consume Parity like tracers.
It's totally fine to rename the Go struct's field name though.

@fjl
Copy link
Contributor

fjl commented Oct 30, 2024

eth/tracers/native/call_flat.go:299:45: unnecessary conversion (unconvert)
			CreationMethod: strings.ToLower(vm.OpCode(input.Type).String()),
			                                         ^

please fix the lint issue

@jsvisa
Copy link
Contributor Author

jsvisa commented Oct 31, 2024

@ziogaschr sorry for the delay response, I think this field has not been used before, so rename to createType is ok. @s1na WDYT.

@ziogaschr
Copy link
Contributor

@jsvisa if we want to be compatible with the old OpenEthereum label, then we have to use creationMethod. On the other hand, nobody noticed it was missing...

@fjl
Copy link
Contributor

fjl commented Oct 31, 2024

I think it makes more sense to use the label that was used before, and not create a new tracing format.

@jsvisa
Copy link
Contributor Author

jsvisa commented Nov 4, 2024

@ziogaschr thanks for the advice, seems Parity(legacy OpenEthereum) did include the creationMethod in v3.0.1(but not formal released), and Besu had this field in their codebase tracing/flat/Action.java#L44:

public class Action {

  private final String creationMethod;
  private final String callType;
  private final String from;
  private final String gas;
  private final String input;
  private final String to;
  private final String init;
  private final String value;
  private final String address;
  private final String balance;
  private final String refundAddress;
  private final String author;
  private final String rewardType;

So I think the creationMethod is better, reverted back. @s1na @fjl PTAL.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Thanks all!

@s1na
Copy link
Contributor

s1na commented Nov 4, 2024

CI was red because a new testcase that was on master (but not pulled in to this PR) was failing. It is so interesting but also in this case useful.

Anyway merged in master changes and fixed that test case. Should be good to go now.

@s1na s1na merged commit 229ce64 into ethereum:master Nov 5, 2024
3 checks passed
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.

4 participants