feat: DA client, remove interface part 1: copy subset of types needed for the client using blob rpc.

[randygrok]

Parent Epic: #2796

Overview

[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 3, 2025, 5:24 PM

[claude]

Claude finished @randygrok's task —— View job

---

Code Review Complete

Tasks

• Read and analyze pkg/blob package files
• Review code quality and architecture
• Check error handling and resilience
• Assess security considerations
• Evaluate performance and resource efficiency
• Review testing coverage and quality
• Verify documentation completeness
• Provide comprehensive feedback

---

Summary

This PR introduces a new pkg/blob package that copies a minimal subset of types from celestia-node to maintain JSON compatibility with the blob RPC while avoiding heavy Cosmos SDK dependencies. The approach is sound and aligns with the project's modular architecture goals. Overall, the code is clean and well-structured, with good documentation explaining the rationale.

✅ Strengths

1. Clear Documentation: The README.md provides excellent context on why this copy exists and how to keep it in sync
2. Minimal Dependencies: Successfully avoids pulling in Cosmos SDK while maintaining wire compatibility
3. JSON Compatibility: Properly mirrors the celestia-node JSON shape with custom Marshal/Unmarshal methods
4. Error Handling: Consistent error wrapping with context using fmt.Errorf with %w
5. Code Style: Follows idiomatic Go conventions and project patterns

---

Issues & Recommendations

🔴 Critical Issues

1. Missing Test Coverage (pkg/blob/blob.go:140-159)

The following functions have zero test coverage (contributing to the 18 missing lines reported by Codecov):

• MakeID() - Only partially tested in TestMakeAndSplitID with a simple case
• SplitID() - Not tested with edge cases (empty slice, exactly 8 bytes, malformed IDs)
• EqualCommitment() - Not tested at all

Recommendation: Add comprehensive tests:

func TestSplitID_EdgeCases(t *testing.T) {
    tests := []struct {
        name       string
        id         []byte
        wantHeight uint64
        wantCom    Commitment
    }{
        {"empty", []byte{}, 0, nil},
        {"exactly 8 bytes", make([]byte, 8), 0, nil},
        {"less than 8 bytes", []byte{1, 2, 3}, 0, nil},
        {"valid id", MakeID(100, []byte{0xaa, 0xbb}), 100, []byte{0xaa, 0xbb}},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            height, com := SplitID(tt.id)
            require.Equal(t, tt.wantHeight, height)
            require.Equal(t, tt.wantCom, com)
        })
    }
}

func TestBlob_EqualCommitment(t *testing.T) {
    blob, _ := NewBlobV0(libshare.MustNewV0Namespace([]byte("test")), []byte("data"))
    
    require.True(t, blob.EqualCommitment(blob.Commitment))
    require.False(t, blob.EqualCommitment([]byte{0x00}))
    require.False(t, blob.EqualCommitment(nil))
}

2. UnmarshalJSON Error Handling (pkg/blob/blob.go:119-136)

The UnmarshalJSON method doesn't wrap errors with context, making debugging harder:

if err := json.Unmarshal(data, &jb); err != nil {
    return err  // ❌ No context
}

Recommendation: Add context to all error returns:

if err := json.Unmarshal(data, &jb); err != nil {
    return fmt.Errorf("unmarshal blob JSON: %w", err)
}

ns, err := libshare.NewNamespaceFromBytes(jb.Namespace)
if err != nil {
    return fmt.Errorf("parse namespace: %w", err)
}

blob, err := NewBlob(jb.ShareVersion, ns, jb.Data, jb.Signer)
if err != nil {
    return fmt.Errorf("reconstruct blob: %w", err)
}

---

🟡 Medium Priority Issues

3. Missing Validation in MakeID (pkg/blob/blob.go:140-145)

MakeID doesn't validate inputs. A nil commitment will create a valid-looking ID but cause issues downstream.

Recommendation: Add validation:

func MakeID(height uint64, commitment Commitment) []byte {
    if len(commitment) == 0 {
        // Return empty ID or panic depending on desired behavior
        return nil
    }
    id := make([]byte, 8+len(commitment))
    binary.LittleEndian.PutUint64(id, height)
    copy(id[8:], commitment)
    return id
}

4. Inconsistent Index Handling (pkg/blob/blob.go:42-43, 71, 82)

The index field defaults to -1 but there's no validation or documentation about valid ranges. The comment says "Only blobs retrieved from the chain will have this set" but doesn't explain the -1 semantic.

Recommendation:

• Add a constant: const UnknownIndex = -1
• Add a helper: func (b *Blob) HasIndex() bool { return b.index >= 0 }
• Document the -1 convention more clearly

5. Missing Tests for submit_options.go

The SubmitOptions struct and TxPriority type have no tests. While they're simple data structures, you should verify:

• JSON marshaling/unmarshaling works correctly
• TxPriority constants have expected values
• Omitempty behavior works as intended

Recommendation: Add a test:

func TestSubmitOptions_JSON(t *testing.T) {
    opts := &SubmitOptions{
        GasPrice:      0.5,
        IsGasPriceSet: true,
        TxPriority:    TxPriorityHigh,
        KeyName:       "mykey",
    }
    
    data, err := json.Marshal(opts)
    require.NoError(t, err)
    
    var decoded SubmitOptions
    require.NoError(t, json.Unmarshal(data, &decoded))
    require.Equal(t, opts, &decoded)
}

---

🟢 Minor Improvements

6. Code Organization (pkg/blob/blob.go:20-32)

Consider grouping related constants and types:

const (
    // DefaultMaxBlobSize is the default maximum blob size used by celestia-app (32 MiB).
    DefaultMaxBlobSize = 32 * 1_048_576 // bytes
    
    // subtreeRootThreshold is copied from celestia-app/v6 appconsts.SubtreeRootThreshold.
    subtreeRootThreshold = 64
    
    // UnknownIndex indicates the blob's share index is not yet known
    UnknownIndex = -1
)

type (
    // Commitment is the Merkle subtree commitment for a blob.
    Commitment []byte
    
    // Proof is a set of NMT proofs used to verify a blob inclusion.
    Proof []*nmt.Proof
)

7. Potential Performance Issue (pkg/blob/blob.go:97-103)

MarshalJSON calls multiple accessor methods that may involve data copies. For hot paths, consider caching these values if marshaling happens frequently.

8. Documentation Enhancement (pkg/blob/README.md:14-18)

The sync guidance is helpful but could be more actionable:

## Keeping it in sync
- When celestia-node changes blob JSON or tx config fields, update this package manually.
- **Recommended**: Set up a CI job that:
  1. Clones celestia-node at the tracked version (currently v0.28.4)
  2. Runs structural diffs on key types
  3. Alerts on schema changes

Example commands:
\`\`\`bash
diff -u pkg/blob/blob.go <(curl -s https://raw.githubusercontent.com/celestiaorg/celestia-node/v0.28.4/blob/blob.go)
\`\`\`

9. Missing Package Documentation (pkg/blob/blob.go:1)

Add a package-level comment:

// Package blob provides a minimal, JSON-compatible subset of celestia-node's
// blob types without requiring Cosmos SDK dependencies. This package is
// intentionally a trimmed copy to maintain wire compatibility with celestia-node's
// blob RPC service. See README.md for sync guidance.
package blob

---

Security & Resilience

✅ No security concerns identified:

• No credential handling
• No unsafe operations
• Input validation present in NewBlob via namespace validation
• Error wrapping is consistent (except in UnmarshalJSON)

✅ Resilience considerations:

• The copy strategy reduces dependency risk
• JSON compatibility is well-tested with round-trip tests
• Binary encoding (MakeID/SplitID) uses standard library functions

---

Performance & Resource Efficiency

✅ Generally efficient:

• Minimal allocations in MakeID/SplitID
• No unnecessary copying in hot paths
• Embedded *libshare.Blob avoids duplication

⚠️ Potential concern: Multiple accessor method calls in MarshalJSON (see issue #7)

---

Testing Quality

Current coverage: 73.52% (68 lines added, 18 missing coverage)

Missing coverage areas:

1. MakeID edge cases
2. SplitID edge cases (empty, malformed IDs)
3. EqualCommitment all paths
4. SubmitOptions JSON marshaling
5. UnmarshalJSON error paths

Recommendation: Achieve >90% coverage by addressing Critical Issue #1 and Medium Issue #5.

---

Architecture & Module Design

✅ Excellent alignment with project goals:

• Follows the zero-dependency core package pattern mentioned in CLAUDE.md
• Avoids over-engineering (no premature abstractions)
• Clear separation of concerns
• Well-documented rationale for the copy approach

✅ Future-proofing:

• The README provides clear guidance for maintaining sync with upstream
• The versioned reference (v0.28.4) makes tracking changes easier
• Consider the CI check suggestion to automate drift detection

---

Action Items

Before Merge

1. ✅ Must fix: Add missing tests for critical functions (MakeID edge cases, SplitID edge cases, EqualCommitment)
2. ✅ Should fix: Improve error wrapping in UnmarshalJSON
3. ⚠️ Consider: Add validation to MakeID for nil/empty commitments

Post-Merge (Technical Debt)

4. Add SubmitOptions JSON tests
5. Set up CI drift detection as suggested in README
6. Add package-level documentation

---

Conclusion

This is a solid PR that achieves its goal of providing minimal blob types without heavy dependencies. The main gap is test coverage—addressing the critical testing issues will bring this from good to excellent. The architecture decision to copy types is well-justified and properly documented.

Recommendation: ✅ Approve with requested changes (fix test coverage and error wrapping before merge)

---

[codecov]

Codecov Report

❌ Patch coverage is 73.52941% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.50%. Comparing base (5ee785f) to head (459b570).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/blob/blob.go	73.52%	11 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2905      +/-   ##
==========================================
- Coverage   65.60%   65.50%   -0.10%     
==========================================
  Files          85       86       +1     
  Lines        7838     7906      +68     
==========================================
+ Hits         5142     5179      +37     
- Misses       2139     2162      +23     
- Partials      557      565       +8     

Flag	Coverage Δ
combined	65.50% <73.52%> (-0.10%)	⬇️

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]

should we do a monthly check on this in ci? fetch latest release and diff?

[randygrok]

I will add some mechanism on the last PR

[tac0turtle]

thanks for separating thr prs, its much easier to grok. Should we do some ci magic to verify the diff or you think its fine?

[randygrok]

I will do some last checkings on the last PR, some mechanism to keep track if something changes.