Skip to content

Conversation

@tac0turtle
Copy link
Contributor

Overview

@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2026

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 19, 2026, 3:07 PM

@claude
Copy link
Contributor

claude bot commented Jan 19, 2026

Claude finished @tac0turtle's task in 2m 1s —— View job


Comprehensive Code Review Complete ✓

I've reviewed the calculator enhancements for batching strategies. This is a significant improvement that adds flexibility for cost/latency optimization. Below are my findings organized by priority.


Critical Issues

1. Bug in dataFixedGasPerSubmission calculation (docs/.vitepress/components/CelestiaGasEstimator.vue:1028-1030)

Confirmed Issue Identified by Gemini

The dataFixedGasPerSubmission calculation incorrectly returns non-zero gas costs even when no data blobs exist:

const dataFixedGasPerSubmission = computed(
    () => dataBlobCount.value * GAS_PARAMS.fixedCost,
);

When dataBlobCount.value is 0 (no data being submitted), this correctly returns 0. However, examining line 511 reveals potential confusion in how this is being used. The calculation is technically correct, but the display logic might be misleading users.

Impact: When executionEnv is "cosmos" or EVM with zero transaction types enabled, data costs should be zero but may appear in the UI.

Recommendation: The calculation itself is correct. The display should better handle the zero-data case in the breakdown section (lines 425-489).

Fix this display issue →


High Priority Issues

2. Missing input validation for strategy parameters (docs/.vitepress/components/CelestiaGasEstimator.vue:680-682)

Strategy configuration lacks proper validation and sanitization:

const daBlockTimeSeconds = ref(6);
const batchSizeThreshold = ref(0.8);
const batchMaxDelaySeconds = ref(0);
const batchMinItems = ref(1);

Issues:

  • No v-model sanitization like txPerSecondInput has (lines 699-710)
  • User can input negative values or NaN for daBlockTimeSeconds
  • batchMinItems allows 0 or negative values which breaks calculations (line 740, 919)
  • No upper bounds on daBlockTimeSeconds or batchMaxDelaySeconds

Impact: Invalid inputs cause NaN propagation through calculations, breaking the UI display.

Recommendation: Add computed properties with sanitization similar to txPerSecondInput pattern:

Fix input validation →

3. Edge case: Division by zero potential (docs/.vitepress/components/CelestiaGasEstimator.vue:895-900, 929-932)

While dataBytesPerSecond checks prevent most issues, there's a subtle edge case:

const dataBytesPerSecond = computed(() => {
    if (executionEnv.value !== "evm") {
        return 0;
    }
    return txPerSecond.value * averageCalldataBytes.value;
});

And later:

if (strategy === "size") {
    const timeToThreshold = targetBlobBytes.value / bytesPerSecond;
    return Math.max(minInterval, timeToThreshold);
}

Issue: When bytesPerSecond is exactly 0, division returns Infinity, which then gets passed to Math.max() returning Infinity. This cascades through calculations.

Impact: With zero throughput, interval becomes infinite, causing submissions/year calculations to show 0 or NaN.

Current mitigation: Line 913 returns early when bytesPerSecond <= 0, which is good. However, the logic could be clearer.

Recommendation: Add explicit guards in size-based calculations to avoid any edge case where division by zero could occur.


Medium Priority Issues

4. Inconsistent handling of "adaptive" strategy minimum (docs/.vitepress/components/CelestiaGasEstimator.vue:758-764, 935-939)

The adaptive strategy logic differs between header and data submission intervals:

Headers (lines 758-764):

if (strategy === "adaptive") {
    const delayBlocks = Math.ceil(effectiveMaxDelaySeconds.value / blockSeconds);
    const blocksToThreshold = Math.ceil(targetBlobBytes.value / headerBytesPerBlock);
    return blockSeconds * Math.min(
        Math.max(minItems, delayBlocks),
        Math.max(minItems, blocksToThreshold)
    );
}

Data (lines 935-939):

if (strategy === "adaptive") {
    const timeToThreshold = targetBlobBytes.value / bytesPerSecond;
    return Math.max(minInterval, Math.min(timeToThreshold, effectiveMaxDelaySeconds.value));
}

Issue: The header logic ensures minItems is respected in both branches of the min, while data logic only applies minItems via minInterval once. This creates subtle differences in behavior that may confuse users.

Recommendation: Unify the adaptive logic patterns for consistency, or document why they differ.

5. Magic number for max blob size (docs/.vitepress/components/CelestiaGasEstimator.vue:564)

const MAX_BLOB_SIZE = 7 * 1024 * 1024; // 7 MB max blob size (from common/consts.go)

Issue: While well-commented, this references a Go file that may not be in the docs directory. If the Go constant changes, this becomes stale.

Recommendation: Consider adding a note in CLAUDE.md or the calculator documentation about keeping this in sync with the actual codebase constant.


Low Priority / Code Quality

6. Missing error boundaries for computed properties (Throughout)

Many computed properties assume valid inputs but don't have comprehensive error handling for edge cases:

  • headerSubmissionIntervalSeconds returns NaN for invalid inputs (line 736)
  • dataSubmissionIntervalSeconds returns NaN (line 909)
  • These NaN values propagate through dependent computations

Impact: While formatNumber() and formatInteger() handle NaN by returning "0" (lines 1099, 1109), the intermediate calculations may produce confusing results.

Recommendation: Consider wrapping more computed properties with finite checks or providing user-visible warnings when inputs are invalid.

7. Type safety could be improved (docs/.vitepress/components/CelestiaGasEstimator.vue:556-589)

The component uses TypeScript but doesn't leverage type guards or stricter validation:

type BatchingStrategy = "immediate" | "size" | "time" | "adaptive";

Recommendation: Add runtime type validation for user inputs that could be tampered with via browser dev tools.

8. Performance: Excessive computed dependencies

Many computed properties depend on each other in deep chains (e.g., feePerSecondTIAheaderFeePerSubmissionTIAheaderGasheaderSharesheaderBytesTotalnormalizedHeaderCountheaderSubmissionIntervalSeconds).

Impact: While Vue 3's reactivity system is efficient, this many nested computations could impact performance with rapid input changes.

Recommendation: Profile with rapid slider input. Consider debouncing or adding watchEffect with throttling if needed.


Documentation & Testing

9. Documentation is well-written

The markdown guide (docs/guides/celestia-gas-calculator.md) clearly explains:

  • Each batching strategy and its use case
  • Parameter meanings and defaults
  • Limitations and caveats

Positive: The "Important" note about estimates being planning guides is excellent.

10. Missing unit tests

Observation: No test files found for the calculator component.

Recommendation: Add unit tests for:

  • Each batching strategy calculation
  • Edge cases (zero throughput, zero block time, minimum values)
  • Boundary conditions (NaN handling, Infinity handling)
  • Data chunking logic when blobs exceed 500 KiB

Security Review

11. No XSS or injection risks identified

All user inputs are numeric and properly bound with v-model.number. No dynamic HTML rendering or v-html usage detected.

12. No sensitive data exposure

No API keys, credentials, or sensitive data in the component.


Workflow Changes

13. Workflow cleanup is appropriate (.github/workflows/docs_deploy.yml)

The workflow was simplified by removing unnecessary Setup Pages action configuration. The change from actions/upload-pages-artifact + actions/deploy-pages to peaceiris/actions-gh-pages@v4 is cleaner and more reliable.

Positive: Good simplification.

14. Package.json script additions (docs/package.json:6,8)

"predev": "[ -d node_modules ] || npm install",
"prebuild": "[ -d node_modules ] || npm install",

Issue: These pre-scripts use shell syntax that may not work consistently across platforms (Windows compatibility issue with [ -d ] test).

Recommendation: Use cross-platform approach:

"predev": "node -e \"const fs=require('fs');fs.existsSync('node_modules')||process.exit(1)\" || npm install"

Or rely on npm ci / npm install idempotency instead of pre-scripts.

Fix cross-platform compatibility →


Summary

Overall Assessment: This is a well-designed enhancement that significantly improves the calculator's utility. The batching strategy implementation is sound, and the UI clearly explains trade-offs.

Must Fix Before Merge:

  1. Input validation for strategy parameters (Issue Peer discovery #2)
  2. Cross-platform package.json scripts (Issue P2P bootstrapping #14)

Should Fix Soon:
3. Data costs display when no data (Issue #1)
4. Adaptive strategy consistency (Issue #4)
5. Add unit tests (Issue #10)

Consider for Future:
6. Performance profiling with rapid inputs (Issue #8)
7. More robust error boundaries (Issue #6)

Tasks Completed

  • Read and analyze all changed files
  • Review calculation logic and verify bug identified by Gemini
  • Check for additional edge cases and potential issues
  • Review code quality, security, and performance
  • Verify documentation completeness
  • Post comprehensive review feedback

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly upgrades the Celestia Gas Estimator by integrating flexible batching strategies. Users can now simulate various block submission approaches to the Data Availability layer, enabling more accurate cost and performance analysis based on their specific latency, cost efficiency, and throughput requirements. This enhancement provides a more comprehensive tool for understanding and optimizing rollup operations.

Highlights

  • New Batching Strategies: The Celestia Gas Estimator now includes a "Batching strategy" section, allowing users to choose between Immediate, Size-based, Time-based, and Adaptive submission methods.
  • Configurable Parameters: New input fields have been added for DA block time, batch size threshold, batch max delay, and batch minimum items, providing granular control over the chosen strategy.
  • Updated Calculation Logic: The underlying computations for header and data submission intervals, as well as associated costs, have been refactored to dynamically adapt to the selected batching strategy.
  • Documentation Update: The celestia-gas-calculator.md guide has been revised to explain the new batching strategies and their parameters.
  • Build Script Enhancements: predev and prebuild scripts were added to package.json to ensure dependencies are installed before development or build processes.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request significantly enhances the Celestia Gas Calculator by introducing configurable batching strategies. The new strategies (immediate, size-based, time-based, and adaptive) allow for more nuanced and realistic cost estimations. The underlying calculation logic has been refactored to accurately model these strategies, separating header and data submission cadences and costs. The UI and documentation have been updated accordingly. The code is well-structured, but I've identified a couple of issues in the calculation and display logic that could lead to incorrect or misleading estimations.

@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.32%. Comparing base (66ac65a) to head (52a9a6f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2995      +/-   ##
==========================================
+ Coverage   59.25%   59.32%   +0.06%     
==========================================
  Files         106      106              
  Lines       10023    10023              
==========================================
+ Hits         5939     5946       +7     
+ Misses       3456     3449       -7     
  Partials      628      628              
Flag Coverage Δ
combined 59.32% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tac0turtle tac0turtle added this pull request to the merge queue Jan 19, 2026
Merged via the queue into main with commit 9ad4016 Jan 19, 2026
28 of 30 checks passed
@tac0turtle tac0turtle deleted the marko/calculator branch January 19, 2026 15:49
@github-actions
Copy link
Contributor

PR Preview Action v1.8.0
Preview removed because the pull request was closed.
2026-01-19 15:50 UTC

alpe added a commit that referenced this pull request Jan 19, 2026
* main:
  chore: update calculator for strategies  (#2995)
  chore: adding tracing for da submitter (#2993)
  feat(tracing): part 10 da retriever tracing (#2991)
  chore: add da posting strategy to docs (#2992)
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