Skip to content

Conversation

@tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Nov 28, 2025

Overview

since we never looked at blobids it was set to an empty namespace this pr replaces it with what is needed

@github-actions
Copy link
Contributor

github-actions bot commented Nov 28, 2025

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedNov 28, 2025, 3:12 PM

@claude
Copy link
Contributor

claude bot commented Nov 28, 2025

Claude finished @tac0turtle's task —— View job


Code Review Complete

Tasks

  • Analyze PR changes and read modified files
  • Review code quality and architecture
  • Assess error handling and resilience
  • Check security considerations
  • Evaluate performance and resource efficiency
  • Review testing and reliability
  • Assess documentation and developer experience
  • Provide comprehensive feedback

Summary

This PR fixes the DA visualizer by adding namespace support for blob retrieval. The changes enable the visualization dashboard to properly fetch blob details by passing the namespace parameter through the submission recording, URL links, and blob retrieval logic.

Overall Assessment: Good implementation with minor improvement opportunities

The core functionality is solid and addresses the namespace requirement correctly. The changes are backward-compatible, well-tested, and follow Go conventions. However, there are some areas for improvement around error handling, test coverage, and potential edge cases.


Detailed Feedback

1. Code Quality & Architecture ✅

Strengths:

  • Clean separation of concerns - namespace handling added at appropriate layers (submitter → visualization server → HTML template)
  • Good use of Go idioms (hex encoding/decoding, mutex protection)
  • Consistent method signature updates across the codebase
  • Template variable scoping in HTML is correctly implemented

Minor Improvements:

da_visualization.go:182-190 - Namespace parsing logic could benefit from explicit error logging:

if nsParam != "" {
    if ns, err := coreda.ParseHexNamespace(nsParam); err == nil {
        namespace = ns.Bytes()
        found = true
    } else {
        ns := coreda.NamespaceFromString(nsParam)
        namespace = ns.Bytes()
        found = true
    }
}

Consider logging when ParseHexNamespace fails and fallback is used. This helps debugging:

if ns, err := coreda.ParseHexNamespace(nsParam); err == nil {
    namespace = ns.Bytes()
    found = true
} else {
    s.logger.Debug().Err(err).Str("namespace_param", nsParam).Msg("failed to parse hex namespace, falling back to string namespace")
    ns := coreda.NamespaceFromString(nsParam)
    namespace = ns.Bytes()
    found = true
}

da_visualization.go:193-212 - Nested loops with early break pattern is correct but could be slightly cleaner:
The double-nested loop with break logic works but consider extracting to a helper method:

func (s *DAVisualizationServer) findNamespaceForBlob(blobID string) ([]byte, bool) {
    s.mutex.RLock()
    defer s.mutex.RUnlock()
    
    for _, submission := range s.submissions {
        for _, subBlobID := range submission.BlobIDs {
            if subBlobID == blobID {
                if submission.Namespace != "" {
                    if ns, err := hex.DecodeString(submission.Namespace); err == nil {
                        return ns, true
                    }
                }
                return nil, false
            }
        }
    }
    return nil, false
}

2. Error Handling & Resilience ⚠️

Issues Found:

da_visualization.go:186 - Silent error handling in fallback path:

} else {
    ns := coreda.NamespaceFromString(nsParam)
    namespace = ns.Bytes()
    found = true
}

NamespaceFromString might also fail or produce unexpected results. Consider validating the result or at minimum documenting expected behavior.

da_visualization.go:199-201 - Silent error handling when decoding namespace from submissions:

if ns, err := hex.DecodeString(submission.Namespace); err == nil {
    namespace = ns
    found = true
}

If hex decoding fails, we silently continue. Consider logging this as it indicates data corruption:

if ns, err := hex.DecodeString(submission.Namespace); err == nil {
    namespace = ns
    found = true
} else {
    s.logger.Warn().Err(err).Str("blob_id", blobID).Str("namespace", submission.Namespace).Msg("failed to decode stored namespace")
}

da_visualization.go:214-217 - Error message is good but could be more actionable:
The error message correctly identifies the issue but could guide users on how to fix it:

http.Error(w, "Namespace required to retrieve blob. Provide namespace as query parameter: ?namespace=<hex_or_string>", http.StatusBadRequest)

3. Security 🔒

No Critical Issues Found

Good practices observed:

  • Input validation on blob ID (hex decoding with error check)
  • Namespace parameter sanitization through ParseHexNamespace and NamespaceFromString
  • No SQL injection risk (no database queries)
  • No XSS vulnerabilities in template (Go templates auto-escape by default)
  • Mutex protection prevents race conditions on shared state

Minor consideration:

  • da_visualization.go:159-162 - The blob ID is user-provided input. While hex decoding validates format, consider adding length limits to prevent resource exhaustion from extremely long strings:
blobID := r.URL.Query().Get("id")
if blobID == "" {
    http.Error(w, "Missing blob ID parameter", http.StatusBadRequest)
    return
}
if len(blobID) > 1024 { // Reasonable limit for hex-encoded blob ID
    http.Error(w, "Blob ID too long", http.StatusBadRequest)
    return
}

4. Performance & Resource Efficiency ⚡

Good practices:

  • Namespace stored as hex string in memory (efficient serialization)
  • Read lock used appropriately when searching submissions (allows concurrent reads)
  • Limited to 100 submissions in memory (bounded memory usage)

Potential optimization:

da_visualization.go:193-212 - O(n*m) search through submissions and blob IDs:
For large numbers of blobs, this could become slow. Consider:

  1. Adding an index map: map[string]string // blobID -> namespace
  2. Updating it when recording submissions
  3. Only fall back to linear search if not found in index

Example:

type DAVisualizationServer struct {
    da            coreda.DA
    logger        zerolog.Logger
    submissions   []DASubmissionInfo
    blobNamespace map[string]string // blobID -> namespace (hex)
    mutex         sync.RWMutex
    isAggregator  bool
}

// In RecordSubmission:
for _, blobID := range blobIDs {
    s.blobNamespace[blobID] = hex.EncodeToString(namespace)
}

// In handleDABlobDetails:
if !found {
    s.mutex.RLock()
    if ns, exists := s.blobNamespace[blobID]; exists {
        if decoded, err := hex.DecodeString(ns); err == nil {
            namespace = decoded
            found = true
        }
    }
    s.mutex.RUnlock()
}

Note: The index should also respect the 100-submission limit to avoid unbounded growth.

5. Testing & Reliability ⚠️

Code Coverage Concern:
Codecov reports 28 lines missing coverage with only 6.66% patch coverage. The changes have:

  • da_visualization.go: 27 lines missing coverage
  • da_submitter.go: 1 line missing coverage

Missing Test Cases:

  1. Namespace parameter parsing - No tests for:

    • Valid hex namespace in query parameter
    • Invalid hex namespace (triggers string fallback)
    • Namespace lookup from recent submissions
    • Namespace not found scenario (line 214-217)
  2. Edge cases not tested:

    • Empty namespace parameter
    • Malformed hex strings
    • Blob ID exists but has no namespace
    • Concurrent access during namespace lookup

Recommended additions to da_visualization_test.go:

func TestHandleDABlobDetailsWithNamespaceParam(t *testing.T) {
    da := &mocks.MockDA{}
    logger := zerolog.New(nil)
    server := NewDAVisualizationServer(da, logger, true)
    
    testNamespace := []byte("test-namespace")
    blobID := []byte("test-blob-id")
    
    // Mock DA to return blob
    da.On("Get", mock.Anything, mock.Anything, testNamespace).Return([][]byte{[]byte("blob-content")}, nil)
    
    req, err := http.NewRequest("GET", fmt.Sprintf("/da/blob?id=%s&namespace=%s", 
        hex.EncodeToString(blobID), hex.EncodeToString(testNamespace)), nil)
    require.NoError(t, err)
    
    rr := httptest.NewRecorder()
    server.handleDABlobDetails(rr, req)
    
    assert.Equal(t, http.StatusOK, rr.Code)
}

func TestHandleDABlobDetailsNamespaceFromSubmissions(t *testing.T) {
    da := &mocks.MockDA{}
    logger := zerolog.New(nil)
    server := NewDAVisualizationServer(da, logger, true)
    
    blobID := "746573742d626c6f622d6964" // "test-blob-id" in hex
    testNamespace := []byte("test-namespace")
    
    // Record a submission with this blob
    result := &coreda.ResultSubmit{
        BaseResult: coreda.BaseResult{
            Code:      coreda.StatusSuccess,
            Height:    100,
            BlobSize:  1024,
            Timestamp: time.Now(),
            IDs:       [][]byte{[]byte("test-blob-id")},
        },
    }
    server.RecordSubmission(result, 0.5, 1, testNamespace)
    
    // Mock DA
    da.On("Get", mock.Anything, mock.Anything, testNamespace).Return([][]byte{[]byte("content")}, nil)
    
    // Request without namespace param - should find from submissions
    req, err := http.NewRequest("GET", fmt.Sprintf("/da/blob?id=%s", blobID), nil)
    require.NoError(t, err)
    
    rr := httptest.NewRecorder()
    server.handleDABlobDetails(rr, req)
    
    assert.Equal(t, http.StatusOK, rr.Code)
}

func TestHandleDABlobDetailsNoNamespace(t *testing.T) {
    da := &mocks.MockDA{}
    logger := zerolog.New(nil)
    server := NewDAVisualizationServer(da, logger, true)
    
    req, err := http.NewRequest("GET", "/da/blob?id=deadbeef", nil)
    require.NoError(t, err)
    
    rr := httptest.NewRecorder()
    server.handleDABlobDetails(rr, req)
    
    assert.Equal(t, http.StatusBadRequest, rr.Code)
    assert.Contains(t, rr.Body.String(), "Namespace required")
}

da_submitter_test.go - Should verify namespace is passed correctly:

func TestSubmitToDAPassesNamespace(t *testing.T) {
    // Verify that RecordSubmission receives correct namespace parameter
    // This would catch regression if namespace parameter is dropped
}

6. Documentation & Developer Experience 📚

Good:

  • HTML template clearly documents the new namespace parameter in API examples (line 85)
  • Function signature change is self-documenting
  • Template comments explain the namespace variable scoping

Improvements:

da_visualization.go:54 - Function comment should document the new parameter:

// RecordSubmission records a DA submission for visualization
// Only keeps the last 100 submissions in memory for the dashboard display
// Parameters:
//   - result: The submission result from the DA layer
//   - gasPrice: Gas price used for the submission (use 0 if not applicable)
//   - numBlobs: Number of blobs in the submission
//   - namespace: DA namespace where the blobs were submitted
func (s *DAVisualizationServer) RecordSubmission(result *coreda.ResultSubmit, gasPrice float64, numBlobs uint64, namespace []byte)

da_visualization.go:176-190 - Add comment explaining the namespace resolution strategy:

// Resolve namespace using two-tier approach:
// 1. First, check if provided as query parameter (allows explicit override)
// 2. Fall back to searching recent submissions for this blob ID
// This provides flexibility while maintaining backwards compatibility
var namespace []byte
found := false

templates/da_visualization.html:183 - Good inline documentation in template. Consider adding tooltip or help text for users.


Additional Observations

Positive Patterns:

  1. Backwards compatibility: Empty namespace ([]byte{}) in tests shows the code handles empty namespaces gracefully
  2. Consistent encoding: Uses hex encoding consistently for namespace serialization
  3. Defensive programming: Multiple validation checks before attempting DA layer calls

Typo in Commit Message:

Commit d0ff255 has "fix visulaizer" → should be "visualizer"


Recommendations Priority

High Priority:

  1. ✅ Add test coverage for namespace parameter handling (raises coverage from 6.66% to acceptable level)
  2. ✅ Add error logging for namespace parsing failures (aids debugging)

Medium Priority:

  1. ⚠️ Consider adding blob ID length validation (prevents potential DoS)
  2. ⚠️ Add documentation comments for new parameter (improves maintainability)
  3. ⚠️ Extract namespace lookup to helper method (improves readability)

Low Priority (Future Optimization):

  1. 💡 Consider index-based blob→namespace mapping if performance becomes an issue
  2. 💡 Add more detailed error messages with actionable guidance

Conclusion

This PR successfully fixes the DA visualizer namespace issue. The implementation is sound and follows Go best practices. The main concern is test coverage - adding the recommended test cases would significantly improve confidence in the changes and prevent regressions.

The changes are production-ready but would benefit from the high-priority improvements listed above.

@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 6.66667% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.59%. Comparing base (bab058d) to head (a638837).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/rpc/server/da_visualization.go 6.89% 27 Missing ⚠️
block/internal/submitting/da_submitter.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2895      +/-   ##
==========================================
- Coverage   64.81%   64.59%   -0.23%     
==========================================
  Files          81       81              
  Lines        7347     7374      +27     
==========================================
+ Hits         4762     4763       +1     
- Misses       2043     2069      +26     
  Partials      542      542              
Flag Coverage Δ
combined 64.59% <6.66%> (-0.23%) ⬇️

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 marked this pull request as ready for review November 28, 2025 15:12
@tac0turtle tac0turtle added this pull request to the merge queue Dec 1, 2025
Merged via the queue into main with commit b5f5640 Dec 1, 2025
26 of 29 checks passed
@tac0turtle tac0turtle deleted the marko/visualizer_fix branch December 1, 2025 09:00
alpe added a commit that referenced this pull request Dec 3, 2025
* main:
  build(deps): Bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /docs in the npm_and_yarn group across 1 directory (#2900)
  refactor(block): centralize timeout in client (#2903)
  build(deps): Bump the all-go group across 2 directories with 3 updates (#2898)
  chore: bump default timeout (#2902)
  fix: revert default db (#2897)
  refactor: remove obsolete // +build tag (#2899)
  fix:da visualiser namespace  (#2895)
  refactor: omit unnecessary reassignment (#2892)
  build(deps): Bump the all-go group across 5 directories with 6 updates (#2881)
  chore: fix inconsistent method name in retryWithBackoffOnPayloadStatus comment (#2889)
  fix: ensure consistent network ID usage in P2P subscriber (#2884)
  build(deps): Bump golangci/golangci-lint-action from 9.0.0 to 9.1.0 (#2885)
  build(deps): Bump actions/checkout from 5 to 6 (#2886)
alpe added a commit that referenced this pull request Dec 8, 2025
* main:
  refactor(sequencers): persist prepended batch (#2907)
  feat(evm): add force inclusion command (#2888)
  feat: DA client, remove interface part 1: copy subset of types needed for the client using blob rpc. (#2905)
  feat: forced inclusion (#2797)
  fix: fix and cleanup metrics (sequencers + block) (#2904)
  build(deps): Bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /docs in the npm_and_yarn group across 1 directory (#2900)
  refactor(block): centralize timeout in client (#2903)
  build(deps): Bump the all-go group across 2 directories with 3 updates (#2898)
  chore: bump default timeout (#2902)
  fix: revert default db (#2897)
  refactor: remove obsolete // +build tag (#2899)
  fix:da visualiser namespace  (#2895)
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