Talk to the blockchain #857
Conversation
|
Warning Rate limit exceeded@neekolas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (20)
WalkthroughThis change replaces the previous Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as SubmitterWorker
participant Manager as ReportsManager
participant Blockchain as PayerReportManagerContract
Worker->>Manager: SubmitPayerReport(ctx, report)
Manager->>Blockchain: Submit(report fields, attestation signatures)
Blockchain-->>Manager: PayerReportSubmitted event
Manager-->>Worker: Return result
Worker->>Manager: GetReport(ctx, originatorNodeID, index)
Manager->>Blockchain: GetReport(originatorNodeID, index)
Blockchain-->>Manager: On-chain report data
Manager-->>Worker: Return PayerReport
Worker->>Manager: GetReportID(ctx, payerReport)
Manager->>Blockchain: ComputeDigest(payerReport fields)
Blockchain-->>Manager: ReportID
Manager-->>Worker: Return ReportID
Worker->>Manager: GetDomainSeparator(ctx)
Manager->>Blockchain: DomainSeparator()
Blockchain-->>Manager: domainSeparator
Manager-->>Worker: Return domainSeparator
Suggested reviewers
✨ 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. |
Replace PayerReportsAdmin interface with PayerReportsManager to enable blockchain interaction for payer report submission and retrievalReplaces the stub • Creates new reportsManager.go implementing blockchain interaction methods 📍Where to StartStart with the new Macroscope summarized 6322ffa. (Automatic summaries will resume when PR exits draft mode or review begins). |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
pkg/server/server.go
Outdated
| } | ||
|
|
||
| if options.Sync.Enable { | ||
| signer, err := blockchain.NewPrivateKeySigner( |
There was a problem hiding this comment.
Will need to figure out a tidier way to get the Domain Separator into sync worker
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/server/server.go (1)
190-193: Consider caching domain separator to avoid repeated blockchain calls.The domain separator is fetched from the blockchain during every server startup. If this value is expected to be static, consider caching it to improve startup performance and reduce blockchain dependency.
Since the domain separator typically doesn't change frequently, you might want to verify if caching is already implemented in the ReportsManager or add startup-time caching to reduce blockchain calls.
pkg/blockchain/reportsManager_test.go (1)
37-49: Good test coverage for domain separator functionality.The test verifies both the basic functionality (32-byte domain separator) and caching behavior (consistent results across calls). This covers the critical path for domain separator retrieval.
Consider adding error case testing:
+func TestDomainSeparatorError(t *testing.T) { + // Test with invalid RPC URL to verify error handling + logger := testutils.NewLog(t) + signer, err := blockchain.NewPrivateKeySigner( + testutils.GetPayerOptions(t).PrivateKey, + 1, // Invalid chain ID + ) + require.NoError(t, err) + + client, err := blockchain.NewClient(t.Context(), "http://invalid-url") + require.Error(t, err) // Should fail to connect +}pkg/blockchain/reportsManager.go (1)
70-74: Consider reducing log verbosity.Logging the entire report and signatures could be verbose in production. Consider logging only key fields or using debug level.
- r.log.Info( + r.log.Debug( "submitting report", - zap.Any("report", report), - zap.Any("signatures", signatures), + zap.Uint32("originator_node_id", report.OriginatorNodeID), + zap.Uint64("start_sequence_id", report.StartSequenceID), + zap.Uint64("end_sequence_id", report.EndSequenceID), + zap.Int("signature_count", len(signatures)), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
pkg/blockchain/interface.go(1 hunks)pkg/blockchain/reportsAdmin.go(0 hunks)pkg/blockchain/reportsManager.go(1 hunks)pkg/blockchain/reportsManager_test.go(1 hunks)pkg/config/options.go(1 hunks)pkg/payerreport/report.go(2 hunks)pkg/payerreport/report_test.go(1 hunks)pkg/payerreport/store.go(1 hunks)pkg/payerreport/workers/integration_test.go(11 hunks)pkg/payerreport/workers/submitter.go(4 hunks)pkg/registrant/registrant.go(1 hunks)pkg/server/server.go(2 hunks)pkg/server/server_test.go(4 hunks)pkg/testutils/config.go(2 hunks)pkg/testutils/server/server.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/blockchain/reportsAdmin.go
🧰 Additional context used
🧬 Code Graph Analysis (5)
pkg/server/server_test.go (2)
pkg/testutils/config.go (2)
NewContractsOptions(28-49)BLOCKCHAIN_RPC_URL(10-10)pkg/testutils/server/server.go (1)
NewTestServer(19-53)
pkg/payerreport/workers/submitter.go (2)
pkg/blockchain/interface.go (1)
PayerReportsManager(54-66)pkg/payerreport/interface.go (1)
IPayerReportStore(37-68)
pkg/payerreport/store.go (1)
pkg/payerreport/report.go (1)
BuildPayerReportID(141-170)
pkg/payerreport/report_test.go (1)
pkg/payerreport/report.go (1)
BuildPayerReportID(141-170)
pkg/blockchain/interface.go (2)
pkg/payerreport/report.go (3)
PayerReportWithStatus(104-113)PayerReport(69-83)ReportID(63-63)pkg/db/queries/models.go (1)
PayerReport(65-76)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
- GitHub Check: Code Review
🔇 Additional comments (26)
pkg/payerreport/store.go (1)
259-259: LGTM: Function call updated to use exported version.The change from
buildPayerReportIDtoBuildPayerReportIDcorrectly aligns with the function being exported inpkg/payerreport/report.go. This enables external access to the report ID building functionality, which appears necessary for the blockchain integration.pkg/config/options.go (1)
52-52: LGTM: Well-structured configuration addition.The new
PayerReportManagerAddressfield follows the established pattern for contract address configuration. The naming convention, environment variable, and description are consistent with other contract address fields in the struct.pkg/payerreport/report.go (2)
141-141: LGTM: Function correctly exported.The function
buildPayerReportIDis properly renamed toBuildPayerReportIDto make it publicly accessible. This follows Go naming conventions for exported functions and enables external packages to access the report ID building functionality.
179-179: LGTM: Internal call updated consistently.The function call is correctly updated to use the new exported name
BuildPayerReportID, maintaining consistency with the function rename.pkg/registrant/registrant.go (1)
123-123:Details
❓ Verification inconclusive
Verify cryptographic signature change across the system.
The signing logic has been changed from signing a hash digest (likely involving
reportIDanddomainSeparator) to signing the rawreportIDbytes directly. This is a significant cryptographic behavior change that could break signature validation in other components.Please verify that all signature validation logic has been updated to match this new signing approach:
Also note that the
domainSeparatorparameter is still present in the method signature but is no longer used in the signing operation, which could be confusing.
🏁 Script executed:
#!/bin/bash # Description: Search for signature validation logic related to payer report attestations # Expected: All validation should expect raw reportID signatures, not hash digests # Search for signature verification patterns echo "=== Searching for signature verification patterns ===" rg -A 10 -B 5 "VerifySignature|crypto\.VerifySignature|secp256k1\.VerifySignature" --type go echo "=== Searching for payer report attestation verification ===" rg -A 10 -B 5 "PayerReportAttestation.*[Vv]erif" --type go echo "=== Searching for reportID signature handling ===" rg -A 5 -B 5 "reportID.*[Ss]ignature|[Ss]ignature.*reportID" --type goLength of output: 1458
🏁 Script executed:
#!/bin/bash # Locate actual implementation of SignPayerReportAttestation to verify domainSeparator usage echo "=== Searching for SignPayerReportAttestation implementation ===" rg -n -A 10 -B 2 "SignPayerReportAttestation" --type goLength of output: 8594
Verify downstream signature validation and domain separation usage
No verification routines were found in this repo that consume the new raw-reportID signature, and the
domainSeparatorparameter is still present yet unused inSignPayerReportAttestation. This is a breaking change in cryptographic guarantees—ensure all consumers and validators of these signatures have been updated accordingly.Please review and update the following locations as needed:
- pkg/registrant/registrant.go (around line 123):
sig, err := r.sign(reportID[:])now ignoresdomainSeparator.- pkg/registrant/interface.go: consider removing or re-using the
domainSeparatorparameter.- pkg/payerreport/workers/attestation.go: downstream attestation builder still passes
w.domainSeparatorbut validation code is absent here.- Tests in pkg/payerreport/workers/attestation_test.go: no checks on the actual bytes being signed or domain separation—add or adjust mocks to reflect the new behavior.
Ensure that any external services or smart contracts verifying these signatures expect the raw
reportIDpayload (and domain separation, if intended) rather than the previous hash digest.pkg/testutils/config.go (2)
10-10: LGTM! Port change follows Ethereum standards.The port change from 7545 to 8545 aligns with the standard HTTP-RPC port for Ethereum development nodes.
45-45: LGTM! Configuration properly supports new blockchain integration.The addition of
PayerReportManagerAddressto the settlement chain configuration correctly supports the newReportsManagerimplementation that interacts with the Payer Report Manager smart contract.pkg/payerreport/report_test.go (1)
104-111: LGTM! Correctly uses the newly exported function.The change from
buildPayerReportIDtoBuildPayerReportIDproperly reflects the function being made public to support blockchain integration where report ID computation is needed externally.pkg/server/server_test.go (3)
47-47: LGTM! Proper migration to blockchain-backed configuration.The replacement of random domain separator generation with
contractsOptionsenables dynamic retrieval from the blockchain reports manager, supporting more realistic testing scenarios.
56-56: LGTM! Consistent parameter update across test server creation.Both test server instances correctly use the new
contractsOptionsparameter, maintaining consistency with the updatedNewTestServersignature.Also applies to: 65-65
162-162: LGTM! Test function consistently updated.The
TestReadOwnWritesGuaranteefunction properly follows the same pattern asTestCreateServer, usingcontractsOptionsfor test server creation.Also applies to: 171-171
pkg/blockchain/interface.go (3)
54-54: LGTM! Interface rename better reflects its purpose.The rename from
PayerReportsAdmintoPayerReportsManagermore accurately describes the interface's role in managing payer reports rather than just administering them.
56-60: LGTM! GetReport method enables blockchain report retrieval.The
GetReportmethod properly supports querying the blockchain for existing reports by originator node ID and index, which is essential for verification and synchronization with on-chain data.
62-66: LGTM! GetReportID method supports report ID computation.The
GetReportIDmethod enables computing report IDs fromPayerReportWithStatus, supporting consistency verification between local and on-chain report representations.pkg/testutils/server/server.go (2)
26-26: Good improvement: Using ContractsOptions for better configuration.The change from a hardcoded domain separator to
contractsOptionsprovides better flexibility and aligns with the dynamic blockchain approach implemented in the main server.
30-31: Excellent: Using test context for proper lifecycle management.Using
t.Context()instead ofcontext.Background()ensures the server is properly canceled when the test completes, and usingcontractsOptionsmaintains consistency with the new blockchain integration approach.pkg/payerreport/workers/submitter.go (1)
24-24: Interface update aligns with blockchain integration.The change from
PayerReportsAdmintoPayerReportsManageris consistent with the broader refactoring to use the new blockchain-backed reports manager. The interface provides the sameSubmitPayerReportmethod, ensuring compatibility.Also applies to: 32-32, 42-42
pkg/blockchain/reportsManager_test.go (1)
12-35: Well-structured test helper function.The
buildReportsManagerhelper properly sets up all required dependencies including logger, Anvil test node, contracts options, signer, and client. This provides a realistic test environment for blockchain interactions.pkg/payerreport/workers/integration_test.go (5)
9-36:
49-52: LGTM!The scaffold struct additions properly support the blockchain integration.
136-247: LGTM!The modifications properly integrate the blockchain components into the test setup.
293-325: LGTM!Excellent test for verifying report ID consistency between local and blockchain storage.
434-459: LGTM!The submitter worker integration completes the end-to-end test flow, properly verifying that reports are submitted to and retrievable from the blockchain.
pkg/blockchain/reportsManager.go (3)
106-124: LGTM!Clean implementation with proper error handling for report retrieval.
126-137: LGTM!Simple and correct implementation for retrieving report IDs from the contract.
153-177: LGTM!Well-structured helper function that cleanly transforms on-chain data to the local format.
| } | ||
|
|
||
| func (w *SubmitterWorker) submitReports(ctx context.Context) error { | ||
| func (w *SubmitterWorker) SubmitReports(ctx context.Context) error { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if method export is intentional.
The method was renamed from submitReports to SubmitReports (exported). Ensure this export is intentional and doesn't expose internal implementation details inappropriately.
🏁 Script executed:
#!/bin/bash
# Description: Check if the exported SubmitReports method is used outside the package
# Expected: Find usage patterns to verify if export is needed
rg -A 3 "SubmitReports" --type goLength of output: 915
Confirm Export Intent for SubmitReports
The SubmitReports method is only referenced in:
pkg/payerreport/workers/submitter.go(self-call)pkg/payerreport/workers/integration_test.go
No external packages invoke this method.
If you don’t intend to expose it beyond this package, consider reverting it to an unexported submitReports. If your integration tests live in a separate _test package and rely on it, keeping it exported is acceptable.
Please verify your intended public API before merging.
func (w *SubmitterWorker) SubmitReports(ctx context.Context) error {
// …
}🤖 Prompt for AI Agents
In pkg/payerreport/workers/submitter.go at line 70, the method SubmitReports was
changed from unexported to exported but is only used internally and in
integration tests. Confirm if you want this method accessible outside the
package; if not, rename it back to submitReports to keep it unexported. If your
integration tests require access and are in a separate _test package, keeping it
exported is fine. Adjust the method name accordingly based on your intended API
exposure.
neekolas
force-pushed
the
05-27-talk_to_the_blockchain
branch
from
6322ffa to
d075523
Compare
| ) (*payerreport.PayerReport, error) { | ||
| report, err := r.reportManagerContract.GetPayerReport(&bind.CallOpts{ | ||
| Context: ctx, | ||
| }, originatorNodeID, big.NewInt(int64(index))) |
There was a problem hiding this comment.
The conversion of index from uint64 to int64 when calling big.NewInt may overflow for values greater than math.MaxInt64, leading to negative results and unexpected behavior in GetPayerReport. Consider using new(big.Int).SetUint64(index) to correctly support the full uint64 range.
- }, originatorNodeID, big.NewInt(int64(index)))
+ }, originatorNodeID, new(big.Int).SetUint64(index))🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
neekolas
force-pushed
the
05-27-talk_to_the_blockchain
branch
from
d075523 to
405462a
Compare
| // The smart contract expects the signature to use the legacy EIP-155 recovery ID | ||
| // which is the last byte of the signature. We need to add 27 to the last byte | ||
| // to get the correct recovery ID. | ||
| sig := make([]byte, len(signature.Signature)) |
There was a problem hiding this comment.
The code in the loop over report.AttestationSignatures accesses sig[64] without first ensuring that the original signature.Signature slice is at least 65 bytes long. If a shorter slice were ever provided, this would cause a runtime panic. Consider adding a guard—checking len(signature.Signature) >= 65—and returning an error if the signature is unexpectedly short before modifying its recovery ID.
+ if len(signature.Signature) < 65 {
+ return nil, fmt.Errorf("invalid signature length for node %d: got %d bytes", signature.NodeID, len(signature.Signature))
+ }🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
neekolas
force-pushed
the
05-27-talk_to_the_blockchain
branch
from
405462a to
f134a52
Compare
neekolas
force-pushed
the
05-16-use_real_merkle_generator
branch
2 times, most recently
from
18200e3 to
f98e9fb
Compare
neekolas
force-pushed
the
05-27-talk_to_the_blockchain
branch
from
f134a52 to
a8afe7e
Compare
neekolas
force-pushed
the
05-16-use_real_merkle_generator
branch
from
f98e9fb to
b76b9b9
Compare
neekolas
force-pushed
the
05-27-talk_to_the_blockchain
branch
from
a8afe7e to
b3265fd
Compare
TL;DR
Replaced
ReportsAdminwith a fully implementedReportsManagerthat interacts with the blockchain to manage payer reports.What changed?
PayerReportsAdmininterface toPayerReportsManagerand added new methods:GetReport,GetReportIDReportsAdminimplementation with a completeReportsManagerimplementation that interacts with the blockchainBuildPayerReportIDfunction public to allow external verificationHow to test?
pkg/payerreport/workers/integration_test.goGetDomainSeparatormultiple timesWhy make this change?
This change implements the blockchain integration for payer reports, allowing the system to submit reports to the blockchain and verify their integrity. The implementation enables the full payer report lifecycle - from generation to attestation to on-chain submission - which is critical for the settlement process. Making the report ID calculation consistent between local and blockchain environments ensures data integrity throughout the system.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor