Skip to content

feat(taiko-client): add local proposal metadata API#21791

Open
smtmfft wants to merge 2 commits into
mainfrom
feat/taiko-client-local-proposal-api-main
Open

feat(taiko-client): add local proposal metadata API#21791
smtmfft wants to merge 2 commits into
mainfrom
feat/taiko-client-local-proposal-api-main

Conversation

@smtmfft

@smtmfft smtmfft commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a disabled-by-default loopback-only driver API for Shasta proposal metadata
  • expose GET /internal/shasta/proposals/{proposal_id} with reconstructed proposal, hashProposal, and L1 event coordinates
  • wire proposalApi.enabled / proposalApi.addr flags and reject non-loopback binds

Tests

  • go test ./packages/taiko-client/pkg/proposalapi ./packages/taiko-client/cmd/flags
  • go test ./packages/taiko-client/driver -run 'TestNewConfigFromCliContextRejectsRemoteProposalAPIAddress|TestResolveEndpointsPrefersWSOverHTTP'\n- git diff --check\n\n## Notes\n- Full go test ./packages/taiko-client/driver was not usable in this local environment: the existing driver suite expects devnet env vars/JWT and fails during internal/testutils setup with empty env parsing / invalid JWT length before reaching these changes.\n- golangci-lint is not installed in this environment (command not found).

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

🐋 DeepSeek Code Review

🔴 Critical Issues

No critical issues found.

🟡 Warnings

  • Double-header write on JSON encode failure inside proposal API handler
    handleProposal sets the Content-Type header, begins encoding the response body, and if json.NewEncoder(w).Encode() fails it calls writeError – which attempts to write headers again and may cause a panic (http.ErrAbortHandler or “superfluous response.WriteHeader”).
    File: packages/taiko-client/pkg/proposalapi/server.go:231-233
    Fix: Do not attempt to send an error response after encoding has started; just log the error and return.
    w.Header().Set("Content-Type", "application/json")
    if err := json.NewEncoder(w).Encode(response); err != nil {
        // headers already sent; log and bail out
        log.Error("failed to encode proposal response", "error", err)
    }

🔵 Suggestions

  • NewRPCSource validation of unnecessary L2/L2Engine fields
    The constructor checks that client.L2, client.L2Engine are non-nil, but the ProposalByID method only uses client.L1 and client.ShastaClients.Inbox. These checks may prevent the driver from starting even when the API can work without an L2 engine. Consider removing the irrelevant checks or at least clarifying their intent.

  • Driver.Start cancels the whole driver on proposal API server failure
    When d.proposalAPIServer.Start() returns an error, the code calls d.cancel(), which tears down the entire driver (same behaviour as preconf block server). This might be too aggressive for a secondary metadata API; consider only logging the error unless the API is mission-critical.

  • Consistent error creation in eventOriginBlockNumber
    The function returns errors.New(...) for some cases and uses fmt.Errorf for others; using fmt.Errorf consistently would improve style.

🟢 What Looks Good

  • Loopback-only binding is enforced by ValidateLoopbackAddress, rejecting 0.0.0.0, empty hosts, and non-loopback IPs.
  • The proposal metadata path is well-scoped (/internal/shasta/proposals/), the ID parsing is careful, and the source abstraction allows easy testing.
  • Test coverage is thorough: loopback validation, invalid ID, successful metadata retrieval, not-found and source-error mapping, shutdown with cancelled context, and config rejection.
  • Timeout handler is used to cap request latency, and graceful shutdown falls back to Close() if the context is already done.
  • Flags are correctly registered, have sensible defaults, and reject remote addresses at config parsing time.

Automatically triggered on PR update • model: deepseek-v4-pro

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f5411830c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread packages/taiko-client/pkg/proposalapi/server.go Outdated
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 53.36538% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 32.28%. Comparing base (a9adcce) to head (c9e3f9f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
packages/taiko-client/pkg/proposalapi/server.go 56.98% 63 Missing and 17 partials ⚠️
packages/taiko-client/driver/driver.go 0.00% 16 Missing and 1 partial ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
packages/taiko-client/driver/config.go 75.25% <100.00%> (+1.34%) ⬆️
packages/taiko-client/driver/driver.go 13.86% <0.00%> (-0.60%) ⬇️
packages/taiko-client/pkg/proposalapi/server.go 56.98% <56.98%> (ø)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 027e612...c9e3f9f. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants