Generate report IDs #701
Generate report IDs #701
Conversation
WalkthroughThis update introduces new functionality in the payerreport package to support Ethereum smart contract interactions for payer reports. A new file defines argument structures for encoding report data. The Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant PR as PayerReport
participant H as HashPayerReportInput (utils)
T->>PR: Call ID()
PR->>PR: Pack report fields (originatorNodeID, sequenceIDs, Merkle root, activeNodeIds)
PR->>H: Compute Keccak256 hash on packed bytes
H-->>PR: Return 32-byte hash
PR-->>T: Return unique identifier (ID)
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
pkg/utils/hash.go (1)
35-37: Consider adding a domain separation label for consistencyThe new
HashPayerReportInputfunction takes a different approach compared to other hash functions in this file - it doesn't use any domain separation label. While this may be intentional based on your requirements, adding a domain separation label would be consistent with the pattern established by other hash functions (HashPayerSignatureInput,HashJWTSignatureInput, etc.).func HashPayerReportInput(packedBytes []byte) []byte { - return ethcrypto.Keccak256(packedBytes) + return ethcrypto.Keccak256( + []byte(constants.PAYER_REPORT_DOMAIN_SEPARATION_LABEL), + packedBytes, + ) }This would require defining a new constant in the constants package.
pkg/utils/slice.go (1)
3-7: Add validation for input slice lengthThe current implementation silently truncates input slices longer than 32 bytes, which could lead to unexpected behavior. Consider adding validation or documentation to make this behavior explicit.
func SliceToArray32(slice []byte) [32]byte { var array [32]byte copy(array[:], slice) + // Note: If slice is longer than 32 bytes, only the first 32 bytes are used return array }Alternatively, if truncation is undesirable:
func SliceToArray32(slice []byte) ([32]byte, error) { var array [32]byte + if len(slice) > 32 { + return array, fmt.Errorf("input slice length %d exceeds maximum of 32 bytes", len(slice)) + } copy(array[:], slice) return array, nil }pkg/payerreport/report_test.go (2)
9-24: Enhance test coverage with multiple test casesThe current test validates basic functionality but only tests one set of input values. Consider adding multiple test cases with different inputs to ensure robustness, including edge cases.
func TestPayerReportID(t *testing.T) { - report := PayerReport{ - OriginatorNodeID: 1, - StartSequenceID: 1, - EndSequenceID: 10, - PayersMerkleRoot: []byte{1, 2, 3}, - PayersLeafCount: 1, - NodesMerkleRoot: []byte{4, 5, 6}, - NodesLeafCount: 1, - } - - id, err := report.ID() - require.NoError(t, err) - require.NotNil(t, id) - require.Len(t, id, 32) + testCases := []struct { + name string + report PayerReport + wantErr bool + }{ + { + name: "valid report", + report: PayerReport{ + OriginatorNodeID: 1, + StartSequenceID: 1, + EndSequenceID: 10, + PayersMerkleRoot: []byte{1, 2, 3}, + PayersLeafCount: 1, + NodesMerkleRoot: []byte{4, 5, 6}, + NodesLeafCount: 1, + }, + wantErr: false, + }, + { + name: "zeros", + report: PayerReport{ + OriginatorNodeID: 0, + StartSequenceID: 0, + EndSequenceID: 0, + PayersMerkleRoot: make([]byte, 0), + PayersLeafCount: 0, + NodesMerkleRoot: make([]byte, 0), + NodesLeafCount: 0, + }, + wantErr: false, + }, + // Add more test cases as needed + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + id, err := tc.report.ID() + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.NotNil(t, id) + require.Len(t, id, 32) + }) + } }
14-17: Consider validating the actual ID valueThe test only checks that the ID is not nil and has the expected length, but it doesn't validate that the actual ID value is correct. Consider adding an expected value check to ensure the ID generation logic is working as intended.
func TestPayerReportID(t *testing.T) { report := PayerReport{ OriginatorNodeID: 1, StartSequenceID: 1, EndSequenceID: 10, PayersMerkleRoot: []byte{1, 2, 3}, PayersLeafCount: 1, NodesMerkleRoot: []byte{4, 5, 6}, NodesLeafCount: 1, } id, err := report.ID() require.NoError(t, err) require.NotNil(t, id) require.Len(t, id, 32) + // Expected ID value (precomputed) + expectedID := [32]byte{ + // Fill in the expected bytes here + } + require.Equal(t, expectedID, id, "ID does not match expected value") }pkg/payerreport/id.go (2)
5-34: Missing documentation for the payerReportMessageHash variableConsider adding documentation to explain the purpose of this variable and how it's used in the context of the application. This would help other developers understand its role in the report ID generation process.
+// payerReportMessageHash defines the Ethereum ABI encoding structure used for packing +// PayerReport data before hashing for ID generation or smart contract interactions. +// Each field maps to a corresponding field in the PayerReport struct with appropriate +// types for Ethereum compatibility. var payerReportMessageHash = abi.Arguments{ { Name: "originatorNodeID", Type: abi.Type{T: abi.UintTy, Size: 32}, }, // ... rest of the definition }
5-34: Consider making variable unexported if it's not used outside the packageIf
payerReportMessageHashis only used internally within thepayerreportpackage, consider making it unexported (lowercase first letter) to follow Go's visibility conventions.-var payerReportMessageHash = abi.Arguments{ +var payerReportMessageHash = abi.Arguments{If it's intended to be used by other packages, then it should have proper documentation as per the previous comment.
pkg/payerreport/interface.go (5)
31-41: Consider adding function documentation.The
ToProto()method correctly maps the struct fields to their protobuf representation, but lacks documentation explaining its purpose and return value.Add a comment before the function:
+// ToProto converts a PayerReport to its Protocol Buffers representation func (p *PayerReport) ToProto() *proto.PayerReport {
43-58: Consider adding nil checks for merkle root fields.The method doesn't verify if
p.PayersMerkleRootorp.NodesMerkleRootare nil before passing them toutils.SliceToArray32. If these could be nil, consider adding validation.func (p *PayerReport) ID() ([]byte, error) { + if p.PayersMerkleRoot == nil || p.NodesMerkleRoot == nil { + return nil, fmt.Errorf("merkle root fields cannot be nil") + } packedBytes, err := payerReportMessageHash.Pack(
43-58: Add documentation for the ID method.The
ID()method should include documentation explaining its purpose, return value, and possible errors.+// ID computes a unique identifier for the PayerReport by packing its fields and returning the keccak256 hash +// Returns the hash as a byte slice and any error that occurred during packing func (p *PayerReport) ID() ([]byte, error) {
44-52: The payerReportMessageHash variable is not defined in this file.The code refers to
payerReportMessageHashwhich appears to be defined elsewhere. Consider adding a comment referencing where this variable is defined to improve code readability.func (p *PayerReport) ID() ([]byte, error) { + // payerReportMessageHash is defined in pkg/payerreport/id.go packedBytes, err := payerReportMessageHash.Pack(
53-55: Consider adding more context to the error.When returning an error from
Pack(), add more context to help with debugging.if err != nil { - return nil, err + return nil, fmt.Errorf("failed to pack payer report: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/payerreport/id.go(1 hunks)pkg/payerreport/interface.go(2 hunks)pkg/payerreport/report_test.go(1 hunks)pkg/utils/hash.go(1 hunks)pkg/utils/slice.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pkg/payerreport/report_test.go (1)
pkg/payerreport/interface.go (1)
PayerReport(12-29)
pkg/payerreport/interface.go (2)
pkg/utils/slice.go (1)
SliceToArray32(3-7)pkg/utils/hash.go (1)
HashPayerReportInput(35-37)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Upgrade Tests
🔇 Additional comments (4)
pkg/payerreport/id.go (1)
5-34:❓ Verification inconclusive
Verify sizes match Solidity contract expectations
Ensure that the sizes defined in these ABI arguments match the expectations of the Solidity smart contract that will be interacting with this data. For example, the definition uses
UintTy, Size: 32for the originatorNodeID, which would be auint256in Solidity.Please verify that these type definitions match the corresponding smart contract structure:
- originatorNodeID: uint256 (UintTy, Size: 32)
- startSequenceID: uint64 (UintTy, Size: 64)
- endSequenceID: uint64 (UintTy, Size: 64)
- payersMerkleRoot: bytes32 (FixedBytesTy, Size: 32)
- payersLeafCount: uint32 (UintTy, Size: 32)
- nodesMerkleRoot: bytes32 (FixedBytesTy, Size: 32)
- nodesLeafCount: uint32 (UintTy, Size: 32)
🏁 Script executed:
#!/bin/bash # Find smart contract files and check for PayerReport structure definitions rg -t solidity -A 10 "struct PayerReport" || rg -t solidity -A 10 "function.*PayerReport"Length of output: 156
Verify ABI Type Mappings Against Solidity Contract
The ABI definitions in
pkg/payerreport/id.goappear to follow the intended Solidity types:
- originatorNodeID:
UintTy, Size: 32should correspond to Solidity’suint256- startSequenceID & endSequenceID:
UintTy, Size: 64should map to Solidity’suint64- payersMerkleRoot & nodesMerkleRoot:
FixedBytesTy, Size: 32match Solidity’sbytes32- payersLeafCount & nodesLeafCount:
UintTy, Size: 32should correspond to Solidity’suint32I attempted to locate the Solidity definitions (e.g. a
struct PayerReport) using a file-type filter, but the query using-t solidityfailed due to an unsupported file type. To ensure confidence, please verify manually (or update the search criteria) that the Solidity smart contract indeed declares the expected types for thePayerReportstructure.pkg/payerreport/interface.go (3)
8-9: Good additions to the imports.The added imports support the new functionality - proto package for the protobuf conversion and utils package for hashing and byte array operations.
25-28: Good addition of node-related fields with clear documentation.The new fields
NodesMerkleRootandNodesLeafCountare well-documented and follow the same pattern as the existing payer-related fields.
31-41:❓ Verification inconclusive
Verify all necessary fields are mapped in the protobuf conversion.
The
Payersfield from the struct is not included in the protobuf mapping. Is this intentional or an oversight?
🏁 Script executed:
#!/bin/bash # Check the proto definition to see if it has a field for Payers grep -rn "message PayerReport" --include="*.proto" .Length of output: 54
Action Required: Verify Proto Field Mapping for
PayerReportOur initial automated search for a proto definition (using the pattern
"message PayerReport") did not yield any output, so we couldn’t confirm whether the corresponding protobuf message is defined with aPayersfield. As a result, it's unclear if the omission of thePayersfield in theToProtoconversion is intentional.
- Verify the Proto Definition Manually: Please check the repository’s proto files to determine if the
PayerReportmessage includes a field forPayers.- Mapping Confirmation: If the proto message is meant to include the
Payersfield, update theToProtofunction inpkg/payerreport/interface.go(lines 31-41) to include it. Otherwise, add a clarifying comment indicating that the omission is intentional.
neekolas
force-pushed
the
04-04-generate_report_ids
branch
from
1599a16 to
b5f8759
Compare
neekolas
force-pushed
the
04-04-add_payer_report_protos
branch
from
0dd174f to
139fa98
Compare
neekolas
force-pushed
the
04-04-generate_report_ids
branch
from
b5f8759 to
a1a1fee
Compare
neekolas
force-pushed
the
04-04-add_payer_report_protos
branch
from
139fa98 to
e93cad7
Compare
neekolas
force-pushed
the
04-04-generate_report_ids
branch
from
a1a1fee to
4434194
Compare
neekolas
force-pushed
the
04-04-add_payer_report_protos
branch
from
e93cad7 to
9621647
Compare
neekolas
force-pushed
the
04-04-generate_report_ids
branch
from
4434194 to
824f3d0
Compare
neekolas
force-pushed
the
04-04-add_payer_report_protos
branch
from
9621647 to
69ccbe5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/payerreport/id.go (1)
5-26: Consider adding proper documentation for the ABI structureThe
payerReportMessageHashvariable would benefit from a documented explanation of its purpose and how it relates to Ethereum ABI encoding for payer reports. This would help maintainers understand why this specific structure was chosen and how it should be used.+// payerReportMessageHash defines the ABI arguments structure used to generate a unique hash +// for payer reports. This structure must match the equivalent structure used in smart contracts +// that verify these reports. The packed encoding of these fields is hashed to create the report ID. var payerReportMessageHash = abi.Arguments{pkg/payerreport/interface.go (2)
37-50: Consider adding unit tests for edge casesWhile the PR objectives mention tests for ID generation, make sure to include tests for edge cases such as:
- Empty
ActiveNodeIdsslice- Very large values in
ActiveNodeIds- Empty or nil
PayersMerkleRootThese tests would help ensure the robustness of the ID generation mechanism.
37-50: Add error documentation for ID() methodThe
ID()method returns an error, but there's no documentation about what types of errors callers should expect or how they should handle them. Consider adding a comment explaining possible error scenarios.+// ID generates a unique identifier for the report by hashing its fields using Ethereum's ABI encoding. +// It returns the 32-byte keccak256 hash or an error if ABI encoding fails. func (p *PayerReport) ID() ([]byte, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/payerreport/id.go(1 hunks)pkg/payerreport/interface.go(2 hunks)pkg/payerreport/manager.go(0 hunks)pkg/payerreport/manager_test.go(0 hunks)pkg/payerreport/report_test.go(1 hunks)pkg/utils/hash.go(1 hunks)pkg/utils/slice.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/payerreport/manager.go
- pkg/payerreport/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/utils/hash.go
- pkg/utils/slice.go
- pkg/payerreport/report_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/payerreport/interface.go (2)
pkg/utils/slice.go (1)
SliceToArray32(3-7)pkg/utils/hash.go (1)
HashPayerReportInput(35-37)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Test (Node)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
🔇 Additional comments (1)
pkg/payerreport/interface.go (1)
27-35: LGTM! Good implementation of ToProto conversionThe implementation of the
ToProto()method is clean and concise, providing a straightforward conversion from the domain model to the protobuf representation.
neekolas
force-pushed
the
04-04-generate_report_ids
branch
from
824f3d0 to
88fd499
Compare
Add PayerReport.ID() method to generate unique identifiers for payer reports using Keccak256 hashing
📍Where to StartStart with the Macroscope summarized 44734b5. |
neekolas
force-pushed
the
04-04-add_payer_report_protos
branch
from
69ccbe5 to
bbf4aa9
Compare
neekolas
force-pushed
the
04-04-generate_report_ids
branch
from
88fd499 to
7af7d25
Compare
neekolas
force-pushed
the
04-04-add_payer_report_protos
branch
from
bbf4aa9 to
a4918db
Compare
neekolas
force-pushed
the
04-04-generate_report_ids
branch
from
7af7d25 to
44734b5
Compare
Merge activity
|
### TL;DR Added functionality to generate unique IDs for payer reports based on their contents. ### What changed? - Added a new `ID()` method to the `PayerReport` struct that generates a unique identifier by hashing the report's fields - Created a new `payerreport/id.go` file with ABI definitions for hashing payer report data - Added `NodesMerkleRoot` and `NodesLeafCount` fields to the `PayerReport` struct - Implemented a `ToProto()` method to convert a `PayerReport` to its protobuf representation - Added utility functions for hashing payer report data and converting byte slices to fixed-size arrays - Added tests to verify the payer report ID generation ### How to test? Run the new test in `pkg/payerreport/report_test.go` to verify that the ID generation works correctly: ``` go test -v ./pkg/payerreport -run TestPayerReportID ``` ### Why make this change? This change enables unique identification of payer reports based on their content, which is necessary for tracking, referencing, and validating reports throughout the system. The ID generation follows Ethereum's ABI encoding standards to ensure compatibility with smart contract verification. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Updated payer reports to track active node IDs instead of leaf counts. - Added conversion of report data into a protocol-friendly format. - Enhanced unique report ID generation with improved hashing of report details. - Introduced utilities for byte slice conversion and secure hashing. - **Tests** - Added tests to validate the correctness of report ID generation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
TL;DR
Added functionality to generate unique IDs for payer reports based on their contents.
What changed?
ID()method to thePayerReportstruct that generates a unique identifier by hashing the report's fieldspayerreport/id.gofile with ABI definitions for hashing payer report dataNodesMerkleRootandNodesLeafCountfields to thePayerReportstructToProto()method to convert aPayerReportto its protobuf representationHow to test?
Run the new test in
pkg/payerreport/report_test.goto verify that the ID generation works correctly:Why make this change?
This change enables unique identification of payer reports based on their content, which is necessary for tracking, referencing, and validating reports throughout the system. The ID generation follows Ethereum's ABI encoding standards to ensure compatibility with smart contract verification.
Summary by CodeRabbit
New Features
Tests