Skip to content

Conversation

@zzzzzzch
Copy link
Contributor

Hi, this is a great perf tool. I found that spammer sends EIP-1559 txs by default, which is fine, but sometimes eth-forked/custom node clients may not support EIP-1559 and therefore it is needed to be specified to send legacy txs.
So this PR adds the legacy option to specify sending legacy txs.

please review and feel free to comment.

@zeroXbrock
Copy link
Member

hey @zzzzzzch thanks for the PR! Sorry it took me so long to get back to you -- I recently did a bunch of refactoring and wasn't sure how this would fit until later.

This is a good idea, but I think it would be even better if instead of a boolean for legacy, we had an enum to support many tx types. That way, we don't have to add additional params later on; just more enum values.

The codebase has been shuffled around quite a bit since you pushed this so you might want to start this on a fresh branch and cherry-pick the code you need from this PR, but it's up to you :) Looking forward to your next commits!

@zeroXbrock zeroXbrock mentioned this pull request Feb 5, 2025
@zzzzzzch zzzzzzch closed this Feb 19, 2025
@zzzzzzch zzzzzzch reopened this Feb 19, 2025
@zzzzzzch zzzzzzch changed the title add legacy option to send legacy txs [WIP] Set TxType for scenario txs Feb 19, 2025
@zzzzzzch
Copy link
Contributor Author

finally found some time to refactor this PR, and apologize for the delay. I noticed there's also a recent PR #115 supporting 4844, and I referenced some comments from that PR during the refactoring.
Currently, there are still some tests that need to be fixed, I'm working on this.
please review and feel free to comment.

@zzzzzzch zzzzzzch changed the title [WIP] Set TxType for scenario txs Set TxType for scenario txs Feb 21, 2025
@zzzzzzch
Copy link
Contributor Author

all work is done, and some code changes such as type definitions are similar to PR #115. Therefore one of the two PRs will need to be rebased onto the other before making further changes.
please review and feel free to comment

Copy link
Member

@zeroXbrock zeroXbrock left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I like the design, just had a few minor concerns.

@zzzzzzch zzzzzzch force-pushed the main branch 4 times, most recently from 02d3366 to 0f99bfd Compare February 26, 2025 03:05
@zeroXbrock
Copy link
Member

thanks @zzzzzzch !!

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.

2 participants