DA Client remove interface: Part 2.5, create e2e test to validate that a blob is posted in DA layer. #2920
Conversation
This reverts commit 1d8c9f2.
…node into add-da-blob-api-client
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @randygrok's task in 2m 37s —— View job Code Review CompleteI've completed a comprehensive review of this PR. Overall, this is a well-structured E2E test that validates DA posting functionality. The test orchestrates a complex multi-container environment using tastora and successfully validates the end-to-end flow. However, there are several areas for improvement across code quality, error handling, performance, and maintainability. 🎯 High Priority Issues1. Missing Function Definitions (test/e2e/da_posting_integration_test.go:48)configurePrefixOnce.Do(configureCelestiaBech32Prefix)Issue: Neither Fix: These should be imported or defined. Looking at
Recommendation: Create a shared helper file to avoid duplication: // test/e2e/test_helpers.go
package e2e
import (
"sync"
sdk "github.com/cosmos/cosmos-sdk/types"
)
var configurePrefixOnce sync.Once
func configureCelestiaBech32Prefix() {
conf := sdk.GetConfig()
conf.SetBech32PrefixForAccount("celestia", "celestiapub")
}2. Fragile Namespace Matching Logic (test/e2e/da_posting_integration_test.go:268)if string(attr.Key) == "namespaces" && strings.Contains(string(attr.Value), dataNSB64) {Issue: Using Recommendation: Decode and parse the attribute value properly: if string(attr.Key) == "namespaces" {
// Parse the attribute value (likely a comma-separated or JSON list)
namespaces := parseNamespacesAttribute(attr.Value)
for _, ns := range namespaces {
if ns == dataNSB64 {
pfbHeight = tx.Height
return true, nil
}
}
}3. Unchecked wait.ForCondition Errors (lines 234, 253, 278)wait.ForCondition(ctx, 30*time.Second, 2*time.Second, func() (bool, error) {
// ... condition check
})Issue: Three Fix: Add error checks: err = wait.ForCondition(ctx, 30*time.Second, 2*time.Second, func() (bool, error) {
res, err := cli.Get(ctx, "/kv?key="+key)
if err != nil {
return false, nil
}
return string(res) == value, nil
})
require.NoError(t, err, "key-value should be retrievable from ev-node")Apply the same fix to lines 253 and 278. 🔧 Medium Priority Issues4. Resource Leak in HTTP Client (test/e2e/da_posting_integration_test.go:310)func (c *httpClient) Get(ctx context.Context, path string) ([]byte, error) {
// ...
resp, err := http.DefaultClient.Do(req)
if err != nil {
return nil, err
}
defer resp.Body.Close()Issue: Using Recommendation: Create a properly configured HTTP client: type httpClient struct {
baseURL string
client *http.Client
}
func newHTTPClient(host, port string) (*httpClient, error) {
return &httpClient{
baseURL: fmt.Sprintf("http://%s:%s", host, port),
client: &http.Client{
Timeout: 30 * time.Second,
Transport: &http.Transport{
MaxIdleConns: 10,
IdleConnTimeout: 30 * time.Second,
DisableCompression: false,
},
},
}, nil
}5. Error Messages Could Be More Descriptive (throughout)Several error messages lack context. For example: require.NoError(t, err, "bridge wallet")
require.NoError(t, err, "bridge network info")Recommendation: Add more context: require.NoError(t, err, "failed to get bridge wallet for funding")
require.NoError(t, err, "failed to get bridge network info for DA connection")6. Hardcoded Magic Numbers (throughout)sendAmt := sdk.NewInt64Coin(chain.Config.Denom, 5_000_000_000)
// ...
"--fees", fmt.Sprintf("1000%s", chain.Config.Denom),Recommendation: Extract to constants with documentation: const (
// Bridge wallet initial funding in base denomination
bridgeWalletInitialFunding = 5_000_000_000
// Standard transaction fee in base denomination
standardTxFee = 1000
// DA client max blob size
maxBlobSize = seqcommon.AbsoluteMaxBlobSize
)📊 Code Quality & Architecture7. Test Timeout ManagementThe test uses a 5-minute timeout (line 50), but internal operations have their own timeouts (2 minutes for validator RPC, 1 minute for blob searches). This is good defensive programming, but the timeouts could be better organized. Recommendation: Define timeout constants at the top of the file: const (
testTimeout = 5 * time.Minute
validatorReadyTimeout = 2 * time.Minute
blobSearchTimeout = 1 * time.Minute
kvReadyTimeout = 30 * time.Second
)8. Code Duplication with da_client_integration_test.goSignificant code duplication exists:
Recommendation: Extract common setup logic to shared test utilities: // test/e2e/celestia_setup.go
func SetupCelestiaChainAndBridge(t *testing.T, ctx context.Context, uniqueTestName string, dockerClient, networkID) (*Chain, *BridgeNode, error) {
// Common setup logic
}9. Missing Cleanup for DA Client (line 246)daClient, err := jsonrpc.NewClient(ctx, zerolog.Nop(), daRPCAddr, authToken, seqcommon.AbsoluteMaxBlobSize)
require.NoError(t, err, "new da client")
defer daClient.Close()Good: Proper cleanup with 🔒 Security Considerations10. Auth Token HandlingauthToken, err := bridge.GetAuthToken()
require.NoError(t, err, "bridge auth token")Status: ✅ Good - The auth token is obtained from the bridge and not hardcoded. However, ensure logging doesn't expose it. Recommendation: Verify that the // If you ever add logging, ensure auth tokens are redacted
logger := zerolog.New(os.Stdout).With().
Str("component", "da-client").
Logger().
Hook(zerolog.HookFunc(func(e *zerolog.Event, level zerolog.Level, msg string) {
// Redact sensitive fields
}))11. Input Validation in httpClientThe body := strings.NewReader(fmt.Sprintf("%s=%s", key, value))Issue: No validation or escaping of ⚡ Performance & Resource Efficiency12. Sequential Height Checking in Final Validation (lines 282-293)for h := pfbHeight; h <= pfbHeight+10; h++ {
ids, err := daClient.DA.GetIDs(ctx, uint64(h), dataNamespace.Bytes())
if err != nil {
t.Logf("GetIDs data height=%d err=%v", h, err)
continue
}
if ids != nil && len(ids.IDs) > 0 {
return true, nil
}
}Issue: This checks up to 11 heights sequentially within a retry loop. If the blob appears at a later height, this wastes time checking earlier heights repeatedly. Recommendation: Track which heights have been checked to avoid redundant queries: checkedHeights := make(map[int64]bool)
err = wait.ForCondition(ctx, time.Minute, 5*time.Second, func() (bool, error) {
if pfbHeight == 0 {
return false, nil
}
currentHeight, err := getLatestHeight(ctx)
if err != nil {
return false, nil
}
for h := pfbHeight; h <= min(pfbHeight+10, currentHeight); h++ {
if checkedHeights[h] {
continue
}
ids, err := daClient.DA.GetIDs(ctx, uint64(h), dataNamespace.Bytes())
checkedHeights[h] = true
if err != nil {
t.Logf("GetIDs data height=%d err=%v", h, err)
continue
}
if ids != nil && len(ids.IDs) > 0 {
return true, nil
}
}
return false, nil
})13. Potential Goroutine Leak in wait.ForConditionThe test doesn't explicitly cancel contexts in all failure paths. While 🧪 Testing Patterns & Reliability14. Test Skip Logicif testing.Short() {
t.Skip("skip integration in short mode")
}Status: ✅ Good - Proper use of short mode skipping for long-running E2E tests. 15. Retry Logic InconsistencyThe test uses for attempt := 0; attempt < 3; attempt++ {
// ... submit blob
}Recommendation: Replace with 16. Environment Variable Handlingfunc getEnvDefault(key, def string) string {
if v := strings.TrimSpace(os.Getenv(key)); v != "" {
return v
}
return def
}Status: ✅ Good pattern for environment variable handling with defaults. 17. Docker Image Validationif repo == "evstack" && tag == "local-dev" {
if err := exec.Command("docker", "image", "inspect", "evstack:local-dev").Run(); err != nil {
t.Fatalf("missing docker image evstack:local-dev; run `make docker-build`...")
}
}Status: ✅ Excellent - Fails fast with a clear error message if the required Docker image is missing. 📚 Documentation & Developer Experience18. Function Documentation// TestEvNode_PostsToDA spins up celestia-app, a celestia bridge node and an
// EV Node (aggregator) via tastora, then verifies the EV Node actually posts
// data to DA by confirming blobs exist in the ev-data namespace via the DA
// JSON-RPC client.
func TestEvNode_PostsToDA(t *testing.T) {Status: ✅ Good - Clear, comprehensive test documentation. Enhancement: Consider adding more details about what this validates: // TestEvNode_PostsToDA validates the complete DA posting flow:
// 1. Sets up a local Celestia devnet (celestia-app + bridge node)
// 2. Launches an EV Node aggregator configured to post to Celestia
// 3. Submits a transaction to the EV Node
// 4. Verifies the resulting block data appears in the Celestia DA layer
// 5. Confirms the blob can be retrieved via the Celestia JSON-RPC API
//
// This test ensures that the DA client integration works end-to-end and
// validates that refactoring the DA client interface maintains functionality.19. Helper Function DocumentationMost helper functions lack documentation: func newHTTPClient(host, port string) (*httpClient, error) {
func getEvNodeImage(t *testing.T) container.Image {
func getEnvDefault(key, def string) string {Recommendation: Add godoc comments: // newHTTPClient creates a simple HTTP client for testing EV Node endpoints.
// It constructs the base URL from the provided host and port.
func newHTTPClient(host, port string) (*httpClient, error) {
// getEvNodeImage resolves the Docker image to use for EV Node tests.
// It checks EV_NODE_IMAGE_REPO and EV_NODE_IMAGE_TAG environment variables,
// falling back to evstack:local-dev. If using the local default, it verifies
// the image exists to provide clear error messages.
func getEvNodeImage(t *testing.T) container.Image {🔄 CI/CD Integration20. Makefile ChangesThe changes to test-e2e: build build-da build-evm docker-build-if-localStatus: ✅ Good - Proper dependency chain ensures binaries are built before E2E tests run. 21. GitHub Actions WorkflowThe CI workflow at - name: Build evstack:local-dev (cached)
uses: docker/build-push-action@v6
with:
cache-from: type=gha
cache-to: type=gha,mode=maxStatus: ✅ Excellent - Uses GitHub Actions cache to speed up builds. ✅ What's Done Well
🎬 SummaryThis PR successfully adds an important E2E test to validate DA posting functionality. The test is well-structured and comprehensive, but would benefit from:
The test validates critical functionality and will be valuable for ensuring the DA client refactoring maintains correct behavior. Once the compilation issue is fixed and error handling is improved, this will be a solid addition to the test suite. 📋 Actionable RecommendationsMust Fix Before Merge:
Should Fix:
Nice to Have:
|
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 introduces a critical new end-to-end integration test designed to validate the functionality of the EV Node's interaction with the Celestia Data Availability layer. The test simulates a complete operational environment, ensuring that data blobs are correctly posted and verifiable on the DA layer. This is a crucial step in confirming the robustness of the new DA client implementation as part of an ongoing refactoring effort, guaranteeing that core data posting mechanisms remain functional and reliable. 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
|
![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 new end-to-end test to validate that blobs are correctly posted to the Data Availability (DA) layer by the EV Node. The test sets up a comprehensive environment using the tastora framework, including celestia-app, a celestia-node bridge, and the EV Node itself. This is a crucial addition for verifying the functionality of the new client implementation. While the test structure is sound, some minor improvements can be made to enhance robustness and maintainability.
| dataNSB64 := base64.StdEncoding.EncodeToString(dataNamespace.Bytes()) | ||
| for _, tx := range res.Txs { | ||
| if tx.TxResult.Code != 0 { | ||
| continue | ||
| } | ||
| for _, ev := range tx.TxResult.Events { | ||
| if ev.Type != "celestia.blob.v1.EventPayForBlobs" { | ||
| continue | ||
| } | ||
| for _, attr := range ev.Attributes { | ||
| if string(attr.Key) == "namespaces" && strings.Contains(string(attr.Value), dataNSB64) { |
There was a problem hiding this comment.
The current logic for checking the namespaces attribute relies on strings.Contains with a base64 encoded string. This approach is fragile as it assumes the exact string representation of the attribute value. If the format of the namespaces attribute changes (e.g., it becomes a comma-separated list or a different encoding), this check could fail unexpectedly. A more robust solution would involve decoding the attribute value and then performing an exact match or checking for presence in a parsed list.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2920 +/- ##
==========================================
- Coverage 63.54% 63.53% -0.02%
==========================================
Files 90 90
Lines 8572 8572
==========================================
- Hits 5447 5446 -1
- Misses 2543 2545 +2
+ Partials 582 581 -1
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:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| with: | ||
| go-version-file: ./go.mod | ||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium test
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
| - name: Build evstack:local-dev (cached) | ||
| uses: docker/build-push-action@v6 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium test
* main: build(deps): Bump actions/cache from 4 to 5 (#2934) build(deps): Bump actions/download-artifact from 6 to 7 (#2933) build(deps): Bump actions/upload-artifact from 5 to 6 (#2932) feat: DA Client remove interface part 3, replace types with new code (#2910) DA Client remove interface: Part 2.5, create e2e test to validate that a blob is posted in DA layer. (#2920)
Create e2e test to validate that a blob is posted into DA.
This will verify that once the final refactor is done it still continue working with the new client implementation.
Overview