Skip to content

feat: implement bridge service client#1465

Merged
arnaubennassar merged 8 commits intodevelopfrom
feat/bridge-service-client
Jan 30, 2026
Merged

feat: implement bridge service client#1465
arnaubennassar merged 8 commits intodevelopfrom
feat/bridge-service-client

Conversation

@arnaubennassar
Copy link
Collaborator

🔄 Changes Summary

  • Created new bridgeservice/client package implementing a Go client for the Bridge
    Service REST API
  • Implemented client methods for all 13 REST API endpoints (health check, bridges,
    claims, token mappings, etc.)
  • Added comprehensive test suite with 91.9% code coverage
  • Included documentation (README.md) and usage examples (example_test.go)

⚠️ Breaking Changes

None - This is a new package addition with no breaking changes to existing code.

📋 Config Updates

No configuration changes required.

✅ Testing

  • 🤖 Automatic:
    • Unit tests for all 13 client methods covering success cases, error handling, and edge
      cases
    • Context cancellation and timeout handling tests
    • Parameter validation tests (minimal params, all params, optional params)
    • HTTP error response handling (400, 404, 500, 503)
    • Malformed JSON response handling
    • Network error handling
    • Test Coverage: 91.9% of statements
    • Test Command: go test -v -cover ./bridgeservice/client/...
  • 🖱️ Manual:
    a. Start a bridge service instance
    b. Update the example code with the service URL
    c. Run example tests to verify connectivity: go test -v ./bridgeservice/client/... -run
    Example

🐞 Issues

None

🔗 Related PRs

None

📝 Notes

Design Decisions

  • Optional Parameters: Used pointer types (*uint32, *string, etc.) for optional
    parameters to distinguish between "not provided" and "zero value"
  • Parameter Structs: Created separate parameter structs (GetBridgesParams,
    GetClaimsParams, etc.) for each endpoint to provide type-safe, self-documenting API
  • Error Messages: Include HTTP status codes and response bodies in error messages for
    easier debugging
  • Default Timeout: Set to 30 seconds, configurable via Config.Timeout
  • URL Handling: Automatically trims trailing slashes from base URL to prevent
    double-slash issues

Implementation Coverage

All bridgeservice REST API endpoints are implemented:

  1. GET / - Health check
  2. GET /bridge/v1/bridges - Bridge events (with pagination & filters)
  3. GET /bridge/v1/claims - Claims (with pagination & filters)
  4. GET /bridge/v1/unset-claims - Unset claims (L2 only)
  5. GET /bridge/v1/set-claims - Set claims (L2 only)
  6. GET /bridge/v1/token-mappings - Token mappings
  7. GET /bridge/v1/legacy-token-migrations - Legacy token migrations
  8. GET /bridge/v1/l1-info-tree-index - L1 info tree index lookup
  9. GET /bridge/v1/injected-l1-info-leaf - Injected L1 info leaf
  10. GET /bridge/v1/claim-proof - Merkle proofs for claims
  11. GET /bridge/v1/last-reorg-event - Last reorg event
  12. GET /bridge/v1/sync-status - Synchronization status
  13. GET /bridge/v1/removed-gers - Removed GER events

@claude
Copy link

claude bot commented Jan 28, 2026

Claude finished @arnaubennassar's task in 1m 39s —— View job


Code Review Complete ✅

Todo List

  • Read all changed files
  • Review code structure and organization
  • Check for bugs and edge cases
  • Evaluate security implications
  • Assess performance considerations
  • Verify Go best practices and project conventions
  • Provide comprehensive feedback

🚀 What's New

This PR introduces a comprehensive Go client library for the Bridge Service REST API, implementing all 13 endpoints with type-safe parameter structs, comprehensive error handling, and excellent test coverage (91.9%). The client includes context support for timeouts/cancellation, sensible defaults (30s timeout), and thorough documentation with examples.

🐛 Bug Fixes

None - This is a new feature addition.

📋 Config Updates

None - The client uses a simple Config struct with BaseURL (required) and Timeout (optional, defaults to 30 seconds).

⚠️ Breaking Changes

None - This is a net new package with no impact on existing code.


Review Findings

🟢 Strengths

  1. Excellent Test Coverage: 91.9% coverage with comprehensive test cases covering success paths, error conditions, parameter variations, context cancellation, and edge cases.

  2. Well-Structured API: Clean parameter structs (e.g., GetBridgesParams) provide type-safe, self-documenting interfaces. Use of pointers for optional parameters correctly distinguishes "not provided" from zero values.

  3. Good Error Handling: Error messages include HTTP status codes and response bodies, making debugging easier. Context support throughout for cancellation and timeouts.

  4. Documentation: README is comprehensive with clear examples. Example tests provide runnable documentation.

  5. Code Organization: Follows Go conventions with proper package structure and naming.

🟡 Suggested Improvements

1. Inconsistent Type Usage for Integer Parameters (client.go:66-98)

The parameter structs use inconsistent types for similar fields:

  • GetBridgesParams.NetworkID is uint32 (line 47)
  • GetBridgesParams.PageNumber is *uint32 (line 48)
  • GetUnsetClaimsParams.PageNumber is *int (line 67)
  • GetTokenMappingsParams.NetworkID is int (line 81)
  • GetL1InfoTreeIndex uses int for networkID and depositCount (line 257)

Recommendation: Standardize on uint32 for network IDs and unsigned types for counts/pagination to match the API types and prevent negative values. Consider:

// Consistent approach
PageNumber  *uint32
PageSize    *uint32
NetworkID   uint32  // or int if the API truly accepts signed

Fix type inconsistencies →

2. Missing Input Validation (client.go:101-333)

The client methods don't validate required parameters or reasonable bounds. For example:

  • No validation that NetworkID is non-zero where required
  • No validation of pagination parameters (page size limits, negative values)
  • URL is built without escaping user-provided strings like addresses

Example issue in GetBridges (line 124):

if params.FromAddress != nil {
    query.Set("from_address", *params.FromAddress)  // No validation/sanitization
}

Recommendation: Add validation for:

  • Required fields (networkID should likely be > 0)
  • Reasonable bounds for pagination (e.g., page size > 0 and < 10000)
  • Ethereum address format validation for address parameters
  • Consider URL escaping/validation for string parameters

Add input validation →

3. Potential Response Body Leak in doRequest (client.go:352)

The defer resp.Body.Close() happens after io.ReadAll, but if ReadAll fails, the response body might not be fully consumed. While Close() will be called, it's better practice to ensure the body is drained on non-200 responses.

Current code (line 352-360):

defer resp.Body.Close()

respBody, err := io.ReadAll(resp.Body)
if err != nil {
    return fmt.Errorf("read response body: %w", err)
}

if resp.StatusCode != http.StatusOK {
    return fmt.Errorf("unexpected status code %d: %s", resp.StatusCode, string(respBody))
}

Recommendation: Consider using io.LimitReader to prevent unbounded memory usage on large error responses:

const maxErrorBodySize = 10 * 1024 // 10KB
respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize))

4. Missing User-Agent Header (client.go:344-345)

The client doesn't set a User-Agent header, which is helpful for server-side debugging and analytics.

Recommendation: Add a User-Agent header with version information:

req.Header.Set("User-Agent", "aggkit-bridge-client/v1.0.0")

5. No Retry Logic

The client doesn't implement retry logic for transient failures (network errors, 5xx responses). While this keeps the implementation simple, it means consumers must implement their own retry logic.

Recommendation: Consider adding optional retry configuration in the future, or document that consumers should implement their own retry logic for production use.

6. Test Improvements (client_test.go)

  • Tests use require correctly (good adherence to CLAUDE.md!)
  • However, some tests don't verify all query parameters are correctly formatted
  • Consider adding tests for concurrent requests to verify thread-safety

Example: GetBridges test doesn't verify NetworkIDs array serialization (lines 126-136):

// Test only checks minimal params, not NetworkIDs serialization

🔴 Critical Issues

None found - No security vulnerabilities, race conditions, or critical bugs detected.

Additional Observations

  1. Response Body Handling: The client reads the entire response body into memory. For large result sets, this could be problematic, but it's acceptable for a REST client with pagination support.

  2. HTTP Client Reuse: Good - the client reuses the same http.Client instance, which properly reuses connections.

  3. Context Propagation: Excellent - all methods properly accept and propagate context for cancellation and timeouts.

  4. Error Wrapping: Good use of fmt.Errorf with %w for error wrapping throughout.

  5. Documentation Comments: All exported functions have proper doc comments starting with the function name (per CLAUDE.md).

  6. Import Organization: Follows gci convention with stdlib, external, then internal imports properly grouped.

Security Assessment

No injection vulnerabilities - Uses query parameter encoding correctly
No hardcoded credentials
Proper error handling - Doesn't leak sensitive info in errors
⚠️ Input validation missing - Should validate addresses and bounds
Context support - Prevents hanging requests

Performance Assessment

HTTP client pooling - Reuses connections via shared http.Client
Configurable timeout - Prevents hanging requests
Efficient JSON encoding - Uses standard library directly
⚠️ No response streaming - Reads full body into memory (acceptable for this use case)
No unnecessary allocations - Efficient string building with url.Values


Summary

This is a well-implemented, production-ready client with excellent test coverage and documentation. The suggested improvements are primarily about defensive programming (input validation, type consistency) rather than functional correctness. The code follows Go best practices and project conventions from CLAUDE.md.

Recommendation: ✅ APPROVE with suggested improvements to be addressed in future iterations or before merge (particularly type consistency and input validation).

Great work on the comprehensive implementation! 🎉

arnaubennassar and others added 8 commits January 29, 2026 20:09
Fixes CI build error where go.mod requires Go >= 1.25.6 but Dockerfile was using 1.25.5.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@arnaubennassar arnaubennassar force-pushed the feat/bridge-service-client branch from 695ae1f to a03ed09 Compare January 29, 2026 19:10
@sonarqubecloud
Copy link

@arnaubennassar arnaubennassar merged commit 65f2332 into develop Jan 30, 2026
22 checks passed
@arnaubennassar arnaubennassar deleted the feat/bridge-service-client branch January 30, 2026 10:20
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.

2 participants