Skip to content

Conversation

AlexeyKrasnoperov
Copy link
Member

No description provided.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@AlexeyKrasnoperov AlexeyKrasnoperov marked this pull request as ready for review April 24, 2025 13:58
export const ethGetTransactionReceipt = {
name: "eth_getTransactionReceipt",
params: [
"0xb0ccffb14e1e79f0b6c6ceec223bb4bbf6fc302ae76067f1f9225bae9d6f3fdc",
Copy link
Member

Choose a reason for hiding this comment

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

I'd also like to mention it here so it doesn't get lost. It's okay for specific parameters to be hard-coded (though they still should be configurable without touching this file).

  1. It should be possible to run benchmarks on both calibnet and mainnet.
  2. Tests should not break over time. This transaction will likely be unavailable on nodes bootstrapped with a minimal snapshot. This is a blocker for any real usage of this benchmark.

The limitations should be clearly stated in the README, not to give false impressions and allow potential users to resolve their issues more easily.

For now, let's at least group some of the parameters, especially those that appear a few times, into named constants.

}
}

function safeMethodName(methodName) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's document the purpose of this function.

Comment on lines +10 to +20
const methodTrends = {};
const methodErrors = {};
for (const method of allMethods) {
const key = safeMethodName(method.name);
if (!methodTrends[key]) {
methodTrends[key] = new Trend(key);
}
if (!methodErrors[key]) {
methodErrors[key] = new Rate(`_success_rate_${key}`);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add some comments around it? It's not clear from a quick glimpse what's the purpose of those collections.

@AlexeyKrasnoperov
Copy link
Member Author

Replaced by #2

@AlexeyKrasnoperov AlexeyKrasnoperov deleted the benchmark-top-50-methods branch August 8, 2025 08:12
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