Suggestions from matt

lightning/src/onion_message/packet.rs

@@ -209,7 +209,7 @@ impl<H: CustomOnionMessageHandler> ReadableArgs<(SharedSecret, &H)> for Payload<
 		let rho = onion_utils::gen_rho_from_shared_secret(&encrypted_tlvs_ss.secret_bytes());
 		let mut message_type: Option<u64> = None;
 		let mut message = None;
-		decode_tlv_stream!(&mut rd, {
+		decode_tlv_stream_with_custom_tlv_decode!(&mut rd, {
 			(2, reply_path, option),
 			(4, read_adapter, (option: LengthReadableArgs, rho)),
 		}, |msg_type, msg_reader| {

lightning/src/util/ser_macros.rs

@@ -12,6 +12,7 @@
 
 /// Implements serialization for a single TLV record.
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _encode_tlv {
 	($stream: expr, $type: expr, $field: expr, (default_value, $default: expr)) => {
@@ -41,6 +42,42 @@ macro_rules! _encode_tlv {
 }
 
 /// Implements the TLVs serialization part in a Writeable implementation of a struct.
+///
+/// This should be called inside a method which returns `Result<_, `[`io::Error`]`>`, such as
+/// [`Writeable::write`]. It will only return an `Err` if the stream `Err`s or [`Writeable::write`]
+/// on one of the fields `Err`s.
+///
+/// `$stream` must be a `&mut `[`Writer`] which will receive the bytes for each TLV in the stream.
+///
+/// Fields MUST be sorted in `$type`-order.
+///
+/// Note that the lightning TLV requirements require that a single type not appear more than once,
+/// that TLVs are sorted in type-ascending order, and that any even types be understood by the
+/// decoder.
+///
+/// Any `option` fields which have a value of `None` will not be serialized at all.
+///
+/// For example,
+/// ```
+/// # use lightning::encode_tlv_stream;
+/// # fn write<W: lightning::util::ser::Writer> (stream: &mut W) -> Result<(), lightning::io::Error> {
+/// let mut required_value = 0u64;
+/// let mut optional_value: Option<u64> = None;
+/// encode_tlv_stream!(stream, {
+///     (0, required_value, required),
+///     (1, Some(42u64), option),
+///     (2, optional_value, option),
+/// });
+/// // At this point `required_value` has been written as a TLV of type 0, `42u64` has been written
+/// // as a TLV of type 1 (indicating the reader may ignore it if it is not understood), and *no*
+/// // TLV is written with type 2.
+/// # Ok(())
+/// # }
+/// ```
+///
+/// [`io::Error`]: crate::io::Error
+/// [`Writeable::write`]: crate::util::ser::Writeable::write
+/// [`Writer`]: crate::util::ser::Writer
 #[macro_export]
 macro_rules! encode_tlv_stream {
 	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),* $(,)*}) => { {
@@ -72,6 +109,7 @@ macro_rules! encode_tlv_stream {
 
 /// Adds the length of the serialized field to a LengthCalculatingWriter.
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _get_varint_length_prefixed_tlv_length {
 	($len: expr, $type: expr, $field: expr, (default_value, $default: expr)) => {
@@ -98,6 +136,7 @@ macro_rules! _get_varint_length_prefixed_tlv_length {
 
 /// See the documentation of write_tlv_fields!().
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _encode_varint_length_prefixed_tlv {
 	($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),*}) => { {
@@ -117,6 +156,7 @@ macro_rules! _encode_varint_length_prefixed_tlv {
 
 /// Errors if there are missing required TLV types between the last seen type and the type currently being processed.
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _check_tlv_order {
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, (default_value, $default: expr)) => {{
@@ -152,6 +192,7 @@ macro_rules! _check_tlv_order {
 
 /// Errors if there are missing required TLV types after the last seen type.
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _check_missing_tlv {
 	($last_seen_type: expr, $type: expr, $field: ident, (default_value, $default: expr)) => {{
@@ -187,6 +228,7 @@ macro_rules! _check_missing_tlv {
 
 /// Implements deserialization for a single TLV record.
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _decode_tlv {
 	($reader: expr, $field: ident, (default_value, $default: expr)) => {{
@@ -221,17 +263,59 @@ macro_rules! _decode_tlv {
 
 /// Implements the TLVs deserialization part in a Readable implementation of a struct.
 ///
+/// This should be called inside a method which returns `Result<_, `[`DecodeError`]`>`, such as
+/// [`Readable::read`]. It will either return an `Err` or ensure all `required` fields have been
+/// read and optionally read `optional` fields.
+///
+/// `$stream` must be a [`Read`] and will be fully consumed, reading until no more bytes remain
+/// (i.e. it returns [`DecodeError::ShortRead`]).
+///
+/// Fields MUST be sorted in `$type`-order.
+///
+/// Note that the lightning TLV requirements require that a single type not appear more than once,
+/// that TLVs are sorted in type-ascending order, and that any even types be understood by the
+/// decoder.
+///
+/// For example,
+/// ```
+/// # use lightning::decode_tlv_stream;
+/// # fn read<R: lightning::io::Read> (stream: R) -> Result<(), lightning::ln::msgs::DecodeError> {
+/// let mut required_value = 0u64;
+/// let mut optional_value: Option<u64> = None;
+/// decode_tlv_stream!(stream, {
+///     (0, required_value, required),
+///     (2, optional_value, option),
+/// });
+/// // At this point, `required_value` has been overwritten with the TLV with type 0.
+/// // `optional_value` may have been overwritten, setting it to `Some` if a TLV with type 2 was
+/// // present.
+/// # Ok(())
+/// # }
+/// ```
+///
+/// [`DecodeError`]: crate::ln::msgs::DecodeError
+/// [`Readable::read`]: crate::util::ser::Readable::read
+/// [`Read`]: crate::io::Read
+/// [`DecodeError::ShortRead`]: crate::ln::msgs::DecodeError::ShortRead
+#[macro_export]
+macro_rules! decode_tlv_stream {
+	($stream: expr, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}) => {
+		let rewind = |_, _| { unreachable!() };
+		$crate::_decode_tlv_stream_range!($stream, .., rewind, {$(($type, $field, $fieldty)),*});
+	}
+}
+
+/// Similar to [`decode_tlv_stream`] with a custom TLV decoding capabilities.
+///
 /// `$decode_custom_tlv` is a closure that may be optionally provided to handle custom message types.
 /// If it is provided, it will be called with the custom type and the `FixedLengthReader` containing
 /// the message contents. It should return `Ok(true)` if the custom message is successfully parsed,
 /// `Ok(false)` if the message type is unknown, and `Err(DecodeError)` if parsing fails.
-#[macro_export]
-macro_rules! decode_tlv_stream {
+macro_rules! decode_tlv_stream_with_custom_tlv_decode {
 	($stream: expr, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
 	 $(, $decode_custom_tlv: expr)?) => { {
 		let rewind = |_, _| { unreachable!() };
-		use core::ops::RangeBounds;
-		$crate::_decode_tlv_stream_range!(
+		_decode_tlv_stream_range!(
 			$stream, .., rewind, {$(($type, $field, $fieldty)),*} $(, $decode_custom_tlv)?
 		);
 	} }
@@ -242,6 +326,7 @@ macro_rules! decode_tlv_stream {
 macro_rules! _decode_tlv_stream_range {
 	($stream: expr, $range: expr, $rewind: ident, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*}
 	 $(, $decode_custom_tlv: expr)?) => { {
+		use core::ops::RangeBounds;
 		use $crate::ln::msgs::DecodeError;
 		let mut last_seen_type: Option<u64> = None;
 		let mut stream_ref = $stream;
@@ -434,6 +519,7 @@ macro_rules! read_tlv_fields {
 
 /// Initializes the struct fields.
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _init_tlv_based_struct_field {
 	($field: ident, (default_value, $default: expr)) => {
@@ -452,6 +538,7 @@ macro_rules! _init_tlv_based_struct_field {
 
 /// Initializes the variable we are going to read the TLV into.
 /// This is exported for use by other exported macros, do not use directly.
+#[doc(hidden)]
 #[macro_export]
 macro_rules! _init_tlv_field_var {
 	($field: ident, (default_value, $default: expr)) => {