Skip to content

Conversation

@fbac
Copy link
Collaborator

@fbac fbac commented May 26, 2025

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added support for monitoring and handling events from blockchain-based Payer Registry and Payer Report Manager contracts, including deposit, withdrawal, usage settlement, and report submission events.
    • Introduced new configuration options for specifying payer registry and payer report manager addresses, along with a maximum chain disconnect time.
    • Integrated lifecycle management for the settlement chain component, ensuring proper initialization, startup, and shutdown.
  • Bug Fixes

    • Improved validation for configuration fields, ensuring correct address formats and positive disconnect durations.
  • Tests

    • Added comprehensive tests for event handling and error scenarios related to Payer Registry and Payer Report Manager contracts.

@fbac fbac requested a review from a team as a code owner May 26, 2025 10:43
@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 26, 2025

Warning

Rate limit exceeded

@fbac has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 0 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 4a5ec24 and aae0fe7.

📒 Files selected for processing (5)
  • pkg/config/options.go (1 hunks)
  • pkg/indexer/app_chain/app_chain.go (3 hunks)
  • pkg/indexer/indexer.go (4 hunks)
  • pkg/indexer/settlement_chain/settlement_chain.go (1 hunks)
  • pkg/testutils/config.go (2 hunks)

Walkthrough

This change extends the settlement chain configuration with payer-related contract addresses and a disconnect timeout, adds validation for these fields, and integrates a new SettlementChain component into the indexer. It implements PayerRegistry and PayerReportManager contract interfaces with event streaming, log handling, and storage, including comprehensive unit tests.

Changes

File(s) Change Summary
pkg/config/options.go, pkg/config/validation.go, pkg/testutils/config.go Added PayerRegistryAddress, PayerReportManagerAddress, and MaxChainDisconnectTime to settlement chain config and validation; updated test config constants and defaults.
pkg/indexer/indexer.go Integrated SettlementChain lifecycle management into Indexer: initialization, start, and stop.
pkg/indexer/settlement_chain/settlement_chain.go Added SettlementChain struct managing Ethereum client, contracts, log streamer, event and reorg channels, and lifecycle methods.
pkg/indexer/settlement_chain/contracts/payer_registry.go Implemented PayerRegistry contract interface with event topics, contract binding, block tracking, and reorg handling.
pkg/indexer/settlement_chain/contracts/payer_registry_storer.go Added PayerRegistryStorer implementing log storage and event parsing with error handling for payer registry events.
pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go Added unit tests for PayerRegistryStorer covering event handling, error cases, and log parsing.
pkg/indexer/settlement_chain/contracts/payer_report_manager.go Implemented PayerReportManager contract interface with event topics, contract binding, block tracking, and reorg handling.
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go Added PayerReportManagerStorer implementing log storage and event parsing with error handling for payer report manager events.
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer_test.go Added unit tests for PayerReportManagerStorer covering event handling, error cases, and log parsing.
pkg/blockchain/rpcLogStreamer.go, pkg/blockchain/rpcLogStreamer_test.go Removed chainID parameter from NewRpcLogStreamer and updated logger initialization and test calls accordingly.
pkg/indexer/app_chain/app_chain.go Adjusted logger usage in NewRpcLogStreamer call; added Ethereum client close on stop; removed commented logs.

Sequence Diagram(s)

sequenceDiagram
    participant Indexer
    participant SettlementChain
    participant LogStreamer
    participant PayerRegistry
    participant PayerReportManager
    participant DB

    Indexer->>SettlementChain: NewSettlementChain(cfg, db, ...)
    SettlementChain->>PayerRegistry: Initialize contract binding
    SettlementChain->>PayerReportManager: Initialize contract binding
    SettlementChain->>LogStreamer: Configure with contract topics and addresses
    Indexer->>SettlementChain: Start()
    SettlementChain->>LogStreamer: Start streaming logs
    LogStreamer-->>SettlementChain: Emit contract events
    SettlementChain->>PayerRegistry: Dispatch events to storer
    SettlementChain->>PayerReportManager: Dispatch events to storer
    PayerRegistry->>DB: Store event data
    PayerReportManager->>DB: Store event data
    Indexer->>SettlementChain: Stop()
    SettlementChain->>LogStreamer: Stop streaming
    SettlementChain->>Ethereum Client: Close client and cancel context
Loading

Possibly related PRs

  • xmtp/xmtpd#852: Adds settlement chain configuration and payer report manager contract implementation, closely related to this PR’s settlement chain features.
  • xmtp/xmtpd#800: Defines common indexer interfaces and modular multi-chain architecture extended by this PR’s settlement chain integration.

Suggested reviewers

  • mkysel
✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@graphite-app
Copy link

graphite-app bot commented May 26, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • Queue - adds this PR to the back of the merge queue
  • Hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@macroscopeapp
Copy link

macroscopeapp bot commented May 26, 2025

Add settlement chain indexing with PayerRegistry and PayerReportManager contract support to the indexer

The indexer now supports settlement chain functionality alongside the existing app chain indexing. Key changes include:

• Add new SettlementChain component in settlement_chain.go that manages PayerRegistry and PayerReportManager contracts
• Implement PayerRegistry contract interface in payer_registry.go to handle deposit, withdrawal, and usage settlement events
• Implement PayerReportManager contract interface in payer_report_manager.go to handle report submission and settlement events
• Update Indexer struct in indexer.go to initialize and manage both app chain and settlement chain components
• Modify configuration structure in options.go to move BackfillBlockSize from general contracts to chain-specific options and add settlement chain fields including PayerRegistryAddress, PayerReportManagerAddress, and MaxChainDisconnectTime
• Remove chainID parameter from NewRpcLogStreamer function in rpcLogStreamer.go

📍Where to Start

Start with the NewIndexer function in indexer.go to see how the settlement chain component is initialized alongside the app chain.


Macroscope summarized aae0fe7.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (2)
pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go (1)

140-147: Address the TODO comment for event-specific log generation.

The TODO indicates this is a placeholder implementation. Consider implementing event-specific log generation methods for more realistic testing.

Would you like me to help implement the event-specific log generation methods (newDepositLog, newWithdrawalRequestedLog, etc.) mentioned in the TODO?

pkg/indexer/settlement_chain/settlement_chain.go (1)

20-23: Track the TODO for RPC log streamer changes.

The TODO comment indicates this value may need modification after changes to the RPC log streamer.

Would you like me to create an issue to track this TODO?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72f16dd and 1cd8b4a.

📒 Files selected for processing (8)
  • pkg/config/options.go (1 hunks)
  • pkg/config/validation.go (1 hunks)
  • pkg/indexer/indexer.go (4 hunks)
  • pkg/indexer/settlement_chain/contracts/payer_registry.go (1 hunks)
  • pkg/indexer/settlement_chain/contracts/payer_registry_storer.go (1 hunks)
  • pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go (1 hunks)
  • pkg/indexer/settlement_chain/settlement_chain.go (1 hunks)
  • pkg/testutils/config.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint-Go
pkg/indexer/settlement_chain/contracts/payer_registry.go

[failure] 20-20:
File is not properly formatted (gofumpt)

🪛 GitHub Actions: Lint
pkg/indexer/settlement_chain/contracts/payer_registry.go

[error] 20-20: File is not properly formatted (gofumpt)

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
  • GitHub Check: Code Review
  • GitHub Check: Test (Node)
  • GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
  • GitHub Check: Upgrade Tests
🔇 Additional comments (15)
pkg/config/options.go (1)

52-53: LGTM! Configuration fields are well-structured.

The new fields follow the existing patterns consistently:

  • Environment variable naming convention is maintained
  • Default value for MaxChainDisconnectTime matches the pattern used in AppChainOptions
  • Field descriptions are clear and concise
pkg/testutils/config.go (1)

22-22: Test configuration properly implemented.

The test utilities correctly incorporate the new configuration fields:

  • PAYER_REGISTRY_ADDRESS follows the existing constant naming pattern
  • The 10-second MaxChainDisconnectTime is appropriate for test environments (faster than the 300s production default)

Also applies to: 42-43

pkg/config/validation.go (1)

168-177: Validation logic correctly implemented.

The new validations are consistent with the existing pattern:

  • PayerRegistryAddress is validated as a hex address, ensuring it's non-empty and valid
  • MaxChainDisconnectTime is validated to be positive, preventing configuration errors
pkg/indexer/settlement_chain/contracts/payer_registry_storer.go (4)

1-15: LGTM!

Imports are well-organized with meaningful aliases.


17-26: LGTM!

Constants are well-defined with clear documentation about the unhandled WithdrawalFinalized event.


28-35: LGTM!

Type definition is clean with proper interface compliance check.


36-51: LGTM!

Constructor is well-structured with proper error handling and logger naming.

pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go (2)

1-25: LGTM!

Test setup with appropriate imports and well-structured test helper struct.


27-73: LGTM!

Comprehensive error case testing with proper error type assertions.

pkg/indexer/settlement_chain/settlement_chain.go (3)

25-34: LGTM!

Well-structured type definition with all necessary components for chain management.


107-136: LGTM!

Well-implemented lifecycle methods with proper panic recovery and resource cleanup.


138-144: LGTM!

Clean accessor methods following good encapsulation practices.

pkg/indexer/indexer.go (1)

10-10: Well-structured integration of the settlement chain!

The implementation follows the existing pattern established by appChain, maintaining consistency in:

  • Field naming and struct organization
  • Initialization with proper error handling and context cancellation
  • Lifecycle management in Close() and StartIndexer() methods
  • Nil-checking before cleanup operations

Also applies to: 21-21, 48-59, 76-79, 88-88

pkg/indexer/settlement_chain/contracts/payer_registry.go (2)

40-85: Robust constructor implementation with comprehensive error handling!

The NewPayerRegistry constructor follows best practices:

  • Proper error propagation at each initialization step
  • Logger namespacing with contract address for better debugging
  • Clean separation of concerns with dedicated components for tracking, storing, and handling reorgs

109-128: Clean utility functions for contract management!

  • PayerRegistryName: Provides chain-specific naming for multi-chain environments
  • payerRegistryTopics: Properly handles ABI parsing with comprehensive error handling

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go (1)

149-157: Placeholder implementation aligns with current scope.

The newLog helper method appropriately serves the current testing needs by creating logs with correct event topics. The TODO comment indicates planned enhancement with event-specific log creators, which will be valuable when database storage is implemented.

Consider adding a timeline or issue reference to the TODO comment to track when the enhanced log creators should be implemented:

-// TODO: Placeholder. This will be replaced with newDepositLog, newWithdrawalRequestedLog, etc.
+// TODO: Placeholder. This will be replaced with newDepositLog, newWithdrawalRequestedLog, etc.
+// when database storage is implemented (see related issue/PR).
pkg/indexer/settlement_chain/contracts/payer_registry_storer.go (1)

52-80: LGTM: Intentional placeholder implementation as documented.

Based on the retrieved learnings, this method intentionally only logs events without database storage as the actual storage implementation is planned for a future PR. The event parsing and logging logic is correct.

Consider adding a TODO comment to make the placeholder nature explicit:

 func (s *PayerRegistryStorer) StoreLog(
 	ctx context.Context,
 	log types.Log,
 ) re.RetryableError {
+	// TODO: Implement actual database storage in future PR
 	if len(log.Topics) == 0 {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 92b7845 and 3ebde16.

📒 Files selected for processing (3)
  • pkg/indexer/settlement_chain/contracts/payer_registry_storer.go (1 hunks)
  • pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go (1 hunks)
  • pkg/indexer/settlement_chain/settlement_chain.go (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#847
File: pkg/indexer/settlement_chain/contracts/payer_registry_storer.go:67-81
Timestamp: 2025-05-26T10:52:06.707Z
Learning: In pkg/indexer/settlement_chain/contracts/payer_registry_storer.go, the StoreLog method currently only logs events without storing them in the database. The actual database storage implementation is planned for a future PR, so this is intentionally incomplete as a placeholder.
pkg/indexer/settlement_chain/contracts/payer_registry_storer.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#847
File: pkg/indexer/settlement_chain/contracts/payer_registry_storer.go:67-81
Timestamp: 2025-05-26T10:52:06.707Z
Learning: In pkg/indexer/settlement_chain/contracts/payer_registry_storer.go, the StoreLog method currently only logs events without storing them in the database. The actual database storage implementation is planned for a future PR, so this is intentionally incomplete as a placeholder.
pkg/indexer/settlement_chain/settlement_chain.go (2)
Learnt from: fbac
PR: xmtp/xmtpd#847
File: pkg/indexer/settlement_chain/settlement_chain.go:71-71
Timestamp: 2025-05-26T11:00:12.930Z
Learning: The `GetLatestBlock()` method in `PayerRegistry` (and likely other similar components) returns block information without an error return, so using the blank identifier (_) to ignore the second return value is the correct approach.
Learnt from: fbac
PR: xmtp/xmtpd#800
File: pkg/indexer/app_chain/app_chain.go:70-71
Timestamp: 2025-05-19T09:19:00.749Z
Learning: The `GetLatestBlock()` method in `BlockTracker` returns `(uint64, []byte)` representing the latest block number and hash, with no error return. It defaults to returning 0 for the block number when no block has been stored yet, as it maintains values in an in-memory cache.
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
  • GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
  • GitHub Check: Test (Node)
  • GitHub Check: Upgrade Tests
  • GitHub Check: Code Review
🔇 Additional comments (20)
pkg/indexer/settlement_chain/contracts/payer_registry_storer_test.go (6)

1-20: LGTM! Imports are well-organized and comprehensive.

The package declaration and imports are appropriate for testing blockchain event handling functionality.


22-25: Good test helper design.

The payerRegistryStorerTester struct appropriately encapsulates test dependencies for clean test organization.


27-73: Excellent error handling test coverage.

The error tests comprehensively cover important edge cases:

  • Missing topics in logs
  • Unknown event signatures
  • Unhandled event types

The error validation using ErrorAs ensures proper error type checking.


75-113: Complete test coverage for supported events.

All supported events are properly tested, and I can see the WithdrawalCancelled test has been added since the previous review. The minimal test approach is appropriate given the current placeholder implementation.


115-147: Robust test setup with proper resource management.

The buildPayerRegistryStorerTester function properly initializes all dependencies with appropriate error handling and cleanup. The use of testutils ensures consistency across the test suite.


1-157: Well-structured test suite for current implementation scope.

This test file provides excellent coverage for the PayerRegistryStorer component, appropriately balancing comprehensive error testing with minimal success validation. The implementation aligns well with the learning that this is currently a placeholder that logs events without database storage.

The test structure demonstrates good practices:

  • Comprehensive error case coverage
  • Complete event type testing
  • Robust test setup with proper cleanup
  • Clear separation of concerns with helper methods
pkg/indexer/settlement_chain/contracts/payer_registry_storer.go (4)

1-14: LGTM: Clean package structure and imports.

The package declaration and imports are well-organized and include all necessary dependencies for the PayerRegistry event handling functionality.


16-25: LGTM: Well-defined constants with clear intent.

The constants are appropriately named and the comment about WithdrawalFinalized being potentially redundant provides useful context for future development.


27-33: LGTM: Proper struct design with interface compliance.

The struct fields are appropriately typed and the interface compliance check is a good practice that ensures compile-time verification.


35-50: LGTM: Robust constructor with proper error handling.

The constructor properly initializes the ABI, handles errors appropriately, and uses named loggers for better debugging capabilities.

pkg/indexer/settlement_chain/settlement_chain.go (10)

1-23: LGTM: Well-structured package with appropriate constants.

The imports are comprehensive and the TODO comment about modifying lagFromHighestBlock provides useful context for future improvements.


25-34: LGTM: Comprehensive struct design for settlement chain management.

The struct includes all necessary components for managing the settlement chain lifecycle, including proper context management and synchronization primitives.


36-52: LGTM: Proper initialization with context management.

The constructor correctly sets up context cancellation and logger configuration. The error handling after NewClient is appropriate - no client.Close() call is needed since client would be nil on failure.


54-68: LGTM: Appropriate resource management in error handling.

The client.Close() call in this error block is safe because it's only reached if the NewClient call succeeded, ensuring client is not nil.


70-70: LGTM: Correct usage of GetLatestBlock without error handling.

Based on retrieved learnings, GetLatestBlock() doesn't return an error, so using the blank identifier to ignore the second return value is the correct approach.


72-93: LGTM: Proper streamer initialization with resource cleanup.

The RpcLogStreamer is configured with appropriate contract parameters. The client.Close() call in the error block is safe because client is guaranteed to be valid at this point in the execution flow.


95-104: LGTM: Clean struct initialization and return.

All struct fields are properly initialized with the created components and configuration values.


106-122: LGTM: Robust start method with panic recovery.

The Start method properly initiates the streamer and uses tracing.GoPanicWrap for safe goroutine execution with panic recovery.


124-135: LGTM: Comprehensive shutdown procedure.

The Stop method follows proper shutdown patterns: stopping the streamer, canceling context, waiting for goroutines, and providing debug logging.


137-143: LGTM: Clean accessor methods for event channels.

The accessor methods provide proper encapsulation for accessing event and reorganization channels using chain-specific identifiers.

Closes #851 

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Added support for tracking and handling events from the
"PayerReportManager" contract on the settlement chain.
- Introduced new configuration options for specifying payer registry and
payer report manager contract addresses, as well as a maximum allowed
disconnect time.
- **Bug Fixes**
- Improved validation for new configuration fields to ensure correct
contract addresses and disconnect time settings.
- **Tests**
- Added comprehensive tests for event handling and error scenarios
related to the PayerReportManager contract.
- **Chores**
- Updated test utilities to include new contract addresses and
configuration options.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (1)

49-76: Acknowledge placeholder implementation with improvement suggestions.

Based on retrieved learnings, this logging-only approach is intentional as actual database storage is planned for a future PR. The current implementation properly handles error cases and event identification.

Consider these improvements for enhanced observability:

 	switch event.Name {
 	case payerReportManagerPayerReportSubmittedEvent:
-		s.logger.Info("PayerReportSubmitted", zap.Any("log", log))
+		s.logger.Info("PayerReportSubmitted", 
+			zap.String("txHash", log.TxHash.Hex()),
+			zap.Uint64("blockNumber", log.BlockNumber),
+			zap.Any("log", log))
 	case payerReportManagerPayerReportSubsetSettledEvent:
-		s.logger.Info("PayerReportSubsetSettled", zap.Any("log", log))
+		s.logger.Info("PayerReportSubsetSettled",
+			zap.String("txHash", log.TxHash.Hex()),
+			zap.Uint64("blockNumber", log.BlockNumber),
+			zap.Any("log", log))
 	default:
-		s.logger.Info("Unknown event", zap.String("event", event.Name))
+		s.logger.Warn("Unknown event", 
+			zap.String("event", event.Name),
+			zap.String("txHash", log.TxHash.Hex()))
 		return re.NewNonRecoverableError(
 			ErrPayerReportManagerUnhandledEvent,
-			errors.New(event.Name),
+			errors.New("unknown event: " + event.Name),
 		)
pkg/indexer/settlement_chain/settlement_chain.go (1)

20-23: Track the TODO for modifying lagFromHighestBlock.

The TODO comment indicates planned changes to the RPC log streamer configuration. Consider creating an issue to track this work.

Would you like me to create an issue to track the implementation of the RPC log streamer changes?

pkg/indexer/settlement_chain/contracts/payer_report_manager.go (3)

9-9: Consider using a more descriptive import alias.

The import alias p for the payerreportmanager package could cause confusion since the struct is also named PayerReportManager. Consider using a more descriptive alias like prmabi or payerReportManagerABI.

-	p "github.com/xmtp/xmtpd/pkg/abi/payerreportmanager"
+	prmabi "github.com/xmtp/xmtpd/pkg/abi/payerreportmanager"

93-101: Update function calls if import alias changes.

If the import alias is updated as suggested above, remember to update the function calls accordingly.

 func payerReportManagerContract(
 	address common.Address,
 	client *ethclient.Client,
-) (*p.PayerReportManager, error) {
-	return p.NewPayerReportManager(
+) (*prmabi.PayerReportManager, error) {
+	return prmabi.NewPayerReportManager(
 		address,
 		client,
 	)
 }

107-122: Update ABI access if import alias changes.

If the import alias is updated as suggested above, remember to update the ABI metadata access.

 func payerReportManagerTopics() ([]common.Hash, error) {
-	abi, err := p.PayerReportManagerMetaData.GetAbi()
+	abi, err := prmabi.PayerReportManagerMetaData.GetAbi()
 	if err != nil {
 		return nil, err
 	}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ebde16 and 4a5ec24.

📒 Files selected for processing (10)
  • pkg/blockchain/rpcLogStreamer.go (1 hunks)
  • pkg/blockchain/rpcLogStreamer_test.go (0 hunks)
  • pkg/config/options.go (1 hunks)
  • pkg/config/validation.go (1 hunks)
  • pkg/indexer/app_chain/app_chain.go (2 hunks)
  • pkg/indexer/settlement_chain/contracts/payer_report_manager.go (1 hunks)
  • pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (1 hunks)
  • pkg/indexer/settlement_chain/contracts/payer_report_manager_storer_test.go (1 hunks)
  • pkg/indexer/settlement_chain/settlement_chain.go (1 hunks)
  • pkg/testutils/config.go (2 hunks)
💤 Files with no reviewable changes (1)
  • pkg/blockchain/rpcLogStreamer_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • pkg/config/validation.go
  • pkg/config/options.go
  • pkg/testutils/config.go
🧰 Additional context used
🧠 Learnings (2)
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (1)
Learnt from: fbac
PR: xmtp/xmtpd#847
File: pkg/indexer/settlement_chain/contracts/payer_registry_storer.go:67-81
Timestamp: 2025-05-26T10:52:06.726Z
Learning: In pkg/indexer/settlement_chain/contracts/payer_registry_storer.go, the StoreLog method currently only logs events without storing them in the database. The actual database storage implementation is planned for a future PR, so this is intentionally incomplete as a placeholder.
pkg/indexer/settlement_chain/settlement_chain.go (3)
Learnt from: fbac
PR: xmtp/xmtpd#847
File: pkg/indexer/settlement_chain/settlement_chain.go:71-71
Timestamp: 2025-05-26T11:00:12.951Z
Learning: The `GetLatestBlock()` method in `PayerRegistry` (and likely other similar components) returns block information without an error return, so using the blank identifier (_) to ignore the second return value is the correct approach.
Learnt from: fbac
PR: xmtp/xmtpd#847
File: pkg/indexer/settlement_chain/settlement_chain.go:64-68
Timestamp: 2025-05-26T11:06:57.065Z
Learning: In the xmtpd codebase, `blockchain.NewClient()` returns a non-nil client object even when an error occurs, making it safe to call `client.Close()` in error handling paths without nil checks.
Learnt from: fbac
PR: xmtp/xmtpd#800
File: pkg/indexer/app_chain/app_chain.go:70-71
Timestamp: 2025-05-19T09:19:00.749Z
Learning: The `GetLatestBlock()` method in `BlockTracker` returns `(uint64, []byte)` representing the latest block number and hash, with no error return. It defaults to returning 0 for the block number when no block has been stored yet, as it maintains values in an in-memory cache.
🔇 Additional comments (12)
pkg/indexer/app_chain/app_chain.go (2)

96-96: LGTM: Improved logger consistency.

Using chainLogger instead of log is more consistent since it includes the chainID context that was added earlier in the function.


175-177: LGTM: Good defensive programming.

Adding a null check before calling client.Close() prevents potential nil pointer dereferences and follows good resource cleanup practices.

pkg/blockchain/rpcLogStreamer.go (1)

107-107: LGTM: Simplified logger initialization.

Removing the chainID parameter simplifies the logger initialization. This aligns with the pattern where chain-specific loggers are created at the caller level (as seen in app_chain.go with chainLogger).

pkg/indexer/settlement_chain/contracts/payer_report_manager_storer_test.go (4)

27-39: LGTM: Comprehensive error handling test.

The test properly verifies that logs without topics return a non-recoverable error with the expected error type and message.


41-57: LGTM: Unknown event handling test.

The test correctly verifies that unknown events are handled with appropriate non-recoverable errors, ensuring the system fails fast for unexpected events.


59-77: LGTM: Success case coverage.

Both recognized events (PayerReportSubmitted and PayerReportSubsetSettled) are properly tested to ensure they process without errors.


79-111: LGTM: Well-structured test setup.

The test setup properly initializes all dependencies including database, blockchain client, and contract instances. The use of a tester struct with helper methods follows good testing patterns.

pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (1)

24-47: LGTM: Clean struct design and constructor.

The struct properly implements the ILogStorer interface with appropriate dependencies. The constructor correctly initializes the ABI and sets up structured logging.

pkg/indexer/settlement_chain/settlement_chain.go (1)

164-179: LGTM! Proper resource cleanup implemented.

The Stop method correctly handles all resource cleanup including closing the Ethereum client, which addresses the previous review feedback about preventing HTTP connection leaks.

pkg/indexer/settlement_chain/contracts/payer_report_manager.go (3)

34-79: LGTM! Well-structured constructor with proper error handling.

The constructor follows established patterns and properly initializes all components with appropriate error handling throughout. The composition pattern with embedded interfaces is well-implemented.


81-91: LGTM! Clean interface implementation.

The interface methods are correctly implemented as simple getters, maintaining consistency with the contract interface pattern.


18-21: LGTM! Well-defined event topics.

The event topics are clearly defined and match the expected events for a payer report manager contract: PayerReportSubmitted and PayerReportSubsetSettled.

Francisco de Borja Aranda Castillejo added 2 commits May 28, 2025 12:46
Different RPC might have different block page sizes.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Backfill block size configuration is now available separately for each
chain type, allowing more granular control for both app chains and
settlement chains.
- Added a new setting for maximum chain disconnect time in settlement
chain options.

- **Chores**
- Updated internal configuration handling to remove the global backfill
block size setting and move it to individual chain configurations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@fbac fbac merged commit 6b8d6c8 into main May 28, 2025
10 checks passed
@fbac fbac deleted the 05-25-settlement_chain branch May 28, 2025 10:57
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