feat: DA Client remove interface part 3, replace types with new code

[randygrok]

Remove core/da folder and da folder

Replace the old jsonrpc and internal/da clients with new one based on blob endpoint.

Overview

Closes: #2796

[github-actions]

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

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Dec 15, 2025, 8:02 PM

[claude]

Claude finished @randygrok's task in 4m 21s —— View job

---

Code Review - DA Client Refactoring (Part 3)

This PR represents a significant architectural shift from a generic DA interface to a Celestia-specific implementation. I've conducted a comprehensive code review focusing on code quality, architecture, error handling, security, performance, testing, and documentation.

---

Executive Summary

Overall Assessment: This is a well-executed refactoring that improves code clarity and reduces dependency footprint. The implementation is solid with good error handling and test coverage. However, there are several areas that need attention before merging.

Key Findings:

• ✅ Good: Clean separation of concerns, improved type safety, comprehensive error mapping
• ⚠️ Concerns: Test coverage decreased (63.54% → 60.22%), some unresolved review comments, potential issues with ID validation
• 🔴 Critical: Several unresolved review comments that need addressing

---

1. Code Quality & Architecture

✅ Strengths

Clean Interface Design (block/internal/da/interface.go:9-42):

• Well-defined separation between Client, Verifier, and FullClient interfaces
• Clear contract for DA operations
• Good use of composition with FullClient combining both interfaces

Improved Type Safety:

• Moved from generic []byte types to specific Blob, ID, Commitment, Proof types in pkg/da/types/types.go
• Namespace validation properly encapsulated in pkg/da/types/namespace.go
• JSON-RPC blob handling cleanly separated in pkg/da/jsonrpc/blob.go

Namespace Management:
The namespace implementation (pkg/da/types/namespace.go) is well-designed:

• Clear validation for version-0 namespaces (18-byte zero prefix requirement)
• Deterministic namespace generation from strings via SHA256
• Proper error handling for malformed namespaces

⚠️ Concerns

1. Inconsistent ID Validation

The SplitID function has two different implementations:

In pkg/da/jsonrpc/blob.go:153-158:

func SplitID(id []byte) (uint64, Commitment) {
    if len(id) <= 8 {
        return 0, nil  // Returns nil instead of error
    }
    return binary.LittleEndian.Uint64(id[:8]), id[8:]
}

In pkg/da/types/types.go:78-84:

func SplitID(id []byte) (uint64, []byte, error) {
    if len(id) <= 8 {
        return 0, nil, fmt.Errorf("invalid ID length: %d", len(id))
    }
    // ...
}

Recommendation: Consolidate these two SplitID functions or clearly document why they differ. Having two functions with the same name but different signatures in different packages is confusing and error-prone.

2. Missing Pagination (acknowledged in review comments)

Issue #2929 was created to track this, but the current implementation in client.go:201 (GetAll) doesn't handle pagination:

blobs, err := c.blobAPI.GetAll(getCtx, height, []share.Namespace{ns})

For heights with many blobs (40k+ mentioned in review), this could cause timeouts or memory issues. This should be addressed in this PR or prominently documented as a limitation.

3. Queue Changes (sequencers/single/queue.go)

The changes to the Prepend method documentation are good, but several reviewers asked why this change was made:

• julienrbrt: "what's this? this feels unrelated and this is breaking. we do not need it."
• tac0turtle: "why this change?"

This appears unrelated to the DA refactoring and should either be reverted or moved to a separate PR.

---

2. Error Handling & Resilience

✅ Strengths

Comprehensive Error Mapping (client.go:122-143):
The Submit method has excellent error categorization:

switch {
case errors.Is(err, context.Canceled):
    code = datypes.StatusContextCanceled
case strings.Contains(err.Error(), datypes.ErrTxTimedOut.Error()):
    code = datypes.StatusNotIncludedInBlock
// ... more cases
}

This provides good observability and allows callers to handle different error scenarios appropriately.

Proper Context Handling:

• Timeout contexts properly created and cancelled (client.go:198-199, client.go:315-316, etc.)
• Context cancellation properly detected and mapped to specific status codes

Blob Size Validation (client.go:90-97):
Pre-validation before submission prevents unnecessary network calls:

if uint64(len(raw)) > common.DefaultMaxBlobSize {
    return datypes.ResultSubmit{
        BaseResult: datypes.BaseResult{
            Code: datypes.StatusTooBig,
            Message: datypes.ErrBlobSizeOverLimit.Error(),
        },
    }
}

⚠️ Concerns

1. Error Handling by String Matching

The code uses strings.Contains(err.Error(), ...) extensively (lines 127-136, 205, 215). This is fragile because:

• Error messages from celestia-node RPC may change
• Wrapped errors may not contain the expected substring
• No type safety

Recommendation: Consider using error wrapping with errors.Is() or custom error types where possible. If string matching is necessary due to RPC limitations, document this explicitly.

2. Silent Failure in Validate (client.go:404-408)

included, err := c.blobAPI.Included(getCtx, height, ns, &proof, commitment)
if err != nil {
    c.logger.Debug().Err(err).Uint64("height", height).Msg("blob inclusion check returned error")
}
results[i] = included

Errors are logged but not returned. This means validation failures are silently treated as "not included". This could mask real issues.

Recommendation: Return the error or at least use .Warn() instead of .Debug() for better visibility.

---

3. Security

✅ Strengths

No Hardcoded Secrets: Configuration properly externalized
Input Validation: Namespace and blob size validation before processing
Context Timeouts: Prevents resource exhaustion from hanging requests

⚠️ Concerns

1. Missing Rate Limiting

There's no apparent rate limiting for DA submissions. A malicious or buggy sequencer could spam the DA layer.

Recommendation: Consider adding rate limiting or at least documenting the expected behavior under heavy load.

2. Blob Size Limits

The DefaultMaxBlobSize is checked at the application level. Ensure this aligns with Celestia's actual blob size limits to avoid submission failures.

---

4. Performance & Resource Efficiency

✅ Strengths

Pre-calculated Namespace Bytes (client.go:59-67):
Namespaces are converted to bytes once during initialization rather than on every operation:

namespaceBz: datypes.NamespaceFromString(cfg.Namespace).Bytes(),

Proper Context Timeouts: Prevents resource leaks from hanging requests

⚠️ Concerns

1. Sequential Blob Fetching in Get (client.go:319-334)

for _, id := range ids {
    height, commitment := blobrpc.SplitID(id)
    b, err := c.blobAPI.Get(getCtx, height, ns, commitment)
    // ...
}

This fetches blobs sequentially. For large numbers of IDs, this could be slow.

Recommendation: Consider concurrent fetching with goroutines + errgroup, or batch fetching if the RPC supports it. (tac0turtle mentioned this in review: "we should be able to make it more streaming oriented")

2. No Connection Pooling Visible

The JSON-RPC client doesn't show obvious connection pooling. Verify the underlying filecoin-project/go-jsonrpc library handles this.

---

5. Testing & Reliability

⚠️ Major Concerns

1. Code Coverage Decreased (from 63.54% to 60.22%)

The codecov report shows:

• Patch coverage: 39.59% (331 lines missing coverage)
• Largest gaps:
  • block/internal/da/client.go: 55.60% (70 missing, 21 partials)
  • tools/local-da/rpc.go: 0% (75 missing)
  • tools/local-da/local.go: 0% (19 missing)
  • pkg/da/jsonrpc/mocks/blob_module_mock.go: 0% (64 missing - mocks are expected)

Recommendation: Add integration tests for the new client implementation. The existing unit tests (client_test.go) are good but don't cover all error paths.

2. Test Quality

The existing tests in client_test.go are well-structured:

• Good use of table-driven tests for error mapping (TestClient_Submit_ErrorMapping)
• Proper mocking with testify
• Clear test names

However, missing test coverage for:

• Concurrent operations
• Timeout scenarios
• Large blob batches
• Edge cases in forced inclusion retrieval

✅ Strengths

Good Mock Usage: Tests properly use mocks to isolate the client from the RPC layer
Clear Test Structure: Tests are well-organized and easy to understand

---

6. Documentation & Developer Experience

⚠️ Concerns

1. Limited Package Documentation

block/internal/da/client.go has good inline comments, but package-level documentation is missing. Developers need to understand:

• When to use Client vs FullClient vs Verifier
• How namespaces work
• ID format and lifecycle

2. Migration Guide Missing

This is part 3 of a multi-part refactoring. There should be documentation explaining:

• What changed from previous DA interface
• How to migrate existing code
• Breaking changes (if any)

3. Local DA Documentation (tools/local-da/)

The local DA server is useful for testing, but has zero coverage and minimal documentation. This should be documented as "testing only" and include usage examples.

---

7. Review Comment Resolution

Several review comments remain unresolved:

Critical (Must Address Before Merge)

1. julienrbrt: sequencers/single/queue.go changes - "what's this? this feels unrelated and this is breaking. we do not need it."

   • Status: UNRESOLVED
   • Action: Revert or explain why this is needed

2. julienrbrt: block/public.go:51-52 - "This should be reimplemented. This was a recent addition and this got lost in the refactor" + "The max blob size will always be empty, which will fall back to the default value. This may be fine, but don't we want to put it back as an arg?"

   • Status: UNRESOLVED (tac0turtle says it wasn't an arg before, but julienrbrt disagrees)
   • Action: Verify if max blob size parameter was lost

3. julienrbrt: .github/workflows/test.yml - "why this change?"

   • Status: Explained but could be clearer
   • Action: OK as is

Important (Should Address)

4. tac0turtle: Pagination for GetAll (DA Retrieve uses blob.GetAll (no pagination) — may fail with many blobs at one height #2929)

   • Status: Issue created, deferred to follow-up
   • Action: Document limitation in code comments

5. tac0turtle: Subscribe implementation (DA client: implement Subscribe (part 4) #2928)

   • Status: Issue created, deferred to follow-up
   • Action: OK to defer, but leave TODO comment in code

6. julienrbrt: Removed log line at client.go:165

   • Status: tac0turtle said "added back"
   • Action: Verify log line is at the right place (line 169 has a debug log)

7. tac0turtle: Can we avoid the function call during runtime for namespace bytes (client.go)?

   • Status: UNRESOLVED
   • Action: The namespaces are pre-calculated during initialization, so this is likely fine, but clarify

---

8. Specific Code Issues

Issue 1: Inconsistent Error Handling in Get (client.go:326-332)

b, err := c.blobAPI.Get(getCtx, height, ns, commitment)
if err != nil {
    return nil, err  // Returns immediately on first error
}
if b == nil {
    continue  // Silently skips nil blobs
}

Should nil blobs return an error? This behavior should be documented.

Issue 2: Forced Inclusion Namespace Check (client.go:272-280)

if !c.hasForcedNamespace {
    return datypes.ResultRetrieve{
        BaseResult: datypes.BaseResult{
            Code:    datypes.StatusError,
            Message: "forced inclusion namespace not configured",
            Height:  height,
        },
    }
}

This returns StatusError, but there's a specific error ErrForceInclusionNotConfigured exported from the package. Consider using a specific status code or at least using the exported error constant in the message.

Issue 3: Empty Blob Submission (client.go:155-163)

if len(blobs) == 0 {
    return datypes.ResultSubmit{
        BaseResult: datypes.BaseResult{
            Code:     datypes.StatusSuccess,
            BlobSize: blobSize,
            Height:   height,
        },
    }
}

Empty submissions are allowed and return success. Is this intentional? Should be documented.

---

Recommendations

Must Fix Before Merge

1. Resolve all "Critical" review comments (especially queue.go changes and block.go max blob size)
2. Verify ID splitting error handling is correct (consolidate or document the two SplitID implementations)
3. Add integration test for the new client to improve coverage above 60%
4. Document pagination limitation in code comments

Should Fix

5. Consolidate SplitID functions (two different signatures)
6. Add package-level documentation for block/internal/da
7. Document local-da as testing-only with usage examples
8. Consider concurrent blob fetching in Get() method
9. Use .Warn() instead of .Debug() for validation errors in Validate()

Nice to Have

10. Add migration guide for DA interface changes
11. Add benchmark tests for large blob batches
12. Consider error type refactoring instead of string matching
13. Document empty blob submission behavior

---

Conclusion

This PR is architecturally sound and represents good engineering. The code is cleaner, more type-safe, and easier to understand than the previous generic interface approach. However, it should not be merged until:

1. Critical review comments are resolved (queue.go, max blob size)
2. Test coverage is improved (at least above 60% patch coverage)
3. ID handling inconsistencies are clarified

Once these issues are addressed, this will be a solid improvement to the codebase.

Recommendation: Request Changes ⚠️

---

[gemini-code-assist]

Summary of Changes

Hello @randygrok, I'm Gemini Code Assist¹! 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 undertakes a significant refactoring of the Data Availability (DA) client architecture. The primary goal is to streamline interactions with the Celestia network by introducing a dedicated Celestia-specific client and replacing abstract DA interface types with concrete, purpose-built structures. This approach enhances type safety and reduces the project's dependency footprint on larger Celestia libraries, while ensuring full compatibility with the Celestia blob RPC. The changes are supported by comprehensive dependency updates and new integration tests to verify the robustness of the new implementation.

Highlights

• DA Client Refactoring: The core Data Availability (DA) client interface has been refactored to remove generic types and replace them with new, more specific types defined in the pkg/da/types package. This change impacts how DA submission and retrieval results are handled.
• New Celestia Client Implementation: A new CelestiaBlobClient has been introduced in block/internal/da, providing a concrete implementation for interacting with the Celestia blob RPC. This client handles namespace management, blob submission, and retrieval, including detailed error mapping.
• Celestia-Node Type Integration: Key data structures and RPC client definitions from celestia-node (e.g., Blob, Commitment, SubmitOptions, BlobAPI) have been selectively copied and trimmed into new da/celestia and pkg/blob packages. This strategy aims to maintain JSON compatibility with Celestia's blob RPC while minimizing external dependencies.
• Dependency Updates: Numerous go.mod and go.sum files across apps/evm, apps/grpc, apps/testapp, da, and the root module have been updated. This includes adding new Celestia-related dependencies like celestiaorg/go-square/merkle and celestiaorg/nmt, as well as other utility libraries.
• New Integration Tests: A new end-to-end integration test (test/e2e/integration/celestia_client_integration_test.go) has been added to validate the CelestiaBlobClient's functionality against a real Celestia network, ensuring proper blob submission and retrieval.

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. ↩

[gemini-code-assist]

Code Review

This pull request continues the refactoring of the Data Availability (DA) client by moving types, errors, and namespace logic out of the core/da package and into new, more modular packages like pkg/da/types and pkg/namespace. The changes are mostly mechanical, replacing old type references with new ones across several files. The refactoring is well-executed, and new tests have been added for the extracted namespace logic. I found one issue where a change in a function's signature led to the loss of error handling for malformed IDs. I've provided a suggestion to fix this.

[julienrbrt]

mostly lgtm. left some nits. ACK once CI is green.

[julienrbrt]

utACK! :Amazing cleaup

[github-actions]

PR Preview Action v1.6.3
Preview removed because the pull request was closed.
2025-12-15 23:20 UTC