Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Document Seq064K #1046

Closed
wants to merge 1 commit into from
Closed

doc: Document Seq064K #1046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ impl<'a, T: GetSize> GetSize for Seq0255<'a, T> {
}
}

/// Fixed size data sequence up to a length of 65535
///
/// Byte Length Calculation:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this referring to? where this byte calculation is happening?

Copy link
Collaborator

@Fi3 Fi3 Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is how is defined in spec. Actual byte calculation happen in get_size.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

/// Return the encoded byte size of an `Encodable` comprehensive of the header, if any
pub trait GetSize {
    fn get_size(&self) -> usize;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should this doc live here or in the binary-sv2 and link to it?
another question please, what does L refer to?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which L ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a ok the one in docs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that doc come directly from the spec and L stand for the number of elments in the sequnce
https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md#31-data-types-mapping

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, its mentioned here twice Decsription: 2-byte length L, unsigned little-endian integer 16-bits, followed by a sequence of L elements of type T. Allowed range of length is 0 to 65535.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw yes we could link the the GetSize and SizeHint trait in that doc. GetSize is implemented for Encodable and return the total len SizeHint is implemented for Decodable and retutrn total len - header len

/// - For fixed-size T: 2 + LENGTH * size_of::<T>()
/// - For variable-length T: 2 + seq.map(|x| x.length()).sum()
/// Decsription: 2-byte length L, unsigned little-endian integer 16-bits, followed by a sequence of L elements of type T. Allowed range of length is 0 to 65535.
///
/// Used for listing channel ids, tx short hashes, tx hashes, list indexes, and full transaction data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Is it possible to also explain why this type is used in the mentioned scenarios?

///
/// The liftime is here only for type compatibility with serde-sv2
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct Seq064K<'a, T>(pub(crate) Vec<T>, PhantomData<&'a T>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its still not clear to me what the Vec<T> and PhantomData represent here tbh

Expand Down
1 change: 1 addition & 0 deletions protocols/v2/binary-sv2/serde-sv2/src/primitives/sequences/seq064k.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::{
use alloc::vec::Vec;
use serde::{ser, ser::SerializeTuple, Deserialize, Deserializer, Serialize};

/// See `binary_sv2::Seq064k`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this link to the binary_sv2::Seq064k in docs.rs ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbesraa, good eye/thought process. The answer is no, but check out this comment I left on the #970 discussion.

Let me know your thoughts on making this part of our documentation standard.

#[derive(Debug, Clone)]
pub struct Seq064K<'s, T: Clone + Serialize + TryFromBSlice<'s>> {
seq: Option<Seq<'s, T>>,
Expand Down
Loading