-
Notifications
You must be signed in to change notification settings - Fork 127
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.
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
doc: Document Seq064K #1046
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 3 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK. Just one nit
protocols/v2/binary-sv2/no-serde-sv2/codec/src/datatypes/non_copy_data_types/seq_inner.rs
Outdated
Show resolved
Hide resolved
a849ccb
to
3c0fa5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7f71069
to
7d3e563
Compare
@marathon-gary has updated this PR with |
@@ -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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which L
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
/// 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. | ||
/// | ||
/// 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>); |
There was a problem hiding this comment.
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
/// - 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. |
There was a problem hiding this comment.
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?
@@ -9,6 +9,7 @@ use crate::{ | |||
use alloc::vec::Vec; | |||
use serde::{ser, ser::SerializeTuple, Deserialize, Deserializer, Serialize}; | |||
|
|||
/// See `binary_sv2::Seq064k` |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
Still no update on my GitHub support issue for actions being disabled on my account. |
Bencher Report
🚨 3 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
|
Bencher Report
🚨 31 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
|
Bencher Report
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1046 +/- ##
=======================================
Coverage 19.36% 19.36%
=======================================
Files 164 164
Lines 10811 10811
=======================================
Hits 2094 2094
Misses 8717 8717
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
0eb7c0c
to
cd776ad
Compare
Takes specific of bytes, types and sizes from the specification. Lists some use cases for this struct. Work toward: stratum-mining#1011 fix: cargo fmt for docstrings
cd776ad
to
f0494c1
Compare
@Shourya742 will cherry pick these commits into their #1231 PR. |
Takes specific of bytes, types and sizes from the specification. Lists some use cases for this struct.
Work toward: #1011