Skip to content

Conversation

neithanmo
Copy link
Collaborator

@neithanmo neithanmo commented Aug 22, 2025

Summary

This PR implements comprehensive support for Horizon (V2) TAP receipts by adding dual domain separator configuration and proper V2 verifier address handling across all services. This completes the foundation for full Horizon migration support in the indexer-rs ecosystem.

Changes Made

Configuration Updates

  • Added receipts_verifier_address_v2 field to BlockchainConfig for separate V2 contract addresses
  • Updated configuration examples (minimal and maximal) with documentation for the new V2 verifier field
  • Enhanced contrib scripts to extract and use both V1 (TAPVerifier) and V2 (GraphTallyCollector) addresses from horizon.json

Service Updates

  • Updated indexer-service to use correct V2 verifier address for domain separator creation
  • Updated tap-agent to use correct V2 verifier address for global domain separators
  • Added invariant validation: When horizon.enabled = true, receipts_verifier_address_v2 must be explicitly configured (fails fast with clear error)
  • Graceful fallback: When horizon.enabled = false, V2 operations fall back to V1 verifier address if V2 isn't specified

Infrastructure Updates

  • Updated integration test setup to support direct RAV testing workflows

Key Behavioral Changes

Horizon Configuration Invariants

  1. When horizon.enabled = true: Both services require explicit V2 verifier configuration and will fail fast with:

    receipts_verifier_address_v2 is required when Horizon is enabled
    
  2. When horizon.enabled = false: Both services gracefully fall back to V1 addresses for V2 domains

Domain Separator Logic

  • V1 domains: Always use receipts_verifier_address
  • V2 domains:
    • If Horizon enabled: Use receipts_verifier_address_v2 (required)
    • If Horizon disabled: Use receipts_verifier_address_v2 or fallback to receipts_verifier_address

Configuration Example

[blockchain]
chain_id = 1337
# V1 TAP verifier contract address
receipts_verifier_address = "0x1111111111111111111111111111111111111111"
# V2 (Horizon) verifier contract address - required when horizon.enabled = true
receipts_verifier_address_v2 = "0x2222222222222222222222222222222222222222"
subgraph_service_address = "0x3333222222222222233332222200002221122323"


[horizon]
# Enable Horizon migration support
enabled = true

Impact

This change resolves the "No sender found for signer" errors that occurred when V2 receipts were validated against incorrect verifier contracts. The dual domain approach ensures:

  • Correct cryptographic validation for both V1 and V2 receipt formats
  • Smooth migration path from legacy V1 to Horizon V2
  • Clear error reporting when configuration is incomplete
  • Backward compatibility for existing V1-only deployments

Testing

  • Configuration validation works correctly
  • Services fail fast when Horizon is enabled without V2 verifier
  • Services fall back gracefully when Horizon is disabled
  • Integration tests pass with dual domain support

@neithanmo neithanmo marked this pull request as draft August 22, 2025 14:40
Copy link
Contributor

github-actions bot commented Aug 22, 2025

Pull Request Test Coverage Report for Build 18019626405

Details

  • 755 of 3370 (22.4%) changed or added relevant lines in 50 files are covered.
  • 76 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-8.3%) to 67.651%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/service/src/metrics.rs 0 1 0.0%
crates/service/src/middleware/sender.rs 2 3 66.67%
crates/service/src/middleware/tap_receipt.rs 5 6 83.33%
crates/service/src/tap/checks/deny_list_check.rs 0 1 0.0%
crates/allocation/src/lib.rs 0 2 0.0%
crates/service/src/main.rs 0 2 0.0%
crates/service/src/routes/static_subgraph.rs 0 2 0.0%
crates/tap-agent/src/metrics.rs 0 2 0.0%
crates/service/src/tap/checks/value_check.rs 0 3 0.0%
crates/tap-agent/src/tap/context/rav.rs 5 8 62.5%
Files with Coverage Reduction New Missed Lines %
crates/monitor/src/allocations.rs 1 0.0%
crates/tap-agent/src/agent/sender_allocation.rs 1 81.54%
integration-tests/src/signature_test.rs 1 0.0%
crates/service/src/service.rs 2 0.0%
integration-tests/src/utils.rs 2 0.0%
crates/tap-agent/src/agent/sender_account.rs 3 91.8%
crates/tap-agent/src/agent.rs 4 0.0%
integration-tests/src/rav_tests.rs 7 0.0%
crates/tap-agent/src/agent/sender_accounts_manager.rs 55 82.05%
Totals Coverage Status
Change from base Build 17920170048: -8.3%
Covered Lines: 13012
Relevant Lines: 19234

💛 - Coveralls

@neithanmo neithanmo force-pushed the feat/horizon branch 2 times, most recently from e186490 to 3928a0b Compare September 4, 2025 14:08
Copy link
Collaborator

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

Wow @neithanmo! Awesome work figuring all this out and capturing all your findings in so many useful additions! I'm going to keep digesting your findings and will try out running the tests locally 🫡

@neithanmo neithanmo marked this pull request as ready for review September 16, 2025 21:21
suchapalaver
suchapalaver previously approved these changes Sep 16, 2025
Copy link
Collaborator

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

Great work @neithanmo 🙌

suchapalaver
suchapalaver previously approved these changes Sep 17, 2025
suchapalaver
suchapalaver previously approved these changes Sep 17, 2025
suchapalaver
suchapalaver previously approved these changes Sep 18, 2025
Copy link
Collaborator

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

Has been tested locally using commit https://github.com/semiotic-ai/local-network/commit/b18f40147a01aa050c1172a7f524b8443a7a3d32 running just setup then just test-local-v2!

Amazing work @neithanmo 🫡

Adds indexer-cli Docker container built from graphprotocol/indexer horizon branch
to enable allocation management via indexer-agent API for integration testing.

- Builds from horizon branch with ESM compatibility fix
- Auto-connects to indexer-agent on startup
- Integrated into docker-compose and setup-test-network.sh
- Enables allocation operations: list, close with POI, etc.
  - Replace verbose debug logs with structured fields
  - Use consistent sender_type_str formatting (V1/V2)
  - Simplify error messages while preserving key information
  - Remove excessive escrow account mapping dumps
  - Consolidate allocation address handling logic***
  - Replace hardcoded values with imports from constants module
  - Use SUBGRAPH_ID constant to match local-network/.env SUBGRAPH value
  - Consolidate import statements for better organization
  - Remove duplicate env var lookup in test_subgraph_deployment
  - Maintain env variable override functionality while standardizing defaults
  Add minimal Docker-based wrapper around indexer-cli service to enable
  programmatic allocation operations in integration tests.

  Features:
  - List active allocations via graph indexer allocations get
  - Close allocations with zero POI for testing
  - Configurable container name via environment variables
  - Ethereum address parsing with deduplication

  Note: Integration with test suite pending further investigation.
  - Replace MetricsChecker with DatabaseChecker for more reliable state verification
  - Add comprehensive V1 TAP test implementation with database validation
  - Enable early exit on successful RAV creation in V2 tests
  - Add detailed progress tracking with trigger threshold calculations
  - Implement delayed RAV creation detection with timeout handling
  - Remove commented-out legacy metrics code and unused imports
  - Disable invalid_chain_id test temporarily
  - Consolidate import statements across test modules

  The database checker provides direct access to TAP state rather than relying
  on metrics parsing, which are less reliable.
  - Add V1 TAP escrow signer authorization with proof generation
  - Add V2 PaymentsEscrow signer authorization
  - Use separate gateway signer accounts (ACCOUNT1) for authorization
  - Add verification checks for both V1 and V2 signer authorization
  - Improve logging with clearer role descriptions and error handling
…renames

•  Replace string-interpolated logs with structured fields (tracing) for machine-parsable, consistent logs and lower formatting overhead.
•  main, metrics, routes, service: convert logs to structured fields (error, deployment, addr, config_path, etc.); fix typo “occoured” -> “occurred”.
•  tap-agent:
•  sender_account, sender_accounts_manager, sender_allocation, metrics, tracker/, tap/: widespread conversion to structured logs; clarify messages and add useful fields (allocation_id, sender, fees, counts, channel, code, etc.).
•  Derive Debug for DenyListVersion to improve log output.
•  No functional changes intended; only logging structure, readability, and typo fixes.
test: remove commented code; fix typos

build: move deps to workspace
test: use our custom local-network

    to use subgraph-deploy patch which consist on simple
    loop to wait for TAP subgraph to be ready

test: fix gateway path
…ith isLegacy + sender_type

  - Remove global allocation typing in Manager; keep a single raw watcher
  - Add per-sender normalization using Network Subgraph Allocation.isLegacy + escrow-derived sender_type
  - Pass typed Receiver<HashSet<AllocationId>> to SenderAccount; remove in-actor retyping
  - Make DatabaseChecker hold TestConfig
  - improve receipts and rav checks using tap-agent config
  - V1 queries: fix table/column usage to scalar_tap_receipts/scalar_tap_ravs and signer_address
      - has_rav_for_identifier (V1)
      - get_pending_receipt_value (V1)
  - V2 pending value: compute receipts newer than last RAV and older than buffer cutoff (remove rav IS NULL join)
This script leverages local-network setup
by adding some extra overrides to compile from source
Copy link
Collaborator

@suchapalaver suchapalaver left a comment

Choose a reason for hiding this comment

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

Let's fix the formatting and get this merged!

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.

3 participants