Gas comparison - #144
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a comprehensive gas benchmarking framework for rollup inbox functions. It adds new Solidity test contracts, mock contracts, Python scripts for data collection and analysis, JSON files with gas usage statistics, a markdown report summarizing findings, and configuration changes to enable file system write permissions for gas reports. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Gas Test Contract (TaikoInboxTest)
participant Mock as MockProposerFees
participant Registry as BlobRefRegistry
participant Inbox as TaikoInbox
participant File as Gas Report File
Tester->>Mock: Deploy MockProposerFees
Tester->>Registry: Deploy BlobRefRegistry
Tester->>Inbox: Deploy TaikoInbox (with dependencies)
loop For each publication (x20)
Tester->>Inbox: publish(blob, anchorBlockId)
end
Tester->>File: Write average gas usage to JSON
sequenceDiagram
participant Script as get_taiko_gas.py
participant API as Tenderly API
participant File as propose_batch_gas_analysis.json
Script->>API: Fetch recent transactions for TaikoInbox
API-->>Script: Return transaction data
Script->>Script: Filter proposeBatch calls with blobs
Script->>File: Write gas usage statistics to JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
test/mocks/MockProposerFee.sol (1)
8-10: Silence unused-parameter warnings & clarify payable statusBoth
proposerandisDelayedare unused. Prefixing with an underscore (or removing the names) prevents static-analysis noise:-function payPublicationFee(address proposer, bool isDelayed) external override {} +function payPublicationFee(address /*proposer*/, bool /*isDelayed*/) external override {}Also confirm the interface does not declare the function
payable; if it does, add thepayablemodifier to keep the signature identical.snapshots/TaikoInboxTest.json (1)
1-3: Missing trailing newline (nit)Add a newline at EOF to satisfy POSIX text-file conventions and keep
git diffcleaner.gas-reports/taiko_inbox_publish.txt (1)
1-4: Generated artifact – consider excluding from VCSGas-report outputs change frequently and can create noisy diffs. Unless they are required for CI comparison, move them to
.gitignoreand regenerate on demand, or store canonical values in the JSON snapshot only.gas-reports/inbox_without_provermarket.txt (1)
1-5: Same concern as above – treat as build outputThis looks like transient test output. Evaluate whether it should live in version control or be recreated during CI.
scripts/get_taiko_inbox_gas.sh (2)
7-9: Consider making the script more configurable and robust.The hardcoded repository URL and file path make the script less flexible. Consider parameterizing these values or adding validation.
# Define variables +REPO_URL="${REPO_URL:-https://github.com/taikoxyz/taiko-mono.git}" -REPO_URL="https://github.com/taikoxyz/taiko-mono.git" -TEMP_DIR="taiko-mono-temp" +TEMP_DIR="taiko-mono-temp-$$" # Use process ID for uniqueness -GAS_REPORT_PATH="packages/protocol/gas-reports/inbox_without_provermarket.txt" +GAS_REPORT_PATH="${GAS_REPORT_PATH:-packages/protocol/gas-reports/inbox_without_provermarket.txt}"
25-26: Preserve original filename in the output.The current implementation changes the filename when copying. Consider preserving the original filename or making this behavior configurable.
# Copy the file to output directory - cp "$TEMP_DIR/$GAS_REPORT_PATH" "$OUTPUT_DIR/" - echo "Gas report copied to: $OUTPUT_DIR/inbox_without_provermarket.txt" + ORIGINAL_FILENAME=$(basename "$GAS_REPORT_PATH") + cp "$TEMP_DIR/$GAS_REPORT_PATH" "$OUTPUT_DIR/$ORIGINAL_FILENAME" + echo "Gas report copied to: $OUTPUT_DIR/$ORIGINAL_FILENAME"test/Gas/Inbox.t.sol (2)
23-25: Document the rationale for test parameter values.The hardcoded values for
lookaheadAddr,maxAnchorBlockIdOffset, andinclusionDelaylack documentation explaining their significance for gas measurement.- address lookaheadAddr = address(0); - uint256 maxAnchorBlockIdOffset = uint256(10); - uint256 inclusionDelay = uint256(10000000); + address lookaheadAddr = address(0); // No lookahead for gas testing + uint256 maxAnchorBlockIdOffset = uint256(10); // Small offset for testing + uint256 inclusionDelay = uint256(10000000); // Large delay to avoid timing issues
35-44: Consider making test data more realistic.The test uses minimal data (single blob with index 0). Consider testing with more realistic data sets to get representative gas measurements.
function test_gas_TaikoPublishFunction() public { - uint256[] memory blobIndices = new uint256[](1); - blobIndices[0] = 0; + // Test with multiple blobs for more realistic gas measurement + uint256[] memory blobIndices = new uint256[](3); + blobIndices[0] = 0; + blobIndices[1] = 1; + blobIndices[2] = 2; - bytes32[] memory blobHashes = new bytes32[](1); - blobHashes[0] = keccak256(abi.encode(0)); + bytes32[] memory blobHashes = new bytes32[](3); + for (uint256 i = 0; i < blobHashes.length; i++) { + blobHashes[i] = keccak256(abi.encode(i)); + }scripts/compare_gas.py (4)
13-14: Improve exception chaining for better error traceability.Following Python best practices, use
raise ... from errto maintain the exception chain and provide better debugging information.except FileNotFoundError: - raise FileNotFoundError(f"File not found: {file_path}") + raise FileNotFoundError(f"File not found: {file_path}") from None
19-22: Fix unused variable and improve exception chaining.The variable
eis assigned but never used, and the exception should use proper chaining.try: return int(match.group(1)) - except (IndexError, ValueError) as e: - raise ValueError(f"Failed to parse gas value from match: {match.group(0)}") + except (IndexError, ValueError) as e: + raise ValueError(f"Failed to parse gas value from match: {match.group(0)}") from e
32-33: Consider making regex patterns configurable.The hardcoded regex patterns make the script less flexible for different report formats. Consider making them configurable or adding validation.
def main(): - pattern1 = r"Gas per proposing:\s*(\d+)" - pattern2 = r"Gas for publication:\s*(\d+)" + # Configurable patterns for different report formats + pattern1 = r"Gas per proposing:\s*(\d+)" + pattern2 = r"Gas for publication:\s*(\d+)" + + # Validate patterns contain capture groups + for pattern in [pattern1, pattern2]: + if '(' not in pattern or ')' not in pattern: + raise ValueError(f"Invalid regex pattern (missing capture group): {pattern}")
36-37: Add validation for extracted gas values.Consider adding validation to ensure extracted gas values are reasonable (positive, within expected ranges).
gas_proposing = extract_gas_value(TAIKO_GAS_REPORT, pattern1) gas_publication = extract_gas_value(MINIMAL_GAS_REPORT, pattern2) + + # Validate extracted values are reasonable + if gas_proposing <= 0 or gas_publication <= 0: + raise ValueError("Gas values must be positive") + if gas_proposing > 10**9 or gas_publication > 10**9: # 1B gas seems unreasonable + raise ValueError("Gas values seem unreasonably high")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
foundry.toml(1 hunks)gas-reports/inbox_without_provermarket.txt(1 hunks)gas-reports/taiko_inbox_publish.txt(1 hunks)scripts/compare_gas.py(1 hunks)scripts/get_taiko_inbox_gas.sh(1 hunks)snapshots/TaikoInboxTest.json(1 hunks)test/Gas/Inbox.t.sol(1 hunks)test/mocks/MockProposerFee.sol(1 hunks)
🪛 Ruff (0.12.2)
scripts/compare_gas.py
14-14: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
21-21: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🧰 Additional context used
🪛 Ruff (0.12.2)
scripts/compare_gas.py
14-14: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
21-21: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
foundry.toml (1)
17-18: Scope the newfs_permissionsmore tightly or document the intentGranting blanket write access to
./gas-reports/is reasonable for the new gas-report workflow, but it also opens the door for any contract test to create or overwrite arbitrary files under that directory. Consider:
- Narrowing the path (e.g.
./gas-reports/*.txt) or- Adding a short comment next to this line explaining that the write permission is required for Foundry’s
vm.writeFilein gas tests.Either keeps future reviewers from having to reverse-engineer why broader permissions were introduced.
test/mocks/MockProposerFee.sol (1)
14-16: Mark functionpureto save gas
getCurrentFeesdoes not read state; marking itpureinstead ofviewlets the optimizer omit unnecessary ext-code-load checks:-function getCurrentFees() external view override returns (uint96 fee, uint96 delayedFee) { +function getCurrentFees() external pure override returns (uint96 fee, uint96 delayedFee) {[ suggest_optional_refactor ]
… be consistent with taikos test
Changes to gas cost
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
scripts/get_taiko_inbox_gas.sh (1)
15-16: Add user-friendly error handling for git clone operation.The git clone operation lacks explicit error handling that would provide clear feedback to users about potential network, authentication, or repository access issues.
Consider adding explicit error handling as suggested in the previous review to improve user experience when the clone operation fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gas-reports/taiko_inbox_publish.txt(1 hunks)gas-reports/test_inbox_measure_gas_used.txt(1 hunks)scripts/compare_gas.py(1 hunks)scripts/get_taiko_inbox_gas.sh(1 hunks)snapshots/TaikoInboxTest.json(1 hunks)test/Gas/Inbox.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- gas-reports/test_inbox_measure_gas_used.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- snapshots/TaikoInboxTest.json
- test/Gas/Inbox.t.sol
🧰 Additional context used
🪛 Ruff (0.12.2)
scripts/compare_gas.py
14-14: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
21-21: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (4)
gas-reports/taiko_inbox_publish.txt (1)
1-3: LGTM! Simple and effective gas report format.The file correctly records the gas measurement with clear traceability to the source test function. The format is suitable for the comparison script to parse.
scripts/compare_gas.py (3)
25-28: LGTM! Solid calculation logic with proper edge case handling.The calculation correctly handles the division by zero case and provides both absolute and percentage differences.
39-56: LGTM! Clear and informative output formatting.The comparison output is well-formatted and provides clear interpretation of the gas usage differences with appropriate conditional messaging.
57-65: Good error handling structure with room for minor improvement.The error handling covers the main exception types appropriately. The structure is solid for a utility script.
| TAIKO_GAS_REPORT = "gas-reports/test_inbox_measure_gas_used.txt" | ||
| MINIMAL_GAS_REPORT = "gas-reports/taiko_inbox_publish.txt" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify file paths match the actual gas report locations.
The file paths defined here may not match the actual locations of the gas reports based on the AI summary and other files in this PR.
Please verify these file paths are correct:
🏁 Script executed:
#!/bin/bash
# Check if the gas report files exist at the specified paths
echo "Checking gas report file paths..."
ls -la gas-reports/test_inbox_measure_gas_used.txt 2>/dev/null || echo "File not found: gas-reports/test_inbox_measure_gas_used.txt"
ls -la gas-reports/taiko_inbox_publish.txt 2>/dev/null || echo "File not found: gas-reports/taiko_inbox_publish.txt"Length of output: 349
compare_gas.py: fix static analysis issues
The gas-report file paths are correct and already exist in gas-reports/. Remaining actions:
- Exception chaining (B904): in each
exceptblock, useraise NewError(…) from einstead of a bareraiseorraise NewError(…), so the original traceback is preserved. - Unused variable (F841): remove the unused
eor rename it to_if you need to capture it without using it (e.g.,except SomeError as _:).
These changes will satisfy the linter and keep your error handling clear.
🤖 Prompt for AI Agents
In scripts/compare_gas.py around lines 6 to 7, update all except blocks to use
exception chaining by raising new exceptions with 'from e' to preserve the
original traceback, and remove or rename unused exception variables to '_' to
fix unused variable warnings. This will resolve static analysis issues related
to exception handling and unused variables.
| except FileNotFoundError: | ||
| raise FileNotFoundError(f"File not found: {file_path}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception chaining for better error context.
The exception should be chained to preserve the original error context.
Apply this fix:
- except FileNotFoundError:
- raise FileNotFoundError(f"File not found: {file_path}")
+ except FileNotFoundError as e:
+ raise FileNotFoundError(f"File not found: {file_path}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except FileNotFoundError: | |
| raise FileNotFoundError(f"File not found: {file_path}") | |
| except FileNotFoundError as e: | |
| raise FileNotFoundError(f"File not found: {file_path}") from e |
🧰 Tools
🪛 Ruff (0.12.2)
14-14: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In scripts/compare_gas.py around lines 13 to 14, the FileNotFoundError is
re-raised without chaining the original exception, which loses the original
error context. Modify the raise statement to include "from e" where "e" is the
caught exception variable, by first capturing the exception as "except
FileNotFoundError as e" and then raising the new FileNotFoundError with the
original exception chained using "from e".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
scripts/get_taiko_inbox_gas.sh (2)
17-17: Add error handling for git clone operation.The git clone operation lacks error handling for potential network, authentication, or repository access failures.
22-23: Fix filename consistency in copy operation and output message.There's a mismatch between the actual copied filename and the reported filename in the echo statement. The script copies
$GAS_REPORT_FILEbut the echo message should reflect the actual filename used.The current variables show:
GAS_REPORT_FILE="test_inbox_measure_gas_used.txt"- Copy operation uses this filename correctly
- Echo message also uses this filename correctly
However, the past review comments suggest there may have been an inconsistency that was partially addressed.
scripts/compare_gas.py (2)
13-14: Improve exception chaining for better error context.The exception should be chained to preserve the original error context.
19-22: Fix unused variable and improve exception chaining.The exception handling has an unused variable and missing exception chaining.
🧹 Nitpick comments (3)
scripts/get_taiko_inbox_gas.sh (1)
8-10: Consider making the script more configurable.The script hardcodes specific paths and filenames. Consider adding command-line parameters or environment variable support to make it more flexible for different use cases.
For example:
+# Allow overriding via environment variables +TAG="${TAIKO_TAG:-taiko-alethia-protocol-v2.3.0}" +GAS_REPORT_FILE="${GAS_REPORT_FILE:-test_inbox_measure_gas_used.txt}" +OUTPUT_DIR="${OUTPUT_DIR:-gas-reports}"gas-reports/report.md (2)
140-140: Fix compound adjective and improve word choice.The phrase "real world scenarios" should be hyphenated, and consider replacing the overused intensifier "extremely".
Apply this fix:
-- Currently this is only measuring the difference based on imitating the test case. These conditions are extremely basic and are not indicative of real world scenarios (for example the blob/calldata is given in the test is extremely small compared to that of a real rollup etc..) +- Currently this is only measuring the difference based on imitating the test case. These conditions are quite basic and are not indicative of real-world scenarios (for example the blob/calldata given in the test is very small compared to that of a real rollup etc.)
142-142: Complete the incomplete bullet point.There's an empty bullet point that should either be completed or removed.
--
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gas-reports/minimal_inbox_publish.txt(1 hunks)gas-reports/report.md(1 hunks)scripts/compare_gas.py(1 hunks)scripts/get_taiko_inbox_gas.sh(1 hunks)test/Gas/Inbox.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- gas-reports/minimal_inbox_publish.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Gas/Inbox.t.sol
🧰 Additional context used
🪛 LanguageTool
gas-reports/report.md
[uncategorized] ~140-~140: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...tremely basic and are not indicative of real world scenarios (for example the blob/calldat...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~140-~140: As an alternative to the over-used intensifier ‘extremely’, consider replacing this phrase.
Context: ...e blob/calldata is given in the test is extremely small compared to that of a real rollup etc.....
(EN_WEAK_ADJECTIVE)
🪛 Ruff (0.12.2)
scripts/compare_gas.py
14-14: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
gas-reports/report.md (1)
120-121: Verify consistency with comparison script expectations.The gas values shown in the table should be consistent with what the comparison script (
scripts/compare_gas.py) expects to parse from the report files.The table shows specific gas values that should match the regex patterns used in the comparison script:
- Pattern 1:
r"Gas per proposeBatches:\s*(\d+)"expects 654,502- Pattern 2:
r"Gas for publication:\s*(\d+)"expects 44,689Please ensure the actual report files contain text that matches these patterns exactly.
scripts/compare_gas.py (1)
31-34: Regex patterns validatedBoth regex patterns correctly match the contents of the sample gas report files. No further changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
gas-reports/report.md (1)
145-145: Complete the incomplete sentence and add proper conclusion.The report ends abruptly with an incomplete bullet point and lacks a proper conclusion section.
-- +- The testing environment may not fully replicate production conditions + +## Conclusion + +This preliminary analysis shows a significant 93.17% gas reduction when using the minimal rollup's `publish` function compared to Taiko's `proposeBatch`. However, these results should be interpreted with caution given the simplified test conditions and different data handling approaches (calldata vs blob transactions). More comprehensive testing on realistic scenarios will be conducted once the minimal rollup infrastructure is deployed to a testnet.
♻️ Duplicate comments (2)
scripts/compare_gas.py (2)
13-14: Fix exception chaining to preserve error context.The
FileNotFoundErrorshould be chained to preserve the original exception context.Apply this fix:
- except FileNotFoundError: - raise FileNotFoundError(f"File not found: {file_path}") + except FileNotFoundError as e: + raise FileNotFoundError(f"File not found: {file_path}") from e
21-22: Fix exception chaining for ValueError.The
ValueErrorshould be chained to preserve the original exception context.Apply this fix:
- except (IndexError, ValueError): - raise ValueError(f"Failed to parse gas value from match: {match.group(0)}") + except (IndexError, ValueError) as e: + raise ValueError(f"Failed to parse gas value from match: {match.group(0)}") from e
🧹 Nitpick comments (1)
gas-reports/report.md (1)
140-140: Consider replacing "very small" with a more specific descriptor.As flagged by the style checker, "very" is an overused intensifier. Consider a more precise description.
-... the blob/calldata Given in the test is very small compared to that of a real rollup etc.) +... the blob/calldata given in the test is minimal compared to that of a real rollup etc.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gas-reports/report.md(1 hunks)scripts/compare_gas.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
gas-reports/report.md
[style] ~140-~140: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... the blob/calldata given in the test is very small compared to that of a real rollup etc.)...
(EN_WEAK_ADJECTIVE)
🪛 Ruff (0.12.2)
scripts/compare_gas.py
14-14: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
gas-reports/report.md (1)
131-132: Gas values appear to be updated correctly.Based on the past review comments, the
gas_proposingvalue has been updated from the outdated 609,813 to 654,502, which should now match the current test results. The values look consistent with the comparison table above.scripts/compare_gas.py (1)
31-69: Well-structured gas comparison script.The main function and overall script structure are well-designed with:
- Clear separation of concerns
- Comprehensive error handling
- Formatted output that matches the corrected variable names
- Appropriate patterns for extracting gas values
The script effectively automates the gas comparison process described in the report.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
gas-reports/compare_gas_reports.py (1)
18-22: Improve consistency in error messages.The error messages have inconsistent capitalization: "proposeBatch" vs "Publish". Consider standardizing the formatting.
- return {"error": "proposeBatch average gas is zero"} + return {"error": "ProposeBatch average gas is zero"}or
- return {"error": "Publish average gas is zero"} + return {"error": "publish average gas is zero"}gas-reports/report.md (1)
131-131: Improve writing confidence by removing qualifier.The word "somewhat" weakens the statement and makes the analysis sound uncertain.
-It is worth noting this is somewhat consistent with taikos [gas analysis] +It is worth noting this is consistent with taikos [gas analysis]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
gas-reports/compare_gas_reports.py(1 hunks)gas-reports/gas_comparison_results.json(1 hunks)gas-reports/get_taiko_gas.py(1 hunks)gas-reports/minimal_inbox_publish.json(1 hunks)gas-reports/propose_batch_gas_analysis.json(1 hunks)gas-reports/report.md(1 hunks)snapshots/TaikoInboxTest.json(1 hunks)test/Gas/Inbox.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- gas-reports/minimal_inbox_publish.json
- gas-reports/gas_comparison_results.json
- gas-reports/propose_batch_gas_analysis.json
🚧 Files skipped from review as they are similar to previous changes (2)
- snapshots/TaikoInboxTest.json
- test/Gas/Inbox.t.sol
🧰 Additional context used
🧬 Code Graph Analysis (2)
gas-reports/get_taiko_gas.py (1)
gas-reports/compare_gas_reports.py (1)
main(39-71)
gas-reports/compare_gas_reports.py (1)
gas-reports/get_taiko_gas.py (1)
main(47-71)
🪛 LanguageTool
gas-reports/report.md
[style] ~131-~131: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...n: ~18,357 It is worth noting this is somewhat consistent with taikos [gas analysis](h...
(SOMEWHAT)
🪛 markdownlint-cli2 (0.17.2)
gas-reports/report.md
117-117: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
118-118: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (7)
gas-reports/compare_gas_reports.py (3)
3-12: LGTM! Well-implemented error handling.The function properly handles both file system and JSON parsing errors with specific exception handling and meaningful error messages.
33-36: LGTM! Correct percentage calculation with division by zero protection.The function properly calculates percentage decrease and handles the edge case where the first value is zero.
50-71: LGTM! Good output formatting and file handling.The console output is well-formatted with proper number formatting and clear messaging. The JSON output saves all relevant comparison data.
gas-reports/get_taiko_gas.py (2)
9-19: LGTM! Proper HTTP request handling with error handling.The function correctly uses
raise_for_status()and handlesRequestExceptionwith appropriate error messaging.
28-35: LGTM! Correct filtering and gas tracking logic.The condition properly filters for
proposeBatchtransactions with blob data, and the min/max tracking logic correctly handles initialization.gas-reports/report.md (2)
7-105: LGTM! Comprehensive and well-structured methodology.The hybrid approach combining real on-chain data with controlled testing provides a solid foundation for the gas comparison analysis. The code snippets effectively illustrate the implementation details.
134-157: LGTM! Well-balanced analysis with appropriate caveats.The limitations section appropriately acknowledges the constraints of the current methodology, and the conclusion presents the findings with appropriate context. The next steps provide a clear path forward for more comprehensive analysis.
| results = compare_gas_usage(propose_batch_data, publish_data) | ||
| print(results) | ||
|
|
||
| propose_batch_avg_gas = results['propose_batch_avg_gas'] | ||
| publish_avg_gas = results['publish_avg_gas'] | ||
| difference = results['difference'] |
There was a problem hiding this comment.
Add error handling for comparison results.
The code assumes compare_gas_usage always returns valid comparison data, but it can return an error dictionary. This will cause a KeyError when trying to access keys like 'propose_batch_avg_gas'.
results = compare_gas_usage(propose_batch_data, publish_data)
-print(results)
+
+if 'error' in results:
+ print(f"Error: {results['error']}")
+ return
propose_batch_avg_gas = results['propose_batch_avg_gas']
publish_avg_gas = results['publish_avg_gas']
difference = results['difference']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| results = compare_gas_usage(propose_batch_data, publish_data) | |
| print(results) | |
| propose_batch_avg_gas = results['propose_batch_avg_gas'] | |
| publish_avg_gas = results['publish_avg_gas'] | |
| difference = results['difference'] | |
| results = compare_gas_usage(propose_batch_data, publish_data) | |
| if 'error' in results: | |
| print(f"Error: {results['error']}") | |
| return | |
| propose_batch_avg_gas = results['propose_batch_avg_gas'] | |
| publish_avg_gas = results['publish_avg_gas'] | |
| difference = results['difference'] |
🤖 Prompt for AI Agents
In gas-reports/compare_gas_reports.py around lines 43 to 48, the code assumes
compare_gas_usage always returns valid data, but it may return an error
dictionary causing KeyError when accessing keys. Add error handling to check if
the results indicate an error before accessing keys like
'propose_batch_avg_gas'. If an error is present, handle it appropriately, such
as logging the error or raising an exception, to prevent KeyError.
| if tx.get("gas_used") > max_gas_used or max_gas_used == 0: | ||
| max_gas_used = tx.get("gas_used") | ||
|
|
||
| average_gas_used = math.ceil(sum_gas_used / num_propose_batch) |
There was a problem hiding this comment.
Fix division by zero error.
If no proposeBatch transactions are found, num_propose_batch will be 0, causing a ZeroDivisionError when calculating the average.
+ if num_propose_batch == 0:
+ return {
+ "total_transactions": 0,
+ "average_gas_used_proposeBatch": 0,
+ "minimum_gas_used": 0,
+ "maximum_gas_used": 0
+ }
+
average_gas_used = math.ceil(sum_gas_used / num_propose_batch)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| average_gas_used = math.ceil(sum_gas_used / num_propose_batch) | |
| if num_propose_batch == 0: | |
| return { | |
| "total_transactions": 0, | |
| "average_gas_used_proposeBatch": 0, | |
| "minimum_gas_used": 0, | |
| "maximum_gas_used": 0 | |
| } | |
| average_gas_used = math.ceil(sum_gas_used / num_propose_batch) |
🤖 Prompt for AI Agents
In gas-reports/get_taiko_gas.py at line 37, the calculation of average_gas_used
divides by num_propose_batch without checking if it is zero, which can cause a
ZeroDivisionError. Add a condition to check if num_propose_batch is greater than
zero before performing the division; if it is zero, set average_gas_used to zero
or handle it appropriately to avoid the error.
| if response is None: | ||
| print("Failed to fetch transactions") | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for API response.
The fetch_transactions function returns an empty dict {} on error, not None. The current check won't catch API failures.
- if response is None:
+ if not response:
print("Failed to fetch transactions")
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if response is None: | |
| print("Failed to fetch transactions") | |
| return | |
| if not response: | |
| print("Failed to fetch transactions") | |
| return |
🤖 Prompt for AI Agents
In gas-reports/get_taiko_gas.py around lines 53 to 55, the code checks if
response is None to detect API failures, but fetch_transactions returns an empty
dict {} on error. Update the condition to check if the response is empty (e.g.,
if not response) instead of None to properly handle API failure cases.
| print(f"Found {propose_batch_txs_summary['total_transactions']} proposeBatch transactions") | ||
| print("-" * 60) | ||
|
|
||
| print(f"Average gas_used over {propose_batch_txs_summary["total_transactions"]} propose transactions: {propose_batch_txs_summary['average_gas_used_proposeBatch']:,}") |
There was a problem hiding this comment.
Fix syntax error with quote usage.
The f-string contains conflicting quote usage that will cause a SyntaxError.
- print(f"Average gas_used over {propose_batch_txs_summary["total_transactions"]} propose transactions: {propose_batch_txs_summary['average_gas_used_proposeBatch']:,}")
+ print(f"Average gas_used over {propose_batch_txs_summary['total_transactions']} propose transactions: {propose_batch_txs_summary['average_gas_used_proposeBatch']:,}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(f"Average gas_used over {propose_batch_txs_summary["total_transactions"]} propose transactions: {propose_batch_txs_summary['average_gas_used_proposeBatch']:,}") | |
| print(f"Average gas_used over {propose_batch_txs_summary['total_transactions']} propose transactions: {propose_batch_txs_summary['average_gas_used_proposeBatch']:,}") |
🤖 Prompt for AI Agents
In gas-reports/get_taiko_gas.py at line 66, the f-string uses conflicting double
quotes inside the string, causing a syntax error. Fix this by changing the inner
quotes around dictionary keys to single quotes or by escaping the inner double
quotes to ensure proper string delimitation.
| | Implementation | Gas per Operation | Data Source | | ||
| |---|---|---| | ||
| | Taiko `proposeBatch` (on-chain) | 181,669 | Real mainnet data (18 txs avg) | | ||
| | Our `publish` (test) | 44,563 | Foundry test environment (20 txs avg) | | ||
| | **Absolute Difference** | **137,106** | | ||
| | **Percentage Decrease** | **75.47%** | |
There was a problem hiding this comment.
Fix table formatting issues.
The table has inconsistent column counts. Lines 117-118 are missing cells for the third column, which will cause rendering issues.
| Implementation | Gas per Operation | Data Source |
|---|---|---|
| Taiko `proposeBatch` (on-chain) | 181,669 | Real mainnet data (18 txs avg) |
| Our `publish` (test) | 44,563 | Foundry test environment (20 txs avg) |
-| **Absolute Difference** | **137,106** |
-| **Percentage Decrease** | **75.47%** |
+| **Absolute Difference** | **137,106** | - |
+| **Percentage Decrease** | **75.47%** | - |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Implementation | Gas per Operation | Data Source | | |
| |---|---|---| | |
| | Taiko `proposeBatch` (on-chain) | 181,669 | Real mainnet data (18 txs avg) | | |
| | Our `publish` (test) | 44,563 | Foundry test environment (20 txs avg) | | |
| | **Absolute Difference** | **137,106** | | |
| | **Percentage Decrease** | **75.47%** | | |
| | Implementation | Gas per Operation | Data Source | | |
| |---|---|---| | |
| | Taiko `proposeBatch` (on-chain) | 181,669 | Real mainnet data (18 txs avg) | | |
| | Our `publish` (test) | 44,563 | Foundry test environment (20 txs avg) | | |
| | **Absolute Difference** | **137,106** | - | | |
| | **Percentage Decrease** | **75.47%** | - | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
117-117: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
118-118: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
In gas-reports/report.md around lines 113 to 118, the table rows for "Absolute
Difference" and "Percentage Decrease" have only two columns instead of three,
causing formatting issues. Add a third empty cell to these rows to maintain
consistent column counts across the table.
|
|
||
| - Minimum gas used: 164,334 | ||
| - Maximum gas used: 182,691 | ||
| - Average gas used: 18,1669 |
There was a problem hiding this comment.
Fix typo in average gas usage.
There's a formatting error in the average gas used value.
-- Average gas used: 18,1669
+- Average gas used: 181,669📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Average gas used: 18,1669 | |
| - Average gas used: 181,669 |
🤖 Prompt for AI Agents
In gas-reports/report.md at line 127, the average gas used value contains a typo
with an incorrect comma placement. Correct the formatting by replacing the comma
with a period or removing it to properly represent the number as 181669 or
18.1669, depending on the intended value format.
aff22d0 to
5d96f7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
gas-reports/report.md (2)
113-118: Fix the two-column rows to restore table renderingRows 117-118 only provide two cells, violating the 3-column header and triggering MD056.
Add an empty third cell or combine the “label/value” pair into one cell.-| **Absolute Difference** | **137,106** | -| **Percentage Decrease** | **75.47%** | +| **Absolute Difference** | **137,106** | - | +| **Percentage Decrease** | **75.47%** | - |
125-128: Correct the average gas value formatting
Average gas used: 18,1669is malformed (a stray comma).
Based on the min/max figures the average should be181,669.- Average gas used: 18,1669 + Average gas used: 181,669
🧹 Nitpick comments (1)
gas-reports/report.md (1)
131-132: Minor wording & capitalization polishConsider removing the weakener “somewhat” and capitalize “Taiko’s”.
-It is worth noting this is somewhat consistent with taikos ... +It is worth noting this is consistent with Taiko’s ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gas-reports/report.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
gas-reports/report.md
[style] ~131-~131: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...n: ~18,357 It is worth noting this is somewhat consistent with taikos [gas analysis](h...
(SOMEWHAT)
🪛 markdownlint-cli2 (0.17.2)
gas-reports/report.md
117-117: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
118-118: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (1)
gas-reports/report.md (1)
115-116: Verify the Taiko average gas figure (181 ,669) is up-to-datePrevious review indicated
gas_proposingshould be654,502.
Please confirm the latest extraction fromtest_inbox_measure_gas_used.txt; update the table if necessary.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
gas-reports/report.md (2)
111-116: Fix table: third column missing on two rowsMarkdown requires the same number of cells per row. Rows for “Absolute Difference” and “Percentage Decrease” currently have only two cells, breaking the table render.
-| **Absolute Difference** | **137,106** | -| **Percentage Decrease** | **75.47%** | +| **Absolute Difference** | **137,106** | - | +| **Percentage Decrease** | **75.47%** | - |
123-126: Correct average gas value formatting
Average gas used: 18,1669has an extra comma and an incorrect grouping, rendering the number ambiguous.- Average gas used: 18,1669 + Average gas used: 181,669
🧹 Nitpick comments (1)
gas-reports/report.md (1)
128-130: Capitalisation and tone“taikos” should be “Taiko’s” and the qualifier “somewhat” can be dropped for a more confident statement.
-It is worth noting this is somewhat consistent with taikos +It is worth noting this is consistent with Taiko’s
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gas-reports/report.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
gas-reports/report.md
[style] ~129-~129: Qualifiers like “somewhat” can weaken your message and make your writing sound uncertain. Consider removing it or choosing an alternative to sound more confident.
Context: ...n: ~18,357 It is worth noting this is somewhat consistent with taikos [gas analysis](h...
(SOMEWHAT)
🪛 markdownlint-cli2 (0.17.2)
gas-reports/report.md
115-115: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
116-116: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
| // Pre-populate publications to simulate active rollup state | ||
| ProposeMultiplePublications(10) | ||
|
|
||
| // Measure 10 publications | ||
| uint256 numPublications = 20; | ||
| vm.startSnapshotGas("publish"); | ||
| _publishMultiplePublications(numPublications); | ||
| uint256 publishGas = vm.stopSnapshotGas("publish"); | ||
| uint256 gasPerPublication = publishGas / numPublications; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace stray call and align publication count
ProposeMultiplePublications(10) is undefined and lacks a semicolon, while the subsequent measurement block uses numPublications = 20 even though the comment says “Measure 10 publications”. This will confuse readers and could mislead anyone copy-pasting the snippet.
-// Pre-populate publications to simulate active rollup state
-ProposeMultiplePublications(10)
-
-// Measure 10 publications
-uint256 numPublications = 20;
+// Pre-populate publications to simulate active rollup state
+_publishMultiplePublications(10);
+
+// Measure 10 publications
+uint256 numPublications = 10;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Pre-populate publications to simulate active rollup state | |
| ProposeMultiplePublications(10) | |
| // Measure 10 publications | |
| uint256 numPublications = 20; | |
| vm.startSnapshotGas("publish"); | |
| _publishMultiplePublications(numPublications); | |
| uint256 publishGas = vm.stopSnapshotGas("publish"); | |
| uint256 gasPerPublication = publishGas / numPublications; | |
| // Pre-populate publications to simulate active rollup state | |
| _publishMultiplePublications(10); | |
| // Measure 10 publications | |
| uint256 numPublications = 10; | |
| vm.startSnapshotGas("publish"); | |
| _publishMultiplePublications(numPublications); | |
| uint256 publishGas = vm.stopSnapshotGas("publish"); | |
| uint256 gasPerPublication = publishGas / numPublications; |
🤖 Prompt for AI Agents
In gas-reports/report.md around lines 66 to 74, replace the undefined call
ProposeMultiplePublications(10) with a properly defined function call that
matches the context and add a missing semicolon. Also, align the numPublications
variable to 10 to match the comment "Measure 10 publications" for consistency
and clarity. Ensure the code snippet is syntactically correct and the comments
accurately reflect the code behavior.
reword style up up
cf15167 to
48c54d0
Compare
2 participants
Summary by CodeRabbit
New Features
publishfunction.Documentation
Chores