Add payer reports table #703
Conversation
WalkthroughThis pull request refactors existing SQL queries in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PayerReportStore
participant Database
Client->>PayerReportStore: StoreReport(report)
PayerReportStore->>Database: InsertOrIgnorePayerReport SQL query
Database-->>PayerReportStore: Insert result (or conflict resolution)
PayerReportStore-->>Client: ReportID or error
Client->>PayerReportStore: StoreAttestation(attestation)
PayerReportStore->>Database: InsertOrIgnorePayerReportAttestation SQL query
Database-->>PayerReportStore: Insert result (or conflict resolution)
PayerReportStore-->>Client: success or error
Client->>PayerReportStore: FetchReport(reportID)
PayerReportStore->>Database: FetchPayerReport SQL query
Database-->>PayerReportStore: PayerReport data
PayerReportStore-->>Client: PayerReport or error
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 Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ 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 (
|
|
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.
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 (2)
pkg/migrations/00011_store-payer-reports.up.sql (2)
1-12: Validate 'payer_reports' table schema design.
The table schema is clear and meets the reporting requirements. As a nitpick, verify that the use of the BYTEA data type for the primary key (id) is intentional and consider whethercreated_atshould be timezone-aware using TIMESTAMPTZ for consistency.
24-24: Consider assigning explicit names to indexes.
While the index onpayer_report_attestations (payer_report_id)is correctly set up, explicitly naming indexes (e.g., usingCREATE INDEX idx_payer_report_attestations_payer_report_id ON payer_report_attestations(payer_report_id);) can improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/db/sqlc/payerReports.sql(1 hunks)pkg/migrations/00011_store-payer-reports.down.sql(1 hunks)pkg/migrations/00011_store-payer-reports.up.sql(1 hunks)
⏰ 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: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
🔇 Additional comments (10)
pkg/migrations/00011_store-payer-reports.up.sql (2)
14-14: Index creation on submission_status.
The index onsubmission_statusis appropriate for optimizing queries filtering by report status.
16-22: Define 'payer_report_attestations' table with proper constraints.
The table structure is well-defined with a composite primary key and a foreign key referencingpayer_reports. The use ofON DELETE CASCADEensures referential integrity upon deletion.pkg/db/sqlc/payerReports.sql (8)
1-7: 'FindOrCreatePayer' upsert statement review.
The SQL upsert pattern is clearly formatted. Just ensure that a unique constraint or index onaddressexists in thepayerstable to support theON CONFLICTclause effectively.
8-26: Incrementing usage with upsert logic.
TheIncrementUnsettledUsagequery uses an effective upsert pattern with proper aggregation usingGREATEST. The formatting improvements enhance readability.
27-40: Aggregation query in 'GetPayerUnsettledUsage'.
The query effectively usesCOALESCEto handle nulls and provides a clear aggregation of values.
41-50: 'BuildPayerReport' reporting query review.
The join betweenunsettled_usageandpayersalong with the grouping is clear and appropriately structured for generating the report.
51-56: Review of 'GetGatewayEnvelopeByID'.
The query is concise with a consolidated WHERE clause. The inline comment about leveraging the primary key index is helpful.
57-72: Effective use of CTE in 'GetSecondNewestMinute'.
The common table expression (CTE) neatly encapsulates querying the second newest minute. The use of LIMIT and OFFSET appears appropriate for this purpose.
73-92: Insertion command 'InsertOrIgnorePayerReport'.
The statement properly handles insertions intopayer_reportsby usingON CONFLICT DO NOTHINGto avoid duplicate entries. All required columns are supplied while relying on table defaults (e.g., forcreated_at).
93-95: Insertion command 'InsertPayerReportAttestation'.
The command effectively avoids inserting duplicate attestations with the composite key (payer_report_id,node_id) by usingON CONFLICT DO NOTHING.
| DROP TABLE IF EXISTS payer_reports; | ||
|
|
||
| DROP TABLE IF EXISTS payer_report_attestations; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Reorder DROP statements to respect foreign key dependencies.
Since payer_report_attestations references payer_reports, dropping the parent table first might cause a dependency error. It is recommended to drop the child table first.
Proposed diff:
- DROP TABLE IF EXISTS payer_reports;
-
- DROP TABLE IF EXISTS payer_report_attestations;
+ DROP TABLE IF EXISTS payer_report_attestations;
+
+ DROP TABLE IF EXISTS payer_reports;Action Required: Reorder DROP Statements in Migration File
The current migration file contains the following statements:
DROP TABLE IF EXISTS payer_reports;
DROP TABLE IF EXISTS payer_report_attestations;Since the table payer_report_attestations references payer_reports, dropping the parent table (payer_reports) first can trigger a dependency error. Please update the file by dropping the child table first. The corrected order should be:
- DROP TABLE IF EXISTS payer_reports;
-
- DROP TABLE IF EXISTS payer_report_attestations;
+ DROP TABLE IF EXISTS payer_report_attestations;
+
+ DROP TABLE IF EXISTS payer_reports;
neekolas
force-pushed
the
04-04-add_support_for_new_envelope_types
branch
from
7d1bea7 to
79ffefb
Compare
neekolas
force-pushed
the
04-04-add_payer_reports_table
branch
from
39ab894 to
49c4502
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| EndSequenceId: p.EndSequenceID, | ||
| PayersMerkleRoot: p.PayersMerkleRoot[:], | ||
| PayersLeafCount: p.PayersLeafCount, | ||
| } |
There was a problem hiding this comment.
In the ToProto function, the NodesHash and NodesCount fields of the PayerReport struct are not being mapped to the corresponding NodesMerkleRoot and NodesLeafCount fields in the proto message. If these node details are intended to be included in the proto representation, consider mapping them accordingly. If this was an intentional omission, please consider documenting why these fields are omitted.
+ NodesMerkleRoot: p.NodesHash[:],
+ NodesLeafCount: p.NodesCount,🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
pkg/payerreport/manager.go (1)
51-63: Indicate when using placeholder data.The function returns a random merkle root as a placeholder, but doesn't clearly indicate this in the returned structure. Consider adding a flag or field to indicate when using non-production ready placeholder values.
return &FullPayerReport{ PayerReport: PayerReport{ OriginatorNodeID: uint32(originatorID), StartSequenceID: params.LastReportEndSequenceID, EndSequenceID: params.LastReportEndSequenceID, // TODO: Implement merkle calculation PayersMerkleRoot: randomBytes32(), PayersLeafCount: uint32(0), + IsPlaceholder: true, // Indicate this contains placeholder data }, Payers: make(map[common.Address]currency.PicoDollar), }, nilpkg/payerreport/report.go (1)
29-34: Add method to convert from protobuf to Go struct.For complete serialization support, consider adding a static method to convert from protobuf message back to
FullPayerReportorPayerReportstruct.// FromProto creates a PayerReport from its protobuf representation func PayerReportFromProto(proto *proto.PayerReport) (*PayerReport, error) { if proto == nil { return nil, fmt.Errorf("nil protobuf message") } var payersMerkleRoot, nodesHash [32]byte // Handle potential size mismatch if len(proto.PayersMerkleRoot) != 32 { return nil, fmt.Errorf("invalid payers merkle root size: %d", len(proto.PayersMerkleRoot)) } copy(payersMerkleRoot[:], proto.PayersMerkleRoot) // If nodesHash is present in proto if proto.NodesHash != nil { if len(proto.NodesHash) != 32 { return nil, fmt.Errorf("invalid nodes hash size: %d", len(proto.NodesHash)) } copy(nodesHash[:], proto.NodesHash) } return &PayerReport{ OriginatorNodeID: proto.OriginatorNodeId, StartSequenceID: proto.StartSequenceId, EndSequenceID: proto.EndSequenceId, PayersMerkleRoot: payersMerkleRoot, PayersLeafCount: proto.PayersLeafCount, NodesHash: nodesHash, NodesCount: proto.NodesCount, }, nil }pkg/payerreport/interface.go (1)
31-36: Consider updatingAttestReportto useFullPayerReport.The
AttestReportmethod parameters use*PayerReportwhileGenerateReportreturns*FullPayerReport. For consistency, consider updating the interface to use the same types, or document why different types are used.type PayerReportAttester interface { AttestReport( ctx context.Context, - prevReport *PayerReport, - newReport *PayerReport, + prevReport *FullPayerReport, + newReport *FullPayerReport, ) (*PayerReportAttestation, error) }pkg/migrations/00011_store-payer-reports.up.sql (1)
1-13: Consider using an enum type for submission_status.The
submission_statusfield uses numeric values (0, 1, 2) with a comment explaining their meaning. Using a PostgreSQL enum type would make this more self-documenting and prevent invalid states.+CREATE TYPE payer_report_status AS ENUM ('pending', 'submitted', 'settled'); + CREATE TABLE payer_reports ( id BYTEA PRIMARY KEY, originator_node_id INT NOT NULL, start_sequence_id BIGINT NOT NULL, end_sequence_id BIGINT NOT NULL, payers_merkle_root BYTEA NOT NULL, payers_leaf_count BIGINT NOT NULL, nodes_hash BYTEA NOT NULL, nodes_count INT NOT NULL, - -- 0 = pending, 1 = submitted, 2 = settled - submission_status SMALLINT NOT NULL DEFAULT 0, + submission_status payer_report_status NOT NULL DEFAULT 'pending', created_at TIMESTAMP DEFAULT NOW() );pkg/payerreport/store.go (2)
37-45: Consider using constants for clarityWhile the current validation logic is correct, using named constants for the maximum values would improve code readability.
- // The originator node ID is stored as an int32 in the database, but - // a uint32 on the network. Do not allow anything larger than max int32 - if report.OriginatorNodeID > math.MaxInt32 { - return nil, ErrOriginatorNodeIDTooLarge - } - - if report.NodesCount > math.MaxInt32 { - return nil, ErrNodesCountTooLarge - } + // The originator node ID and nodes count are stored as int32 in the database, but + // as uint32 in the domain model. Validate they don't exceed max int32 + const maxDBInt32 = math.MaxInt32 + + if report.OriginatorNodeID > maxDBInt32 { + return nil, ErrOriginatorNodeIDTooLarge + } + + if report.NodesCount > maxDBInt32 { + return nil, ErrNodesCountTooLarge + }
85-92: Consider consolidating error handling for array conversionsThere's some repetition in the error handling for SliceToArray32 conversions. Consider using a helper function to reduce duplication.
- var payersMerkleRoot [32]byte - var nodesHash [32]byte - if payersMerkleRoot, err = utils.SliceToArray32(report.PayersMerkleRoot); err != nil { - return nil, err - } - if nodesHash, err = utils.SliceToArray32(report.NodesHash); err != nil { - return nil, err - } + // Helper function to convert slice to array with error handling + convertToArray32 := func(slice []byte) ([32]byte, error) { + return utils.SliceToArray32(slice) + } + + payersMerkleRoot, err := convertToArray32(report.PayersMerkleRoot) + if err != nil { + return nil, err + } + + nodesHash, err := convertToArray32(report.NodesHash) + if err != nil { + return nil, err + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dev/gen/sqlcis excluded by!**/gen/**
📒 Files selected for processing (13)
pkg/db/queries/models.go(1 hunks)pkg/db/queries/payerReports.sql.go(7 hunks)pkg/db/sqlc/payerReports.sql(1 hunks)pkg/migrations/00011_store-payer-reports.down.sql(1 hunks)pkg/migrations/00011_store-payer-reports.up.sql(1 hunks)pkg/payerreport/id.go(1 hunks)pkg/payerreport/interface.go(1 hunks)pkg/payerreport/manager.go(5 hunks)pkg/payerreport/report.go(1 hunks)pkg/payerreport/report_test.go(1 hunks)pkg/payerreport/store.go(1 hunks)pkg/payerreport/store_test.go(1 hunks)pkg/utils/slice.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/payerreport/id.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/migrations/00011_store-payer-reports.down.sql
- pkg/db/sqlc/payerReports.sql
🧰 Additional context used
🧬 Code Graph Analysis (5)
pkg/payerreport/manager.go (4)
pkg/payerreport/interface.go (1)
PayerReportGenerationParams(17-21)pkg/payerreport/report.go (2)
FullPayerReport(29-34)PayerReport(12-27)pkg/db/queries/models.go (1)
PayerReport(64-75)pkg/currency/currency.go (1)
PicoDollar(10-10)
pkg/payerreport/store.go (6)
pkg/db/queries/db.go (2)
New(19-21)Queries(23-25)pkg/db/queries/models.go (2)
PayerReport(64-75)PayerReportAttestation(77-82)pkg/payerreport/report.go (2)
PayerReport(12-27)ReportID(10-10)pkg/db/queries/payerReports.sql.go (2)
InsertOrIgnorePayerReportParams(251-260)InsertOrIgnorePayerReportAttestationParams(281-285)pkg/payerreport/interface.go (2)
PayerReportAttestation(12-15)NodeSignature(7-10)pkg/utils/slice.go (1)
SliceToArray32(9-17)
pkg/payerreport/report.go (3)
pkg/db/queries/models.go (1)
PayerReport(64-75)pkg/currency/currency.go (1)
PicoDollar(10-10)pkg/utils/hash.go (1)
HashPayerReportInput(35-37)
pkg/payerreport/interface.go (2)
pkg/payerreport/report.go (3)
FullPayerReport(29-34)PayerReport(12-27)ReportID(10-10)pkg/db/queries/models.go (2)
PayerReport(64-75)PayerReportAttestation(77-82)
pkg/db/queries/payerReports.sql.go (3)
pkg/db/queries/db.go (1)
Queries(23-25)pkg/db/queries/models.go (1)
PayerReport(64-75)pkg/payerreport/report.go (1)
PayerReport(12-27)
🪛 GitHub Check: Lint-Go
pkg/utils/slice.go
[failure] 5-5:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Lint
pkg/utils/slice.go
[error] 5-5: File is not properly formatted (gofumpt)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Code Review
🔇 Additional comments (25)
pkg/utils/slice.go (1)
9-17: LGTM: Good enhancement with input validationThe updated function now properly validates that the input slice is exactly 32 bytes long before proceeding with the copy operation. This is a good improvement for error handling and ensures safety when dealing with fixed-size byte arrays.
pkg/payerreport/report_test.go (1)
14-16: LGTM: Updated field names and test data generationThe test has been updated to use the
randomBytes32()helper function for generating test data instead of fixed byte slices, and uses the correct field nameNodesHashwhich aligns with the updated struct definition.pkg/db/queries/models.go (2)
64-75: LGTM: Well-structured PayerReport modelThe new
PayerReportstruct correctly defines all the fields needed to represent a payer report in the database, with appropriate types for each field, including the properly sized ID, merkle roots, and timestamps.
77-82: LGTM: Well-structured PayerReportAttestation modelThe new
PayerReportAttestationstruct correctly models the relationship between attestations and payer reports using thePayerReportIDfield as a foreign key reference, along with appropriate fields for node identification, signatures, and timestamps.pkg/payerreport/store_test.go (3)
13-19: LGTM: Good test setup helperThe
createTestStorehelper function properly sets up a test environment with a new database and logger, and ensures cleanup after the test completes.
21-84: LGTM: Comprehensive test cases for storing and retrieving reportsThis test function covers both valid and invalid scenarios:
- Testing a valid report with proper field values
- Testing validation failures for invalid node IDs that exceed int32 range
- Testing validation failures for invalid node counts that exceed int32 range
The test properly verifies that stored reports can be fetched and that they match the original data.
86-110: LGTM: Effective idempotency testThis test effectively verifies that storing the same report multiple times returns the same report ID, confirming the idempotent behavior of the store operation. The test correctly:
- Generates a report ID directly from the report
- Stores the report and verifies the returned ID matches
- Stores the same report again and verifies the returned ID is the same
This ensures that duplicate reports won't be stored multiple times in the database.
pkg/payerreport/manager.go (2)
57-59: Create tracking issue for TODO item.There's a TODO comment for implementing merkle calculation without a linked ticket or issue number, which risks it being forgotten.
Consider adding a specific issue number to track this implementation work:
- // TODO: Implement merkle calculation + // TODO(#xxx): Implement merkle calculation
25-25: Change to named logger is a good practice.Using a named logger helps with log filtering and organization, making it easier to identify the source of log messages.
pkg/payerreport/interface.go (2)
23-28: Good interface refactoring.Breaking out the
PayerReportGeneratorinterface from the broaderIPayerReportManagerimproves modularity and separation of concerns. This makes the code more maintainable and testable.
43-47: Well-designed storage interface.The
IPayerReportStoreinterface provides a clean abstraction for storage operations with clear method signatures. The separation of storage concerns from business logic is a good architectural practice.pkg/migrations/00011_store-payer-reports.up.sql (1)
26-26: Good index creation for frequent queries.Creating an index on
payer_report_idin the attestations table is a good practice for optimizing queries that filter or join on this field.pkg/db/queries/payerReports.sql.go (8)
57-61: Good addition of a new FetchPayerReport queryThe new SQL query correctly selects all necessary fields from the payer_reports table with appropriate filtering by ID.
63-79: Implementation looks correctThe function properly queries the database and scans all columns from the payer_reports table into the PayerReport struct.
228-249: Well-designed insert query with conflict handlingThe SQL query correctly implements an insert-or-ignore pattern using the ON CONFLICT DO NOTHING clause, which prevents duplicate entries and makes the operation idempotent.
251-260: Parameter struct matches database schemaThe InsertOrIgnorePayerReportParams struct correctly defines all required fields with appropriate types for the database schema.
262-274: Implementation looks correctThe function executes the insert query with the proper parameters and error handling.
276-279: Well-designed insert query with conflict handlingThe SQL query correctly implements an insert-or-ignore pattern using the ON CONFLICT DO NOTHING clause on the composite key (payer_report_id, node_id), making the operation idempotent.
281-285: Parameter struct matches database schemaThe InsertOrIgnorePayerReportAttestationParams struct correctly defines all required fields with appropriate types.
287-290: Implementation looks correctThe function executes the insert query with the proper parameters and error handling.
pkg/payerreport/store.go (5)
13-16: Good error definitions for parameter validationThese error variables appropriately define specific error types for validation failures when converting between uint32 and int32 types.
18-28: Clean Store struct design with proper dependency injectionThe Store struct follows good design patterns by:
- Using dependency injection for the queries and logger dependencies
- Using a named logger for better log organization
30-62: Comprehensive validation and type conversion in StoreReportThe StoreReport method includes appropriate validations to ensure values fit within their database column types and properly converts between in-memory and database types.
64-77: Clean implementation of StoreAttestationThe method properly extracts the report ID, handles errors, and converts types correctly before inserting the attestation.
79-103: Good error handling in FetchReportThe FetchReport method appropriately handles database errors and conversion errors when transforming byte slices to fixed-size arrays.
| import "errors" | ||
|
|
||
| var ( | ||
| ErrSliceNot32Bytes = errors.New("slice must be 32 bytes long") | ||
| ) |
There was a problem hiding this comment.
Error declaration formatting issue
The var declaration block needs formatting according to the linter. This appears to be causing your pipeline failures.
- import "errors"
-
- var (
- ErrSliceNot32Bytes = errors.New("slice must be 32 bytes long")
- )
+ import "errors"
+
+ var ErrSliceNot32Bytes = errors.New("slice must be 32 bytes long")The linting tool (gofumpt) is flagging this as improperly formatted. A single error variable should not be placed in a var block.
📝 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.
| import "errors" | |
| var ( | |
| ErrSliceNot32Bytes = errors.New("slice must be 32 bytes long") | |
| ) | |
| import "errors" | |
| var ErrSliceNot32Bytes = errors.New("slice must be 32 bytes long") |
🧰 Tools
🪛 GitHub Check: Lint-Go
[failure] 5-5:
File is not properly formatted (gofumpt)
🪛 GitHub Actions: Lint
[error] 5-5: File is not properly formatted (gofumpt)
| func randomBytes32() [32]byte { | ||
| var b [32]byte | ||
| //nolint:errcheck | ||
| rand.Read(b[:]) | ||
| return b | ||
| } |
There was a problem hiding this comment.
Handle errors in randomBytes32 function.
The function ignores potential errors from rand.Read with the //nolint:errcheck comment. Even for temporary code, proper error handling is important for cryptographic operations, as failure could lead to predictable values or panics in production.
// Temporary function until we have a real merkle root
func randomBytes32() [32]byte {
var b [32]byte
- //nolint:errcheck
- rand.Read(b[:])
+ _, err := rand.Read(b[:])
+ if err != nil {
+ // Log error and/or use a fallback strategy
+ log.Printf("Failed to generate random bytes: %v", err)
+ }
return b
}Committable suggestion skipped: line range outside the PR's diff.
| func (p *PayerReport) ToProto() *proto.PayerReport { | ||
| return &proto.PayerReport{ | ||
| OriginatorNodeId: p.OriginatorNodeID, | ||
| StartSequenceId: p.StartSequenceID, | ||
| EndSequenceId: p.EndSequenceID, | ||
| PayersMerkleRoot: p.PayersMerkleRoot[:], | ||
| PayersLeafCount: p.PayersLeafCount, | ||
| } | ||
| } |
There was a problem hiding this comment.
Include all fields in ToProto() method.
The ToProto() method doesn't include NodesHash and NodesCount fields from the PayerReport struct, which may lead to incomplete data when serializing/deserializing between Go and protobuf.
func (p *PayerReport) ToProto() *proto.PayerReport {
return &proto.PayerReport{
OriginatorNodeId: p.OriginatorNodeID,
StartSequenceId: p.StartSequenceID,
EndSequenceId: p.EndSequenceID,
PayersMerkleRoot: p.PayersMerkleRoot[:],
PayersLeafCount: p.PayersLeafCount,
+ NodesHash: p.NodesHash[:],
+ NodesCount: p.NodesCount,
}
}📝 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.
| func (p *PayerReport) ToProto() *proto.PayerReport { | |
| return &proto.PayerReport{ | |
| OriginatorNodeId: p.OriginatorNodeID, | |
| StartSequenceId: p.StartSequenceID, | |
| EndSequenceId: p.EndSequenceID, | |
| PayersMerkleRoot: p.PayersMerkleRoot[:], | |
| PayersLeafCount: p.PayersLeafCount, | |
| } | |
| } | |
| func (p *PayerReport) ToProto() *proto.PayerReport { | |
| return &proto.PayerReport{ | |
| OriginatorNodeId: p.OriginatorNodeID, | |
| StartSequenceId: p.StartSequenceID, | |
| EndSequenceId: p.EndSequenceID, | |
| PayersMerkleRoot: p.PayersMerkleRoot[:], | |
| PayersLeafCount: p.PayersLeafCount, | |
| NodesHash: p.NodesHash[:], | |
| NodesCount: p.NodesCount, | |
| } | |
| } |
| func (p *PayerReport) ID() (ReportID, error) { | ||
| packedBytes, err := payerReportMessageHash.Pack( | ||
| p.OriginatorNodeID, | ||
| p.StartSequenceID, | ||
| p.EndSequenceID, | ||
| p.PayersMerkleRoot, | ||
| p.PayersLeafCount, | ||
| p.NodesHash, | ||
| p.NodesCount, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Return the keccak256 hash | ||
| return utils.HashPayerReportInput(packedBytes), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for zero/empty values in ID() method.
The ID() method doesn't validate input fields before ABI packing. Consider adding validation to ensure that none of the byte arrays are empty or zero to prevent unexpected hashing results.
func (p *PayerReport) ID() (ReportID, error) {
+ // Validate critical fields before hashing
+ if p.PayersMerkleRoot == [32]byte{} || p.NodesHash == [32]byte{} {
+ return nil, fmt.Errorf("invalid report: empty merkle root or nodes hash")
+ }
+
packedBytes, err := payerReportMessageHash.Pack(
p.OriginatorNodeID,
p.StartSequenceID,
p.EndSequenceID,
p.PayersMerkleRoot,
p.PayersLeafCount,
p.NodesHash,
p.NodesCount,
)
if err != nil {
return nil, err
}
// Return the keccak256 hash
return utils.HashPayerReportInput(packedBytes), nil
}📝 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.
| func (p *PayerReport) ID() (ReportID, error) { | |
| packedBytes, err := payerReportMessageHash.Pack( | |
| p.OriginatorNodeID, | |
| p.StartSequenceID, | |
| p.EndSequenceID, | |
| p.PayersMerkleRoot, | |
| p.PayersLeafCount, | |
| p.NodesHash, | |
| p.NodesCount, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Return the keccak256 hash | |
| return utils.HashPayerReportInput(packedBytes), nil | |
| } | |
| func (p *PayerReport) ID() (ReportID, error) { | |
| // Validate critical fields before hashing | |
| if p.PayersMerkleRoot == [32]byte{} || p.NodesHash == [32]byte{} { | |
| return nil, fmt.Errorf("invalid report: empty merkle root or nodes hash") | |
| } | |
| packedBytes, err := payerReportMessageHash.Pack( | |
| p.OriginatorNodeID, | |
| p.StartSequenceID, | |
| p.EndSequenceID, | |
| p.PayersMerkleRoot, | |
| p.PayersLeafCount, | |
| p.NodesHash, | |
| p.NodesCount, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Return the keccak256 hash | |
| return utils.HashPayerReportInput(packedBytes), nil | |
| } |
| CREATE TABLE payer_report_attestations ( | ||
| -- Do not reference the payer reports table since attestations may arrive before the report is stored | ||
| payer_report_id BYTEA NOT NULL, | ||
| node_id BIGINT NOT NULL, | ||
| signature BYTEA NOT NULL, | ||
| created_at TIMESTAMP DEFAULT NOW(), | ||
| PRIMARY KEY (payer_report_id, node_id) | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider application-level validation for data integrity.
The comment explicitly notes that foreign keys are not used, which could lead to orphaned attestations. Ensure that application-level validation is implemented to maintain data integrity.
🏁 Script executed:
#!/bin/bash
# Check if the application has proper validation logic for attestations without reports
# Look for validation code in the Store implementation
rg -A 5 "StoreAttestation" pkg/payerreport/Length of output: 568
Add validation to prevent orphaned attestations
The StoreAttestation method in pkg/payerreport/store.go currently does:
reportID, err := attestation.Report.ID()
if err != nil {
return err
}
return s.queries.InsertOrIgnorePayerReportAttestation(…)Without a foreign‐key, this will insert attestations for reports that don’t exist. Add an explicit application‐level check before inserting:
– pkg/payerreport/store.go, in StoreAttestation (before calling InsertOrIgnorePayerReportAttestation)
// ensure the referenced payer report is present
if _, err := s.queries.GetPayerReport(ctx, reportID); err != nil {
if errors.Is(err, sql.ErrNoRows) {
return fmt.Errorf("cannot store attestation: payer report %x not found", reportID)
}
return err
}This will guard against orphaned attestations and maintain data integrity.
Add database schema and SQL queries to store payer reports and their attestations
payer_reportstable to store report metadata including merkle roots, sequence IDs, and submission status in 00011_store-payer-reports.up.sqlpayer_report_attestationstable to store node signatures with foreign key relationships in 00011_store-payer-reports.up.sqlInsertOrIgnorePayerReportandInsertPayerReportAttestationin payerReports.sql for handling report and attestation insertions with duplicate prevention📍Where to Start
Start with the schema definitions in 00011_store-payer-reports.up.sql to understand the data structure, then review the SQL queries in payerReports.sql that interact with these tables.
Macroscope summarized 39ab894.
Summary by CodeRabbit