Review: Introduce Recurrence TLVs for Bolt12 Messages#12
Open
shaavan wants to merge 6 commits into
Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements end-to-end BOLT12 recurrence support by introducing recurrence types for offers, invoices, and invoice requests, updating TLV encoding/decoding to handle new recurrence fields, wiring invoice request builders and response paths to compute and propagate recurrence basetime, and enforcing that refunds and static invoices reject recurrence TLVs. ChangesBOLT12 Recurrence Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightning/src/offers/refund.rs (1)
941-1010:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefund parser silently accepts
recurrence_tokenon the invoice_request side.The destructure at line 941 binds
recurrence_token: _, and the subsequent check at lines 1007-1010 only rejectsrecurrence_counter | recurrence_start | recurrence_cancel. This contradicts the PR's stated intent of rejecting all recurrence TLVs on refunds: a refund containing onlyrecurrence_tokenwill parse successfully. The new testfails_parsing_refund_with_unexpected_recurrence_fieldsdoes not exerciserecurrence_tokeneither, so coverage misses the gap.🛡️ Proposed fix to validate `recurrence_token` consistently
- recurrence_counter, - recurrence_start, - recurrence_cancel, - recurrence_token: _, + recurrence_counter, + recurrence_start, + recurrence_cancel, + recurrence_token, }, @@ - if recurrence_counter.is_some() || recurrence_start.is_some() || recurrence_cancel.is_some() - { + if recurrence_counter.is_some() + || recurrence_start.is_some() + || recurrence_cancel.is_some() + || recurrence_token.is_some() + { return Err(Bolt12SemanticError::UnexpectedRecurrence); }Also extend
fails_parsing_refund_with_unexpected_recurrence_fieldsto settlv_stream.2.recurrence_tokenand assertUnexpectedRecurrence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightning/src/offers/refund.rs` around lines 941 - 1010, The code currently discards recurrence_token when destructuring tlv_stream (recurrence_token: _) so refunds with only recurrence_token slip through; modify the destructure to capture recurrence_token (e.g., recurrence_token) and add it to the existing recurrence checks (the block that returns Bolt12SemanticError::UnexpectedRecurrence when recurrence_counter/start/cancel are present) so that if recurrence_token.is_some() you return UnexpectedRecurrence; also update the test fails_parsing_refund_with_unexpected_recurrence_fields to set tlv_stream.2.recurrence_token and assert it yields UnexpectedRecurrence. Ensure you reference recurrence_token and tlv_stream when making the changes.
🧹 Nitpick comments (1)
lightning/src/offers/invoice.rs (1)
798-807: 💤 Low value
InvoiceRecurrenceispubbut has no constructor or accessors.Both fields are private and no
new(..)/ getters are provided. External consumers of the crate cannot construct or read this type once it surfaces in real invoices, and the only current usage is internal. If this struct is intended to remain crate-private until the recurrence logic is wired up, consider downgrading visibility topub(super)/pub(crate); if it is meant to be part of the public surface, exposing minimal accessors (pub fn recurrence_basetime,pub fn recurrence_token) alongside the struct will avoid a follow-up breaking-change cycle when consumers start needing them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lightning/src/offers/invoice.rs` around lines 798 - 807, The public struct InvoiceRecurrence exposes no way for external code to construct or read its private fields; either reduce its visibility to restrict it to the crate (change InvoiceRecurrence to pub(crate) or pub(super)) if it's not yet part of the public API, or keep it pub and add a minimal public API: a constructor (e.g., InvoiceRecurrence::new(recurrence_basetime: u64, recurrence_token: Vec<u8>)) and/or read-only accessors (pub fn recurrence_basetime(&self) -> u64 and pub fn recurrence_token(&self) -> &[u8]) so callers can create and inspect instances without breaking encapsulation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lightning/src/offers/invoice_request.rs`:
- Around line 731-801: The public enum InvoiceRequestRecurrence is unusable
externally because WithOfferBasetimeRecurrence and
WithoutOfferBasetimeRecurrence have private fields and
InvoiceRequestRecurrence::new and fields() are crate-private; make the API
usable by making InvoiceRequestRecurrence::new pub (or adding a new pub
constructor) and exposing accessors: add pub constructors like
WithOfferBasetimeRecurrence::new(...) and
WithoutOfferBasetimeRecurrence::new(...) or make their fields pub, and provide
pub getter methods (e.g., InvoiceRequestRecurrence::fields() -> (Option<u32>,
Option<u32>, Option<&Vec<u8>>, Option<&()>) as pub) so downstream callers
(including set_invoice_request_recurrence and
InvoiceRequestFields::invoice_request_recurrence) can construct and read
recurrence data.
- Around line 1021-1039: The verify_recurrence_token function currently calls
todo!() when invoice_request_recurrence contains a token which panics; change
this to fail-closed by returning an Err(()) for any token-bearing request (i.e.,
when recurrence_token is Some(_token)) while preserving the existing Ok(None)
path when there is no token; update the branch in verify_recurrence_token (and
any immediate callers if necessary) to treat Err(()) as a request rejection
instead of allowing a panic.
In `@lightning/src/offers/offer.rs`:
- Around line 674-718: The start_time helper on RecurrencePeriod currently
panics via expect on multiple overflow paths; change its signature from fn
start_time(self, basetime: u64, period_count: u32) -> u64 to return a fallible
Result<u64, InvalidRecurrence> (or the crate's semantic recurrence error type),
then replace every .expect(...) and any panicking conversions with checked_*
combinators and map None / conversion errors to Err(InvalidRecurrence). Update
all branches (Seconds, Days, Months) to propagate errors instead of calling
expect (e.g., use .and_then(...).ok_or(InvalidRecurrence) and map_err for
try_from failures), and update callers of RecurrencePeriod::start_time to handle
the Result and convert failures into InvalidRecurrence.
- Around line 1758-1779: The match building offer_recurrence currently ignores
orphan recurrence_paywindow/recurrence_limit TLVs because it only
pattern-matches recurrence_compulsory, recurrence_optional, and recurrence_base;
update the match (or add a guard) to explicitly reject any case where
recurrence_paywindow or recurrence_limit are Some while both
recurrence_compulsory and recurrence_optional are None (i.e., when they appear
without a corresponding recurrence period/base), returning
Err(Bolt12SemanticError::InvalidRecurrence); modify the match arms around
offer_recurrence (using the variables recurrence_compulsory,
recurrence_optional, recurrence_base, recurrence_paywindow, recurrence_limit) so
only the valid Compulsory and Optional constructions produce
Some(Recurrence::Compulsory{...}) or Some(Recurrence::Optional{...}) and all
other combinations (including orphan paywindow/limit) return the
InvalidRecurrence error.
In `@lightning/src/offers/static_invoice.rs`:
- Around line 704-706: The check currently rejects only
invoice_recurrence_basetime/invoice_recurrence_token but misses recurrence TLVs
embedded in the offer; update the same guard to also reject
offer_recurrence_basetime and offer_recurrence_token (the fields parsed by
OfferContents::try_from and later serialized by for_offer_using_derived_keys) so
static invoices cannot carry recurrence, and add the corresponding validation to
the static invoice builder path to mirror this rejection.
---
Outside diff comments:
In `@lightning/src/offers/refund.rs`:
- Around line 941-1010: The code currently discards recurrence_token when
destructuring tlv_stream (recurrence_token: _) so refunds with only
recurrence_token slip through; modify the destructure to capture
recurrence_token (e.g., recurrence_token) and add it to the existing recurrence
checks (the block that returns Bolt12SemanticError::UnexpectedRecurrence when
recurrence_counter/start/cancel are present) so that if
recurrence_token.is_some() you return UnexpectedRecurrence; also update the test
fails_parsing_refund_with_unexpected_recurrence_fields to set
tlv_stream.2.recurrence_token and assert it yields UnexpectedRecurrence. Ensure
you reference recurrence_token and tlv_stream when making the changes.
---
Nitpick comments:
In `@lightning/src/offers/invoice.rs`:
- Around line 798-807: The public struct InvoiceRecurrence exposes no way for
external code to construct or read its private fields; either reduce its
visibility to restrict it to the crate (change InvoiceRecurrence to pub(crate)
or pub(super)) if it's not yet part of the public API, or keep it pub and add a
minimal public API: a constructor (e.g.,
InvoiceRecurrence::new(recurrence_basetime: u64, recurrence_token: Vec<u8>))
and/or read-only accessors (pub fn recurrence_basetime(&self) -> u64 and pub fn
recurrence_token(&self) -> &[u8]) so callers can create and inspect instances
without breaking encapsulation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37f38ee9-4f6e-46bc-a5d4-d18dde507bcb
📒 Files selected for processing (8)
fuzz/src/invoice_request_deser.rslightning/src/ln/offers_tests.rslightning/src/offers/invoice.rslightning/src/offers/invoice_request.rslightning/src/offers/offer.rslightning/src/offers/parse.rslightning/src/offers/refund.rslightning/src/offers/static_invoice.rs
Comment on lines
+731
to
+801
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub enum InvoiceRequestRecurrence { | ||
| /// Recurrence fields for offers with an explicit recurrence basetime. | ||
| WithOfferBasetime(WithOfferBasetimeRecurrence), | ||
| /// Recurrence fields for offers without an explicit recurrence basetime. | ||
| WithoutOfferBasetime(WithoutOfferBasetimeRecurrence), | ||
| } | ||
|
|
||
| /// Recurrence fields for invoice requests whose offer recurrence is anchored to an explicit | ||
| /// basetime. | ||
| /// | ||
| /// `counter` identifies which recurring request this is in the payer's sequence. `start` | ||
| /// identifies the schedule period where the payer began recurring payments. `token`, when | ||
| /// present, is the opaque recurrence token echoed from the previous invoice. `cancel`, when | ||
| /// present, marks the request as a recurrence cancellation. | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct WithOfferBasetimeRecurrence { | ||
| counter: u32, | ||
| start: u32, | ||
| token: Option<Vec<u8>>, | ||
| cancel: Option<()>, | ||
| } | ||
|
|
||
| /// Recurrence fields for invoice requests whose offer recurrence is anchored to the first | ||
| /// successful payment instead of an explicit basetime. | ||
| /// | ||
| /// `counter` identifies which recurring request this is in the payer's sequence. `token`, when | ||
| /// present, is the opaque recurrence token echoed from the previous invoice. `cancel`, when | ||
| /// present, marks the request as a recurrence cancellation. | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct WithoutOfferBasetimeRecurrence { | ||
| counter: u32, | ||
| token: Option<Vec<u8>>, | ||
| cancel: Option<()>, | ||
| } | ||
|
|
||
| impl InvoiceRequestRecurrence { | ||
| pub(crate) fn new( | ||
| recurrence_counter: Option<u32>, recurrence_start: Option<u32>, | ||
| recurrence_token: Option<Vec<u8>>, recurrence_cancel: Option<()>, | ||
| ) -> Result<Option<Self>, ()> { | ||
| match (recurrence_counter, recurrence_start, recurrence_token, recurrence_cancel) { | ||
| (None, None, None, None) => Ok(None), | ||
| // Primary invoice requests (counter 0) cannot include token or cancellation state. | ||
| (Some(0), _, Some(_), _) | (Some(0), _, _, Some(_)) => Err(()), | ||
| (Some(counter), Some(start), token, cancel) => { | ||
| let inner = WithOfferBasetimeRecurrence { counter, start, token, cancel }; | ||
| Ok(Some(Self::WithOfferBasetime(inner))) | ||
| }, | ||
| (Some(counter), None, token, cancel) => { | ||
| let inner = WithoutOfferBasetimeRecurrence { counter, token, cancel }; | ||
| Ok(Some(Self::WithoutOfferBasetime(inner))) | ||
| }, | ||
| _ => Err(()), | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn fields(&self) -> (Option<u32>, Option<u32>, Option<&Vec<u8>>, Option<&()>) { | ||
| match self { | ||
| Self::WithOfferBasetime(inner) => ( | ||
| Some(inner.counter), | ||
| Some(inner.start), | ||
| inner.token.as_ref(), | ||
| inner.cancel.as_ref(), | ||
| ), | ||
| Self::WithoutOfferBasetime(inner) => { | ||
| (Some(inner.counter), None, inner.token.as_ref(), inner.cancel.as_ref()) | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Expose a usable public API for the new recurrence types.
InvoiceRequestRecurrence is public, but both concrete recurrence structs hide all fields and InvoiceRequestRecurrence::new is pub(crate). That makes set_invoice_request_recurrence(...) effectively unusable from downstream crates, and consumers also cannot read counter/start/token/cancel back out of InvoiceRequestFields::invoice_request_recurrence.
Please add public constructors/accessors (or make the fields public) before merging this API surface.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lightning/src/offers/invoice_request.rs` around lines 731 - 801, The public
enum InvoiceRequestRecurrence is unusable externally because
WithOfferBasetimeRecurrence and WithoutOfferBasetimeRecurrence have private
fields and InvoiceRequestRecurrence::new and fields() are crate-private; make
the API usable by making InvoiceRequestRecurrence::new pub (or adding a new pub
constructor) and exposing accessors: add pub constructors like
WithOfferBasetimeRecurrence::new(...) and
WithoutOfferBasetimeRecurrence::new(...) or make their fields pub, and provide
pub getter methods (e.g., InvoiceRequestRecurrence::fields() -> (Option<u32>,
Option<u32>, Option<&Vec<u8>>, Option<&()>) as pub) so downstream callers
(including set_invoice_request_recurrence and
InvoiceRequestFields::invoice_request_recurrence) can construct and read
recurrence data.
Comment on lines
+674
to
+718
| fn start_time(self, basetime: u64, period_count: u32) -> u64 { | ||
| match self { | ||
| RecurrencePeriod::Seconds(seconds) => basetime | ||
| .checked_add( | ||
| u64::from(seconds).checked_mul(period_count.into()).expect("seconds overflow"), | ||
| ) | ||
| .expect("unix time overflow"), | ||
| RecurrencePeriod::Days(days) => { | ||
| let days_since_epoch = basetime / Self::SECONDS_PER_DAY; | ||
| let seconds = basetime % Self::SECONDS_PER_DAY; | ||
| let start_day = days_since_epoch | ||
| .checked_add( | ||
| u64::from(days).checked_mul(period_count.into()).expect("days overflow"), | ||
| ) | ||
| .expect("day overflow"); | ||
| start_day | ||
| .checked_mul(Self::SECONDS_PER_DAY) | ||
| .and_then(|day_seconds| day_seconds.checked_add(seconds)) | ||
| .expect("unix time overflow") | ||
| }, | ||
| RecurrencePeriod::Months(months) => { | ||
| let days_since_epoch = basetime / Self::SECONDS_PER_DAY; | ||
| let seconds = basetime % Self::SECONDS_PER_DAY; | ||
| let (year, month, day) = civil_from_days(days_since_epoch as i128); | ||
| let total_months = year | ||
| .checked_mul(12) | ||
| .and_then(|year_in_months| year_in_months.checked_add(month as i128 - 1)) | ||
| .and_then(|base_month| { | ||
| base_month.checked_add( | ||
| i128::from(months) | ||
| .checked_mul(period_count.into()) | ||
| .expect("months overflow"), | ||
| ) | ||
| }) | ||
| .expect("month overflow"); | ||
| let target_year = total_months.div_euclid(12); | ||
| let target_month = total_months.rem_euclid(12) + 1; | ||
| let target_day = day.min(days_in_month(target_year, target_month as u64)); | ||
| let start_day = days_from_civil(target_year, target_month as u64, target_day); | ||
| u64::try_from(start_day) | ||
| .expect("date before unix epoch") | ||
| .checked_mul(Self::SECONDS_PER_DAY) | ||
| .and_then(|day_seconds| day_seconds.checked_add(seconds)) | ||
| .expect("unix time overflow") | ||
| }, |
There was a problem hiding this comment.
Make recurrence start-time calculation fallible.
RecurrenceBase::basetime is parsed from the wire as an unconstrained u64, but this helper uses expect on overflow paths. A malformed offer can therefore panic later during recurrence handling instead of returning InvalidRecurrence. Please propagate a semantic error here rather than aborting the caller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lightning/src/offers/offer.rs` around lines 674 - 718, The start_time helper
on RecurrencePeriod currently panics via expect on multiple overflow paths;
change its signature from fn start_time(self, basetime: u64, period_count: u32)
-> u64 to return a fallible Result<u64, InvalidRecurrence> (or the crate's
semantic recurrence error type), then replace every .expect(...) and any
panicking conversions with checked_* combinators and map None / conversion
errors to Err(InvalidRecurrence). Update all branches (Seconds, Days, Months) to
propagate errors instead of calling expect (e.g., use
.and_then(...).ok_or(InvalidRecurrence) and map_err for try_from failures), and
update callers of RecurrencePeriod::start_time to handle the Result and convert
failures into InvalidRecurrence.
Add draft BOLT 12 recurrence fields to `Offer`, including the public recurrence types and the TLV mapping for optional and compulsory recurrence. Update offer serialization and parsing to preserve `recurrence`, `recurrence_base`, `recurrence_paywindow`, and `recurrence_limit`. Reject recurrence TLVs on refund-like message types that do not support them, and update offer-related test fixtures for the expanded TLV stream. This introduces the offer-side recurrence data model and wire handling without yet implementing recurrence-aware invoice-request validation or payment flow behavior. Spec reference: BOLT 12 offers recurrence TLV fields draft. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add targeted unit tests for the new offer recurrence TLV behavior. The tests cover compulsory and optional recurrence parsing, invalid recurrence field combinations on offers, direct `start_time` coverage for second/day/month schedules, and rejection of recurrence TLVs on refunds. This locks in the current wire-level recurrence behavior and the calendar-based `start_time` helper semantics, while guarding the message-type boundary where refunds must reject recurrence fields. Co-Authored-By: OpenAI Codex <codex@openai.com>
Add parsing and serialization for recurring invoice request fields, including the recurrence counter, basetime start offset, cancellation flag, and the provisional request-side recurrence token. Validate that invoice requests use the recurrence form required by the offer and thread the new fields through the shared invoice-request and refund TLV plumbing plus the payment context copies used when handling offer payments. Spec reference: https://github.com/rustyrussell/bolts/blob/guilt/offers-recurrence/12-offer-encoding.md#tlv-fields-for-invoice_request Co-Authored-By: OpenAI Codex <codex@openai.com>
Add focused invoice request tests for the new recurrence TLVs. The new coverage exercises both supported recurrence encodings and the invalid combinations that should be rejected during parsing. Also extend refund parsing coverage to assert that invoice-request recurrence TLVs remain rejected on refund messages, which protects the shared TLV plumbing added by the recurrence feature commit. Co-Authored-By: OpenAI Codex <codex@openai.com>
shaavan
force-pushed
the
rust-recurrence-review
branch
from
f426c49 to
f7783a0
Compare
Add invoice-side recurrence fields so recurring offers can carry a resolved basetime and opaque token forward to the next invoice request. This wires recurrence state through invoice creation, serialization, and static-invoice validation. The change is needed to let invoice handling participate in the recurrence flow introduced on offers and invoice requests, while keeping refunds and static invoices strict about rejecting unexpected recurrence data. Document the new public recurrence API surfaced by this work so the commit builds cleanly under deny(missing_docs).
Add targeted coverage for the invoice recurrence fields introduced by the preceding feature commit. The new tests prove that recurring invoice TLVs round-trip on offer invoices, that incomplete recurrence TLVs are rejected, and that refund/static invoice parsers still reject unexpected recurrence data. Co-Authored-By: OpenAI Codex <codex@openai.com>
shaavan
force-pushed
the
rust-recurrence-review
branch
from
f7783a0 to
83863de
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes