Conversation
c3c317c to
4044f30
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the gas-benchmarks adapters to (a) move transaction selection from “N logs” to “required events” matching, and (b) split benchmark method labels to distinguish native ETH vs ERC20 flows across protocols.
Changes:
- Replace
getTransactionsWithNEventswithgetTransactionsWithEvents, passing explicit ABI event lists per protocol/method. - Update Tornado Cash, Privacy Pools, and Railgun benchmark adapters to use the new selection and to write metrics under
*_eth/*_erc20method keys. - Expand protocol constants from single-event ABIs/counts into ordered event ABI arrays with richer documentation links.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| gas-benchmarks/src/utils/rpc.ts | Introduces event-list-based receipt filtering for transaction selection. |
| gas-benchmarks/src/tornado-cash/index.ts | Switches Tornado adapter to event-list selection and renames output method keys to *_eth. |
| gas-benchmarks/src/tornado-cash/constants.ts | Replaces single-event/count constants with SHIELD_ETH_EVENTS / UNSHIELD_ETH_EVENTS arrays and adds references. |
| gas-benchmarks/src/railgun/index.ts | Switches Railgun adapter to event-list selection and renames output method keys to *_erc20. |
| gas-benchmarks/src/railgun/constants.ts | Replaces ABIs/count constants with ERC20-focused event arrays and notes Railgun’s lack of native ETH support. |
| gas-benchmarks/src/privacy-pools/index.ts | Switches Privacy Pools adapter to event-list selection and renames output method keys to *_eth. |
| gas-benchmarks/src/privacy-pools/constants.ts | Replaces ABIs/count constants with SHIELD_ETH_EVENTS / UNSHIELD_ETH_EVENTS arrays and adds references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@NicoSerranoP I've opened a new pull request, #16, to work on those changes. Once the pull request is ready, I'll request review from you. |
JohnGuilding
left a comment
There was a problem hiding this comment.
Nice improvements. I'm thinking of adding some additional changes for #11, so will mark it as complete, but might make some more changes later once this is in.
native ETH should be the target for the benchmarks MVP and v1
So yeah, initial MVP could be ETH transfers by default, then ERC20 token transfers as replacement. Then in the dashboard we could show both separately as they would have different gas costs. If we find out that many protocols don't support native token transfers, then maybe we can reassess.
* Initial plan * Optimize encodeEventTopics by precomputing event topics Co-authored-by: NicoSerranoP <38594836+NicoSerranoP@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: NicoSerranoP <38594836+NicoSerranoP@users.noreply.github.com>
5ae1f7a to
3c42f68
Compare
Two main features:
Better tx selection guided by comments in Event selection improvements #11. It is worth pointing out that we still have a fragile tx selection (e.g. if a txs has wrap before the private transfer execution then it will be discarded. This is because txs with more than the minimal private transfer logs will have higher gas than the ones with the minimal logs. We will have better metrics if we compare txs with the same number of logs). Closes Event selection improvements #11
native ETH should be the target for the benchmarks MVP and v1. Some projects (e.g. Railgun) do not support native ETH. I changed the constant names to allow for independent comparisons of native ETH and ERC20 transfers. Closes Consider ETH and ERC-20 shield/unshield/transfer #14