Claude finished @julienrbrt's task in 2m 40s —— View job

Code Review: feat(pkg/da): support celestia-app

Tasks

• Gather context and understand the PR
• Review new pkg/da/app/client.go (celestia-app client)
• Review refactored pkg/da/node/client.go (celestia-node client)
• Review pkg/da/types/types.go (BlobClient interface)
• Review pkg/da/factory/factory.go (DA factory)
• Review block/internal/da/ changes
• Review test coverage
• Summarize findings

Overview

This PR introduces a clean architectural refactoring by extracting a BlobClient interface and adding a new celestia-app client backed by CometBFT RPC. The direction is sound and the code is generally well-structured. However, there are a few critical functional gaps and subtle regressions that should be addressed before merging.

Critical Issues

1. pkg/da/app/client.go:76 — Submit is permanently unimplemented

The Submit method always returns StatusError for any valid input:

return datypes.ResultSubmit{
 BaseResult: datypes.BaseResult{
 Code: datypes.StatusError,
 Message: "Submit not implemented: requires transaction signing...",
 },
}

This means celestia-app is a read-only DA client. Nodes configured as aggregators (IsAggregator: true) are always redirected to the node client by the factory, but non-aggregator nodes can also call Submit indirectly (e.g., via forced inclusion). Any node that somehow ends up using the app client for submission will fail silently at the interface level.

The PR title says "support celestia-app" but submission — the core operation of a DA layer for a sequencer — is missing. This should either be implemented (requires signer integration) or the factory should hard-fail if an aggregator tries to force ClientTypeApp. Fix this →

2. pkg/da/node/client.go:62-65 — gasPrice and options are silently dropped

var opts SubmitOptions
if len(options) > 0 {
 // Options would be parsed here if needed
}

The gasPrice parameter is also discarded. SubmitOptions has a GasPrice field and IsGasPriceSet bool that were clearly designed to carry this value. Callers setting a gas price will get unexpected behavior with no error.

Fix this →

High Priority Issues

3. pkg/da/node/client.go:70-75 — Lost detailed error mapping from the old client

The previous jsonrpc/client.go mapped RPC errors to specific status codes (StatusTooBig, StatusAlreadyInMempool, StatusIncorrectAccountSequence, etc.). The new implementation returns a single generic StatusError for all Blob.Submit failures. This degrades the node's ability to react to specific DA conditions (e.g., correctly backing off vs. retrying on StatusAlreadyInMempool).

4. pkg/da/factory/factory.go:106-124 — Auto-detection silently falls back to node

// Default to node client if detection fails
return ClientTypeNode

If the address is a valid celestia-app endpoint that's temporarily unreachable during startup, the factory silently falls back to creating a node client, which will then fail differently. This can make misconfiguration very hard to debug. Consider logging a warning, or when ForceType == ClientTypeAuto, returning an error on detection failure rather than a misleading default. Fix this →

5. pkg/da/factory/factory.go:259-264 — Timeout not propagated to app client

func createAppClient(cfg Config) datypes.BlobClient {
 return daapp.NewClient(daapp.Config{
 RPCAddress: cfg.Address,
 Logger: cfg.Logger,
 DefaultTimeout: 0, // Use default ← intentionally drops the user's setting
 })
}

The configured timeout in factory.Config is never passed to the app client. This should forward cfg's timeout if one exists, or accept a DefaultTimeout field on the factory Config.

Medium Issues

6. pkg/da/app/client.go:160,173 — Fragile string-based error detection

if strings.Contains(err.Error(), "height") && strings.Contains(err.Error(), "future") { ... }
if strings.Contains(err.Error(), "is not available, lowest height is") { ... }

CometBFT error messages are not part of its stable API. These patterns could break silently on CometBFT version upgrades, causing StatusHeightFromFuture errors to be returned as generic StatusError (or vice versa). Consider matching on rpcError.Code values (CometBFT uses numeric error codes) rather than message text.

7. pkg/da/app/client.go:209-213 — Base64 fallback logic is incorrect

txBytes, err := base64.StdEncoding.DecodeString(tx)
if err != nil {
 // Try raw bytes if not base64
 txBytes = []byte(tx)
}

When tx is a string containing non-base64 data, []byte(tx) simply converts the UTF-8 string to bytes — it doesn't decode any binary format. This silently masks base64 decode errors and processes garbled data. If CometBFT always returns base64-encoded transactions, the fallback should be removed and the error should be logged.

8. pkg/da/node/client.go:155-158 — Silent nil commitment in Get

height, commitment := SplitID(id)
if commitment == nil {
 continue // silently skips
}

The local SplitID (in blob.go) returns nil commitment for malformed IDs, and the loop skips them silently. This could cause Get to return fewer results than IDs requested with no indication. The datypes.SplitID counterpart returns an explicit error; consider using that or logging the skip.

Low Priority / Nitpicks

9. pkg/da/app/client.go:57-61 — NewClient returns nil for empty address

if cfg.RPCAddress == "" {
 return nil
}

Returning nil instead of an error makes it harder for callers to distinguish "intentionally no client" from a bug. The caller at factory.go:260 doesn't check for nil, so a blank address would propagate as a nil BlobClient and panic at first use.

10. pkg/da/app/client.go:195-200 — Shadowed err variable

blockTime := time.Now()
if blockResult.Block.Header.Time != "" {
 if t, err := time.Parse(time.RFC3339Nano, blockResult.Block.Header.Time); err == nil {

The inner err shadows the outer one. This is syntactically valid but can be confusing. Using a distinct name like parseErr would be clearer.

11. pkg/da/factory/factory.go:127-185 — Double HTTP client per detection

Both isNodeEndpoint and isAppEndpoint create their own http.Client with a 5s timeout and add a context.WithTimeout. The HTTP client-level timeout is redundant when the context already has a deadline. Minor, but consider using a shared client or just relying on the context.

12. pkg/da/node/client_test.go — Limited high-level Submit/Retrieve test coverage

The test in client_test.go verifies RPC forwarding at the BlobAPI level but doesn't test the Submit/Retrieve/GetProofs/Validate methods of the Client struct (the BlobClient implementation). Edge cases like error status code translation are not covered.

What's Well Done

• The BlobClient interface is clean and well-scoped — it correctly separates blob operations from namespace wiring.
• Compile-time interface assertions (var _ datypes.BlobClient = (*Client)(nil)) in both clients.
• The factory auto-detection pattern is a nice UX improvement; users don't need to know which client to configure.
• The celestia-app client is correct to return explicit errors for GetProofs/Validate rather than silently returning empty results.
• Context propagation is consistent and correct throughout.
• The tracing layer (block/internal/da/tracing.go) is properly extended for GetLatestDAHeight.
• pkg/da/app/client_test.go has solid coverage for the new code paths.
• The RetrieveForcedInclusion wrapper in block/internal/da/client.go cleanly handles the "namespace not configured" case.