Generate payer reports #646
Conversation
WalkthroughThe changes introduce modifications to existing SQL queries and the addition of new queries to enhance data retrieval capabilities related to unsettled usage and payer reports. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as PayerReportManager
participant Q as Queries (DB)
C->>M: GenerateReport(ctx, params)
M->>Q: getStartMinute(ctx, sequenceID, originatorID)
Q-->>M: startMinute value
M->>Q: getEndMinute(ctx, originatorID, startMinute)
Q-->>M: (endMinute, endSequenceID)
M->>Q: BuildPayerReport(ctx, BuildPayerReportParams)
Q-->>M: Report rows
M->>M: buildPayersMap(rows)
M-->>C: Return PayerReport object
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/payerreport/generator.go (3)
29-60: Consider addressing the Merkle calculation TODO.
An empty placeholder is fine for now, but it’s easy to forget. Tracking this in an issue or implementing soon helps avoid technical debt.Do you want me to open a new issue or supply more detail on how to implement the Merkle calculation?
62-83: Optional: Log empty results.
If theBuildPayerReportquery returns an empty slice, you might consider logging this situation to assist with debugging.
119-174: Potential performance bottleneck and minor typo.
The loop-based descending query might be expensive for large datasets. Consider a more direct strategy to find the boundary minute. Also, there's a small typo in the docstring: "descendding" → "descending."- * It does this by getting envelopes from the database in descendding order until ... + * It does this by getting envelopes from the database in descending order until ...pkg/db/queries/queries.sql.go (2)
257-270: Optional: Handle sql.ErrNoRows explicitly.
While returning an error is standard, you could add a check for sql.ErrNoRows to produce a clearer error message if no envelope is found.
367-373: Parameterize the limit if needed.
Currently, the query fetches up to 1000 envelopes. If usage scales, consider making this limit configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/db/queries.sql(1 hunks)pkg/db/queries/queries.sql.go(3 hunks)pkg/payerreport/generator.go(1 hunks)pkg/payerreport/generator_test.go(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pkg/payerreport/generator.go (2)
pkg/payerreport/interface.go (2) (2)
PayerReportGenerationParams(35-39)PayerReport(10-23)pkg/db/queries/queries.sql.go (4) (4)
BuildPayerReportParams(30-34)GetGatewayEnvelopeByIDParams(252-255)GetOlderGatewayEnvelopesParams(375-378)BuildPayerReportRow(36-39)
pkg/db/queries/queries.sql.go (2)
pkg/db/queries/db.go (2) (2)
q(27-31)Queries(23-25)pkg/db/queries/models.go (1) (1)
GatewayEnvelope(27-34)
🪛 GitHub Check: Lint-Go
pkg/payerreport/generator_test.go
[failure] 49-49:
Error return value of dbHelpers.InsertGatewayEnvelopeAndIncrementUnsettledUsage is not checked (errcheck)
🔇 Additional comments (24)
pkg/testutils/envelopes/envelopes.go (3)
5-5: Appropriate import added for the new timestamp functionality.The time package import is correctly added to support the new timestamp-related functionality.
112-140: Well-implemented timestamp functionality.The new function
CreateOriginatorEnvelopeWithTimestampappropriately extends the envelope creation capability by allowing custom timestamps. The implementation correctly uses theUnixNano()method to convert the timestamp to nanoseconds.
142-154: Good backward compatibility pattern.The original
CreateOriginatorEnvelopefunction is nicely refactored to call the new function with a default timestamp, maintaining backward compatibility while eliminating code duplication.pkg/db/queries.sql (3)
266-279: Well-structured payer report query.The
BuildPayerReportquery effectively aggregates spending data by payer address with appropriate time range filtering. The JOIN operation with the payers table and GROUP BY clause are correctly implemented.
280-285: Efficient envelope retrieval query.The
GetGatewayEnvelopeByIDquery correctly includes both sequence ID and node ID in the WHERE clause, which will effectively utilize the primary key index as noted in the comment.
286-291: Well-designed pagination for older envelopes.The
GetOlderGatewayEnvelopesquery provides an efficient way to retrieve older envelopes with proper ordering and pagination. The condition(@originator_sequence_id = 0 OR originator_sequence_id < @originator_sequence_id)allows for flexible querying.pkg/payerreport/interface.go (3)
10-23: Well-structured PayerReport definition.The PayerReport struct is well-designed with appropriate fields and clear comments explaining each field's purpose. The use of
common.Addressas the key type for the Payers map is appropriate for Ethereum address mapping.
25-33: Clear node signature and attestation structures.The NodeSignature and PayerReportAttestation structs provide a good foundation for the report signing and verification system.
35-51: Well-defined interfaces and generation parameters.The PayerReportGenerationParams struct and the IPayerReportGenerator/IPayerReportAttester interfaces provide a clean API for report generation and attestation. The separation of concerns between generation and attestation is a good design choice.
pkg/payerreport/generator_test.go (2)
20-27: Good test setup pattern.The setup function properly initializes the test environment and uses t.Cleanup to ensure resources are released after the test.
72-196: Comprehensive test coverage.The test suite provides good coverage of different scenarios including first reports, multiple payers, and cases with no messages. The test structure is clean with clear assertions that validate the expected behavior.
pkg/payerreport/generator.go (4)
14-17: Good struct design.
Injecting the logger and queries into the struct is a clean approach that enhances testability and separation of concerns.
19-27: Constructor is straightforward.
No issues found here; the initialization logic is concise and clear.
85-117: Efficient start-minute retrieval.
The method correctly returns zero for an uninitialized sequence ID, and handles errors appropriately for envelope lookups.
176-184: Straightforward payers map construction.
Your approach here properly maps each payer address to its total spend in picodollars.pkg/db/queries/queries.sql.go (9)
15-28: Confirm time boundary logic.
This query usesminutes_since_epoch > $2, which excludes the boundary minute at $2. Verify that this strictly-exclusionary condition is intentional.
30-34: Proper parameters structure.
All fields for start/end minutes and originator ID are well-defined. No issues here.
36-39: Sensible row struct.
Payer address and total spend fields are clearly mapped for the report.
41-62: Query execution flow is solid.
Standard sqlc-generated method; properly handles row scanning and errors.
246-250: Gateway envelope retrieval logic looks good.
Query filters by sequence ID and originator node ID. Straightforward usage of the table’s primary key.
252-255: Accurate param mapping.
No concerns with struct naming or data types for these query parameters.
375-378: Parameter struct is clear.
Though unusual to seeinterface{}for numeric parameters, this is presumably sqlc’s approach to optional parameters. Implementation is consistent.
380-384: Row struct is properly defined.
Includes only sequence ID and envelope bytes. No issues with the schema mapping.
385-406: Function handles row scanning correctly.
Standard practice for a sqlc-generated method. Looks good overall.
neekolas
force-pushed
the
03-18-generate_payer_reports
branch
2 times, most recently
from
2ff5da1 to
0e5c7fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
pkg/payerreport/manager_test.go (1)
29-67: Consider parameterizing the spend amount.The
addEnvelopefunction has a hardcoded value of 100 forSpendPicodollars. Consider making this a parameter to increase test flexibility.-func addEnvelope( - t *testing.T, - db *sql.DB, - originatorID int32, - sequenceID int64, - payerAddress common.Address, - timestamp time.Time, -) { +func addEnvelope( + t *testing.T, + db *sql.DB, + originatorID int32, + sequenceID int64, + payerAddress common.Address, + timestamp time.Time, + spendPicodollars int64, +) { ... - SpendPicodollars: 100, + SpendPicodollars: spendPicodollars,Then update the function calls with the current value (100) or different values as needed.
pkg/payerreport/manager.go (5)
1-2: Consider adding a package-level comment.It’s often helpful to include a brief overview of this package’s purpose at the top of the file (e.g., “Package payerreport implements payer report generation…”).
14-17: Add doc comment forPayerReportManager.Adding a doc comment clarifies this struct’s responsibilities and how it’s intended to be used.
+// PayerReportManager provides methods to generate payer reports based on historical message data. type PayerReportManager struct { log *zap.Logger queries *queries.Queries }
50-60: Clarify end-sequence check.When
endSequenceID == 0, the code returns an empty report. However, the PR objectives mention that at least two messages from different minutes are needed to generate a valid report. Consider verifying that this condition truly captures the scenario of having insufficient envelopes.
119-174: Consider a more efficient approach to pinpoint the end minute.Iterating over envelopes in descending order could become expensive for large datasets. A binary search approach or adding an indexed
originator_timecolumn might significantly speed up the process, as noted in the PR description.Do you want me to propose a binary-search-based logic or open a new issue to track a potential schema update?
176-184: Add concurrency guard if used across multiple threads.
buildPayersMapreturns a shared map that might be accessed in parallel ifGenerateReportis called concurrently. Although simple, a potential race condition might arise if the returned map is accidentally mutated.Consider returning an immutable data structure or ensuring a concurrency-safe usage pattern.
pkg/db/queries/queries.sql.go (2)
30-39: Add parameter validations in higher-level code if needed.The
BuildPayerReportParamsstruct allows specifying a minute range that can be negative or reversed in some edge cases. Upstream validations can help prevent unexpected queries.
367-406: Restrict the limit or consider pagination.
GetOlderGatewayEnvelopesretrieves up to 1000 envelopes each iteration. If there are scenarios requiring a much larger scan, you may want to implement pagination or a more direct bounding condition to avoid large loops in the application layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/db/queries.sql(1 hunks)pkg/db/queries/queries.sql.go(3 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(1 hunks)pkg/payerreport/manager_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pkg/payerreport/manager.go (2)
pkg/payerreport/interface.go (2) (2)
PayerReportGenerationParams(35-39)PayerReport(10-23)pkg/db/queries/queries.sql.go (4) (4)
BuildPayerReportParams(30-34)GetGatewayEnvelopeByIDParams(252-255)GetOlderGatewayEnvelopesParams(375-378)BuildPayerReportRow(36-39)
pkg/db/queries/queries.sql.go (2)
pkg/db/queries/db.go (2) (2)
q(27-31)Queries(23-25)pkg/db/queries/models.go (1) (1)
GatewayEnvelope(27-34)
🔇 Additional comments (13)
pkg/testutils/envelopes/envelopes.go (2)
112-140: Implementation looks good!This new function allows creating envelopes with a custom timestamp, which is essential for testing the payer report feature that relies on timestamp-based calculations.
142-154: Good refactoring approach.The original function is now refactored to use the new timestamp-enabled function, maintaining backward compatibility by passing a Unix epoch zero timestamp as the default.
pkg/payerreport/interface.go (3)
10-23: Well-structured report definition with clear documentation.The
PayerReportstruct contains all necessary fields for tracking originator messages, sequence ranges, and payer information with clear field documentation.
25-33: Clean signature and attestation structures.The
NodeSignatureandPayerReportAttestationstructures are well-defined for report verification purposes.
35-48: Interface design looks good.The parameters struct and manager interface provide a clean API for generating and attesting payer reports. The interface methods have appropriate context parameters for managing request lifetimes.
pkg/payerreport/manager_test.go (3)
20-27: Clean test setup.The setup function properly initializes the database and manager for testing, with appropriate cleanup.
73-97: Comprehensive test coverage.The tests cover important scenarios including:
- First report generation
- Multiple payers
- No messages
- Second report with no new messages
- Second report with additional messages
This ensures the payer report generation is robust across different situations.
Also applies to: 99-121, 123-139, 141-159, 161-195
182-183:❓ Verification inconclusive
Duplicate sequence ID in test.
There are two envelopes with the same sequence ID (4) in the test, which may cause issues if the database enforces uniqueness on sequence IDs.
🏁 Script executed:
#!/bin/bash # Check if the database schema enforces uniqueness on sequence IDs grep -A10 "CREATE TABLE.*gateway_envelopes" pkg/db/schema.sqlLength of output: 114
Attention: Duplicate Envelope Sequence ID Issue
In
pkg/payerreport/manager_test.go(Lines 182–183), the test adds two envelopes using the same sequence ID (4). This could lead to issues if the database enforces uniqueness on sequence IDs. However, our initial check for a unique constraint in the database schema (specifically in a file likepkg/db/schema.sql) did not find the expected file.
- Action Required: Please manually verify that the table (likely named
gateway_envelopes) either enforces or permits duplicate sequence IDs. If a uniqueness constraint exists, update the test to use distinct sequence IDs. Otherwise, consider documenting why duplicate IDs are acceptable in this test context.pkg/db/queries.sql (3)
266-278: Well-structured query for payer report.The
BuildPayerReportquery correctly aggregates spending data by payer, joining with the payers table to get the address and filtering by originator and time range.
280-284: Optimized envelope retrieval.The query comment correctly notes the use of the primary key index for efficiency.
286-291:❓ Verification inconclusive
Fixed limit on older envelope retrieval.
The
GetOlderGatewayEnvelopesquery has a hard limit of 1000 records, which may not be sufficient if there are many messages within the time range needed for report generation.
🏁 Script executed:
#!/bin/bash # Description: Check typical message volume to determine if 1000 limit is sufficient # Verify if the manager implementation has logic to handle pagination when >1000 records exist fd --type file "manager.go" pkg/payerreport/ --exec grep -A20 "GetOlderGatewayEnvelopes" {} # Check if there's a better index for this query that includes timestamp grep -A10 "CREATE INDEX" pkg/db/schema.sql | grep gateway_envelopesLength of output: 782
Verify Pagination Logic and Indexing in Envelope Retrieval
It appears that the
GetOlderGatewayEnvelopesquery inpkg/db/queries.sql(lines 286–291) is intentionally limited to 1000 records per call. However, the caller inpkg/payerreport/manager.goloops over the results—suggesting that pagination is implemented by repeatedly fetching envelopes until none are returned.Please verify the following:
- The update of parameters (especially the
originator_sequence_id) between iterations correctly advances the pagination so that all envelopes are eventually retrieved.- That the appropriate indexing on the
gateway_envelopestable (potentially including timestamp columns) exists. (Note: the expected schema filepkg/db/schema.sqlwasn’t found via the initial grep; confirm if index definitions exist elsewhere.)pkg/payerreport/manager.go (1)
29-83:❓ Verification inconclusive
Ensure that all input parameters are used and validated.
While the
GenerateReportmethod acceptsparams.NumHours, the logic never references it. If time-window-based generation is intended, consider integratingNumHoursto filter or slice data accordingly. Otherwise, remove the unused parameter for clarity.Could you confirm whether ignoring
NumHoursis intentional? You can run searches in the codebase for references toNumHours:
🏁 Script executed:
#!/bin/bash rg -A 3 'NumHours'Length of output: 203
Action Required: Clarify Usage of
params.NumHoursin Report GenerationThe interface in
pkg/payerreport/interface.godefinesNumHours, but the implementation inpkg/payerreport/manager.go'sGenerateReportmethod does not reference it. Please confirm whether the omission is intentional. If time-window filtering is intended, integrate logic to utilize and validateparams.NumHours; otherwise, consider removing it from both the interface and implementation for consistency.pkg/db/queries/queries.sql.go (1)
246-270: Check for envelope existence before scanning.Users might call
GetGatewayEnvelopeByIDwith IDs that are valid but refer to envelopes that are logically pruned. If a zero-row scenario is possible, returning an empty struct might be more intuitive than an error.Would you like to handle non-existent envelopes differently, e.g., returning a custom error code or nil instead of a DB error?
neekolas
force-pushed
the
03-18-generate_payer_reports
branch
from
0e5c7fa to
c6ba6d8
Compare
Add payer report generation with sequence ID trackingAdded payer report generation functionality with sequence ID tracking in unsettled usage records. Added 📍Where to StartThe Macroscope summarized 900622b. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
pkg/db/gatewayEnvelope_test.go (1)
142-142:⚠️ Potential issueFix return type mismatch in assertion
The same issue occurs in this test as well -
GetPayerUnsettledUsagenow returns aqueries.GetPayerUnsettledUsageRowstruct.-require.Equal(t, payerSpend, int64(100)) +require.Equal(t, payerSpend.TotalSpendPicodollars, int64(100))🧰 Tools
🪛 GitHub Actions: Test
[error] 142-142: TestInsertAndIncrementParallel failed: Not equal: expected: queries.GetPayerUnsettledUsageRow(queries.GetPayerUnsettledUsageRow{TotalSpendPicodollars:100, LastSequenceID:10}), actual: int64(100)
🧹 Nitpick comments (6)
pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql (1)
1-2: Consider using an IF EXISTS clause when dropping the table.
Dropping a table that may not exist could cause failures if the table does not exist in certain environments.-DROP TABLE unsettled_usage; +DROP TABLE IF EXISTS unsettled_usage;pkg/payerreport/manager_test.go (4)
29-67: Confirm concurrency model when adding envelopes.
addEnvelopeboth inserts envelopes and increments unsettled usage. If tests run in parallel, consider data collisions or requiring unique resource setup per test.Suggest adding test seeds or random offsets in the parameters for
addEnvelopein concurrent/parallel tests.
69-71: Time utility clarity.
FunctiongetMinuteis straightforward, but be mindful of time zone or daylight savings changes if real-world usage expands.
99-121: Test multiple payers scenario thoroughly.
This test demonstrates coverage for two payers. Consider adding a scenario with zero or unknown payer to confirm the system handles invalid addresses gracefully.
161-195: Consider verifying partial usage in TestSecondReport.
Currently, the test checks usage across two reports. Consider adding an intermediate check if someone adds usage between sequence IDs 2 and 4 to confirm partial updates.pkg/payerreport/manager.go (1)
29-83: GenerateReport handles insufficient envelopes gracefully.
Returning an empty report whenendSequenceIDis 0 helps avoid errors in edge cases. Future improvements might log warnings so users know no data was available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/db/gatewayEnvelope.go(1 hunks)pkg/db/gatewayEnvelope_test.go(2 hunks)pkg/db/queries.sql(2 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(7 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql(1 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(1 hunks)pkg/payerreport/manager_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/payerreport/interface.go
- pkg/testutils/envelopes/envelopes.go
🧰 Additional context used
🧬 Code Definitions (4)
pkg/db/gatewayEnvelope_test.go (2)
pkg/db/gatewayEnvelope.go (1) (1)
InsertGatewayEnvelopeAndIncrementUnsettledUsage(13-72)pkg/db/queries/queries.sql.go (1) (1)
GetPayerUnsettledUsageParams(381-385)
pkg/payerreport/manager_test.go (5)
pkg/payerreport/manager.go (2) (2)
PayerReportManager(14-17)NewPayerReportManager(19-27)pkg/testutils/envelopes/envelopes.go (1) (1)
CreateOriginatorEnvelopeWithTimestamp(112-140)pkg/db/gatewayEnvelope.go (1) (1)
InsertGatewayEnvelopeAndIncrementUnsettledUsage(13-72)pkg/db/queries/queries.sql.go (2) (2)
InsertGatewayEnvelopeParams(603-610)IncrementUnsettledUsageParams(526-532)pkg/payerreport/interface.go (1) (1)
PayerReportGenerationParams(35-39)
pkg/payerreport/manager.go (2)
pkg/payerreport/interface.go (2) (2)
PayerReportGenerationParams(35-39)PayerReport(10-23)pkg/db/queries/queries.sql.go (4) (4)
BuildPayerReportParams(30-34)GetGatewayEnvelopeByIDParams(252-255)GetSecondNewestMinuteParams(466-469)BuildPayerReportRow(36-39)
pkg/db/queries/queries.sql.go (2)
pkg/db/queries/models.go (1) (1)
GatewayEnvelope(27-34)pkg/db/types.go (1) (1)
Topic(10-10)
🪛 GitHub Actions: Test
pkg/db/gatewayEnvelope_test.go
[error] 67-67: TestInsertAndIncrement failed: Not equal: expected: queries.GetPayerUnsettledUsageRow(queries.GetPayerUnsettledUsageRow{TotalSpendPicodollars:100, LastSequenceID:10}), actual: int64(100)
[error] 142-142: TestInsertAndIncrementParallel failed: Not equal: expected: queries.GetPayerUnsettledUsageRow(queries.GetPayerUnsettledUsageRow{TotalSpendPicodollars:100, LastSequenceID:10}), actual: int64(100)
🔇 Additional comments (34)
pkg/db/queries/models.go (1)
75-76: LGTM: Added LastSequenceID field to support payer report generationThe addition of the LastSequenceID field to the UnsettledUsage struct is appropriate for tracking the highest sequence ID seen for each payer/originator combination, which is necessary for determining report boundaries.
pkg/db/gatewayEnvelope.go (1)
36-39: LGTM: Ensuring LastSequenceID gets properly setGood addition to set the sequence ID for tracking the highest sequence ID seen per payer/originator. This supports the report generation feature by ensuring we always have a valid sequence ID for establishing message boundaries.
pkg/db/gatewayEnvelope_test.go (1)
152-191: LGTM: Good test for handling out-of-order sequence IDsThis test properly verifies that when messages are received out of order, the system correctly maintains the highest sequence ID seen. This is essential for the payer report generation feature which uses sequence IDs to determine report boundaries.
pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql (2)
4-11: Validate table schema and integration with existing constraints.
Double-check that the newly addedlast_sequence_iddoes not conflict with any existing logic or constraints in the system. Ensuring consistent handling of negative or zero values is also important.Would you like me to generate a script that searches other database references to confirm no conflicting foreign key constraints exist around
unsettled_usage?
13-14: Index choice looks appropriate.
Indexing(originator_id, minutes_since_epoch DESC)is sensible for queries that retrieve the most recent usage records by minute, which is a common pattern in usage reporting.pkg/payerreport/manager_test.go (4)
20-27: Setup function is concise and effective.
The cleanup mechanism is well-placed, ensuring the test database is handled properly.
73-97: Verify aggregated values in TestFirstReport.
Overall logic checks out. Ensure that none of the test coverage conditions are overshadowed by default database states or other tests run concurrently.
123-139: TestReportWithNoMessages covers the edge case well.
The assertions are correct for an empty usage scenario. Good coverage of the “no messages” path.
141-159: Second report with no messages scenario is consistent.
The test clarifies how consecutive report generation behaves if there is no new usage. This is well-implemented.pkg/db/queries.sql (5)
184-190: Great use of GREATEST for last_sequence_id.
This approach ensures thatlast_sequence_idnever decreases. Double-check that any subsequent queries properly handle concurrent increments, so that re-inserts don't overwrite a higher sequence ID with an older one.
193-195: Including last_sequence_id in GetPayerUnsettledUsage is beneficial.
This offers direct visibility into the highest relevant sequence ID for the payer.
267-280: BuildPayerReport query looks sound.
Aggregating total spend per payer is correct for usage-based billing. You might want to ensure that no partial merges or negative usage adjustments are missed in this step.Would you like a script to scan references to see if negative spend or usage corrections ever get inserted, potentially impacting final aggregator results?
282-286: GetGatewayEnvelopeByID aligns with primary key usage.
Filtering on(originator_sequence_id, originator_node_id)is efficient. Looks good for retrieving a specific envelope.
288-305: Check offset usage in GetSecondNewestMinute.
Query usesLIMIT 1 OFFSET 1to retrieve the second newest minute and then aggregates bylast_sequence_id. Confirm logic remains correct if fewer than two minutes of data exist.pkg/payerreport/manager.go (5)
14-17: PayerReportManager structure is well-defined.
Storing both logger and queries in this manager keeps dependencies clear.
19-27: Factory method is straightforward.
NewPayerReportManageris consistent with recommended patterns, returning a properly initialized manager.
85-117: getStartMinute conscientious approach.
It fetches the relevant envelope before determining the minute. Make sure the envelope is always consistent with the requested sequence.
119-147: getEndMinute logic.
Retrieves the “second newest minute” to define the end boundary. Confirm that usage older thanstartMinuteis not accidentally included.
149-157: buildPayersMap is neat and direct.
The function is succinct. Consider returning an empty map by default if rows is empty to avoid any nil references.pkg/db/queries/queries.sql.go (15)
15-28: Ensure index coverage for originator/time filters
As flagged in a past review, please confirm that an index on(originator_id, minutes_since_epoch)is in place to avoid full table scans when filtering on these columns.
30-34: No concerns
The parameter structBuildPayerReportParamsis straightforward and aligns well with the SQL placeholders.
36-39: No concerns
TheBuildPayerReportRowfields neatly match the columns from the SQL query, ensuring clarity and consistency.
41-62: Query execution logic is standard
The loop thoroughly scans rows and handles potential errors properly.
246-250: Query matches the new fields
GetGatewayEnvelopeByIDaccurately retrieves all required columns based on the updated schema.
252-255: Parameter struct clarity
GetGatewayEnvelopeByIDParamscleanly maps query parameters to struct fields.
257-270: Function logic is correct
The method properly scans and returns a singleGatewayEnvelope, handling row retrieval as expected.
369-370: Index coverage reminder
Confirm that an index on(payer_id, minutes_since_epoch)exists to optimize queries filtering on these columns.
387-390: Struct matches new columns
GetPayerUnsettledUsageRowcorrectly includes bothTotalSpendPicodollarsandLastSequenceID, reflecting the updated query.
392-397: Query method logic is consistent
The row scanning and error handling are standard and align with the modified SELECT statement.
446-497: Potential performance concern
As previously flagged, ensure that queries involvingunsettled_usagewith these filtering and ordering clauses are backed by an appropriate(originator_id, minutes_since_epoch)index for efficiency.
518-519: INSERT statement extended
Addinglast_sequence_idto the insert column list aligns with the updated schema requirements.
522-523: Correct approach to maintain max sequence ID
UsingGREATEST(unsettled_usage.last_sequence_id, $5)ensures concurrency safety by retaining the highest known sequence ID.
531-531: New SequenceID parameter
IntroducingSequenceIDintoIncrementUnsettledUsageParamsproperly reflects the added column in the query.
540-540: Passing SequenceID
Forwardingarg.SequenceIDinto theExecContextcall is consistent with the param struct and SQL placeholder usage.
pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql
Outdated
Show resolved
Hide resolved
neekolas
force-pushed
the
03-18-generate_payer_reports
branch
from
c6ba6d8 to
c34186f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pkg/db/gatewayEnvelope_test.go (1)
142-142:⚠️ Potential issueUpdate assertion to match the new return type
Similar to the issue in
TestInsertAndIncrement, this test is failing becauseGetPayerUnsettledUsagenow returns a struct instead of anint64. Update the assertion to check theTotalSpendPicodollarsfield.-require.Equal(t, payerSpend, int64(100)) +require.Equal(t, payerSpend.TotalSpendPicodollars, int64(100))🧰 Tools
🪛 GitHub Actions: Test
[error] 142-142: TestInsertAndIncrementParallel failed: Not equal: expected: queries.GetPayerUnsettledUsageRow(queries.GetPayerUnsettledUsageRow{TotalSpendPicodollars:100, LastSequenceID:10}), actual: int64(100)
♻️ Duplicate comments (1)
pkg/db/gatewayEnvelope_test.go (1)
67-67:⚠️ Potential issueUpdate assertion to match the new return type
The test is failing because
GetPayerUnsettledUsagenow returns aqueries.GetPayerUnsettledUsageRowstruct instead of anint64. You need to update the assertion to check theTotalSpendPicodollarsfield.-require.Equal(t, payerSpend, int64(100)) +require.Equal(t, payerSpend.TotalSpendPicodollars, int64(100))🧰 Tools
🪛 GitHub Actions: Test
[error] 67-67: TestInsertAndIncrement failed: Not equal: expected: queries.GetPayerUnsettledUsageRow(queries.GetPayerUnsettledUsageRow{TotalSpendPicodollars:100, LastSequenceID:10}), actual: int64(100)
🧹 Nitpick comments (6)
pkg/db/queries/queries.sql.go (2)
246-270: Handle no rows found scenario gracefully.Currently, if
GetGatewayEnvelopeByIDquery returns no rows, the method simply returns ansql.ErrNoRowserror. Consider differentiating between “no envelope found” and other errors for clearer downstream handling. For instance, a customErrEnvelopeNotFoundcan help you avoid mixing up “not found” with genuine query failures.
367-397: Check feasibility of partial or specialized indexing forGetPayerUnsettledUsage.Because the query can filter by
payer_idand optionally byminutes_since_epochrange, you might see performance gains by adding an index on(payer_id, minutes_since_epoch)if you anticipate frequent large scans on this table.I can help generate a migration script to add a partial or composite index if desired.
pkg/payerreport/interface.go (1)
10-23: Plan to compute the Merkle root if needed.The
PayerReportstruct referencesPayersMerkleRootbut there's only a placeholder “fix me” in the code. Ensure that the logic for generating the merkle root is implemented or flagged as a separate future task to avoid shipping incomplete features.pkg/payerreport/manager.go (3)
29-83: Return a specialized response on insufficient data.When
endSequenceID == 0, you return an empty report. This is fine if it’s the desired behavior. However, consider logging a distinct message or returning a specialized error/flag, so callers know that the system lacked enough data to produce a fully meaningful report.
85-118: Separate error handling for missing envelope vs. other errors.
getStartMinutedoes not differentiate between “envelope not found” (sql.ErrNoRows) and actual query errors. If distinguishing these cases would be valuable, add logic to return a custom “not found” error, or default to a zero minute.
149-158: Handle invalid or unexpected payer addresses.
buildPayersMapdirectly converts strings to Ethereum addresses. If there's any invalid or unexpected address format from the database, this may cause errors. Consider gracefully handling or validating the address string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/db/gatewayEnvelope.go(1 hunks)pkg/db/gatewayEnvelope_test.go(2 hunks)pkg/db/queries.sql(2 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(7 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql(1 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(1 hunks)pkg/payerreport/manager_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/db/gatewayEnvelope.go
- pkg/db/queries/models.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql
- pkg/payerreport/manager_test.go
- pkg/testutils/envelopes/envelopes.go
- pkg/db/queries.sql
🧰 Additional context used
🧬 Code Definitions (2)
pkg/payerreport/manager.go (2)
pkg/payerreport/interface.go (2) (2)
PayerReportGenerationParams(35-39)PayerReport(10-23)pkg/db/queries/queries.sql.go (4) (4)
BuildPayerReportParams(30-34)GetGatewayEnvelopeByIDParams(252-255)GetSecondNewestMinuteParams(466-469)BuildPayerReportRow(36-39)
pkg/db/queries/queries.sql.go (2)
pkg/db/queries/models.go (1) (1)
GatewayEnvelope(27-34)pkg/db/types.go (1) (1)
Topic(10-10)
🪛 GitHub Actions: Test
pkg/db/gatewayEnvelope_test.go
[error] 67-67: TestInsertAndIncrement failed: Not equal: expected: queries.GetPayerUnsettledUsageRow(queries.GetPayerUnsettledUsageRow{TotalSpendPicodollars:100, LastSequenceID:10}), actual: int64(100)
[error] 142-142: TestInsertAndIncrementParallel failed: Not equal: expected: queries.GetPayerUnsettledUsageRow(queries.GetPayerUnsettledUsageRow{TotalSpendPicodollars:100, LastSequenceID:10}), actual: int64(100)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Upgrade Tests
🔇 Additional comments (7)
pkg/db/gatewayEnvelope_test.go (1)
152-191: Good implementation of the out-of-order sequence ID testThis test properly verifies that when an envelope with a lower sequence ID is inserted after one with a higher sequence ID, the system correctly maintains the highest sequence ID as the
LastSequenceID. This is important for ensuring data integrity, especially in distributed systems where messages might arrive out of order.The test follows the established pattern in the codebase and provides good coverage for this edge case. It aligns well with the PR objective of supporting the generation of payer reports, where tracking the correct sequence IDs is critical.
pkg/db/queries/queries.sql.go (3)
15-28: Consider verifying index coverage for filtering by originator/time.When running
SELECT SUM(spend_picodollars)fromunsettled_usagefiltered byoriginator_idandminutes_since_epoch, make sure you have an appropriate composite index (e.g.,(originator_id, minutes_since_epoch)) to avoid full-table scans. Large data volumes could degrade performance otherwise.Would you like me to provide an automated script to check for the presence of that composite index?
446-497: Ensure index coverage for originator/time fields.This query filters and groups by
(originator_id, minutes_since_epoch), and obtains a descending limit with offset. As recommended previously, confirm an index on(originator_id, minutes_since_epoch)to avoid an expensive table scan.
517-543: Verify concurrency handling forIncrementUnsettledUsage.While your
ON CONFLICTupdate logic appears correct for incrementing usage and ensuring the greatest sequence ID, consider whether multiple concurrent inserts for the same(payer_id, originator_id, minutes_since_epoch)might cause contention. If high concurrency is expected, review row-level locking or a queue-based approach to ensure consistent concurrency performance.pkg/payerreport/interface.go (1)
35-39: Validate usage ofNumHours.
NumHoursinPayerReportGenerationParamsis declared but not referenced in the current report generation logic. Confirm if you plan on leveraging it for time-based filtering, or if it’s extraneous and can be removed.pkg/payerreport/manager.go (2)
19-27: Constructor usage looks good.The
NewPayerReportManagerfunction properly initializes dependencies (logger and queries). No issues spotted here.
119-147: Ensure two distinct minutes for valid Payer Report generation.
getEndMinutelogic expects at least two minutes with envelopes to produce a result. Returning(0,0)if there’s only one minute is consistent with the PR requirement that at least two messages in distinct minutes are required. Just confirm the calling code can handle this edge case gracefully.
neekolas
force-pushed
the
03-18-generate_payer_reports
branch
2 times, most recently
from
1b89db0 to
de4ccfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/payerreport/interface.go (1)
10-23: Clarify boundaries and naming of sequence IDs.
It's stated that the report applies to messages with IDs greater thanStartSequenceIDand less than or equal toEndSequenceID, but the fields are named generically. Consider renamingStartSequenceIDto something likeLastProcessedSequenceIDor striking a clear doc comment to avoid off-by-one confusion.pkg/db/queries.sql (3)
193-194: Confirm indexing onlast_sequence_idusage.
You're now retrievingMAX(last_sequence_id). If you plan to filter or sort bylast_sequence_idover large tables, confirm that a suitable index is in place to avoid full-table scans in high-volume usage.
282-287: Verify results for absent matching records.
When no record matches the providedoriginator_sequence_idandoriginator_node_id, this query will fail if you expect exactly one row. Consider whether an error or a default return is appropriate, and handle it in the caller.
288-305: Potential latency in scanning for the second newest minute.
TheGetSecondNewestMinutequery involves sorting minutes in descending order. For wide time windows, you may want to ensure an index on(originator_id, minutes_since_epoch)to avoid scanning or consider a more direct approach (e.g., storing the timestamps in a separate table).pkg/payerreport/manager.go (2)
74-83: Highlight placeholder Merkle computation.
ThePayersMerkleRootfield is set to a placeholder. Ensure future changes properly compute and set Merkle-related fields before finalizing the feature.Do you want me to open a follow-up ticket or provide a reference implementation of a Merkle tree for payers?
123-147: Confirm logic for “second newest minute” data gap.
If there’s only one minute of data (or none), results can be zero-length, returning zero for both minute and sequence ID. Confirm that downstream code properly handles this scenario and that partial data doesn’t lead to an incomplete or misleading report.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/db/gatewayEnvelope.go(1 hunks)pkg/db/gatewayEnvelope_test.go(3 hunks)pkg/db/queries.sql(2 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(7 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql(1 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(1 hunks)pkg/payerreport/manager_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/db/gatewayEnvelope.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql
- pkg/db/queries/models.go
- pkg/testutils/envelopes/envelopes.go
- pkg/payerreport/manager_test.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql
- pkg/db/gatewayEnvelope_test.go
🧰 Additional context used
🧬 Code Definitions (1)
pkg/payerreport/manager.go (2)
pkg/payerreport/interface.go (2) (2)
PayerReportGenerationParams(35-39)PayerReport(10-23)pkg/db/queries/queries.sql.go (4) (4)
BuildPayerReportParams(30-34)GetGatewayEnvelopeByIDParams(252-255)GetSecondNewestMinuteParams(466-469)BuildPayerReportRow(36-39)
🔇 Additional comments (11)
pkg/payerreport/interface.go (1)
41-48: Ensure concurrency safety in IPayerReportManager methods.
If different goroutines may generate or attest reports simultaneously, please clarify concurrency requirements or ensure thread safety (e.g., with locks) in downstream code.pkg/db/queries.sql (2)
184-189: Validate upsert logic forlast_sequence_id.
UsingGREATEST(unsettled_usage.last_sequence_id, @sequence_id)ensures the highest sequence ID is stored, which is correct for monotonic sequence IDs. Confirm that all code paths respect this monotonic assumption so that no anomaly arises if sequence IDs ever decrease.
268-281: Check for large aggregator performance and potential indexes.
TheBuildPayerReportquery does aJOINonpayersand sumsspend_picodollars. For large data sets, consider verifying you have relevant indexes on(originator_id, minutes_since_epoch)to optimize grouping queries.pkg/payerreport/manager.go (3)
29-60: Revisit returning an empty report whenendSequenceIDis zero.
If there's insufficient data to form a useful report, returning an "empty" report might be intentional. However, confirm if upstream logic indeed wants to handle “empty” data gracefully or if an explicit error would be clearer.
92-117: Validate potential envelope parsing edge cases.
getStartMinuteparses envelope data usingNewOriginatorEnvelopeFromBytes. Confirm that untrusted or invalid envelope data is handled gracefully. Consider robust error-handling or fallback if part of the envelope is malformed.
149-157: Consider summing values in case of repeated payer rows.
buildPayersMapoverwrites the map entry if there are multiple rows for the same payer. If the query can return multiple rows for a single address under different conditions, you might need to sum them. Otherwise, confirm each payer is guaranteed unique per query results.pkg/db/queries/queries.sql.go (5)
15-62: LGTM: BuildPayerReport implementation looks goodThe new BuildPayerReport query efficiently aggregates total spending by payer address, with appropriate filtering and grouping logic. The implementation includes proper error handling and result scanning.
246-270: Effective use of primary key index in query designThe GetGatewayEnvelopeByID implementation correctly utilizes the primary key index by incorporating both originator_sequence_id and originator_node_id in the query, which will ensure good performance.
446-497: Consider monitoring performance of the complex CTE queryThe GetSecondNewestMinute implementation uses a CTE with order by, limit, and offset operations which may impact performance with large datasets. While the approach is correct for finding the second newest minute, consider monitoring query performance as data grows.
This query implements the approach mentioned in the PR objectives for determining the end sequence ID. Since this is likely an infrequent operation as mentioned in the PR description, the current implementation is acceptable, but worth monitoring.
367-396: LGTM: Enhanced return type with sequence ID trackingThe modification to include the last_sequence_id in the GetPayerUnsettledUsage query and its corresponding return type is well-implemented. This change supports the requirements for payer report generation.
517-543: LGTM: Updated IncrementUnsettledUsage with sequence ID trackingThe addition of the SequenceID parameter to IncrementUnsettledUsage and the corresponding SQL update using GREATEST() to maintain the highest sequence ID is a good approach. This ensures proper tracking of sequence IDs for report generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
pkg/db/gatewayEnvelope_test.go (1)
152-192: Recommend verifying total spend.This test successfully ensures that the last sequence ID remains the higher one after out-of-order insertion. However, it might also be worthwhile to confirm that the total spend correctly accumulates.
Below is a proposed diff to verify the total spend (assuming each inserted envelope adds 100):
func TestInsertAndIncrementWithOutOfOrderSequenceID(t *testing.T) { ... payerSpend, err := querier.GetPayerUnsettledUsage( ctx, queries.GetPayerUnsettledUsageParams{PayerID: payerID}, ) require.NoError(t, err) require.Equal(t, payerSpend.LastSequenceID, sequenceID) + require.Equal(t, payerSpend.TotalSpendPicodollars, int64(200), "Spend should reflect both envelopes") }pkg/payerreport/manager_test.go (3)
29-67: Flexible test utility with room for extension.
addEnvelopeis a handy helper. Consider adding an optional parameter for different spend picodollar amounts in future tests, rather than a hard-coded value.
73-98: Enhance coverage once Merkle root is implemented.
TestFirstReportchecks the base logic. Consider an additional assertion for thePayersMerkleRootonce it's no longer a placeholder.
99-121: Solid multiple-payments test.
TestReportWithMultiplePayerseffectively checks distribution across two payers. A further check for multiple originators might help ensure full coverage.pkg/db/queries.sql (2)
267-281: Aggregation for PayerReport.
BuildPayerReportprovides a neat summary of spending. Consider an index onoriginator_idif large data sets are expected for performance.
282-287: Ensure stable uniqueness.
GetGatewayEnvelopeByIDappears limited to a single row by the primary key, so aLIMIT 1isn't crucial. However, adding it might highlight that only one record is expected.pkg/db/queries/queries.sql.go (2)
15-62: Well-implemented payer report queryThe BuildPayerReport implementation correctly aggregates spend data by payer address and filters by originator ID and time range. This aligns with the PR objectives for generating Payer Reports.
Consider documenting the purpose and expected usage of this function with a comment similar to the one for GetBlocksInRange (lines 219-222).
446-497: GetSecondNewestMinute addresses sequence ID determinationThis query appears to implement the solution for determining the end sequence ID mentioned in the PR description, by finding the second newest minute for an originator and retrieving its maximum sequence ID.
The query is complex - consider adding a comment explaining its purpose, similar to how GetBlocksInRange is documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/db/gatewayEnvelope.go(1 hunks)pkg/db/gatewayEnvelope_test.go(3 hunks)pkg/db/queries.sql(2 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(7 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql(1 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(1 hunks)pkg/payerreport/manager_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql
- pkg/db/queries/models.go
- pkg/db/gatewayEnvelope.go
- pkg/payerreport/interface.go
- pkg/testutils/envelopes/envelopes.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql
🧰 Additional context used
🧬 Code Definitions (2)
pkg/payerreport/manager_test.go (5)
pkg/payerreport/manager.go (2) (2)
PayerReportManager(14-17)NewPayerReportManager(19-27)pkg/testutils/envelopes/envelopes.go (1) (1)
CreateOriginatorEnvelopeWithTimestamp(112-140)pkg/db/gatewayEnvelope.go (1) (1)
InsertGatewayEnvelopeAndIncrementUnsettledUsage(13-72)pkg/db/queries/queries.sql.go (2) (2)
InsertGatewayEnvelopeParams(603-610)IncrementUnsettledUsageParams(526-532)pkg/payerreport/interface.go (1) (1)
PayerReportGenerationParams(35-39)
pkg/payerreport/manager.go (2)
pkg/payerreport/interface.go (2) (2)
PayerReportGenerationParams(35-39)PayerReport(10-23)pkg/db/queries/queries.sql.go (4) (4)
BuildPayerReportParams(30-34)GetGatewayEnvelopeByIDParams(252-255)GetSecondNewestMinuteParams(466-469)BuildPayerReportRow(36-39)
🔇 Additional comments (23)
pkg/db/gatewayEnvelope_test.go (2)
67-68: Good usage of structured assertions.The updated checks for
payerSpend.TotalSpendPicodollarsandpayerSpend.LastSequenceIDaccurately verify the new fields returned byGetPayerUnsettledUsage.
142-143: Consistent validations of unsettled usage.These assertions properly confirm both the spending amount and the latest sequence ID in the parallel insertion test.
pkg/payerreport/manager_test.go (7)
1-18: Imports and package structure look good.No issues spotted; the chosen imports seem appropriate.
20-27: Concise setup function.This standardized pattern of creating a DB and cleaning up is clear.
69-71: Simple minute conversion.
getMinuteis straightforward and improves test readability.
123-139: Verifies empty scenario gracefully.
TestReportWithNoMessagescorrectly ensures the report is empty and contains no extraneous data.
141-159: Appropriate check for no new envelopes.
TestSecondReportWithNoMessageslogically confirms an unchanged state when no additional messages arrived.
161-196: Duplicate sequence ID usage warrants double-check.
TestSecondReportis robust, but note that both new envelopes (lines 183–185) use the same sequence ID (4). If intentional to simulate duplicates, good; otherwise, verify correctness.
198-221: Good negative coverage.
TestReportWithNoEnvelopesFromOriginatorcorrectly ensures we exclude data from other originators.pkg/db/queries.sql (4)
184-185: Check default for last_sequence_id.By inserting
@sequence_iddirectly, ensure the schema or code provides a default of 0 whensequence_idis not specified.
188-189: Correct handling of an out-of-order sequence.Using
GREATEST(unsettled_usage.last_sequence_id, @sequence_id)prevents lower values from overwriting the stored sequence ID.
193-194: Newly added MAX for last_sequence_id.Collecting both
SUM(spend_picodollars)andMAX(last_sequence_id)inGetPayerUnsettledUsageis consistent. Verify that all consumers of this query handle the two fields properly.
288-305: SecondNewestMinute logic is coherent.Using an offset of 1 to get the second-latest minute is effective. The early return for an empty result set handles corner cases well.
pkg/payerreport/manager.go (7)
1-13: Overall structure and imports are appropriate.No issues with package and imports setup.
14-17: Manager holds essential references.
PayerReportManagercaptures both the logger and query set, aligning well with typical dependency injection patterns.
19-27: Constructor is straightforward.
NewPayerReportManagerneatly wires dependencies and improves testability.
29-83: Well-structured report generation.
GenerateReportcoordinates start/end minute retrieval and aggregates results. The early return onendSequenceID == 0gracefully handles insufficient envelopes.
85-118: Robust retrieval of start minute.
getStartMinutegracefully falls back to 0 ifsequenceIDis 0, and otherwise locates the relevant envelope. Good defensive coding.
119-147: Offset-based retrieval for end minute.
getEndMinuteusesGetSecondNewestMinuteto define the reporting boundary, returning zero if there's insufficient data. This ensures partial or insufficient data doesn't break the process.
149-157: Efficient construction of payer map.
buildPayersMapis concise; converting addresses and sums is done neatly.pkg/db/queries/queries.sql.go (3)
246-270: Good performance consideration with primary key indexNice work including the comment about using the node ID to take advantage of the primary key index. This demonstrates thoughtful query design.
367-397: Enhanced GetPayerUnsettledUsage to include sequence IDThe modification to include the last_sequence_id in the query result is a good addition. This change helps track the latest processed message for each payer and supports the report generation requirements.
517-543: Smart update to IncrementUnsettledUsageGood job using the GREATEST function to ensure last_sequence_id only increases. This prevents potential data regression and maintains data integrity.
This enhancement will correctly track the sequence IDs needed for the payer reports.
neekolas
force-pushed
the
03-18-generate_payer_reports
branch
from
de4ccfe to
10f84bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/payerreport/manager.go (2)
14-17: Consider validating injected dependencies to prevent nil reference errors.
ThePayerReportManagerstruct stores references tologandquerieswithout any nil checks, which could lead to unexpected runtime errors if they are not properly initialized. Adding a simple validation step in the constructor can improve resilience.
48-83: Empty report return and Merkle root TODO.
Returning an empty report whenendSequenceID == 0aligns with the user’s requirement of requiring two distinct minutes for a valid report. However:
- Merkle root is currently a placeholder. Would you like to open a ticket or accept a contribution to implement its calculation?
- Start sequence ID is set to the last report’s end sequence ID. Confirm if this is intentional or if it should derive from the actual envelope for clarity in the generated report.
pkg/db/queries/queries.sql.go (1)
246-270: Consider handling “not found” rows explicitly.
When the row is missing, the call to.Scan()will yield a SQL error. It may be beneficial to return a custom “envelope not found” error to differentiate from other database issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pkg/db/gatewayEnvelope.go(1 hunks)pkg/db/gatewayEnvelope_test.go(3 hunks)pkg/db/queries.sql(2 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(7 hunks)pkg/db/unsettledUsage_test.go(4 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql(1 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(1 hunks)pkg/payerreport/manager_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/db/gatewayEnvelope.go
- pkg/db/queries/models.go
- pkg/payerreport/interface.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql
- pkg/testutils/envelopes/envelopes.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql
🧰 Additional context used
🧬 Code Definitions (3)
pkg/payerreport/manager_test.go (5)
pkg/payerreport/manager.go (2) (2)
PayerReportManager(14-17)NewPayerReportManager(19-27)pkg/testutils/envelopes/envelopes.go (1) (1)
CreateOriginatorEnvelopeWithTimestamp(112-140)pkg/db/gatewayEnvelope.go (1) (1)
InsertGatewayEnvelopeAndIncrementUnsettledUsage(13-72)pkg/db/queries/queries.sql.go (2) (2)
InsertGatewayEnvelopeParams(603-610)IncrementUnsettledUsageParams(526-532)pkg/payerreport/interface.go (1) (1)
PayerReportGenerationParams(35-39)
pkg/payerreport/manager.go (2)
pkg/payerreport/interface.go (2) (2)
PayerReportGenerationParams(35-39)PayerReport(10-23)pkg/db/queries/queries.sql.go (4) (4)
BuildPayerReportParams(30-34)GetGatewayEnvelopeByIDParams(252-255)GetSecondNewestMinuteParams(466-469)BuildPayerReportRow(36-39)
pkg/db/queries/queries.sql.go (4)
pkg/db/queries/db.go (2) (2)
q(27-31)Queries(23-25)pkg/db/queries/models.go (1) (1)
GatewayEnvelope(27-34)pkg/db/types.go (1) (1)
Topic(10-10)pkg/utils/time.go (1) (1)
MinutesSinceEpoch(5-9)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Upgrade Tests
🔇 Additional comments (30)
pkg/db/gatewayEnvelope_test.go (3)
67-68: Return type adjustment correctly implementedThe code now properly asserts against the struct fields returned by
GetPayerUnsettledUsage, checking both theTotalSpendPicodollarsandLastSequenceIDfields.
142-143: Consistent assertion updates in parallel test caseThe same assertions have been properly updated in the parallel test case to match the modified return type.
153-192: Good test for out-of-order sequence ID handlingThis new test verifies an important edge case - ensuring that when envelopes arrive out of order, the system correctly maintains the highest sequence ID seen rather than blindly updating to the latest one received.
The test effectively validates that:
- An envelope with sequence ID 10 is processed first
- A later envelope with a lower sequence ID 5 is processed second
- The last sequence ID remains at 10 (the highest value)
This aligns with the PR objective to track the last known sequence ID for each originator/payer combination.
pkg/db/unsettledUsage_test.go (4)
37-37: Correctly updated assertion for modified return typeThe assertion has been updated to check the
TotalSpendPicodollarsfield, consistent with the change in the return type structure ofGetPayerUnsettledUsage.
53-53: Consistent assertion update for second test caseSimilar to the previous assertion, this one has been properly updated to match the modified return type.
89-89: Updated assertion in filtered query testThe assertion for the filtered query case has been correctly updated to check the
TotalSpendPicodollarsfield.
99-99: Final assertion correctly updatedAll assertions in this file have been consistently updated to check the
TotalSpendPicodollarsfield from the returned struct.pkg/payerreport/manager_test.go (9)
20-27: Well-structured setup function for test casesThe setup function creates a clean test environment, properly handles cleanup, and initializes the PayerReportManager. It follows good testing practices by using
t.Cleanup()to ensure resources are released properly.
29-67: Comprehensive helper function for adding test dataThe
addEnvelopefunction handles all the necessary steps to add test data:
- Creates a payer
- Creates an envelope with the specified timestamp
- Marshals the envelope
- Inserts the envelope and increments the unsettled usage
This approach reduces code duplication and makes the test cases more readable.
69-71: Useful helper function for timestamp conversionThe
getMinutefunction provides a clean way to generate timestamps based on minute offsets from the epoch, making the test cases more readable.
73-97: Thorough test for first report generationThis test case effectively verifies that:
- The report correctly identifies the originator
- The payer count matches expectations
- The end sequence ID reflects the highest sequence in the previous minute
- The total spend amount for the payer is calculated correctly
This aligns with the PR objective of generating payer reports with at least two messages in distinct minutes.
99-121: Comprehensive test for multiple payers scenarioThis test effectively verifies that the payer report correctly represents data from multiple payers, including:
- Accurate payer count
- Correct end sequence ID
- Accurate spend amounts for each payer
This test provides good coverage for a common real-world scenario.
123-139: Proper edge case handling for no messagesThis test verifies that the report handles the edge case of no messages correctly, with:
- Zero payer count
- Zero sequence IDs
- Empty payer map
Testing edge cases is important for ensuring robust behavior.
141-159: Good test for second report with no new messagesThis test verifies that when generating a second report with no new messages, the report:
- Shows zero new payers
- Maintains the correct sequence ID range (start = end = last report's end ID)
This test is important for verifying proper continuity between reports.
161-196: Complete test for second report with new messagesThis test thoroughly verifies the generation of a second report after adding more envelopes:
- First report establishes the baseline
- More envelopes are added
- Second report is generated using the end sequence ID from the first report
- Assertions verify that the second report correctly includes only new activity
This is a critical test for ensuring the incremental nature of reports works correctly.
198-221: Important test for originator isolationThis test verifies that reports for one originator do not include data from other originators, which is crucial for data integrity and security. The test ensures:
- Envelopes from another originator are added
- A report for the target originator shows zero data
This isolation test helps ensure data boundaries are properly maintained.
pkg/db/queries.sql (5)
184-189: Well-implemented sequence ID tracking in unsettled usageThe
IncrementUnsettledUsagequery has been updated to:
- Store the sequence ID in a new
last_sequence_idcolumn- Use the
GREATESTfunction to keep track of the highest sequence ID seenThis implementation ensures that even if messages arrive out of order, the system will always maintain the highest sequence ID for each originator/payer combination within a minute, which is critical for accurate report generation.
193-194: Updated query to return sequence ID informationThe
GetPayerUnsettledUsagequery now returns both the total spend and the last sequence ID, which is necessary for the report generation process. TheCOALESCEfunctions properly handle the case where no data exists.
268-280: Well-structured query for building payer reportsThe
BuildPayerReportquery:
- Joins the
unsettled_usagetable with thepayerstable to get payer addresses- Properly filters by originator ID and time range
- Aggregates spend by payer
- Returns data in the format needed for report generation
This query effectively supports the PR objective of generating payer reports.
282-286: Efficient query for retrieving gateway envelopesThe
GetGatewayEnvelopeByIDquery retrieves gateway envelopes by sequence ID and node ID, with a helpful comment noting that including the node ID allows the query to take advantage of the primary key index for better performance.
288-305: Complex but well-structured query for finding the second newest minuteThe
GetSecondNewestMinutequery:
- Uses a CTE to find the second newest minute for a specific originator
- Retrieves the maximum sequence ID and minute for that time period
- Uses
COALESCEto handle the case where no data existsThis query is important for implementing the requirement that a minimum of two messages, each originating in distinct minutes, are needed to generate a report. The query's complexity is justified by the specific requirements it addresses.
pkg/payerreport/manager.go (5)
19-27: Constructor structure is clear and cohesive.
TheNewPayerReportManagerneatly initializes the logger and queries, keeping responsibilities well-defined. No issues identified here.
29-47: Unused parameter detected in report generation.
ThePayerReportGenerationParamsstruct includes aNumHoursfield, butGenerateReportnever uses it. This might be an oversight or a future placeholder. Please verify whether it's intended to filter or constrain the report generation time frame.
92-117: Verify the correctness of returning minute 0 for sequenceID = 0.
The logic defaults to minute 0 when the sequence ID is zero. Ensure there are no envelopes at or before minute 0 that might create ambiguity. A quick check or documentation note clarifying the assumption could help.
123-147: Second newest minute retrieval meets the two-minute requirement.
This helper successfully enforces the logic needed for generating a valid report only after two distinct minutes. Implementation details and error handling look solid.
149-157: Map builder neatly populates Payer data.
The straightforward transformation from query rows to a map is clean and efficient. No issues detected.pkg/db/queries/queries.sql.go (4)
30-62: SQL execution for building payer report is well-structured.
The method definition and parameter scanning are handled cleanly, with proper error checks and resource closure.
367-397: Aggregation logic for payer unsettled usage is correct.
Summingspend_picodollarsand taking the maximumlast_sequence_idensures complete usage data. Implementation is straightforward.
446-497: Retrieving the second newest minute matches the two-minute requirement.
TheWITHclause and offset approach accurately isolate the second latest minute, satisfying the PR objective for generating valid reports only after two distinct minutes of data.
517-543: Conflict resolution approach inincrementUnsettledUsageis correct.
Relying onON CONFLICTandGREATESTensures that the maximum sequence ID is preserved appropriately across concurrent inserts. This fits well with the real-time nature of usage data updates.
neekolas
force-pushed
the
03-18-generate_payer_reports
branch
from
10f84bb to
900622b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/payerreport/interface.go (1)
35-39: Consider time-based parameters for report generation.The
PayerReportGenerationParamsuses sequence IDs and hours for boundary definition. While this works, consider if explicit timestamp parameters might offer more flexibility in certain reporting scenarios.type PayerReportGenerationParams struct { OriginatorID uint32 LastReportEndSequenceID uint64 NumHours int + // Optional explicit time boundaries that override NumHours when provided + StartTime *time.Time + EndTime *time.Time }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pkg/db/gatewayEnvelope.go(1 hunks)pkg/db/gatewayEnvelope_test.go(3 hunks)pkg/db/queries.sql(2 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(7 hunks)pkg/db/unsettledUsage_test.go(4 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql(1 hunks)pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(1 hunks)pkg/payerreport/manager_test.go(1 hunks)pkg/testutils/envelopes/envelopes.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/db/queries/models.go
- pkg/db/gatewayEnvelope.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.down.sql
- pkg/testutils/envelopes/envelopes.go
- pkg/db/gatewayEnvelope_test.go
- pkg/db/unsettledUsage_test.go
- pkg/payerreport/manager_test.go
- pkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql
- pkg/db/queries.sql
- pkg/payerreport/manager.go
🧰 Additional context used
🧬 Code Definitions (1)
pkg/db/queries/queries.sql.go (4)
pkg/db/queries/db.go (2) (2)
q(27-31)Queries(23-25)pkg/db/queries/models.go (1) (1)
GatewayEnvelope(27-34)pkg/db/types.go (1) (1)
Topic(10-10)pkg/utils/time.go (1) (1)
MinutesSinceEpoch(5-9)
🔇 Additional comments (9)
pkg/db/queries/queries.sql.go (6)
15-62: Ensure index coverage for efficient payer report generation.The
BuildPayerReportquery filters onoriginator_idand applies a range condition onminutes_since_epoch. To prevent full table scans on large datasets, an index on(originator_id, minutes_since_epoch)should be added to theunsettled_usagetable.This is especially important for reporting functionality which typically processes large amounts of data.
#!/bin/bash # Check if an index exists for the originator_id and minutes_since_epoch columns rg -A 3 'CREATE\s+INDEX.*unsettled_usage.*originator_id.*minutes_since_epoch' || \ rg -A 3 'CREATE\s+INDEX.*unsettled_usage.*minutes_since_epoch.*originator_id'
247-271: Good implementation with optimized index usage.The function correctly uses the node ID to take advantage of the primary key index, as indicated by the comment on line 259. This ensures efficient retrieval of gateway envelopes from the database.
368-397: Extend return type to capture sequence ID information.The function has been correctly modified to return both the total spend and the last sequence ID, which is essential for tracking payer usage progression and report generation.
502-509: Track sequence IDs efficiently with GREATEST comparison.The update now correctly maintains the maximum sequence ID seen for each payer-originator-minute combination using the
GREATESTfunction. This is crucial for ensuring accurate report generation and establishing the boundaries for each reporting period.
511-526: Include sequence ID in usage tracking parameters.The
IncrementUnsettledUsageParamsstruct has been extended to include the sequence ID, which aligns with the updated database schema and ensures proper tracking of message progression.
447-482:✅ Verification successful
Verify query performance with nested subqueries.
The
GetSecondNewestMinutequery uses a WITH clause and joins to identify the second newest minute. While functionally correct, this approach with ordering, limiting, and joining could become inefficient on large datasets.
🏁 Script executed:
#!/bin/bash # Check if the unsettled_usage table has an index that would optimize this query rg -A 3 'CREATE\s+INDEX.*unsettled_usage.*minutes_since_epoch'Length of output: 332
Performance Verification Complete – Index in Place
- The
unsettled_usagetable has the indexidx_unsettled_usage_originator_id_minutes_since_epoch(located inpkg/migrations/00010_add-last-sequence-id-to-unsettled-usage.up.sql), which optimizes both the filtering byoriginator_idand the ordering onminutes_since_epoch.- Given this index, the nested WITH clause and JOIN in the
GetSecondNewestMinutequery should perform efficiently even with the ordering, limiting, and OFFSET operations.- No immediate changes are required; however, it's advised to monitor the query performance periodically as the dataset grows.
pkg/payerreport/interface.go (3)
10-23: Well-structured data model for payer reporting.The
PayerReportstruct is well-designed with clear field documentation. It captures all necessary information for tracking payer contributions, including sequence ID ranges and Merkle tree data for potential verification.
25-33: Good separation of concerns for report attestation.The
NodeSignatureandPayerReportAttestationstructs provide a clean separation between the report data and the cryptographic proof of its authenticity. This design allows for flexible verification mechanisms.
41-48: Interface design follows good separation principles.The
IPayerReportManagerinterface is well-defined with context support for cancellation and deadline propagation. The separation between generation and attestation allows for easier testing and flexible implementation.
| if incrementParams.SequenceID == 0 { | ||
| incrementParams.SequenceID = insertParams.OriginatorSequenceID | ||
| } |
There was a problem hiding this comment.
nit: do we need the equals 0 check here? It seems like we'd want to enforce it always to OrigSeqId.
tl;dr
unsettled_usagetable to keep track of the last known sequence ID for each originator/payer combo in a given minute. This makes it cheap to look up when generating a Payer ReportNotes
Minimum Report Size
Tickets
Implements part of #514
Summary by CodeRabbit
New Features
Bug Fixes
Tests