Conversation
There was a problem hiding this comment.
Pull request overview
Adds the Redact (Fhenix confidential token) protocol to the gas-benchmarks package, including Sepolia RPC support and additional RPC rate-limit mitigation to make log/receipt collection more reliable.
Changes:
- Added a new
redact/benchmark implementation + protocol config (events/contracts/tx examples). - Added Sepolia RPC wiring and updated environment configuration to require
SEPOLIA_RPC_URL. - Introduced RPC throttling/retry utilities (
sleep,withRetries) and batched receipt fetching to reduce rate-limit failures.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| gas-benchmarks/src/utils/utils.ts | Adds sleep() utility used for throttling RPC calls. |
| gas-benchmarks/src/utils/rpc.ts | Adds Sepolia client, enables strict log querying, and batches receipt fetching with retry + delays. |
| gas-benchmarks/src/utils/http.ts | New retry helper (withRetries) to handle rate-limit errors with jittered delays. |
| gas-benchmarks/src/utils/constants.ts | Adds SEPOLIA_RPC_URL, BLOCK_WINDOW_ETHEREUM_3_DAYS, and RPC batching constant. |
| gas-benchmarks/src/redact/index.ts | New Redact benchmark runner (encrypt/decrypt ETH metrics, claim flow composition). |
| gas-benchmarks/src/redact/constants.ts | Defines Redact contract address, expected event patterns, and REDACT_CONFIG. |
| gas-benchmarks/src/index.ts | Wires Redact into the main benchmark execution and DB output. |
| gas-benchmarks/.env.example | Adds SEPOLIA_RPC_URL example entry. |
| /** Redact configuration */ | ||
| const REDACT_CONFIG = { | ||
| name: "redact", | ||
| version: "1.0.0", | ||
| contracts: [ |
There was a problem hiding this comment.
constants.test.ts discovers every protocol directory that contains a constants.ts file and asserts it has a matching PROTOCOL_CONFIGS entry. Adding src/redact/constants.ts without also adding REDACT_CONFIG to gas-benchmarks/src/__tests__/constants.test.ts will make the test fail ("Missing PROTOCOL_CONFIGS entries for: redact").
| /** How many RPC calls will be performed concurrently */ | ||
| export const BATCH_SIZE_FOR_RPC_CALLS = 1500; |
There was a problem hiding this comment.
BATCH_SIZE_FOR_RPC_CALLS is set to 1500, which means getValidReceipts() can issue up to 1500 concurrent getTransactionReceipt calls per batch. That level of concurrency is likely to worsen rate limiting (and may exceed provider request/body limits) rather than alleviate it; consider lowering this substantially and/or making it configurable via env var so it can be tuned per RPC provider.
| for (let i = 0; i < logs.length; i += BATCH_SIZE_FOR_RPC_CALLS) { | ||
| const batch = logs.slice(i, i + BATCH_SIZE_FOR_RPC_CALLS); | ||
|
|
||
| // eslint-disable-next-line no-await-in-loop | ||
| const tempReceipts = await Promise.all( | ||
| batch.map((log) => withRetries(() => client.getTransactionReceipt({ hash: log.transactionHash! }))), | ||
| ); | ||
|
|
||
| receipts.push(...tempReceipts); | ||
|
|
||
| // eslint-disable-next-line no-await-in-loop | ||
| await sleep(100); // delay between batches to avoid overwhelming the RPC | ||
| } |
There was a problem hiding this comment.
getValidReceipts() uses a hard-coded await sleep(100) between batches. Since this throttling is now part of the RPC strategy (and interacts with BATCH_SIZE_FOR_RPC_CALLS and withRetries), consider pulling the delay value into a named constant (or env var) to make it easier to tune without code changes, and to clarify the intended rate-limiting behavior.
| export const withRetries = async <T>( | ||
| request: () => Promise<T>, | ||
| maxAttempts = RETRY_MAX_ATTEMPTS, | ||
| attempt = 1, | ||
| ): Promise<T> => { | ||
| try { | ||
| return await request(); | ||
| } catch (error) { | ||
| if (!isRateLimitError(error) || attempt >= maxAttempts) { | ||
| throw error; | ||
| } | ||
|
|
||
| const delay = RETRY_BASE_DELAY_IN_MS + Math.floor(Math.random() * (RETRY_JITTER_IN_MS + 1)); | ||
| await sleep(delay); | ||
|
|
||
| return withRetries(request, maxAttempts, attempt + 1); | ||
| } |
There was a problem hiding this comment.
withRetries() always waits roughly ~1s (+jitter) between attempts regardless of the attempt number. For rate limiting, a backoff that increases per attempt (e.g., exponential or linear backoff using attempt) is typically needed to avoid immediately re-hitting the limit; consider incorporating attempt into the delay calculation.
| async benchmark(): Promise<Record<string, FeeMetrics>> { | ||
| const [encryptEth, decryptEth] = await Promise.all([this.benchmarkEncryptETH(), this.benchmarkDecryptETH()]); | ||
|
|
||
| return { encryptEth, decryptEth }; | ||
| } |
There was a problem hiding this comment.
PR description discusses benchmarking 3 tx types (Shield / Unshield / Transfer), but benchmark() currently only returns { encryptEth, decryptEth }. If Transfer is intended to be part of the Redact benchmarks, it should be included here (or the PR description should be updated to match the implemented scope).
| // TODO: not enough txs at the moment (April 17th 2026). We can activate it later | ||
| async benchmarkEncryptedTokenTransfer(): Promise<FeeMetrics> { |
There was a problem hiding this comment.
benchmarkEncryptedTokenTransfer() is marked TODO and not used because there are not enough txs, but the PR description suggests Transfer is part of the benchmark methodology for this protocol. Consider clarifying this limitation in the PR description (or gating/including it explicitly so consumers know why it's missing).
This PR adds the Redact (confidential token project built with Fhenix) to the current benchmarking methodology. I have found the following issues:
client.getLogs()but it does not seem to be doing anything useful in our case:we assumed that
getLogs({ events: [...] })will filter transactions with a specific log order but it does not work like that. It fetches all logs emitted from the specified address that adhere to any of the events structure. This makes it difficult to differentiate txs. For example in redactAs you can see all 3 types of tx emit ConfidentialTransfer and the only way to differentiate a Transfer from the other 2 is by counting the emitted events and checking it does not contain ShieldedNative or Unshielded.
We can merge this PR as reference and keep the above idea in mind when migrating to the subgraph methodology