REVIEW-98b9bbba.md (1507B)
1 # Review: 98b9bbba 2 3 ## Findings (ordered by severity) 4 5 - Medium: `decodeEnvelope` hard-codes `decodeTlvStreamWith (const False)`, so 6 every even TLV type in extensions is treated as unknown and rejected. This 7 makes it impossible to accept negotiated/known even extension TLVs in the 8 future, and forces odd TLVs to be discarded even if a caller wants to 9 preserve them. Consider a `decodeEnvelopeWith` that accepts an `isKnown` 10 predicate (or returns raw extension bytes) to make extension handling 11 configurable. 12 (`lib/Lightning/Protocol/BOLT1/Codec.hs:300-320`) 13 14 - Low: `encodeMessage` can emit payloads that exceed the 65535 total message 15 limit; only `encodeEnvelope` checks size. Since `encodeMessage` is exported, 16 callers could bypass the limit unintentionally. If you want the API to enforce 17 the spec by default, consider a size check there too. 18 (`lib/Lightning/Protocol/BOLT1/Codec.hs:110-156`) 19 20 - Low: Tests still use `error` via `assertFailure'`, which is a partial failure 21 path (even if only in tests). If you want to keep the “avoid partials” 22 discipline, replace with `assertFailure` in an IO context or a total helper. 23 (`test/Main.hs:356-362`) 24 25 ## Open questions 26 27 - Should extension TLVs be configurable at the API boundary (predicate for 28 known types), or should the library always reject all even extension types 29 until specific ones are modeled? 30 - Should `encodeMessage` enforce the total size limit, or should size 31 validation be strictly an envelope concern? 32