common_messages_sv2 docs
#1242
common_messages_sv2 docs
#1242
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1242 +/- ##
=======================================
Coverage 19.30% 19.30%
=======================================
Files 164 164
Lines 10849 10849
=======================================
Hits 2094 2094
Misses 8755 8755
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
from
bf46414
to
920076b
Compare
protocols/v2/subprotocols/common-messages/src/channel_endpoint_changed.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/common-messages/src/channel_endpoint_changed.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/common-messages/src/setup_connection.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/common-messages/src/setup_connection.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/common-messages/src/setup_connection.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/common-messages/src/setup_connection.rs
Outdated
Show resolved
Hide resolved
protocols/v2/subprotocols/common-messages/src/setup_connection.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@jbesraa, could you also update the sv2.h file to match the changes in the docs? I noticed a few public structures, methods, and enums missing documentation. Maybe add #![deny(missing_docs)] at the top level to catch those and see what's missing?
Also, these methods seem a bit off:
pub fn set_requires_standard_job(&mut self) {
self.flags |= 0b_0000_0000_0000_0000_0000_0000_0000_0001;
}
pub fn set_async_job_negotiation(&mut self) {
self.flags |= 0b_0000_0000_0000_0000_0000_0000_0000_0001;
}Both are setting the LSB and set_requires_standard_job looks like it's only used in tests.
| /// | ||
| /// This message is sent in response to a [`SetupConnection`] message. | ||
| /// | ||
| /// The connection setup process could fail because of protocol version negotiation. In order |
There was a problem hiding this comment.
| /// The connection setup process could fail because of protocol version negotiation. In order | |
| /// The connection setup process could fail because of protocol version negotiation. In order |
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
2 times, most recently
from
b1d08ac
to
aa72fcd
Compare
Thanks for the review. What exactly should be changed in In regards to the function that might be wrong/unused, I would leave that to a separate PR. |
|
@GitGab19 Thanks for the review. I believe I addressed all of your comments. Some of the structs throw a warning(https://github.com/stratum-mining/stratum/actions/runs/11878855683/job/33100048008?pr=1242) for missing documentation for the |
Bencher Report
|
| Branch | 2024-10-29-common-msgs-docs |
| Testbed | sv2 |
🚨 2 Alerts
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| client_sv2_handle_message_mining | RAM Accesses accesses | 📈 plot 🚨 alert (🔔) 🚷 threshold | 141.00 (+1.44%) | 140.89 (100.08%) |
| client_sv2_mining_message_submit_standard_serialize | L2 Accesses accesses | 📈 plot 🚨 alert (🔔) 🚷 threshold | 54.00 (+24.41%) | 53.11 (101.67%) |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Benchmark Result estimated cycles (Result Δ%) | Upper Boundary estimated cycles (Limit %) | Instructions | Benchmark Result instructions (Result Δ%) | Upper Boundary instructions (Limit %) | L1 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | L2 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | RAM Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| client_sv2_handle_message_common | 📈 view plot 🚷 view threshold | 2,129.00 (+1.61%) | 2,208.92 (96.38%) | 📈 view plot 🚷 view threshold | 473.00 (-0.07%) | 488.51 (96.82%) | 📈 view plot 🚷 view threshold | 734.00 (-0.27%) | 757.25 (96.93%) | 📈 view plot 🚷 view threshold | 6.00 (+12.94%) | 13.62 (44.05%) | 📈 view plot 🚷 view threshold | 39.00 (+2.42%) | 41.22 (94.60%) |
| client_sv2_handle_message_mining | 📈 view plot 🚷 view threshold | 8,268.00 (+0.79%) | 8,283.02 (99.82%) | 📈 view plot 🚷 view threshold | 2,137.00 (-0.03%) | 2,143.08 (99.72%) | 📈 view plot 🚷 view threshold | 3,158.00 (-0.09%) | 3,170.55 (99.60%) | 📈 view plot 🚷 view threshold | 35.00 (-1.28%) | 41.78 (83.78%) | 📈 view plot 🚨 view alert (🔔) 🚷 view threshold | 141.00 (+1.44%) | 140.89 (100.08%) |
| client_sv2_mining_message_submit_standard | 📈 view plot 🚷 view threshold | 6,319.00 (+0.61%) | 6,396.26 (98.79%) | 📈 view plot 🚷 view threshold | 1,750.00 (-0.05%) | 1,766.87 (99.05%) | 📈 view plot 🚷 view threshold | 2,549.00 (-0.19%) | 2,575.61 (98.97%) | 📈 view plot 🚷 view threshold | 19.00 (+12.70%) | 24.22 (78.46%) | 📈 view plot 🚷 view threshold | 105.00 (+0.89%) | 107.28 (97.88%) |
| client_sv2_mining_message_submit_standard_serialize | 📈 view plot 🚷 view threshold | 14,786.00 (+0.40%) | 14,949.14 (98.91%) | 📈 view plot 🚷 view threshold | 4,694.00 (-0.02%) | 4,710.87 (99.64%) | 📈 view plot 🚷 view threshold | 6,746.00 (-0.19%) | 6,781.90 (99.47%) | 📈 view plot 🚨 view alert (🔔) 🚷 view threshold | 54.00 (+24.41%) | 53.11 (101.67%) | 📈 view plot 🚷 view threshold | 222.00 (+0.24%) | 227.33 (97.65%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | 📈 view plot 🚷 view threshold | 27,742.00 (+0.74%) | 27,823.82 (99.71%) | 📈 view plot 🚷 view threshold | 10,645.00 (+0.57%) | 10,656.37 (99.89%) | 📈 view plot 🚷 view threshold | 15,507.00 (+0.68%) | 15,519.38 (99.92%) | 📈 view plot 🚷 view threshold | 88.00 (+8.23%) | 89.50 (98.33%) | 📈 view plot 🚷 view threshold | 337.00 (+0.57%) | 341.71 (98.62%) |
| client_sv2_open_channel | 📈 view plot 🚷 view threshold | 4,477.00 (+1.90%) | 4,592.73 (97.48%) | 📈 view plot 🚷 view threshold | 1,461.00 (-0.02%) | 1,476.51 (98.95%) | 📈 view plot 🚷 view threshold | 2,157.00 (-0.14%) | 2,184.11 (98.76%) | 📈 view plot 🚷 view threshold | 9.00 (+2.58%) | 16.89 (53.29%) | 📈 view plot 🚷 view threshold | 65.00 (+3.90%) | 67.92 (95.70%) |
| client_sv2_open_channel_serialize | 📈 view plot 🚷 view threshold | 14,112.00 (+0.45%) | 14,390.77 (98.06%) | 📈 view plot 🚷 view threshold | 5,064.00 (-0.01%) | 5,079.51 (99.69%) | 📈 view plot 🚷 view threshold | 7,317.00 (-0.13%) | 7,351.51 (99.53%) | 📈 view plot 🚷 view threshold | 43.00 (+22.69%) | 43.26 (99.39%) | 📈 view plot 🚷 view threshold | 188.00 (+0.49%) | 196.94 (95.46%) |
| client_sv2_open_channel_serialize_deserialize | 📈 view plot 🚷 view threshold | 22,826.00 (+0.72%) | 22,912.18 (99.62%) | 📈 view plot 🚷 view threshold | 8,040.00 (+0.22%) | 8,072.97 (99.59%) | 📈 view plot 🚷 view threshold | 11,696.00 (+0.22%) | 11,750.08 (99.54%) | 📈 view plot 🚷 view threshold | 77.00 (+3.59%) | 84.96 (90.63%) | 📈 view plot 🚷 view threshold | 307.00 (+1.16%) | 311.12 (98.68%) |
| client_sv2_setup_connection | 📈 view plot 🚷 view threshold | 4,707.00 (+0.47%) | 4,775.18 (98.57%) | 📈 view plot 🚷 view threshold | 1,502.00 (-0.02%) | 1,517.51 (98.98%) | 📈 view plot 🚷 view threshold | 2,277.00 (-0.06%) | 2,299.21 (99.03%) | 📈 view plot 🚷 view threshold | 10.00 (+4.75%) | 16.60 (60.24%) | 📈 view plot 🚷 view threshold | 68.00 (+0.90%) | 69.80 (97.42%) |
| client_sv2_setup_connection_serialize | 📈 view plot 🚷 view threshold | 16,236.00 (+0.43%) | 16,438.51 (98.77%) | 📈 view plot 🚷 view threshold | 5,963.00 (-0.01%) | 5,978.51 (99.74%) | 📈 view plot 🚷 view threshold | 8,656.00 (-0.09%) | 8,691.27 (99.59%) | 📈 view plot 🚷 view threshold | 46.00 (+15.70%) | 52.67 (87.33%) | 📈 view plot 🚷 view threshold | 210.00 (+0.63%) | 215.78 (97.32%) |
| client_sv2_setup_connection_serialize_deserialize | 📈 view plot 🚷 view threshold | 35,728.00 (+0.49%) | 35,769.71 (99.88%) | 📈 view plot 🚷 view threshold | 14,888.00 (+0.25%) | 14,910.16 (99.85%) | 📈 view plot 🚷 view threshold | 21,873.00 (+0.25%) | 21,923.27 (99.77%) | 📈 view plot 🚷 view threshold | 104.00 (+12.86%) | 111.13 (93.58%) | 📈 view plot 🚷 view threshold | 381.00 (+0.46%) | 384.71 (99.04%) |
|
| Branch | 2024-10-29-common-msgs-docs |
| Testbed | sv2 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| client_sv2_handle_message_common | 📈 view plot 🚷 view threshold | 44.15 (-0.89%) | 46.97 (94.00%) |
| client_sv2_handle_message_mining | 📈 view plot 🚷 view threshold | 75.08 (+0.69%) | 80.38 (93.41%) |
| client_sv2_mining_message_submit_standard | 📈 view plot 🚷 view threshold | 14.67 (-0.00%) | 14.82 (98.96%) |
| client_sv2_mining_message_submit_standard_serialize | 📈 view plot 🚷 view threshold | 263.45 (-0.64%) | 319.13 (82.55%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | 📈 view plot 🚷 view threshold | 612.22 (-0.21%) | 715.73 (85.54%) |
| client_sv2_open_channel | 📈 view plot 🚷 view threshold | 166.77 (+8.88%) | 196.70 (84.79%) |
| client_sv2_open_channel_serialize | 📈 view plot 🚷 view threshold | 279.55 (-1.95%) | 316.01 (88.46%) |
| client_sv2_open_channel_serialize_deserialize | 📈 view plot 🚷 view threshold | 376.30 (-0.75%) | 406.39 (92.60%) |
| client_sv2_setup_connection | 📈 view plot 🚷 view threshold | 159.50 (-0.73%) | 171.78 (92.85%) |
| client_sv2_setup_connection_serialize | 📈 view plot 🚷 view threshold | 447.53 (-3.92%) | 542.50 (82.49%) |
| client_sv2_setup_connection_serialize_deserialize | 📈 view plot 🚷 view threshold | 987.39 (+1.01%) | 1,053.54 (93.72%) |
|
| Branch | 2024-10-29-common-msgs-docs |
| Testbed | sv1 |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Benchmark Result estimated cycles (Result Δ%) | Upper Boundary estimated cycles (Limit %) | Instructions | Benchmark Result instructions (Result Δ%) | Upper Boundary instructions (Limit %) | L1 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | L2 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | RAM Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| get_authorize | 📈 view plot 🚷 view threshold | 8,353.00 (-1.33%) | 8,732.17 (95.66%) | 📈 view plot 🚷 view threshold | 3,689.00 (-1.61%) | 3,856.17 (95.66%) | 📈 view plot 🚷 view threshold | 5,158.00 (-1.88%) | 5,424.40 (95.09%) | 📈 view plot 🚷 view threshold | 9.00 (+24.27%) | 13.22 (68.07%) | 📈 view plot 🚷 view threshold | 90.00 (-0.71%) | 95.13 (94.60%) |
| get_submit | 📈 view plot 🚷 view threshold | 95,235.00 (-0.21%) | 95,894.41 (99.31%) | 📈 view plot 🚷 view threshold | 59,346.00 (-0.19%) | 59,681.23 (99.44%) | 📈 view plot 🚷 view threshold | 85,220.00 (-0.21%) | 85,755.75 (99.38%) | 📈 view plot 🚷 view threshold | 50.00 (+11.67%) | 63.38 (78.89%) | 📈 view plot 🚷 view threshold | 279.00 (-0.40%) | 289.20 (96.47%) |
| get_subscribe | 📈 view plot 🚷 view threshold | 7,838.00 (-2.29%) | 8,278.95 (94.67%) | 📈 view plot 🚷 view threshold | 2,765.00 (-2.47%) | 2,937.44 (94.13%) | 📈 view plot 🚷 view threshold | 3,848.00 (-2.87%) | 4,119.69 (93.41%) | 📈 view plot 🚷 view threshold | 14.00 (+17.05%) | 20.62 (67.91%) | 📈 view plot 🚷 view threshold | 112.00 (-2.02%) | 118.53 (94.49%) |
| serialize_authorize | 📈 view plot 🚷 view threshold | 12,074.00 (-1.69%) | 12,569.55 (96.06%) | 📈 view plot 🚷 view threshold | 5,266.00 (-1.03%) | 5,423.66 (97.09%) | 📈 view plot 🚷 view threshold | 7,329.00 (-1.22%) | 7,582.55 (96.66%) | 📈 view plot 🚷 view threshold | 11.00 (+20.24%) | 16.30 (67.47%) | 📈 view plot 🚷 view threshold | 134.00 (-2.63%) | 142.64 (93.94%) |
| serialize_deserialize_authorize | 📈 view plot 🚷 view threshold | 24,606.00 (-0.35%) | 25,225.36 (97.54%) | 📈 view plot 🚷 view threshold | 9,838.00 (-0.54%) | 10,031.81 (98.07%) | 📈 view plot 🚷 view threshold | 13,876.00 (-0.63%) | 14,182.69 (97.84%) | 📈 view plot 🚷 view threshold | 39.00 (+12.48%) | 43.69 (89.28%) | 📈 view plot 🚷 view threshold | 301.00 (-0.18%) | 315.03 (95.55%) |
| serialize_deserialize_handle_authorize | 📈 view plot 🚷 view threshold | 30,122.00 (-0.67%) | 30,751.44 (97.95%) | 📈 view plot 🚷 view threshold | 12,015.00 (-0.55%) | 12,201.76 (98.47%) | 📈 view plot 🚷 view threshold | 16,997.00 (-0.65%) | 17,290.18 (98.30%) | 📈 view plot 🚷 view threshold | 63.00 (+13.34%) | 67.56 (93.25%) | 📈 view plot 🚷 view threshold | 366.00 (-0.99%) | 381.55 (95.93%) |
| serialize_deserialize_handle_submit | 📈 view plot 🚷 view threshold | 126,325.00 (-0.12%) | 126,896.33 (99.55%) | 📈 view plot 🚷 view threshold | 73,200.00 (-0.12%) | 73,507.79 (99.58%) | 📈 view plot 🚷 view threshold | 104,910.00 (-0.15%) | 105,431.82 (99.51%) | 📈 view plot 🚷 view threshold | 111.00 (+3.81%) | 132.56 (83.74%) | 📈 view plot 🚷 view threshold | 596.00 (-0.07%) | 605.13 (98.49%) |
| serialize_deserialize_handle_subscribe | 📈 view plot 🚷 view threshold | 27,726.00 (-0.42%) | 28,598.38 (96.95%) | 📈 view plot 🚷 view threshold | 9,584.00 (-0.68%) | 9,751.31 (98.28%) | 📈 view plot 🚷 view threshold | 13,531.00 (-0.85%) | 13,805.70 (98.01%) | 📈 view plot 🚷 view threshold | 74.00 (+16.46%) | 76.34 (96.93%) | 📈 view plot 🚷 view threshold | 395.00 (-0.37%) | 416.64 (94.81%) |
| serialize_deserialize_submit | 📈 view plot 🚷 view threshold | 115,217.00 (-0.02%) | 115,799.35 (99.50%) | 📈 view plot 🚷 view threshold | 68,060.00 (-0.07%) | 68,391.94 (99.51%) | 📈 view plot 🚷 view threshold | 97,647.00 (-0.09%) | 98,216.96 (99.42%) | 📈 view plot 🚷 view threshold | 70.00 (+11.61%) | 78.26 (89.44%) | 📈 view plot 🚷 view threshold | 492.00 (+0.15%) | 498.38 (98.72%) |
| serialize_deserialize_subscribe | 📈 view plot 🚷 view threshold | 23,138.00 (-0.54%) | 23,985.93 (96.46%) | 📈 view plot 🚷 view threshold | 8,143.00 (-0.75%) | 8,306.70 (98.03%) | 📈 view plot 🚷 view threshold | 11,453.00 (-0.90%) | 11,711.17 (97.80%) | 📈 view plot 🚷 view threshold | 41.00 (+10.02%) | 44.50 (92.13%) | 📈 view plot 🚷 view threshold | 328.00 (-0.34%) | 347.64 (94.35%) |
| serialize_submit | 📈 view plot 🚷 view threshold | 99,624.00 (-0.22%) | 100,303.51 (99.32%) | 📈 view plot 🚷 view threshold | 61,408.00 (-0.16%) | 61,714.56 (99.50%) | 📈 view plot 🚷 view threshold | 88,079.00 (-0.19%) | 88,589.64 (99.42%) | 📈 view plot 🚷 view threshold | 55.00 (+17.69%) | 64.40 (85.40%) | 📈 view plot 🚷 view threshold | 322.00 (-0.83%) | 333.72 (96.49%) |
| serialize_subscribe | 📈 view plot 🚷 view threshold | 11,279.00 (-1.10%) | 11,678.01 (96.58%) | 📈 view plot 🚷 view threshold | 4,118.00 (-1.54%) | 4,280.64 (96.20%) | 📈 view plot 🚷 view threshold | 5,709.00 (-1.89%) | 5,972.53 (95.59%) | 📈 view plot 🚷 view threshold | 15.00 (+16.58%) | 20.76 (72.25%) | 📈 view plot 🚷 view threshold | 157.00 (-0.47%) | 163.13 (96.24%) |
Why do you say it will be fixed when |
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
from
aa72fcd
to
27d54cf
Compare
Because it is complaining about functions inside |
sounds good |
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
from
27d54cf
to
a23801f
Compare
There was a problem hiding this comment.
LGTM. Let's open the review process to anyone to move on with this PR. I also saw that @Shourya742 already started to review it partially, don't know if he can complete it with a second one.
| [](https://github.com/stratum-mining/stratum/blob/main/LICENSE.md) | ||
| [](https://app.codecov.io/gh/stratum-mining/stratum/tree/main/protocols%2Fv2%2Fcommon_messages_sv2) | ||
|
|
||
| `common_messages_sv2` is a Rust crate that implements a set of shared messages used by multiple Stratum V2 roles. |
There was a problem hiding this comment.
Isn't this a bit too much generic?
There was a problem hiding this comment.
I think a more accurate description should highlight that these messages are shared across all Sv2 subprotocols.
| @@ -1,7 +1,13 @@ | |||
| //! # Stratum V2 Common Messages Crate. | |||
| //! | |||
| //! The following crate defines a set of shared messages used across multiple Stratum V2 roles. | |||
There was a problem hiding this comment.
| //! The following crate defines a set of shared messages used across multiple Stratum V2 roles. | |
| //! This crate defines a set of shared messages used across all Stratum V2 subprotocols. |
| [](https://github.com/stratum-mining/stratum/blob/main/LICENSE.md) | ||
| [](https://app.codecov.io/gh/stratum-mining/stratum/tree/main/protocols%2Fv2%2Fcommon_messages_sv2) | ||
|
|
||
| `common_messages_sv2` is a Rust crate that implements a set of shared messages used by multiple Stratum V2 roles. |
There was a problem hiding this comment.
| `common_messages_sv2` is a Rust crate that implements a set of shared messages used by multiple Stratum V2 roles. | |
| `common_messages_sv2` is a Rust crate that implements a set of messages shared across all Stratum V2 subprotocols. |
| pub fn requires_standard_job(&self) -> bool { | ||
| has_requires_std_job(self.flags) | ||
| } | ||
| } | ||
|
|
||
| /// Checks if the flags include the Requires Standard Job flag. |
There was a problem hiding this comment.
| /// Checks if the flags include the Requires Standard Job flag. | |
| /// Helper function to check if [`SetupConnection`] has `REQUIRES_STANDARD_JOBS` bit flag. |
There was a problem hiding this comment.
Those functions dont know anything about SetupConnection really, they just receive the flags
| pub fn has_requires_std_job(flags: u32) -> bool { | ||
| let flags = flags.reverse_bits(); | ||
| let flag = flags >> 31; | ||
| flag != 0 | ||
| } | ||
|
|
||
| /// Checks if the flags include the Version Rolling flag. |
There was a problem hiding this comment.
| /// Checks if the flags include the Version Rolling flag. | |
| /// Helper function to check if [`SetupConnection`] has `REQUIRES_VERSION_ROLLING` bit flag. |
There was a problem hiding this comment.
Those functions dont know anything about SetupConnection really, they just receive the flags
| pub fn has_version_rolling(flags: u32) -> bool { | ||
| let flags = flags.reverse_bits(); | ||
| let flags = flags << 1; | ||
| let flag = flags >> 31; | ||
| flag != 0 | ||
| } | ||
|
|
||
| /// Checks if the flags include the Work Selection flag. |
There was a problem hiding this comment.
| /// Checks if the flags include the Work Selection flag. | |
| /// Helper function to check if [`SetupConnection`] has `REQUIRES_WORK_SELECTION ` bit flag. |
There was a problem hiding this comment.
Those functions dont know anything about SetupConnection really, they just receive the flags
| /// ASCII representation of the device vendor name. | ||
| #[cfg_attr(feature = "with_serde", serde(borrow))] | ||
| pub vendor: Str0255<'decoder>, | ||
| /// ASCII representation of the device hardware version. | ||
| #[cfg_attr(feature = "with_serde", serde(borrow))] | ||
| pub hardware_version: Str0255<'decoder>, | ||
| /// ASCII representation of the device firmware version. | ||
| #[cfg_attr(feature = "with_serde", serde(borrow))] | ||
| pub firmware: Str0255<'decoder>, | ||
| /// ASCII representation of the device identifier. | ||
| #[cfg_attr(feature = "with_serde", serde(borrow))] | ||
| pub device_id: Str0255<'decoder>, |
There was a problem hiding this comment.
do we know for sure this is restricted to ASCII? where did you get this info?
There was a problem hiding this comment.
no only for the enpoint. so device id can be anyhting
| /// ASCII representation of the connection hostname or IP address. | ||
| pub endpoint_host: CVec, | ||
| /// Connection port value. | ||
| pub endpoint_port: u16, | ||
| /// ASCII representation of the device vendor name. | ||
| pub vendor: CVec, | ||
| /// ASCII representation of the device hardware version. | ||
| pub hardware_version: CVec, | ||
| /// ASCII representation of the device firmware version. | ||
| pub firmware: CVec, | ||
| /// ASCII representation of the device identifier. |
There was a problem hiding this comment.
do we know for sure this is ASCII? where did you get this info?
| /// Unsupported feature flags. | ||
| /// | ||
| /// If set to 0, then this error is not due to unsupported flags. |
There was a problem hiding this comment.
semantics of this field should be primarily conditioned to error_code
| /// Unsupported feature flags. | |
| /// | |
| /// If set to 0, then this error is not due to unsupported flags. | |
| /// In case `error_code` is `unsupported-feature-flags`, this field is used to indicate which flag is causing an error |
| #[cfg_attr(feature = "with_serde", serde(borrow))] | ||
| pub error_code: Str0255<'decoder>, | ||
| } | ||
|
|
||
| #[repr(C)] | ||
| #[cfg(not(feature = "with_serde"))] | ||
| #[derive(Debug, Clone)] | ||
| /// C representation of [`SetupConnectionSuccess`] |
There was a problem hiding this comment.
| /// C representation of [`SetupConnectionSuccess`] | |
| /// C representation of [`SetupConnectionError`] |
| /// Message used by a downstream role to initiate a Stratum V2 channel connection with an upstream | ||
| /// role. | ||
| /// | ||
| /// This is usually the first message sent by a downstream role on a newly opened connection, | ||
| /// after completing the handshake process. | ||
| /// | ||
| /// Downstreams that do not wish to provide telemetry data to the upstream role **should** set | ||
| /// [`SetupConnection::device_id`] to an empty string. However, they **must** set | ||
| /// [`SetupConnection::vendor`] to a string describing the manufacturer/developer and firmware | ||
| /// version and **should** set [`SetupConnection::hardware_version`] to a string describing, at | ||
| /// least, the particular hardware/software package in use. | ||
| /// | ||
| /// A valid response to this message from the upstream role can either be [`SetupConnectionSuccess`] | ||
| /// or [`SetupConnectionError`] message. |
There was a problem hiding this comment.
do we want to duplicate this?
CSetupConnectionError only says:
/// C representation of [`SetupConnectionError`]
I prefer providing a summary rather than duplicating the comments
| /// MiningProtocol = [`SV2_MINING_PROTOCOL_DISCRIMINANT`], | ||
| /// JobDeclarationProtocol = [`SV2_JOB_DECLARATION_PROTOCOL_DISCRIMINANT`], | ||
| /// TemplateDistributionProtocol = [`SV2_TEMPLATE_DISTR_PROTOCOL_DISCRIMINANT`], | ||
| /// Represents the different Stratum V2 protocols. |
There was a problem hiding this comment.
| /// Represents the different Stratum V2 protocols. | |
| /// This enum has a list of the different Stratum V2 subprotocols that [`SetupConnection`] can be used under. |
We can track the methods mentioned in the refactor issue for this crate to ensure they aren't forgotten. Regarding the |
| This crate offers feature flags to enable or disable the use of `std`, `serde` or `quickcheck` | ||
| dependencies. |
There was a problem hiding this comment.
Should we add a subheading, such as build-options, for this segment and then explain each feature flag?
There was a problem hiding this comment.
And should we place it above the usage, to keep it consistent with other crate readme's.
| @@ -1,7 +1,13 @@ | |||
| //! # Stratum V2 Common Messages Crate. | |||
There was a problem hiding this comment.
This isn't directly related to documentation, but we can create a separate prop_test module to consolidate all the generation code, instead of applying the feature gate to individual lines. This could be included as part of a refactoring issue.
| /// `extension_type`. | ||
| /// | ||
| /// When a downstream receives such a message, any extension state (including version and extension | ||
| /// support) must be reset and renegotiated. | ||
| #[repr(C)] | ||
| #[derive(Serialize, Deserialize, Debug, Copy, Clone, PartialEq, Eq)] |
There was a problem hiding this comment.
| #[derive(Serialize, Deserialize, Debug, Copy, Clone, PartialEq, Eq)] | |
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] |
This isn't related to documentation, but either Copy or Clone can be removed, as both aren't necessary. Additionally, Eq is not required. This could be added to the refactoring tracker.
| /// Currently must be set to 2. | ||
| pub min_version: u16, | ||
| /// The maximum protocol version the client supports (currently must be 2). | ||
| /// The maximum protocol version supported. | ||
| /// | ||
| /// Currently must be set to 2. |
There was a problem hiding this comment.
Why default for these are set to 2?
There was a problem hiding this comment.
I guess we only have stratum v2
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
2 times, most recently
from
2dd5ca0
to
ff01340
Compare
|
@jbesraa is it ready for a final round of review? I see you still need to address some comments from @Shourya742 |
I still need to fix the header thing, but feel free to review. Addressed all comments. |
.. and fix some styling inconsistency
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
from
ff01340
to
fc1540f
Compare
| /// vendor to a string describing the manufacturer/developer and firmware version and SHOULD | ||
| /// always set hardware_version to a string describing, at least, the particular hardware/software | ||
| /// package in use. | ||
| /// Message used by a downstream role to initiate a Stratum V2 channel connection with an upstream |
There was a problem hiding this comment.
| /// Message used by a downstream role to initiate a Stratum V2 channel connection with an upstream | |
| /// Message used by a downstream role to initiate a Stratum V2 connection with an upstream |
| This crate can be built with the following features: | ||
|
|
||
| - `std`: Enables support for standard library features. | ||
| - `serde`: Enables support for serialization and deserialization using Serde. |
There was a problem hiding this comment.
| - `serde`: Enables support for serialization and deserialization using Serde. | |
| - `serde`: Enables support for serialization and deserialization using Serde. Note that this feature flag is only used for the Message Generator, and deprecated for any other kind of usage. It will likely be fully deprecated in the future. |
As part of the effort to improve Stratum V2 protocols docs, this commit aims to improves and make the documentation more comprehensive and accessible for contributors and end users alike.
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
from
fc1540f
to
6a776a4
Compare
Use the template README used across the different Stratum V2 protocol crates to `common_messages_sv2` crate.
jbesraa
force-pushed
the
2024-10-29-common-msgs-docs
branch
from
6a776a4
to
19651b5
Compare
resolves #1203