IPayerReport interface #581
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Originator
participant IPayer
Originator->>IPayer: settleUsage(originatorNode, reportIndex, payers, amounts)
IPayer-->>Originator: Validate Merkle proof and process settlement
sequenceDiagram
participant Originator
participant Validator
participant IPayerReport
Originator->>IPayerReport: submitPayerReport(PayerReport)
IPayerReport-->>Originator: Emit PayerReportSubmitted event
Validator->>IPayerReport: attestPayerReport(originatorNode, reportIndex)
IPayerReport-->>Validator: Emit PayerReportAttested event
Originator/Other->>IPayerReport: getPayerReport(originatorNode, reportIndex)
IPayerReport-->>Originator: Return PayerReport details
Originator/Other->>IPayerReport: settleUsageBatch(originatorNode, reportIndex, offset, payers, amounts, proof)
IPayerReport-->>Originator: Emit PayerReportPartiallySettled event
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 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 (
|
![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 (4)
contracts/src/interfaces/IPayer.sol (1)
278-285: Validate ranges and offset insettleUsage.While this interface accurately represents the new settlement approach, pay special attention in the implementation to avoid out-of-bounds usage of
offset + payers.length, especially if partial usage slices are processed. Confirm that any leftover usage in the report remains valid or is settled in subsequent calls.contracts/src/interfaces/IPayerReport.sol (3)
60-84: Double-check parameter coverage and validation insubmitPayerReport.All required data for usage tracking is presented (sequence IDs, timestamps, Merkle root, etc.). In the implementation, validate:
- Array length matching for
payersandamountsSpent.- A sensible maximum on
payers.lengthto avoid potential block gas-limit issues.
94-104: Consider partial or malicious attestations inconfirmPayerReport.Majority attestation is critical. Confirm that the final checks account for any malicious or repeated attestations so that only unique node attestations count. Document how “majority” is defined (e.g., number of active node operators).
106-113: Review pagination needs inlistPayerReports.Currently, this returns all usage reports for the node in a single call. If the list grows large, you might consider a paginated approach to prevent potential out-of-gas issues on-chain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/interfaces/IPayer.sol(1 hunks)contracts/src/interfaces/IPayerReport.sol(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Solidity
contracts/src/interfaces/IPayerReport.sol
[warning] 2-2: Version constraint ^0.8.0 contains known severe issues. Refer to the documentation for details.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (5)
contracts/src/interfaces/IPayer.sol (1)
264-277: Ensure Merkle-proof parameters and docstrings remain consistent in implementation.The docstring nicely explains the purpose of offset, payers, amounts, and proof. However, be sure that any implementation enforces array-length checks, offset bounds, and Merkle proof validity. Consider clarifying in the docstring that the
ArrayLengthMismatcherror will be used ifpayers.length != amounts.length.contracts/src/interfaces/IPayerReport.sol (4)
24-38: Confirm array length consistency forevent PayerReportSubmitted.This event publishes arrays
payersandamountsSpent, but the interface omits explicit mention of their expected matching lengths. Ensure the implementation logs consistent arrays to avoid potential indexing or data mismatch.
40-55: EventsPayerReportAttestedandPayerReportConfirmedlook solid.They succinctly capture essential details, including the Merkle root. Just confirm in implementation that you consistently index by
reportIndexwhere necessary, to avoid confusion with only storing thereportMerkleRoot.
85-92: Attestation logic might need access-control checks.While
attestPayerReportis open to “nodes,” ensure the implementation verifies that only legitimate/active node operators can attest. Otherwise, the system may allow fraudulent attestations.
114-141: Ensure storage integrity forgetPayerReport.The return values cover all essential fields. When implementing, carefully map indexes to stored arrays and confirm that the attestation count and
isConfirmedstates cannot be manipulated without a proper majority attestation process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
contracts/src/interfaces/IPayerReport.sol (5)
28-38: Consider indexing thereportMerkleRootfield for easier log filtering.Indexing
reportMerkleRootin thePayerReportSubmittedevent can improve discoverability of relevant logs by Merkle root value.event PayerReportSubmitted( address indexed originatorNode, uint256 indexed reportIndex, uint256 startingSequenceID, uint256 endingSequenceID, uint256 lastMessageTimestamp, uint256 reportTimestamp, - bytes32 reportMerkleRoot, + bytes32 indexed reportMerkleRoot, address[] payers, uint256[] amountsSpent );
43-46: Include the report index in thePayerReportAttestedevent.Capturing the explicit
reportIndexin the event makes it easier to trace back a particular attestation to a specific usage report.event PayerReportAttested( address indexed originatorNode, - bytes32 reportMerkleRoot + uint256 indexed reportIndex, + bytes32 indexed reportMerkleRoot );
51-54: Include the report index in thePayerReportConfirmedevent.This helps downstream services or off-chain indexers easily correlate confirmations with the correct report index, especially if multiple reports share the same Merkle root.
event PayerReportConfirmed( address indexed originatorNode, - bytes32 reportMerkleRoot + uint256 indexed reportIndex, + bytes32 indexed reportMerkleRoot );
74-83: Consider using a struct to reduce the number of parameters.A struct-based parameter approach can improve readability, maintainability, and help avoid "stack too deep" issues in implementations.
-function submitPayerReport( - address originatorNode, - uint256 startingSequenceID, - uint256 endingSequenceID, - uint256 lastMessageTimestamp, - uint256 reportTimestamp, - bytes32 reportMerkleRoot, - address[] calldata payers, - uint256[] calldata amountsSpent -) external; + +struct UsageReport { + address originatorNode; + uint256 startingSequenceID; + uint256 endingSequenceID; + uint256 lastMessageTimestamp; + uint256 reportTimestamp; + bytes32 reportMerkleRoot; + address[] payers; + uint256[] amountsSpent; +} + +function submitPayerReport(UsageReport calldata report) external;
107-113: Clarify the documentation to reflect multiple returned values.The docstring describes a single “first sequence ID,” whereas the function returns an array. For accuracy, rename or clarify the returned arrays in the Solidity doc comments.
/** * @notice Returns a list of all payer reports for a given originator node. - * @return startingSequenceID The first sequence ID in the report. - * @return reportsMerkleRoot The Merkle root for the detailed usage data. + * @return startingSequenceIDs The array of starting sequence IDs for each report. + * @return reportsMerkleRoots The array of Merkle roots for each report. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/interfaces/IPayerReport.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (1)
contracts/src/interfaces/IPayerReport.sol (1)
2-2:Details
✅ Verification successful
Check the validity of Solidity version.
While 0.8.28 may be acceptable if it's officially released, it's prudent to verify its stability and ensure support in your compiler toolchain.
🌐 Web query:
Is "pragma solidity 0.8.28" an official release from the Solc stable versions?💡 Result:
Yes, Solidity v0.8.28 is an official stable release of the Solidity compiler. The release was announced by the Solidity team on October 9, 2024, through multiple official channels including GitHub[1][6], the Solidity blog[6], and developer forums[10]. It includes new features like full support for transient storage state variables of value types (a precursor to complete transient storage functionality)[1][10], compiler optimizations to reduce memory usage[1][6], and bug fixes[1][4].
Key indicators of its official status:
- Listed in the Solidity GitHub repository releases with detailed changelog[1]
- Available through official package managers like npm (
solc@0.8.28)[5]- Mentioned in Solidity's official documentation PDF[3]
- Discussed in the Solidity Language Forum[10]
Developers can safely use
pragma solidity 0.8.28;in contracts, though the latest version (0.8.29 as of March 2025) should generally be preferred for security updates[3][7]. The release follows Semantic Versioning rules, with 0.8.x versions maintaining backward compatibility[3][6].Citations:
- 1: https://github.com/ethereum/solidity/releases
- 2: https://solcx.readthedocs.io/en/latest/version-management.html
- 3: https://docs.soliditylang.org/_/downloads/en/latest/pdf/
- 4: https://outposts.io/article/solidity-0828-release-announcement-3b511420-1fb8-4cda-9366-1848e83e86bd
- 5: https://www.npmjs.com/package/solc?activeTab=versions
- 6: https://soliditylang.org/blog/category/releases/
- 7: https://docs.soliditylang.org/en/latest/installing-solidity.html
- 8: https://app.daily.dev/posts/solidity-0-8-28-release-announcement-wo9akimat
- 9: https://www.npmjs.com/package/solc-version
- 10: https://forum.soliditylang.org/t/solidity-v0-8-28-is-out/3120
Solidity Version Validated
- Verified that
pragma solidity 0.8.28;references an officially released and stable compiler version (released October 9, 2024).- While 0.8.28 is supported and valid for use, note that version 0.8.29 is now available as of March 2025 for additional enhancements and security updates.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
contracts/src/interfaces/IPayer.sol (1)
379-382: Document return values more thoroughly.The function’s doc comment is clear but does not specify how the returned contract value might be used. Consider adding a brief note indicating that downstream contracts or scripts may rely on this address for distribution logic.
contracts/src/interfaces/IPayerReport.sol (6)
28-44: Event emission for full usage details.Emitting full data via
PayerReportSubmittedis valuable for off-chain processing. Double-check that large arrays do not exceed gas limits on mainnet.
46-54: Event clarifies node attestations but watch for spam.
PayerReportAttestedis straightforward. However, if many nodes re-attest frequently, this can bloat logs. Consider implementing measures (e.g., minimum stake or checks) to discourage unnecessary attestations.
78-85: Ensure reentrancy protections while submitting reports.
submitPayerReportshould be guarded by best practices in the implementation to avoid reentrancy across other calls. A simple nonReentrant or checks-effects-interactions pattern can help.
87-95: Attestation logic.
attestPayerReportis flexible, but ensure code addresses stale or duplicate attestations. Possibly store a mapping from(originatorNode, reportIndex, attestor)to track repeated attempts.
96-106: Confirm majority threshold.
confirmPayerReportdepends on a “majority attestation.” Clarify or document how many attestations are needed. This detail, while not in the interface, is crucial for consistent usage in implementations.
108-115: Pagination concerns.
listPayerReportsreturns all stored report roots for the node. For large sets, consider a block-based or offset-based approach in the future to avoid hitting view function call size limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/interfaces/IPayer.sol(5 hunks)contracts/src/interfaces/IPayerReport.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (12)
contracts/src/interfaces/IPayer.sol (5)
71-71: Confirm event usage alignment with implementation.Renaming the parameter to
originatorNodeclarifies the event’s meaning. Verify that any references to the oldnodeIdare updated throughout the codebase to avoid inconsistencies.
73-77: Ensure references to fees destination are updated throughout the documentation.Renaming references from “rewards contract” to “distribution contract” is coherent, but any remaining references or documentation must also be adjusted to maintain consistency.
98-99: Check for orphaned references toNotRewardsContract.Introducing
NotDistributionContractis correct given the shift to a distribution contract. Ensure that all checks preventing calls from the old rewards contract are replaced with the new error as well.
264-279: Validate batch settlement logic parameters.The new
settleUsagemethod signature adds clarity by indicating an offset for a contiguous subset of data. Confirm that the contract enforcing the offset logic fully validates array bounds, preventing out-of-range errors.
362-367: Confirm distribution contract address correctness.The setter function and docs now reference the distribution contract. Ensure that security checks are in place so only authorized addresses can update this contract.
contracts/src/interfaces/IPayerReport.sol (7)
1-2: Solidity version looks current.Using
pragma solidity 0.8.28;addresses prior warnings about outdated 0.8.0 usage. This should mitigate known vulnerabilities or compiler bugs documented for earlier 0.8.x releases.
4-7: Interface documentation is concise and clear.The introduction covers the purpose of handling usage reports and batch settlements. This clarity aids implementers in understanding the interface’s responsibilities.
13-22: Struct definition is well-structured.The
UsageReportfields provide essential metadata and the merkle root for verification. Ensure implementers handle data length checks (e.g., payers vs. amountsSpent arrays) to avoid mismatches or out-of-bounds errors.
55-63: Confirmation workflow is direct.
PayerReportConfirmedallows finalizing a majority-attested report. Ensure that finalization logic in the implementation re-checks the data’s validity in case a new malicious attestation appears after submission.
64-73: UsageSettled event usage.Emitting a
UsageSettledevent in this interface indicates successful settlement steps. Align the event’s parameters with final settlement data so external services can track what’s been consumed from the merkle tree.
116-126: Spot-check array sizes.
getPayerReportconveys the entireUsageReport. Implementations should verify that arrayspayersandamountsSpentremain within practical limits for storage or event emission.
157-162: Getter aligns usage boundaries.
getMaxBatchSizestraightforwardly provides an external limit. Ensure internal logic references it consistently to avoid out-of-bounds or overflows.
| * Emits `BatchUsageSettled` and multiple `UsageSettled` events. | ||
| */ | ||
| function settleUsageBatch( | ||
| address originatorNode, |
There was a problem hiding this comment.
I believe this will also need to include a Batch Merkle Proof, won't it?
There was a problem hiding this comment.
Actually I think offset is not needed in Payer.settleUsage function.
The way I see it is the PayerReport.settleUsageBatch accepts the proofs and verifies them. Only when the []payers and []amounts are verified as part of the merkle tree we call Payer.settleUsage([]payer, []amount), which is a dumb function just to settle what we agreed on.
There was a problem hiding this comment.
Ahhh. I see. Makes sense
Could we do this automatically as the attestations come in. Otherwise I'm not sure who would be responsible for calling this |
| event UsageSettled( | ||
| address indexed originatorNode, | ||
| uint256 indexed reportIndex, | ||
| uint256 offset, |
There was a problem hiding this comment.
For this event to be indexable by the nodes I believe we would need to have the payers and the amounts
There was a problem hiding this comment.
Otherwise the node would have to do a bunch of work to reconstruct the tree and figure out what was being settled.
It's particularly tricky if the report that was confirmed and settled wasn't one the node agreed on. From the perspective of indexing settled usage, I think you'd still want to process the update even if you were in the minority and didn't attest to the report.
There was a problem hiding this comment.
Agree, modifying this part too.
Also, with the latest commit I added a uint16 batches to the PayerReport struct, which should be calculated off-chain based on getMaxBatchSize(). Other nodes can reconstruct that data and agree on it.
I think this is interesting because we don't submit the amount of payers and amounts anymore, so I don't see a reliable way of calculating the amount of batches expected for a single report, tracking what was already settled, etc.
There was a problem hiding this comment.
This makes sense to me. Do we want it to be the number of batches, or just the number of leaves in the tree? I'm fine with either, but the number of leaves lets the caller adjust the batch size as needed.
There was a problem hiding this comment.
I like the number to be leaves in the tree. It reduces ambiguity and hidden knowledge (such as batches has to be N payers max)
@nick I was concerned about having a clear separation of concerns in the contract ABI, so attestPayerReport doesn't return 2 types of events depending the amount of confirmations, and Nodes would attest to a PayerReport while counting the confirmations off-chain, then any node could call confirmPayerReport. But yes, it's definitely possible to combine the logic into |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
contracts/src/interfaces/IPayer.sol (2)
264-271: Clarify prerequisite in documentationThe doc states “Assumes that the PayerReport contract has already verified…” Consider adding a brief note describing what happens if the caller fails to meet this prerequisite, or if the caller is not the PayerReport contract. This helps clarify security assumptions for third parties reading the interface’s docs.
281-281: Minor doc alignmentEnsure the phrasing “to the distribution contract” is consistent with the rest of the file. If usage or examples exist elsewhere, re-check for consistency (e.g., “transfer of pending fees”).
contracts/src/interfaces/IPayerReport.sol (4)
1-3: Pragma and license
pragma solidity 0.8.28;is sufficiently recent. If your build pipeline flags known issues, consider bumping to a newer 0.8.x version.
62-72: Event: PayerReportPartiallySettledThe doc references “Emits a UsageSettled event” in line 134, but the actual event name is
PayerReportPartiallySettled. For consistency, update the doc to reference the correct event name.- * Emits a UsageSettled event. + * Emits a PayerReportPartiallySettled event.
102-109: Function: listPayerReportsReturning only
startingSequenceIDsandreportsMerkleRootsmight be limiting. Consider also returning a status (e.g., attested, confirmed, etc.) to reduce repeated calls.function listPayerReports(address originatorNode) external view - returns (uint256[] memory startingSequenceIDs, bytes32[] memory reportsMerkleRoots); + returns ( + uint256[] memory startingSequenceIDs, + bytes32[] memory reportsMerkleRoots, + bool[] memory isConfirmed + );
145-150: Function: setMaxBatchSizeIf you plan to impose batch size limits, you might also need to track them in the actual settlement logic. Let me know if you’d like a suggested patch verifying
offset + payers.length <= maxBatchSize.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/interfaces/IPayer.sol(5 hunks)contracts/src/interfaces/IPayerReport.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (19)
contracts/src/interfaces/IPayer.sol (9)
71-71: Good rename tooriginatorNodefor claritySwitching from
nodeIdtooriginatorNodemore clearly conveys the purpose of this event parameter.
73-73: Event name is conciseNo issues spotted. The event name and parameter are straightforward.
76-76: Consistent naming for distribution contractRenaming to
DistributionContractUpdatedaligns event naming across the codebase.
98-99: Error type changesRenaming to
NotDistributionContractis consistent with the new terminology for the distribution contract.
273-276: Parameter additionsAdding
originatorNodeandreportIndexto thesettleUsagefunction ensures that usage batches are properly tied to a specific node’s report.
287-292: Improved clarity for fees transferThe updated docstring and method name
transferFeesToDistributionare aligned, making the usage of this function more intuitive.
360-365: Set distribution contract logicThe doc references a single setter function for the distribution contract. Ensure at the implementation level that only authorized roles (e.g., contract owner) may call this method to avoid malicious updates.
Would you like a shell script to confirm usage of an access control check in the implementation?
376-379: Getter naming clarity
getDistributionContract()read function name is consistent withsetDistributionContract().
383-385: Nodes contract retrieval [approve]Renaming and clarifying doc ensures consistent naming. Looks good.
contracts/src/interfaces/IPayerReport.sol (10)
5-7: Interface documentationHigh-level notice accurately describes the handling of usage reports and batch settlements.
9-22:PayerReportstructIncluding
batchesandreportMerkleRootis essential for partial settlement logic. Please ensure external code validates thatbatchesis correctly computed to align with off-chain usage.
Would you like a script to search forbatchesusage in the codebase to check for correct calculations?
27-42: Event: PayerReportSubmittedSufficiently descriptive event captures the necessary details for off-chain indexing of the usage report.
44-51: Event: PayerReportAttestedCaptures attestations in a straightforward manner.
No issues found.
53-60: Event: PayerReportConfirmedClear intention for final report confirmation.
No concerns here.
74-78: Event: PayerReportFullySettledEvent name and parameters convey full settlement status clearly.
84-91: Function: submitPayerReportEnsure that off-chain or other watchers can reconstruct the full usage data from the emitted event. The doc references only partial data stored on chain. If an index or authority is required, confirm references or pointers are included.
Would you like a script to review wheresubmitPayerReportis called and confirm presence of indexing for off-chain watchers?
93-100: Function: attestPayerReportMethod signature is concise. Encouraging nodes to provide attestations fosters trustless validation.
110-120: Function: getPayerReportReading the entire
PayerReportstruct from a single call is convenient for on-chain introspection.
151-155: Function: getMaxBatchSizeA read-only function for batch size checks is a useful complement.
fbac
force-pushed
the
03_03-interface-payerreport
branch
from
e5ec448 to
96c8393
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/src/interfaces/IPayer.sol (1)
385-385: Improved return value documentationThe return value documentation now clearly indicates the return type as
nodesContract, improving clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/interfaces/IPayer.sol(5 hunks)contracts/src/interfaces/IPayerReport.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/src/interfaces/IPayerReport.sol
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (8)
contracts/src/interfaces/IPayer.sol (8)
71-71: The UsageSettled event has been updated with an originatorNode parameterThe event structure has been changed to include the
originatorNodeas an indexed parameter, which allows for more efficient filtering by the node that originated the usage settlement. This aligns with the new flow where reports are generated by specific nodes and verified by others.
73-77: Terminology changed from "rewards" to "distribution" for better clarityThe documentation and naming have been updated to use "distribution contract" instead of "rewards contract," which likely better represents the contract's role in distributing fees rather than simply rewarding nodes.
99-99: Error renamed for consistency with new terminologyThe error has been renamed from
NotRewardsContracttoNotDistributionContractto maintain consistency with the terminology changes throughout the interface.
292-292: Function renamed for consistency with new terminologyThe function has been renamed from
transferFeesToRewardstotransferFeesToDistributionto maintain consistency with the terminology changes throughout the interface.
365-365: Function renamed for consistency with new terminologyThe function has been renamed from
setRewardsContracttosetDistributionContractto maintain consistency with the terminology changes throughout the interface.
379-379: Function renamed for consistency with new terminologyThe function has been renamed from
getRewardsContracttogetDistributionContractto maintain consistency with the terminology changes throughout the interface.
273-273: Parameter added for Merkle proof verificationReferencing the previous discussion around the need for a Batch Merkle Proof, I see you've redesigned the system to handle this concern. The
originatorNodeandreportIndexparameters now serve to reference a specific report that has already had its Merkle proof verified, addressing the concern raised earlier about proof verification.
263-277:Details
❓ Verification inconclusive
New settlement flow using PayerReport and Merkle proofs
The
settleUsagefunction has been significantly modified to work with the new PayerReport interface. It now:
- Takes an
originatorNodeparameter to track which node submitted the report- Includes a
reportIndexparameter to reference the specific report- Assumes the Merkle proof verification has already been performed by the PayerReport contract
The design separates concerns appropriately, with proof verification happening in PayerReport and actual settlement in Payer.
🏁 Script executed:
#!/bin/bash # Check if there's an IPayerReport interface implementation that handles the Merkle proof verification # Expected: a file with IPayerReport interface defining functions for submitting and verifying reports echo "Searching for PayerReport interface definition..." fd -e sol | xargs grep -l "interface IPayerReport" echo "Searching for Merkle proof verification in the codebase..." rg -A 5 -B 5 "verify.*[Mm]erkle" --type solLength of output: 404
Below is a revised review comment that reflects the latest context. Note that the IPayerReport interface is present, confirming the intended separation of concerns. However, since our automated search for Merkle proof verification code required a modified search and further manual verification is advised, please double-check that the actual Merkle proof verification is implemented as expected in the PayerReport.
Settlement Flow Update – Verify Merkle Proof Delegation
The modifications to the
settleUsagefunction incontracts/src/interfaces/IPayer.solcorrectly reflect the new settlement flow. In this setup:
- The addition of the
originatorNodeparameter tracks which node submitted the report.- The inclusion of the
reportIndexparameter clearly references a specific report.- The design assumes that the Merkle proof has already been verified in the PayerReport contract.
Next Steps:
- Verify: Confirm that the IPayerReport interface (located at
contracts/src/interfaces/IPayerReport.sol) implements Merkle proof verification as intended.- Manual check: Ensure that the delegated Merkle proof verification logic is correctly wired with the new settlement flow.
That's probably fine. We'll just need to think through how we ensure that all Payer Reports that can be confirmed do get confirmed. If the node doing the attesting doesn't confirm it, we need some way to remind someone else to do the work or an automation to find dangling reports. |
Context around Merkle Trees in #563
Closes #499
The following is the expected sequence of events:
PayerReport.attestPayerReportif they can confirm its validity.confirmPayerReportcan be called.PayerReportis confirmed, any node can submit batches tosettleUsageBatch. ThePayerReportcontract is expected to track batches submissions to prevent duplicates.settleUsageBatchverifies the provided payers and amounts leafs are included in the PayerReport Merkle tree. If verified, the Payer.settleUsage is called with the list of payers and amounts to settle the usage.Nodes attest to PayerReports, and once majority has been reached
confirmPayerReportcan be called for settlement.Summary by CodeRabbit
Summary by CodeRabbit