Skip to content

feat: support per-protocol encoding in reqresp v1#68

Merged
samcm merged 5 commits intofeat/unoppinionated-reqrespfrom
feat/per-protocol-encoding
Jul 4, 2025
Merged

feat: support per-protocol encoding in reqresp v1#68
samcm merged 5 commits intofeat/unoppinionated-reqrespfrom
feat/per-protocol-encoding

Conversation

@samcm
Copy link
Copy Markdown
Member

@samcm samcm commented Jul 4, 2025

Summary

This PR improves the reqresp v1 package to support per-protocol encoding/compression instead of global configuration. This provides much better flexibility and matches real-world usage patterns.

Problem

The original design forced all protocols to use the same encoder/compressor configured at the service level. This is too restrictive because:

  • Consensus protocols typically use SSZ encoding
  • Metadata protocols might use JSON
  • Some protocols need compression (Snappy), others don't
  • Different protocols have different performance/size tradeoffs

Solution

  • Remove global encoder/compressor from ServiceConfig
  • Add encoder/compressor to HandlerOptions for per-protocol configuration
  • Add SendRequestWithOptions to allow per-request encoding on client side
  • Update examples to demonstrate protocol-specific encoding

Key Changes

  1. Handler Registration: Now requires HandlerOptions with encoder
opts := v1.HandlerOptions{
    Encoder:    JSONEncoder{},     // or SSZEncoder{} 
    Compressor: SnappyCompressor{}, // or nil
    RequestTimeout: 30 * time.Second,
}
v1.RegisterProtocol(service, protocol, handler, opts)
  1. Client Requests: Use SendRequestWithOptions for encoding
reqOpts := v1.RequestOptions{
    Encoder:    JSONEncoder{},  // Must match server
    Compressor: nil,
    Timeout:    5 * time.Second,
}
service.SendRequestWithOptions(ctx, peer, proto, req, &resp, reqOpts)
  1. Improved Examples:
    • Middleware example now shows actual handler wrapping
    • Comments indicate where different encoders would be used

Breaking Changes

  • RegisterProtocol and RegisterChunkedProtocol now require HandlerOptions parameter
  • ClientConfig no longer has Encoder/Compressor fields
  • Service constructor no longer validates encoder presence

Test Plan

  • Package compiles successfully
  • Examples compile and demonstrate usage
  • Unit tests to be added (tracked separately)

🤖 Generated with Claude Code

- Remove global encoder/compressor from service config
- Add HandlerOptions parameter to RegisterProtocol functions
- Add SendRequestWithOptions for per-request encoding
- Update examples to show protocol-specific encoding
- Improve middleware example to demonstrate actual usage

This allows different protocols to use different encoding strategies:
- JSON for metadata protocols
- SSZ for consensus data
- Different compression per protocol

Breaking changes:
- RegisterProtocol now requires HandlerOptions parameter
- ClientConfig no longer has Encoder/Compressor fields

🤖 Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
@samcm samcm requested a review from mattevans as a code owner July 4, 2025 07:44
Copy link
Copy Markdown
Member

@mattevans mattevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 🎖️

samcm and others added 4 commits July 4, 2025 18:00
- Add tests for client.go covering SendRequest, retry logic, timeouts, and chunked requests
- Add tests for handler.go covering request handling, validation, and error responses
- Add tests for chunked_handler.go covering chunked responses and ChunkedResponseWriter
- Add tests for reqresp.go covering service lifecycle and concurrent operations
- Add tests for types.go covering Status, error constants, and configurations
- Create comprehensive mock implementations for host.Host, network.Stream, and other interfaces
- Achieve 72.6% test coverage, exceeding the >70% target

Test coverage breakdown:
- client.go: High coverage for core functionality
- handler.go: Excellent coverage (94.7% for HandleStream)
- chunked_handler.go: Excellent coverage (95% for HandleStream)
- types.go: 100% coverage for key functions
- reqresp.go: Good coverage for service operations
- Add unit tests for all major components
- Add mock implementations for libp2p interfaces
- Add panic recovery to handlers for robustness
- Add proper timeout handling with context
- Fix empty request validation
- Fix integration test race conditions
- All tests now pass successfully

Coverage highlights:
- Total package: 77.9%
- Handler.HandleStream: 94.7%
- ChunkedHandler.HandleStream: 95.0%
- Client.SendRequestWithOptions: 95.5%

🤖 Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
- Add blank line before return statements (nlreturn)
- Add blank line before if statements after assignments (wsl)
- All linting issues now resolved

🤖 Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
@samcm samcm merged commit 4df4145 into feat/unoppinionated-reqresp Jul 4, 2025
1 of 2 checks passed
@samcm samcm deleted the feat/per-protocol-encoding branch July 4, 2025 09: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