-
-
Notifications
You must be signed in to change notification settings - Fork 11
Open
Labels
enhancementNew feature or requestNew feature or requestgood first issueGood for newcomersGood for newcomershelp wantedExtra attention is neededExtra attention is needed
Description
Summary
Refactor BOLT11 invoice validation so that all logical validation (uniqueness and mutual exclusivity of tagged fields) is centralized in InvoiceValidationService, rather than being enforced during list mutation in TaggedFieldList.Add. This improves separation of concerns, makes decoding more tolerant (collect first, validate after), and simplifies testing.
Background / Motivation
- Current behavior enforces certain validations inside
TaggedFieldList.Add(e.g., preventing multiple instances of single‑instance fields, or forbidding mutually exclusive fields). This couples data collection with policy/validation. Invoice.Decode(...)already callsInvoiceValidationService.ValidateInvoice(invoice)after constructing the invoice (seesrc/NLightning.Bolt11/Models/Invoice.cs).- Centralizing policy in
InvoiceValidationServiceallowsTaggedFieldListto remain a simple container and avoids partial state/exception paths during parsing.
Scope
- Move all checks for:
- Uniqueness constraints for single‑instance tagged fields.
- Mutual‑exclusivity constraints between specific tagged fields.
- Remove these checks from
TaggedFieldList.Addand implement them inInvoiceValidationService.
Proposed Changes
- Identify existing validations inside
src/NLightning.Bolt11/Models/TaggedFieldList.cs(withinAdd(...)). - Remove/relocate those checks to
src/NLightning.Bolt11/Services/InvoiceValidationService.csinsideValidateInvoice(...)(or dedicated private helpers), operating on the fully constructedInvoice/TaggedFieldList. - Ensure
TaggedFieldList.Addbecomes a pure append/replace method without cross‑field business rules (only basic argument/null safety as needed). - Update validation errors/messages to match current behavior so that external callers see consistent error reporting (same or clearer messages).
- Adjust or add unit tests to cover:
- Duplicate single‑instance tags (should fail in
InvoiceValidationService). - Mutually exclusive combinations (should fail in
InvoiceValidationService). - Decoding path still reaches
InvoiceValidationServiceand reports errors rather than failing during parse/add.
- Duplicate single‑instance tags (should fail in
Acceptance Criteria
-
TaggedFieldList.Addcontains no business rules for uniqueness or mutual exclusivity. -
InvoiceValidationService.ValidateInvoicecontains the moved checks and returns appropriate errors. - All existing tests in
test/NLightning.Bolt11.Testsand integration tests pass, with adjustments only where they relied onTaggedFieldList.Addthrowing. - New/updated tests assert that duplicates and mutually exclusive fields are caught by
InvoiceValidationService. - Decoding invalid invoices yields
InvoiceSerializationExceptionpopulated fromInvoiceValidationServiceerrors (as today), not low‑levelTaggedFieldListexceptions.
Notes / Implementation Hints
- Relevant files:
src/NLightning.Bolt11/Models/TaggedFieldList.cssrc/NLightning.Bolt11/Services/InvoiceValidationService.cssrc/NLightning.Bolt11/Models/Invoice.cs(calls validation after constructing the invoice)- Tests likely touching this behavior:
test/NLightning.Bolt11.Tests/Models/TaggedFields/*test/NLightning.Integration.Tests/BOLT11/TaggedFields/*
- Keep
InvoiceValidationServicethe single source of truth for cross‑field rules. If any validations already live there, consolidate and avoid duplications. - Preserve error messages where possible to avoid breaking consumers that assert on error text; if improvements are needed, update tests accordingly.
Potential Impacts
- Behavior timing changes: exceptions that previously occurred during
TaggedFieldList.Addwill now surface fromInvoiceValidationService; integration tests may need to expect validation errors at a slightly later stage. - Decoding pipeline becomes more linear: parse first, validate after — which is desirable.
Testing Plan
- Unit tests for
InvoiceValidationServicethat build minimalInvoiceobjects with craftedTaggedFieldListinstances covering:- Duplicate single‑instance fields (e.g., two
PayeePubKeyTaggedFields). - Mutually exclusive pairs (per current rules enforced in
TaggedFieldList.Add).
- Duplicate single‑instance fields (e.g., two
- Integration tests for
Invoice.Decode(...)that confirm invalid invoices are rejected with the correct messages viaInvoiceSerializationExceptionsourced from validation results.
Definition of Done
- Code changes merged with tests updated/added.
- All CI builds/tests green on supported target frameworks (
net8.0,net9.0). - Changelog entry (if you maintain one) noting validation logic relocation.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
enhancementNew feature or requestNew feature or requestgood first issueGood for newcomersGood for newcomershelp wantedExtra attention is neededExtra attention is needed