Skip to content

Add Node Payout Doc section#243

Open
mkysel wants to merge 1 commit intomainfrom
mkysel/add-node-payout-doc
Open

Add Node Payout Doc section#243
mkysel wants to merge 1 commit intomainfrom
mkysel/add-node-payout-doc

Conversation

@mkysel
Copy link
Collaborator

@mkysel mkysel commented Dec 18, 2025

Add Node Payout documentation and restructure payer reports in payer-reports.md

Update payer-reports.md with the Node Payout section, sequence diagrams, PayerReport accounting model, onchain contract interactions, signature requirements, immutable submission parameters, and settlement semantics.

📍Where to Start

Start with the new "Node Payout" section and the updated "PayerReport" overview in payer-reports.md.


Macroscope summarized 5437d59.

Summary by CodeRabbit

  • Documentation
    • Expanded payer report documentation with detailed workflow descriptions, including report submission, settlement, and node payout processes.
    • Added JSON examples and improved error handling coverage.
    • Enhanced readability with clearer organization and explicit section headings.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

Walkthrough

Documentation expansion for payer-report system, replacing high-level exposition with detailed descriptions of PayerReport structure, attestation/signing requirements, onchain submission, settlement mechanics, node payout lifecycle, and protocol-fee handling. Added JSON examples, explicit subheadings, sequence diagram context, and workflow clarifications.

Changes

Cohort / File(s) Change Summary
Payer Report Documentation Expansion
doc/payer-reports.md
Expanded and restructured offchain message handling documentation with detailed PayerReport structure including JSON example, clarified attestation/signing with explicit subheadings and ordering requirements, added onchain submission narrative with sequence diagram context, expanded settlement mechanics and node payout lifecycle (pull-based flow with DistributionManager), clarified protocol-fee accrual and settlement, improved error handling coverage and general formatting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Multiple interrelated sections reorganized and expanded (PayerReport structure, attestation, settlement, payout lifecycle) requiring verification of consistency across sections
  • Addition of concrete examples (JSON snippet) and diagram references requiring accuracy validation
  • Clarifications around contract coordination (PayerReportManager, PayerRegistry, DistributionManager) and flow sequencing need careful review against system design

Possibly related PRs

Suggested labels

documentation

Suggested reviewers

  • fbac
  • deluca-mike
  • neekolas

Poem

🐰 A rabbit hops through payer flows,
Where reports and settle-dance now grows,
Signatures ordered, nodes align,
Node payouts pull—a design so fine!
Through Merkle trees and fee refrain,
Clear docs now guide us once again! 📋✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add Node Payout Doc section' accurately describes the main change: expanding documentation for node payout lifecycle and related concepts in the payer-reports.md file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mkysel/add-node-payout-doc

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bfc44bf and 5437d59.

📒 Files selected for processing (1)
  • doc/payer-reports.md (6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-05-13T06:06:51.203Z
Learnt from: deluca-mike
Repo: xmtp/smart-contracts PR: 59
File: src/settlement-chain/DistributionManager.sol:90-99
Timestamp: 2025-05-13T06:06:51.203Z
Learning: The DistributionManager.claim() function should perform array length validation between originatorNodeIds_ and payerReportIndices_ arrays using the ArrayLengthMismatch() custom error (already defined in IPayerReportManager) before calling getPayerReports(), even though PayerReportManager also performs this check. This provides proper separation of concerns and makes the contract more robust by catching errors earlier.

Applied to files:

  • doc/payer-reports.md
📚 Learning: 2025-05-13T06:02:34.726Z
Learnt from: deluca-mike
Repo: xmtp/smart-contracts PR: 59
File: src/settlement-chain/DistributionManager.sol:102-108
Timestamp: 2025-05-13T06:02:34.726Z
Learning: The `DistributionManager` contract's `claim` function assumes that the array returned by `PayerReportManager.getPayerReports()` preserves the same order as the input arrays. This is a deliberate design decision since both are first-party contracts, and integration tests are expected to catch any issues if implementations change.

Applied to files:

  • doc/payer-reports.md
📚 Learning: 2025-05-13T06:06:51.203Z
Learnt from: deluca-mike
Repo: xmtp/smart-contracts PR: 59
File: src/settlement-chain/DistributionManager.sol:90-99
Timestamp: 2025-05-13T06:06:51.203Z
Learning: The DistributionManager.claim() function should perform array length validation for originatorNodeIds_ and payerReportIndices_ parameters before calling getPayerReports(), even though PayerReportManager also performs this check. This ensures proper separation of concerns and increased robustness. The same error type (ArrayLengthMismatch) should be used for consistency with PayerReportManager.

Applied to files:

  • doc/payer-reports.md
📚 Learning: 2025-05-13T06:06:51.203Z
Learnt from: deluca-mike
Repo: xmtp/smart-contracts PR: 59
File: src/settlement-chain/DistributionManager.sol:90-99
Timestamp: 2025-05-13T06:06:51.203Z
Learning: The DistributionManager.claim() function should perform its own validation of array lengths between originatorNodeIds_ and payerReportIndices_ parameters before calling getPayerReports(), even though PayerReportManager.getPayerReports() also performs this check. This ensures proper separation of concerns and increased robustness.

Applied to files:

  • doc/payer-reports.md
📚 Learning: 2025-05-06T08:42:22.906Z
Learnt from: deluca-mike
Repo: xmtp/smart-contracts PR: 56
File: src/settlement-chain/PayerReportManager.sol:325-333
Timestamp: 2025-05-06T08:42:22.906Z
Learning: The XMTP protocol limits canonical nodes to a maximum that can fit in a `uint8` (255 nodes), which is why the `validSignatureCount_` variable in `PayerReportManager` is typed as `uint8`.

Applied to files:

  • doc/payer-reports.md
📚 Learning: 2025-05-14T08:30:12.114Z
Learnt from: deluca-mike
Repo: xmtp/smart-contracts PR: 64
File: src/settlement-chain/SettlementChainGateway.sol:17-18
Timestamp: 2025-05-14T08:30:12.114Z
Learning: The SettlementChainGateway contract in the xmtp/smart-contracts repository follows the CEI (checks, effects, interactions) pattern as a defense against reentrancy attacks. The team prefers to wait for formal audits to determine if explicit ReentrancyGuard implementations are necessary rather than adding them preemptively.

Applied to files:

  • doc/payer-reports.md
🪛 LanguageTool
doc/payer-reports.md

[grammar] ~22-~22: Ensure spelling is correct
Context: ...LS message types are published directly onchain, and payers interacting through the Gat...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~23-~23: Ensure spelling is correct
Context: ...- Three MLS message types are published offchain to xmtpd nodes via APIs. For the offch...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~25-~25: Ensure spelling is correct
Context: ...chain to xmtpd nodes via APIs. For the offchain message types, _fees are accounted for ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~28-~28: Ensure spelling is correct
Context: ...etwork’s economic model: they reconcile offchain usage with onchain balances, ensure con...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~28-~28: Ensure spelling is correct
Context: ...del: they reconcile offchain usage with onchain balances, ensure consensus among nodes,...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~30-~30: Ensure spelling is correct
Context: ...tor payouts. Payer reports are managed onchain by the PayerReportManager contract an...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~50-~50: Ensure spelling is correct
Context: ...t field order and types are part of the onchain ABI contract. Any mismatch between offc...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~50-~50: Ensure spelling is correct
Context: ...hain ABI contract. Any mismatch between offchain code, interfaces, or consuming contract...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~221-~221: Ensure spelling is correct
Context: ...r node operators. This process is fully onchain, deterministic, and pull-based: node op...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[style] ~253-~253: Consider removing “of” to be more concise
Context: ...yout A node cannot receive fees unless all of the following are true: 1. The payer report...

(ALL_OF_THE)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Slither
  • GitHub Check: Code Coverage
  • GitHub Check: Formatting check
  • GitHub Check: Contacts Sizes
  • GitHub Check: Tests
  • GitHub Check: Gas Report
  • GitHub Check: Lint check
🔇 Additional comments (6)
doc/payer-reports.md (6)

21-30: Excellent expansion of the introduction.

The reworked introduction clearly establishes the context of offchain message types and explains why PayerReports are necessary. The connection to asynchronous fee settlement and the three managing contracts provides good conceptual grounding for readers.


36-50: Well-considered addition of JSON example and ABI consistency warning.

The concrete JSON example provides clarity for implementers, and the "Important" note about field order and type consistency is excellent defensive documentation. This directly prevents subtle encoding mismatches between offchain code and onchain ABI decoding.


127-134: Clarified signature validation requirements with explicit subheadings.

The restructured headings and the addition of the "ordered and unique" requirement for node IDs improve clarity. The struct definition provides helpful context for implementation. However, verify that this ordering and uniqueness requirement is actually enforced in the PayerReportManager contract during submission.

Can you confirm that PayerReportManager.submit() enforces that signing node IDs are ordered and unique before accepting the report?


144-217: Comprehensive and well-diagrammed submission and settlement flow.

The sequence diagrams for both submission and settlement clearly illustrate the multi-contract choreography. The callouts explaining what is locked in at submission (reporting window, fee rate, canonical node set) and the multi-batch settlement capability with offset tracking are important details that reduce ambiguity for implementers.


219-317: Excellent comprehensive documentation of the node payout lifecycle.

The new Node Payout section significantly improves documentation completeness. The preconditions are clearly enumerated, the claim/withdraw distinction is explicitly called out (with emphasis that claim is accounting-only), and the protocol fees are separated from node rewards. The sequence diagram and detailed explanations of the pull-based model and multi-batch withdrawal behavior are helpful.

Verify the following against the actual DistributionManager implementation:

  1. Does the claim() function signature match lines 268–272?
  2. Is withdrawIntoUnderlying() available as documented in line 292?
  3. Does the pull-from-PayerRegistry fallback behavior match line 299?

Can you confirm these implementation details against the actual DistributionManager contract?


21-30: Static analysis flags are acceptable false positives.

The static analysis tool flags "onchain" and "offchain" as spelling errors, but these are standard, widely-accepted technical terms in blockchain documentation and are used consistently throughout the file. Similarly, the suggestion to remove "of" from "all of the following" at line 253 would reduce clarity. No action needed on these flags.

Also applies to: 50-50, 221-221, 253-253


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mkysel mkysel requested review from a team and KevinSmall December 18, 2025 14:23
@github-actions
Copy link

Changes to gas cost

Generated at commit: 4a565d7dd55a5e746976837baf252ebda7806566, compared to commit: bfc44bfbd2ca1523282c49b6caa76d423fef21d7

🧾 Summary (20% most significant diffs)

Contract Method Avg (+/-) %
Proxy __setPauseStatus
fallback
-14,757 ✅
-147,554,121 ✅
-45.83%
-99.62%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
Proxy 105,099 (0) __mint
__setMaxPayloadSize
__setMinPayloadSize
__setPauseStatus
__setPayloadBootstrapper
__setSequenceId
adminParameterKey
contractName
fallback
implementation
maxPayloadSize
migrate
migratorParameterKey
minPayloadSizeParameterKey
name
parameterRegistry
paused
pausedParameterKey
symbol
version
56,427 (0)
821 (0)
804 (0)
826 (0)
48,693 (+228)
7,332 (0)
5,309 (+45)
5,265 (+1)
4,971 (-34)
5,145 (+45)
7,140 (0)
29,341 (+740)
5,310 (+68)
5,308 (0)
8,049 (+62)
5,028 (+23)
7,180 (+32)
5,352 (+67)
8,044 (+45)
5,287 (+56)
0.00%
0.00%
0.00%
0.00%
+0.47%
0.00%
+0.85%
+0.02%
-0.68%
+0.88%
0.00%
+2.59%
+1.30%
0.00%
+0.78%
+0.46%
+0.45%
+1.27%
+0.56%
+1.07%
62,010 (-1,284)
10,491 (-8,046)
11,225 (-4,642)
17,444 (-14,757)
48,693 (+76)
24,891 (-280)
5,309 (+24)
5,294 (0)
562,995 (-147,554,121)
5,163 (+19)
7,162 (-14)
32,769 (+87)
5,323 (+15)
5,308 (-7)
8,049 (+31)
5,044 (+9)
7,191 (+25)
5,352 (+21)
8,044 (+23)
5,309 (+23)
-2.03%
-43.41%
-29.26%
-45.83%
+0.16%
-1.11%
+0.45%
0.00%
-99.62%
+0.37%
-0.20%
+0.27%
+0.28%
-0.13%
+0.39%
+0.18%
+0.35%
+0.39%
+0.29%
+0.44%
56,439 (0)
821 (0)
804 (0)
848 (-47,582)
48,693 (0)
27,232 (0)
5,309 (+24)
5,287 (0)
31,957 (-16,895)
5,167 (+22)
7,162 (-23)
33,609 (+33)
5,330 (+19)
5,308 (0)
8,049 (+31)
5,044 (+16)
7,191 (+22)
5,352 (+17)
8,044 (+23)
5,309 (+28)
0.00%
0.00%
0.00%
-98.25%
0.00%
0.00%
+0.45%
0.00%
-34.58%
+0.43%
-0.32%
+0.10%
+0.36%
0.00%
+0.39%
+0.32%
+0.31%
+0.32%
+0.29%
+0.53%
73,527 (0)
48,425 (0)
48,408 (0)
48,452 (-73)
48,693 (0)
31,336 (0)
5,309 (0)
5,330 (-6)
2,861,555,004 (0)
5,179 (0)
7,185 (0)
37,507 (-35)
5,330 (-48)
5,308 (-22)
8,049 (0)
5,061 (0)
7,202 (0)
5,352 (0)
8,044 (0)
5,331 (0)
0.00%
0.00%
0.00%
-0.15%
0.00%
0.00%
0.00%
-0.11%
0.00%
0.00%
0.00%
-0.09%
-0.89%
-0.41%
0.00%
0.00%
0.00%
0.00%
0.00%
0.00%
46 (-6)
26 (-7)
24 (-5)
32 (-29)
6 (-3)
22 (-1)
1 (-3)
3 (-11)
16,126 (+16,068)
3 (-12)
2 (-3)
5 (-45)
3 (-11)
2 (-1)
1 (-1)
4 (-16)
2 (-15)
2 (-5)
1 (-1)
3 (-11)

@mkysel
Copy link
Collaborator Author

mkysel commented Dec 18, 2025

Relates to xmtp/xmtpd#1447

@github-actions
Copy link

LCOV of commit 5437d59 during Solidity #630

Summary coverage rate:
  lines......: 99.9% (1332 of 1333 lines)
  functions..: 100.0% (381 of 381 functions)
  branches...: no data found

Files changed coverage rate: n/a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants