refactor: update cliento to blob api and remove core da #2887
Conversation
Add native Celestia blob API client using celestia-node's blob API. - Client with JSON-RPC connection to celestia-node - Native types (Namespace, Blob, Commitment, Proof) - Stub methods (Submit, Get, GetAll, GetProof, Included) - Unit tests for types and client
Implement Submit method for Celestia blob API client. Validation is delegated to celestia-node rather than duplicating checks in the client layer.
Implement Get, GetAll, GetProof, and Included methods for the Celestia blob API client. All methods delegate to celestia-node via JSON-RPC with appropriate logging.
Add temporary adapter that implements da.DA interface using the native Celestia blob API client. This allows ev-node to use the new Celestia client while maintaining compatibility with existing code. The adapter bridges the gap between: - Celestia's height-based blob API - ev-node's ID-based DA abstraction IDs are encoded as [height (8 bytes)][commitment] for compatibility.
Replace jsonrpc.NewClient with the new Celestia blob API adapter in testapp initialization. This uses the native Celestia blob API instead of the deprecated DA API.
Replace jsonrpc.NewClient with the new Celestia blob API adapter in evm and grpc app initialization. All apps now use the native Celestia blob API instead of the deprecated DA API.
Remove the generic JSON-RPC DA abstraction layer now that all applications use the Celestia blob API adapter directly. Changes: - Remove da/jsonrpc package (client, server, tests) - Remove tools/da-debug debugging tool - Embed JSON-RPC server directly in local-da for testing - Update documentation to reflect Celestia as the DA implementation - Remove da-debug from tools.mk build configuration
Move all DA-related code from core/da module into a single da/da.go file.
The DA interface is now co-located with its implementation (Celestia adapter)
rather than being a separate core module.
Changes:
- Consolidate core/da/{da.go,errors.go,namespace.go} into da/da.go
- Move DummyDA test utility to da/testing.go
- Update all imports from github.com/evstack/ev-node/core/da to github.com/evstack/ev-node/da
- Remove core/da directory entirely
- Fix package shadowing issues in block/internal/{submitting,syncing}
The DA interface is kept for testability with mocks, but no longer
needs to be a separate module since Celestia is the only implementation.
- Move all DA interface methods (Submit, Get, GetIDs, GetProofs, Commit, Validate) directly into Client - Rename internal Celestia blob API methods to lowercase (submit, get, getAll, getProof, included) - Update all applications to use celestiada.NewClient instead of celestiada.NewAdapter - Remove adapter.go and adapter_test.go (397 lines) - Update client tests to call lowercase internal methods
- Move all DA interface methods (Submit, Get, GetIDs, GetProofs, Commit, Validate) directly into Client - Rename internal Celestia blob API methods to lowercase (submit, get, getAll, getProof, included) - Update all applications to use celestiada.NewClient instead of celestiada.NewAdapter - Remove adapter.go and adapter_test.go (397 lines) - Update client tests to call lowercase internal methods
…v-node into celestia-blob-api-client # Conflicts: # da/celestia/client.go
- bring back command-line tool `da-debug` for inspecting data availability layers in blockchain. - Added functionality to query and search for blobs at specific DA heights and namespaces. - Introduced detailed logging and error handling for better user experience. - Created unit tests for key functions including namespace parsing, hex decoding, and blob data handling.
randygrok
changed the title
|
Claude finished @randygrok's task —— View job Comprehensive Code Review CompleteI've completed a thorough code review of this DA layer refactoring. This PR represents a significant architectural improvement, migrating from the previous 1. Code Quality & Architecture ⭐⭐⭐⭐½Strengths
Areas for Improvement1. Namespace handling could be more type-safe (da/da.go:180-264) // Current approach mixes []byte and Namespace struct
type Namespace struct {
Version uint8
ID [NamespaceIDSize]byte
}
// But many methods accept []byte for namespace
func (c *Client) Submit(ctx context.Context, blobs []da.Blob, gasPrice float64, namespace []byte)Recommendation: Consider consistently using the 2. Duplicate // Both da/da.go and da/celestia/client.go define makeID
func makeID(height uint64, commitment []byte) []byteRecommendation: Export the function from 3. Magic numbers in client (da/celestia/client.go:20, da/celestia/commitment.go:13) const defaultRetrieveTimeout = 10 * time.Second
const subtreeRootThreshold = 64Recommendation: Add comments explaining why these specific values were chosen, particularly 2. Error Handling & Resilience ⭐⭐⭐⭐Strengths
Areas for Improvement1. Error wrapping inconsistency (da/celestia/client.go:238, 290) // Some errors are wrapped, others are not
return nil, fmt.Errorf("invalid ID at index %d: %w", i, err) // Good ✓
return nil, fmt.Errorf("failed to get blobs at height %d: %w", height, err) // Good ✓Most error handling is good, but ensure ALL errors use 2. Silent error in namespace creation (da/da.go:245) func NamespaceFromString(s string) *Namespace {
hash := sha256.Sum256([]byte(s))
ns, _ := NewNamespaceV0(hash[:NamespaceVersionZeroDataSize])
return ns // Ignoring error - could return nil!
}Critical Issue: This function can return 3. Missing timeout on commitment calculation (da/celestia/client.go:305-314) func (c *Client) Commit(ctx context.Context, blobs []da.Blob, namespace []byte) ([]da.Commitment, error) {
// ctx is accepted but never used
for i, blob := range blobs {
commitment, err := CreateCommitment(blob, namespace)
// No timeout or cancellation check
}
}Recommendation: Either use the context for cancellation checks in loops, or remove it from the signature if not needed. 4. Batch size halving could oscillate (block/internal/submitting/da_submitter.go:444-466) case dapkg.StatusTooBig:
if len(items) == 1 {
return fmt.Errorf("unrecoverable error...")
}
half := len(items) / 2
if half == 0 {
half = 1
}
items = items[:half]Observation: This halving strategy works but doesn't account for per-blob vs total-batch size limits. The pre-check at line 392-400 helps, but consider a more sophisticated size calculation. 3. Security ⭐⭐⭐⭐⭐Strengths
Areas for Improvement1. Potential information leak in error messages (da/celestia/client.go:374, 409) Message: fmt.Sprintf("invalid namespace: %s", err.Error()),
Message: fmt.Sprintf("failed to create commitment for blob %d: %s", i, err.Error()),Low Priority: These expose internal error details to callers. Generally acceptable for a blockchain DA layer, but consider if any sensitive info could leak. 2. No rate limiting on client (da/celestia/client.go) 4. Performance & Resource Efficiency ⭐⭐⭐⭐Strengths
Areas for Improvement1. Unbounded memory growth in Get (da/celestia/client.go:222-262) func (c *Client) Get(ctx context.Context, ids []da.ID, namespace []byte) ([]da.Blob, error) {
heightGroups := make(map[uint64][]Commitment)
idToIndex := make(map[blobKey]int)
for i, id := range ids {
heightGroups[height] = append(heightGroups[height], commitment)
idToIndex[blobKey{height, string(commitment)}] = i
}
// Maps grow unbounded with number of IDs
}Recommendation: For very large ID sets, consider batching or streaming to limit memory usage. 2. Missing context cancellation checks in loops (da/celestia/client.go:244-259, 281-300) for i, id := range ids {
select {
case <-ctx.Done():
return nil, ctx.Err()
default:
}
// process ID
}3. Goroutine leak potential in marshaling (block/internal/submitting/da_submitter.go:544-562) for i, item := range items {
go func(idx int, itm T) {
sem <- struct{}{}
defer func() { <-sem }()
// If context is canceled early, some goroutines may still be waiting on sem
}(i, item)
}Minor Issue: If parent context is canceled, goroutines waiting on the semaphore might not exit immediately. 4. Inefficient double namespace filtering (block/internal/syncing/da_retriever.go:80-133) 5. Testing & Reliability ⭐⭐⭐Strengths
Critical Issues1. Very low test coverage on main client (Codecov report)
The main client implementation has minimal test coverage. Most tests just verify error paths with an unreachable server (da/celestia/client_test.go:148-225). Recommendation: Add integration tests with a mock celestia-node server or expand unit tests to cover:
2. No tests for error mapping logic (da/celestia/client.go:500-617) Recommendation: Add unit tests for error classification: func TestHandleSubmitError(t *testing.T) {
tests := []struct{
name string
err error
wantStatus da.StatusCode
}{
{"timeout error", context.DeadlineExceeded, da.StatusContextDeadline},
{"mempool error", errors.New("already in mempool"), da.StatusAlreadyInMempool},
// etc.
}
}3. DummyDA height ticker goroutine not tested (da/testing.go:53-74) 4. No concurrency tests 6. Documentation & Developer Experience ⭐⭐⭐½Strengths
Areas for Improvement1. Missing package-level documentation // da/da.go
package da // No package comment explaining the DA abstractionRecommendation: Add package-level documentation: // Package da provides an abstraction layer for Data Availability layers.
// It defines the DA interface and common types for interacting with DA layers
// such as Celestia. The package handles blob submission, retrieval, and verification
// with proper error handling and status codes.
package da2. Undocumented public types (da/celestia/types.go:19-41) type Blob struct {
Namespace Namespace `json:"namespace"`
Data []byte `json:"data"`
ShareVer uint8 `json:"share_version"` // What is ShareVer?
Commitment Commitment `json:"commitment"`
Signer []byte `json:"signer,omitempty"` // What format?
Index int `json:"index"` // Index of what?
}Recommendation: Add field-level comments explaining format and purpose. 3. No migration guide
4. SubmitOptions not fully documented (da/celestia/types.go:32-41) type SubmitOptions struct {
GasPrice float64 `json:"gas_price,omitempty"`
IsGasPriceSet bool `json:"is_gas_price_set,omitempty"` // Why is this separate?
// Other fields...
}The relationship between fields and their effects on submission isn't clear. 5. Missing examples
7. Additional ObservationsPositive Changes✅ Removed complex JSON-RPC proxy: Deleted Breaking Changes (Expected for Refactor)
Code Smells1. Mixed responsibilities in submitter (block/internal/submitting/da_submitter.go:307-343) 2. Global variable for DA visualization (pkg/rpc/server/server.go likely) 3. Large function (block/internal/submitting/da_submitter.go:346-497) Summary & RecommendationsPriority Issues to Address
Overall AssessmentThis is a solid refactoring that improves the architecture and maintainability of the DA layer. The code quality is generally high, with good error handling patterns and performance considerations. However, the test coverage is concerningly low for such a critical component. Scores:
Overall: ⭐⭐⭐⭐ (4/5) - Approve with recommendations for test coverage improvements. Recommended Actions Before Merge
The architectural improvements are excellent, but the low test coverage (48.69% patch coverage, 333 missing lines) is a concern for a core component. I recommend adding comprehensive tests before merging to production. |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2887 +/- ##
==========================================
- Coverage 64.72% 61.50% -3.22%
==========================================
Files 81 79 -2
Lines 7347 7530 +183
==========================================
- Hits 4755 4631 -124
- Misses 2050 2364 +314
+ Partials 542 535 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix relative paths in apps/grpc/README.md (../../../ -> ../../) - Add missing .md extension in docs/guides/full-node.md link - Wrap localhost URL in backticks in tools/da-debug/README.md
Medium returns 403 for automated requests, causing false positives in the CI link checker.
This reverts commit 97ee61d.
|
close in favor of #2893 |
Overview