Add log storer #863
Add log storer #863
Conversation
WalkthroughThis change introduces a new mechanism to update the Changes
Sequence Diagram(s)sequenceDiagram
participant Contract as PayerReportManager (contract)
participant Storer as PayerReportManagerStorer
participant Store as IPayerReportStore/Store
participant DB as Database
Contract->>Storer: Emit PayerReportSubmitted event
Storer->>Storer: Parse event, build ReportID
Storer->>Store: StoreReport(ctx, report)
Store->>DB: INSERT INTO payer_reports ...
Store-->>Storer: error or success
Storer->>Store: SetReportSubmissionStatus(ctx, reportID, [Pending], Submitted)
Store->>DB: UPDATE payer_reports SET submission_status=... WHERE id=... AND submission_status IN (...)
Store-->>Storer: error or success
Storer-->>Contract: Return (with error handling)
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (12)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Add log storer by persisting
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (1)
128-130: Address the TODO: Add end_minutes_since_epoch to the contractThe
EndMinuteSinceEpochfield is hardcoded to 0, which may affect report processing accuracy.Would you like me to create an issue to track adding the
end_minutes_since_epochfield to the smart contract?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/db/queries/payerReports.sql.go(1 hunks)pkg/db/sqlc/payerReports.sql(1 hunks)pkg/indexer/settlement_chain/contracts/payer_report_manager.go(3 hunks)pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go(4 hunks)pkg/indexer/settlement_chain/contracts/payer_report_manager_storer_test.go(1 hunks)pkg/indexer/settlement_chain/settlement_chain.go(1 hunks)pkg/mocks/payerreport/mock_IPayerReportStore.go(1 hunks)pkg/payerreport/interface.go(2 hunks)pkg/payerreport/store.go(2 hunks)pkg/payerreport/store_test.go(13 hunks)pkg/payerreport/workers/attestation_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
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.
🧬 Code Graph Analysis (6)
pkg/payerreport/interface.go (2)
pkg/payerreport/report.go (3)
PayerReport(69-83)ReportID(63-63)SubmissionStatus(47-47)pkg/db/queries/models.go (1)
PayerReport(65-76)
pkg/indexer/settlement_chain/contracts/payer_report_manager.go (2)
pkg/db/queries/db.go (1)
New(19-21)pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (1)
NewPayerReportManagerStorer(40-64)
pkg/db/queries/payerReports.sql.go (2)
pkg/payerreport/report.go (1)
ReportID(63-63)pkg/db/queries/db.go (1)
Queries(23-25)
pkg/payerreport/store.go (3)
pkg/payerreport/report.go (3)
PayerReport(69-83)ReportID(63-63)SubmissionStatus(47-47)pkg/db/queries/models.go (1)
PayerReport(65-76)pkg/db/queries/payerReports.sql.go (1)
SetReportSubmissionStatusParams(518-522)
pkg/mocks/payerreport/mock_IPayerReportStore.go (1)
pkg/payerreport/report.go (3)
ReportID(63-63)SubmissionStatus(47-47)PayerReport(69-83)
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (6)
pkg/payerreport/interface.go (1)
IPayerReportStore(37-75)pkg/indexer/common/interface.go (1)
ILogStorer(13-15)pkg/payerreport/store.go (1)
NewStore(38-44)pkg/abi/payerreportmanager/PayerReportManager.go (1)
PayerReportManagerPayerReportSubmitted(1143-1152)pkg/errors/errors.go (3)
NewNonRecoverableError(27-29)RetryableError(5-8)NewRecoverableError(48-50)pkg/payerreport/report.go (3)
ReportID(63-63)BuildPayerReportID(141-170)SubmissionStatus(47-47)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
🔇 Additional comments (12)
pkg/indexer/settlement_chain/settlement_chain.go (1)
76-76: LGTM: Improved abstraction by passing raw database handle.The change from
queriertodbaligns with the constructor signature update forNewPayerReportManager. This allows the manager to handle its own query instantiation internally, providing better abstraction.pkg/payerreport/workers/attestation_test.go (1)
52-54: LGTM: Simplified method signature reduces unnecessary return values.The changes reflect the updated
StoreReportmethod signature that now returns only an error instead of(*ReportID, error). Since the report ID is already available inreport.ID, there's no need to return it from the store method, which simplifies the API.pkg/indexer/settlement_chain/contracts/payer_report_manager_storer_test.go (1)
99-99: LGTM: Test updated to match new constructor signature.The change correctly updates the test setup to pass the raw database handle
dbdirectly toNewPayerReportManagerStorer, aligning with the constructor refactor that now accepts*sql.DBinstead of*queries.Queries.pkg/db/sqlc/payerReports.sql (1)
151-155: LGTM: Well-structured query follows established patterns.The new
SetReportSubmissionStatusquery correctly mirrors the existingSetReportAttestationStatuspattern and implements good practices:
- Uses conditional updates with
IN (sqlc.slice(prev_status))to ensure safe state transitions- Follows consistent naming and parameter binding conventions
- Prevents invalid state changes through the WHERE clause condition
This addition properly supports the new submission status tracking functionality.
pkg/payerreport/interface.go (1)
38-38: LGTM! Well-designed interface changes.The interface updates are well-structured:
StoreReportsignature simplification makes sense since thePayerReportalready contains itsIDfieldSetReportSubmissionStatusfollows the established pattern ofSetReportAttestationStatus, maintaining consistency in the interface designAlso applies to: 68-73
pkg/indexer/settlement_chain/contracts/payer_report_manager.go (1)
35-44: LGTM! Constructor refactoring is well-implemented.The changes appropriately refactor the constructor to accept a raw
*sql.DBinstead of*queries.Queries, providing more flexibility while maintaining existing functionality by creating the querier internally. The change to pass the database handle toNewPayerReportManagerStorermaintains consistency with the broader refactoring.Also applies to: 69-69
pkg/payerreport/store_test.go (1)
31-43: LGTM! Test updates correctly reflect the interface changes.The test modifications properly adapt to the new
StoreReportmethod signature:
- Tests now explicitly create
ReportIDvalues usingReportID(randomBytes32())StoreReportcalls no longer expect a return value for the report ID- Report fetching uses the report ID directly instead of dereferencing pointers
- The pattern is applied consistently across all affected test functions
The changes maintain test coverage while properly adapting to the simplified interface.
Also applies to: 133-138, 156-170, 267-280, 293-293, 335-355, 402-414, 487-499
pkg/payerreport/store.go (2)
47-58: LGTM! StoreReport simplification is well-implemented.The method signature change is appropriate since the
PayerReportalready contains itsIDfield, eliminating the need to return it. The implementation correctly maintains all validation and storage logic while simplifying the return signature.
128-144: LGTM! SetReportSubmissionStatus follows established patterns.The new method implementation correctly mirrors the existing
SetReportAttestationStatuspattern:
- Converts
SubmissionStatusenum values toint16array for database compatibility- Uses the same parameter structure and query pattern
- Maintains consistency with the existing codebase design
The implementation is clean and follows established conventions.
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go (2)
40-64: LGTM! Constructor refactoring improves separation of concernsThe refactoring to accept
*sql.DBand instantiate the store internally provides better encapsulation. The domain separator retrieval is correctly handled with appropriate error propagation.
105-148: Well-structured event processing with proper error handlingThe
setReportSubmittedmethod correctly:
- Builds the report ID with domain separator validation
- Distinguishes between recoverable and non-recoverable errors
- Implements idempotent status updates (no-op if already settled)
pkg/mocks/payerreport/mock_IPayerReportStore.go (1)
354-448: LGTM! Mock implementations correctly reflect new interface methodsThe generated mock methods for
SetReportSubmissionStatusandStoreReportproperly implement the new interface requirements and follow standard mockery patterns.
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go
Outdated
Show resolved
Hide resolved
neekolas
force-pushed
the
06-03-add_log_storer
branch
2 times, most recently
from
c14e4fe to
36c895d
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🛑 Comments failed to post (1)
pkg/testutils/contracts.go (1)
125-169: 💡 Verification agent
🧩 Analysis chain
Fix the duplicate parameter in data packing.
The function looks well-structured and follows the established pattern, but there's an issue in the data packing where
activeNodeIDsis passed twice tononIndexed.Pack().Apply this diff to fix the duplicate parameter:
- data, err := nonIndexed.Pack( - startSequenceID, - payersMerkleRoot, - activeNodeIDs, - activeNodeIDs, - ) + data, err := nonIndexed.Pack( + startSequenceID, + payersMerkleRoot, + activeNodeIDs, + )Please verify the correct parameters for the
PayerReportSubmittedevent by checking the ABI definition:
🏁 Script executed:
#!/bin/bash # Description: Check the PayerReportSubmitted event ABI definition to verify correct non-indexed parameters. # Search for PayerReportSubmitted event definition in ABI files fd -e go -e json . --exec grep -l "PayerReportSubmitted" {} \; | head -5 | xargs grep -A 10 -B 2 "PayerReportSubmitted" # Also search for the ABI definition in Solidity files if available fd -e sol . --exec grep -l "PayerReportSubmitted" {} \; | head -3 | xargs grep -A 5 -B 2 "event PayerReportSubmitted"Length of output: 14616
Remove duplicate parameter in data packing
The
PayerReportSubmittedevent defines only three non-indexed parameters (startSequenceID,payersMerkleRootandactiveNodeIDs), but the pack call erroneously suppliesactiveNodeIDstwice.• pkg/testutils/contracts.go (around lines 125–169)
- data, err := nonIndexed.Pack( - startSequenceID, - payersMerkleRoot, - activeNodeIDs, - activeNodeIDs, - ) + data, err := nonIndexed.Pack( + startSequenceID, + payersMerkleRoot, + activeNodeIDs, + )This change brings the test utility in line with the actual ABI.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.data, err := nonIndexed.Pack( startSequenceID, payersMerkleRoot, activeNodeIDs, ) require.NoError(t, err)🤖 Prompt for AI Agents
In pkg/testutils/contracts.go around lines 125 to 169, the call to nonIndexed.Pack incorrectly passes the activeNodeIDs parameter twice. To fix this, remove the duplicate activeNodeIDs argument so that only startSequenceID, payersMerkleRoot, and activeNodeIDs are passed to the Pack function, matching the actual event ABI definition.
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
36c895d to
eb323e4
Compare
neekolas
force-pushed
the
05-15-add_domain_separator_for_proper_signatures
branch
from
739e5f2 to
92d617c
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
eb323e4 to
dd27e70
Compare
neekolas
force-pushed
the
05-15-add_domain_separator_for_proper_signatures
branch
2 times, most recently
from
bba174a to
8b6917a
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
dd27e70 to
557fe72
Compare
neekolas
force-pushed
the
05-15-add_domain_separator_for_proper_signatures
branch
from
8b6917a to
c3aab44
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
2 times, most recently
from
133a20d to
91b8c18
Compare
neekolas
force-pushed
the
05-15-add_domain_separator_for_proper_signatures
branch
2 times, most recently
from
0817ab1 to
9dabc61
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
91b8c18 to
5b5616f
Compare
neekolas
force-pushed
the
05-15-add_domain_separator_for_proper_signatures
branch
from
9dabc61 to
759bca9
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
5b5616f to
3329b62
Compare
neekolas
force-pushed
the
05-15-add_domain_separator_for_proper_signatures
branch
from
759bca9 to
539133d
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
3329b62 to
084593a
Compare
graphite-app
bot
changed the base branch from
05-15-add_domain_separator_for_proper_signatures
to
graphite-base/863
neekolas
force-pushed
the
graphite-base/863
branch
from
539133d to
522d05f
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
084593a to
8be596b
Compare
pkg/indexer/settlement_chain/contracts/payer_report_manager_storer.go
Outdated
Show resolved
Hide resolved
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
8be596b to
9e82e2e
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
9e82e2e to
44b1618
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
44b1618 to
bdc2c75
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
bdc2c75 to
1c36abd
Compare
neekolas
changed the base branch from
graphite-base/863
to
09-02-upgrade_tools_dependencies
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
1c36abd to
edd1eb2
Compare
neekolas
force-pushed
the
06-03-add_log_storer
branch
from
edd1eb2 to
66af1da
Compare
neekolas
force-pushed
the
09-02-upgrade_tools_dependencies
branch
from
13a6e2c to
0a2f8ac
Compare
Merge activity
|
_Macroscope is automatically generating a summary of this pull request..._ <img src="https://prasso.ai/static/prassoai-pr-loader-small-light-no-text.gif" alt="_Macroscope is automatically generating a summary of this pull request..._"> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for updating payer report submission status with controlled status transitions. - Improved event handling to parse and store payer report submissions using contract domain data. - **Bug Fixes** - Enhanced error handling and idempotency in storing and updating payer report states. - **Refactor** - Changed database dependency injection to use raw database connections for report management. - Extended internal interfaces and methods for consistent report storage and submission status updates. - **Tests** - Expanded tests to cover payer report submission event processing and idempotency. - Added helpers for constructing detailed event logs in tests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Macroscope is automatically generating a summary of this pull request...
Summary by CodeRabbit