Add and Revise Receiver Interface Comments - #1710
Conversation
Coverage Report for CI Build 30574097238Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.3%) to 86.494%Details
Uncovered Changes
Coverage Regressions329 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
968f5e6 to
2183b5d
Compare
2183b5d to
438d440
Compare
| /// the increased transaction size. If a sender specifies a receiver output for this purpose, this | ||
| /// function sets that parameter to None so that it is ignored in subsequent steps of the | ||
| /// receiver flow. This protects the receiver from accidentally subtracting fees from their own | ||
| /// outputs. |
There was a problem hiding this comment.
This paragraph about additional_fee_contribution being cleared when it points at a receiver-owned output got dropped in the rewrite. That protection is still implemented, just moved to OriginalContext::new in common/mod.rs:35-40
Worth keeping this detail, it's not obvious from the new "ensure at least one output pays the receiver" wording, and it's a real guarantee callers should know about.
There was a problem hiding this comment.
Updated. What do you think?
There was a problem hiding this comment.
looks better IMO, thank you!
There was a problem hiding this comment.
thank you for your review!
| /// Validates whether the original PSBT contains outputs which pay to the receiver and only | ||
| /// then proceeds to the next typestate. | ||
| /// Identify which outputs in the original transaction belong to the receiver | ||
| /// and ensure at least one output pays the receiver. | ||
| /// | ||
| /// Additionally, this function also protects the receiver from accidentally subtracting fees | ||
| /// from their own outputs: when a sender is sending a proposal, | ||
| /// they can select an output which they want the receiver to subtract fees from to account for | ||
| /// the increased transaction size. If a sender specifies a receiver output for this purpose, this | ||
| /// function sets that parameter to None so that it is ignored in subsequent steps of the | ||
| /// receiver flow. This protects the receiver from accidentally subtracting fees from their own | ||
| /// outputs. |
There was a problem hiding this comment.
Same as the note on v1/mod.rs, this paragraph about additional_fee_contribution sanitization was dropped here too
There was a problem hiding this comment.
Also updated.
Rework the doc comments in the v2 receive module so the state-level comments are concise and point to the method that advances to the next typestate, while the per-method comments align with the wording added to the FFI interface. Method docs now name their explicit transition return type and describe the state each transition yields once successfully persisted.
Apply the same doc comment rework to the v1 receive module: state-level comments are concise and point to the method that advances to the next typestate, while per-method comments align with the wording used in the FFI interface. Unlike v2, v1 methods return the next state directly, so the docs describe the returned state without transition or persistence framing.
438d440 to
0cb183a
Compare
There was a problem hiding this comment.
concept ACK but I think all the docstrings on the impl blocks need to be moved to the struct declaration like you did for UncheckedOriginalPayload, for both v1 and v2 modules (and maybe also the Sender, I haven't checked how they are written there).
correction: let's move the UncheckedOriginalPayload struct docstring to its impl block for consistency with the other typestates.
Add a short doc comment to each v2 receiver typestate struct so the type appears with a one-line summary in rustdoc, with a pointer to the Receiver<State> impl block where the detailed documentation lives.
|
@spacebear21 feedback addressed. I also went ahead and added some super simple doc strings to all the structs to state their purpose and point to the more in depth documentation on the |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
tACK docs build successfully locally and everything looks clean.
5 participants
Summary
This PR builds off of #1653 which added clear and consistent comments to the FFI receiver interface where they were missing before. This PR expands that work to revise the v1 and v2 comments to align closely with the patterns established in the FFI interface and add consistency to the doc strings between all the different typestates and interfaces. The goal of this PR was to make the documentation on how to use these receiver interfaces more clear and concise, and if deemed a strict improvement might be good to include in PDK v1.0 release.
Claude Opus 4.8 was used to collaboratively create and revise these doc strings.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.