feat: DA Client remove interface part 2, add client for celestia blob api #2909
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @randygrok's task in 4m 38s —— View job Comprehensive Code ReviewI've completed a thorough analysis of this PR. Here are my findings organized by priority and category: 🔴 Critical Issues1. Code Duplication - Errors and Types (HIGH PRIORITY)Location: This PR duplicates error definitions and type definitions that already exist in
Impact: This violates DRY principles and creates maintenance burden. When errors or types change in Recommendation:
Note: I see from the review comments that you mentioned "this is temporary, will be deleted when wiring new code." However, this creates technical debt that shouldn't exist even temporarily. Consider importing from 2. Package Structure and Multiple Clients (HIGH PRIORITY)Locations:
Issue: Multiple reviewers (julienrbrt, tac0turtle) raised concerns about having three different client implementations coexisting:
Concerns:
Recommendation:
3. Module Dependencies and Circular Dependency Risk (MEDIUM-HIGH PRIORITY)Location: Issue: // go.mod:40
replace github.com/evstack/ev-node/da => ./daMultiple reviewers (tac0turtle, julienrbrt) flagged concerns:
Concerns:
Recommendation:
🟡 High Priority Issues4. Testing Approach - Hand-written vs Mockery Mocks (HIGH PRIORITY)Location: Issue: tac0turtle requested using mockery-generated mocks instead of hand-written ones. Current state:
Assessment: The mockery configuration in Recommendation: Current approach is acceptable. The tests properly use mockery mocks. 5. Error Handling Patterns (MEDIUM PRIORITY)Location: Issues: a) String-based error matching (lines 131-145, 209-227): case strings.Contains(err.Error(), ErrTxTimedOut.Error()):
case strings.Contains(err.Error(), ErrBlobNotFound.Error()):Problem: This is fragile. If the RPC layer wraps errors differently, matching will fail. The code comment acknowledges this: "Handle known errors by substring because RPC may wrap them." Better approach:
b) Mixed error handling approaches (Submit vs Retrieve):
Recommendation: Standardize on sentinel errors with proper wrapping. Update 6. Context Timeout Management (MEDIUM PRIORITY)Location: Issue: getCtx, cancel := context.WithTimeout(ctx, c.defaultTimeout)
defer cancel()Observations:
Concerns:
Recommendation:
🟢 Medium Priority Issues7. Resource Management and Cleanup (MEDIUM PRIORITY)Location: Issue: func (c *Client) Close() {
if c != nil && c.closer != nil {
c.closer()
}
}Concerns:
Recommendations:
8. Configuration Validation (MEDIUM PRIORITY)Location: Issues: a) Silent nil return: func NewBlobClient(api BlobAPI, cfg BlobConfig) *BlobClient {
if api == nil {
return nil // Silent failure
}
// ...
}Should return error or panic with clear message. b) Namespace validation: namespaceBz: share.MustNewV0Namespace([]byte(cfg.Namespace)).Bytes(),
dataNamespaceBz: share.MustNewV0Namespace([]byte(cfg.DataNamespace)).Bytes(),Uses c) Missing validation:
Recommendation: func NewBlobClient(api BlobAPI, cfg BlobConfig) (*BlobClient, error) {
if api == nil {
return nil, errors.New("BlobAPI cannot be nil")
}
if cfg.Logger == (zerolog.Logger{}) {
return nil, errors.New("Logger must be configured")
}
// Validate and create namespaces with error handling
ns, err := share.NewV0Namespace([]byte(cfg.Namespace))
if err != nil {
return nil, fmt.Errorf("invalid namespace: %w", err)
}
// ... rest of validation
return &BlobClient{...}, nil
}9. Test Coverage Concerns (MEDIUM PRIORITY)Location: Overall test coverage Issue: Codecov reports coverage dropped from 65.94% to 63.21% (-2.74%) Analysis:
Missing coverage in
Recommendation:
10. Logging Practices (LOW-MEDIUM PRIORITY)Location: Observations: Good practices:
Potential improvements:
Recommendation: if code == StatusTooBig {
c.logger.Debug().
Err(err).
Uint64("blob_size", blobSize).
Uint64("max_blob_size", c.maxBlobSize).
Uint64("status", uint64(code)).
Msg("Blob exceeds size limit")
} else {
c.logger.Error().Err(err).Uint64("status", uint64(code)).Msg("DA submission failed")
}🔵 Low Priority Issues & Observations11. Documentation and CommentsGood:
Could improve:
12. Code Style and Go IdiomsGenerally good adherence to Go conventions:
Minor observations:
13. Integration Test QualityLocation: Positive aspects:
Observations:
14. Security ConsiderationsReviewed for common security issues: ✅ No sensitive data exposure:
✅ Input validation:
📊 Architecture AssessmentCurrent State vs. PR ChangesBefore this PR:
This PR adds:
Questions for maintainers:
Recommendation: Update PR description to clearly explain:
🎯 Summary and RecommendationsMust Fix Before Merge:
Should Fix Before Merge:
Nice to Have:
✅ What's Done Well
Overall Assessment: This PR introduces solid infrastructure for the Celestia blob API client, but has some architectural clarity issues and code duplication that should be addressed. The code quality is generally good, but the transition plan and relationship to existing code needs better documentation. |
Summary of ChangesHello @randygrok, I'm Gemini Code Assist1! 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 integrates the application with the Celestia Data Availability layer by introducing a dedicated client for its Blob API. This enhancement allows the system to efficiently submit and retrieve data, leveraging Celestia's capabilities for data availability. The changes include the core client implementation, necessary data structures, and robust testing infrastructure, ensuring reliable interaction with the DA layer. Highlights
Using Gemini Code AssistThe 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
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 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
|
randygrok
changed the title
randygrok
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a client for the Celestia blob API, which is a significant feature. The changes include new packages for the Celestia client (da/celestia) and a wrapper for its usage within the block package (block/internal/da), along with corresponding tests and dependency updates. The overall structure is reasonable, but there are a few key areas for improvement. Most notably, there is significant code duplication of types and error definitions in the block/internal/da package, which should be refactored to use the existing definitions from core/da to improve maintainability. Additionally, there's a minor issue in a test mock related to data cloning that could be improved for test robustness.
| package da | ||
|
|
||
| import "time" | ||
|
|
||
| // StatusCode mirrors DA status codes used in Celestia blob client. | ||
| type StatusCode uint64 | ||
|
|
||
| // Data Availability return codes. | ||
| const ( | ||
| StatusUnknown StatusCode = iota | ||
| StatusSuccess | ||
| StatusNotFound | ||
| StatusNotIncludedInBlock | ||
| StatusAlreadyInMempool | ||
| StatusTooBig | ||
| StatusContextDeadline | ||
| StatusError | ||
| StatusIncorrectAccountSequence | ||
| StatusContextCanceled | ||
| StatusHeightFromFuture | ||
| ) | ||
|
|
||
| // Blob is the data submitted/received from the DA layer. | ||
| type Blob = []byte | ||
|
|
||
| // ID should contain serialized data required by the implementation to find blob in DA. | ||
| type ID = []byte | ||
|
|
||
| // Commitment should contain serialized cryptographic commitment to Blob value. | ||
| type Commitment = []byte | ||
|
|
||
| // Proof should contain serialized proof of inclusion (publication) of Blob in DA. | ||
| type Proof = []byte | ||
|
|
||
| // GetIDsResult holds the result of GetIDs call: IDs and timestamp of corresponding block. | ||
| type GetIDsResult struct { | ||
| IDs []ID | ||
| Timestamp time.Time | ||
| } | ||
|
|
||
| // ResultSubmit contains information returned from DA layer after block headers/data submission. | ||
| type ResultSubmit struct { | ||
| BaseResult | ||
| } | ||
|
|
||
| // ResultRetrieve contains batch of block data returned from DA layer client. | ||
| type ResultRetrieve struct { | ||
| BaseResult | ||
| // Data is the block data retrieved from Data Availability Layer. | ||
| // If Code is not equal to StatusSuccess, it has to be nil. | ||
| Data [][]byte | ||
| } | ||
|
|
||
| // BaseResult contains basic information returned by DA layer. | ||
| type BaseResult struct { | ||
| // Code is to determine if the action succeeded. | ||
| Code StatusCode | ||
| // Message may contain DA layer specific information (like DA block height/hash, detailed error message, etc) | ||
| Message string | ||
| // Height is the height of the block on Data Availability Layer for given result. | ||
| Height uint64 | ||
| // SubmittedCount is the number of successfully submitted blocks. | ||
| SubmittedCount uint64 | ||
| // BlobSize is the size of the blob submitted. | ||
| BlobSize uint64 | ||
| // IDs is the list of IDs of the blobs submitted. | ||
| IDs [][]byte | ||
| // Timestamp is the timestamp of the posted data on Data Availability Layer. | ||
| Timestamp time.Time | ||
| } |
There was a problem hiding this comment.
This file duplicates type definitions like StatusCode, ResultSubmit, ResultRetrieve, and BaseResult which are already defined in core/da/da.go. To avoid code duplication and improve maintainability, this file should be removed and the types from the core/da package should be imported and used instead.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2909 +/- ##
==========================================
- Coverage 65.94% 63.27% -2.68%
==========================================
Files 87 90 +3
Lines 7990 8506 +516
==========================================
+ Hits 5269 5382 +113
- Misses 2147 2542 +395
- Partials 574 582 +8
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:
|
block/internal/da/blob_client.go
Outdated
| "time" | ||
|
|
||
| "github.com/celestiaorg/go-square/v3/share" | ||
| blobrpc "github.com/evstack/ev-node/da/jsonrpc/blob" |
There was a problem hiding this comment.
these imports were the reason we had core, now we import everything into ev-node. can this be an interface defined in block instead of relying on da?
There was a problem hiding this comment.
but pkg da is not a go.mod, what is the benefit that I cant really catch?
…node into add-da-blob-api-client
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
looks better, ci is still red and we should use mockery everywhere
* main: chore: execute goimports to format the code (#2924) refactor(block)!: remove GetLastState from components (#2923) feat(syncing): add grace period for missing force txs inclusion (#2915) chore: minor improvement for docs (#2918) feat: DA Client remove interface part 2, add client for celestia blob api (#2909) chore: update rust deps (#2917) feat(sequencers/based): add based batch time (#2911) build(deps): Bump golangci/golangci-lint-action from 9.1.0 to 9.2.0 (#2914) refactor(sequencers): implement batch position persistance (#2908)
Parent Epic: #2796
New client (namespace blob) - New client not yet wired
Both clients coexist for now (old and new), but the new one is not wired.
The idea is to wire the New client in another PR, because it is the biggest part and it will generate a lot of code changes.