Followup: custom HTLC TLVs #2504
Conversation
Remove print statement, remove some unnecessary checks copied over from test utils, make minor simplifications, wrap especially long lines.
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #2504 +/- ##
=======================================
Coverage 90.39% 90.39%
=======================================
Files 106 106
Lines 56295 56267 -28
Branches 56295 56267 -28
=======================================
- Hits 50887 50863 -24
+ Misses 5408 5404 -4
☔ View full report in Codecov by Sentry. |
| } else { None }; | ||
| let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect(); | ||
| let keysend_tlv = keysend_preimage.map(|preimage| (5482373484, preimage.encode())); | ||
| let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(keysend_tlv.iter()).collect(); |
There was a problem hiding this comment.
I don't buy that we need to collect here. We construct an iterator and then turn it into a vec, and then pass it to the macro as an iterator. The macro uses the iterator twice, iirc, so we can't just generate the iterator here and then give it to the macro, but we can paste this line in the macro call and it should just work (or have the macro clone the iterator before use).
There was a problem hiding this comment.
We sort after we collect into a vec?
There was a problem hiding this comment.
Oh oops, right, hmmm, I mean its definitely possible to avoid the collect but probably not entirely worth writing a custom iterator for it.
There was a problem hiding this comment.
I guess its not that crazy to build the iterator by hand using https://doc.rust-lang.org/std/iter/fn.from_fn.html but no need to bother.
| } else { None }; | ||
| let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect(); | ||
| let keysend_tlv = keysend_preimage.map(|preimage| (5482373484, preimage.encode())); | ||
| let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(keysend_tlv.iter()).collect(); |
There was a problem hiding this comment.
I guess its not that crazy to build the iterator by hand using https://doc.rust-lang.org/std/iter/fn.from_fn.html but no need to bother.
lightning/src/util/ser_macros.rs
Outdated
| @@ -143,6 +144,9 @@ macro_rules! encode_tlv_stream { | |||
| #[macro_export] | |||
| macro_rules! _encode_tlv_stream { | |||
| ($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),* $(,)*}) => { { | |||
| _encode_tlv_stream!($stream, { $(($type, $field, $fieldty)),* }, &[]) | |||
There was a problem hiding this comment.
This should be $crate::_encode_tlv_stream
lightning/src/util/ser_macros.rs
Outdated
| @@ -580,6 +577,7 @@ macro_rules! _decode_tlv_stream_range { | |||
| /// | |||
| /// For example, | |||
| /// ``` | |||
| /// # use lightning::_encode_tlv_stream; | |||
There was a problem hiding this comment.
Then we can drop the doc changes.
Don't collect iterators to compare, minorly simplify encoding the keysend TLV, combine the _encode_tlv_stream variants to check that the ordering of TLVs is correct including custom TLVs.
alecchendev
force-pushed
the
2023-08-custom-tlvs-followup
branch
from
1077825 to
83f0dbc
Compare
Couple follow ups on #2308, let me know if I missed anything.
No idea why the compiler wanted me to add the import on the doc tests