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

Jump to bottom

framing-sv2 doc cmts to conform to standard #1179

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

framing-sv2 doc cmts to conform to standard #1179

wants to merge 4 commits into from

Conversation

rrybarczyk
Copy link
Collaborator

Addresses #1178.

@rrybarczyk rrybarczyk added documentation Improvements or additions to documentation protocols Lowest level protocol logic labels Oct 2, 2024
@rrybarczyk rrybarczyk self-assigned this Oct 2, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 2, 2024 •
edited
Loading

🐰 Bencher Report

Branch1179/merge
Testbedsv2
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)		Upper Boundary
nanoseconds (ns)
(Limit %)
client_sv2_handle_message_common📈 view plot
🚷 view threshold		44.00
(-1.20%)		46.52
(94.60%)
client_sv2_handle_message_mining📈 view plot
🚷 view threshold		75.30
(-2.42%)		102.18
(73.70%)
client_sv2_mining_message_submit_standard📈 view plot
🚷 view threshold		14.62
(-0.58%)		15.25
(95.85%)
client_sv2_mining_message_submit_standard_serialize📈 view plot
🚷 view threshold		270.27
(+1.79%)		288.37
(93.72%)
client_sv2_mining_message_submit_standard_serialize_deserialize📈 view plot
🚷 view threshold		591.45
(-0.80%)		659.11
(89.73%)
client_sv2_open_channel📈 view plot
🚷 view threshold		147.24
(-10.12%)		192.09
(76.65%)
client_sv2_open_channel_serialize📈 view plot
🚷 view threshold		286.08
(+3.90%)		310.29
(92.20%)
client_sv2_open_channel_serialize_deserialize📈 view plot
🚷 view threshold		371.83
(-1.68%)		417.76
(89.01%)
client_sv2_setup_connection📈 view plot
🚷 view threshold		162.60
(+1.04%)		176.56
(92.09%)
client_sv2_setup_connection_serialize📈 view plot
🚷 view threshold		501.09
(+10.98%)		545.39
(91.88%)
client_sv2_setup_connection_serialize_deserialize📈 view plot
🚷 view threshold		977.43
(-0.75%)		1,102.11
(88.69%)
🐰 View full continuous benchmarking report in Bencher

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 2, 2024 •
edited
Loading

🐰 Bencher Report

Branch1179/merge
Testbedsv1
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)		Upper Boundary
nanoseconds (ns)
(Limit %)
client-submit-serialize📈 view plot
🚷 view threshold		7,179.40
(+4.28%)		7,791.48
(92.14%)
client-submit-serialize-deserialize📈 view plot
🚷 view threshold		8,101.60
(+2.34%)		8,703.93
(93.08%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle📈 view plot
🚷 view threshold		8,714.00
(+2.79%)		9,338.44
(93.31%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle📈 view plot
🚷 view threshold		973.55
(+4.62%)		1,028.72
(94.64%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize📈 view plot
🚷 view threshold		749.06
(+2.65%)		822.39
(91.08%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize📈 view plot
🚷 view threshold		249.96
(-0.32%)		276.89
(90.28%)
client-sv1-get-authorize/client-sv1-get-authorize📈 view plot
🚷 view threshold		155.65
(-2.00%)		171.21
(90.91%)
client-sv1-get-submit📈 view plot
🚷 view threshold		7,005.10
(+5.13%)		7,468.56
(93.79%)
client-sv1-get-subscribe/client-sv1-get-subscribe📈 view plot
🚷 view threshold		292.56
(+4.61%)		308.94
(94.70%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle📈 view plot
🚷 view threshold		799.53
(+2.10%)		876.75
(91.19%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize📈 view plot
🚷 view threshold		665.48
(+3.32%)		728.83
(91.31%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize📈 view plot
🚷 view threshold		204.06
(-1.89%)		236.45
(86.30%)
🐰 View full continuous benchmarking report in Bencher

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 2, 2024 •
edited
Loading

🐰 Bencher Report

Branch1179/merge
Testbedsv1
Click to view all benchmark results
BenchmarkEstimated CyclesBenchmark Result
estimated cycles
(Result Δ%)		Upper Boundary
estimated cycles
(Limit %)		InstructionsBenchmark Result
instructions
(Result Δ%)		Upper Boundary
instructions
(Limit %)		L1 AccessesBenchmark Result
accesses
(Result Δ%)		Upper Boundary
accesses
(Limit %)		L2 AccessesBenchmark Result
accesses
(Result Δ%)		Upper Boundary
accesses
(Limit %)		RAM AccessesBenchmark Result
accesses
(Result Δ%)		Upper Boundary
accesses
(Limit %)
get_authorize📈 view plot
🚷 view threshold		8,480.00
(+0.23%)		8,830.52
(96.03%)		📈 view plot
🚷 view threshold		3,775.00
(+0.63%)		3,887.55
(97.10%)		📈 view plot
🚷 view threshold		5,300.00
(+0.72%)		5,449.53
(97.26%)		📈 view plot
🚷 view threshold		6.00
(-14.67%)		11.94
(50.25%)		📈 view plot
🚷 view threshold		90.00
(-0.41%)		96.15
(93.61%)
get_submit📈 view plot
🚷 view threshold		95,407.00
(-0.05%)		96,164.47
(99.21%)		📈 view plot
🚷 view threshold		59,525.00
(+0.08%)		59,807.13
(99.53%)		📈 view plot
🚷 view threshold		85,512.00
(+0.10%)		85,896.10
(99.55%)		📈 view plot
🚷 view threshold		47.00
(+2.05%)		67.62
(69.50%)		📈 view plot
🚷 view threshold		276.00
(-1.36%)		289.05
(95.48%)
get_subscribe📈 view plot
🚷 view threshold		8,003.00
(-0.23%)		8,379.10
(95.51%)		📈 view plot
🚷 view threshold		2,851.00
(+0.46%)		2,966.25
(96.11%)		📈 view plot
🚷 view threshold		3,988.00
(+0.51%)		4,140.28
(96.32%)		📈 view plot
🚷 view threshold		12.00
(-1.29%)		21.42
(56.02%)		📈 view plot
🚷 view threshold		113.00
(-0.96%)		120.13
(94.06%)
serialize_authorize📈 view plot
🚷 view threshold		12,263.00
(-0.11%)		12,649.28
(96.95%)		📈 view plot
🚷 view threshold		5,346.00
(+0.45%)		5,458.55
(97.94%)		📈 view plot
🚷 view threshold		7,463.00
(+0.52%)		7,612.66
(98.03%)		📈 view plot
🚷 view threshold		8.00
(-9.70%)		14.63
(54.70%)		📈 view plot
🚷 view threshold		136.00
(-1.00%)		143.66
(94.67%)
serialize_deserialize_authorize📈 view plot
🚷 view threshold		24,798.00
(+0.46%)		25,274.19
(98.12%)		📈 view plot
🚷 view threshold		9,923.00
(+0.28%)		10,053.47
(98.70%)		📈 view plot
🚷 view threshold		14,023.00
(+0.38%)		14,204.76
(98.72%)		📈 view plot
🚷 view threshold		34.00
(-2.11%)		44.06
(77.17%)		📈 view plot
🚷 view threshold		303.00
(+0.63%)		316.04
(95.87%)
serialize_deserialize_handle_authorize📈 view plot
🚷 view threshold		30,356.00
(+0.14%)		30,795.08
(98.57%)		📈 view plot
🚷 view threshold		12,100.00
(+0.11%)		12,222.34
(99.00%)		📈 view plot
🚷 view threshold		17,141.00
(+0.14%)		17,308.18
(99.03%)		📈 view plot
🚷 view threshold		60.00
(+7.85%)		67.84
(88.44%)		📈 view plot
🚷 view threshold		369.00
(-0.03%)		382.33
(96.51%)
serialize_deserialize_handle_submit📈 view plot
🚷 view threshold		126,521.00
(+0.03%)		127,194.40
(99.47%)		📈 view plot
🚷 view threshold		73,363.00
(+0.09%)		73,695.46
(99.55%)		📈 view plot
🚷 view threshold		105,191.00
(+0.11%)		105,658.97
(99.56%)		📈 view plot
🚷 view threshold		115.00
(+6.30%)		137.19
(83.83%)		📈 view plot
🚷 view threshold		593.00
(-0.50%)		605.22
(97.98%)
serialize_deserialize_handle_subscribe📈 view plot
🚷 view threshold		27,946.00
(+0.47%)		28,662.77
(97.50%)		📈 view plot
🚷 view threshold		9,669.00
(+0.19%)		9,786.44
(98.80%)		📈 view plot
🚷 view threshold		13,676.00
(+0.18%)		13,841.47
(98.80%)		📈 view plot
🚷 view threshold		68.00
(+7.30%)		75.64
(89.90%)		📈 view plot
🚷 view threshold		398.00
(+0.60%)		417.82
(95.26%)
serialize_deserialize_submit📈 view plot
🚷 view threshold		115,415.00
(+0.15%)		115,982.26
(99.51%)		📈 view plot
🚷 view threshold		68,223.00
(+0.16%)		68,510.62
(99.58%)		📈 view plot
🚷 view threshold		97,935.00
(+0.19%)		98,348.61
(99.58%)		📈 view plot
🚷 view threshold		66.00
(+4.40%)		79.86
(82.65%)		📈 view plot
🚷 view threshold		490.00
(-0.18%)		499.78
(98.04%)
serialize_deserialize_subscribe📈 view plot
🚷 view threshold		23,328.00
(+0.39%)		24,083.70
(96.86%)		📈 view plot
🚷 view threshold		8,228.00
(+0.28%)		8,343.39
(98.62%)		📈 view plot
🚷 view threshold		11,598.00
(+0.34%)		11,750.76
(98.70%)		📈 view plot
🚷 view threshold		36.00
(-3.54%)		44.43
(81.02%)		📈 view plot
🚷 view threshold		330.00
(+0.50%)		349.70
(94.37%)
serialize_submit📈 view plot
🚷 view threshold		99,800.00
(-0.07%)		100,522.90
(99.28%)		📈 view plot
🚷 view threshold		61,569.00
(+0.08%)		61,855.73
(99.54%)		📈 view plot
🚷 view threshold		88,355.00
(+0.09%)		88,748.10
(99.56%)		📈 view plot
🚷 view threshold		49.00
(+3.11%)		67.28
(72.83%)		📈 view plot
🚷 view threshold		320.00
(-1.38%)		333.73
(95.89%)
serialize_subscribe📈 view plot
🚷 view threshold		11,340.00
(-0.53%)		11,758.53
(96.44%)		📈 view plot
🚷 view threshold		4,198.00
(+0.32%)		4,313.25
(97.33%)		📈 view plot
🚷 view threshold		5,845.00
(+0.35%)		5,997.50
(97.46%)		📈 view plot
🚷 view threshold		14.00
(+8.41%)		20.91
(66.95%)		📈 view plot
🚷 view threshold		155.00
(-1.57%)		164.12
(94.44%)
🐰 View full continuous benchmarking report in Bencher

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 2, 2024 •
edited
Loading

🐰 Bencher Report

Branch1179/merge
Testbedsv2
Click to view all benchmark results
BenchmarkEstimated CyclesBenchmark Result
estimated cycles
(Result Δ%)		Upper Boundary
estimated cycles
(Limit %)		InstructionsBenchmark Result
instructions
(Result Δ%)		Upper Boundary
instructions
(Limit %)		L1 AccessesBenchmark Result
accesses
(Result Δ%)		Upper Boundary
accesses
(Limit %)		L2 AccessesBenchmark Result
accesses
(Result Δ%)		Upper Boundary
accesses
(Limit %)		RAM AccessesBenchmark Result
accesses
(Result Δ%)		Upper Boundary
accesses
(Limit %)
client_sv2_handle_message_common📈 view plot
🚷 view threshold		2,087.00
(-0.17%)		2,201.35
(94.81%)		📈 view plot
🚷 view threshold		473.00
(-0.10%)		490.84
(96.37%)		📈 view plot
🚷 view threshold		737.00
(+0.12%)		761.05
(96.84%)		📈 view plot
🚷 view threshold		4.00
(-27.58%)		14.25
(28.08%)		📈 view plot
🚷 view threshold		38.00
(+0.25%)		41.02
(92.63%)
client_sv2_handle_message_mining📈 view plot
🚷 view threshold		8,196.00
(-0.18%)		8,308.24
(98.65%)		📈 view plot
🚷 view threshold		2,137.00
(-0.03%)		2,143.08
(99.72%)		📈 view plot
🚷 view threshold		3,161.00
(+0.01%)		3,170.74
(99.69%)		📈 view plot
🚷 view threshold		34.00
(-5.02%)		43.01
(79.05%)		📈 view plot
🚷 view threshold		139.00
(-0.12%)		141.37
(98.32%)
client_sv2_mining_message_submit_standard📈 view plot
🚷 view threshold		6,285.00
(+0.07%)		6,384.97
(98.43%)		📈 view plot
🚷 view threshold		1,750.00
(-0.06%)		1,769.06
(98.92%)		📈 view plot
🚷 view threshold		2,550.00
(-0.16%)		2,580.06
(98.83%)		📈 view plot
🚷 view threshold		19.00
(+12.38%)		24.72
(76.86%)		📈 view plot
🚷 view threshold		104.00
(-0.05%)		106.94
(97.25%)
client_sv2_mining_message_submit_standard_serialize📈 view plot
🚷 view threshold		14,678.00
(-0.46%)		15,020.27
(97.72%)		📈 view plot
🚷 view threshold		4,694.00
(-0.02%)		4,713.06
(99.60%)		📈 view plot
🚷 view threshold		6,758.00
(-0.01%)		6,785.36
(99.60%)		📈 view plot
🚷 view threshold		44.00
(+0.84%)		53.57
(82.13%)		📈 view plot
🚷 view threshold		220.00
(-0.89%)		229.26
(95.96%)
client_sv2_mining_message_submit_standard_serialize_deserialize📈 view plot
🚷 view threshold		27,478.00
(-0.20%)		27,751.24
(99.02%)		📈 view plot
🚷 view threshold		10,585.00
(+0.09%)		10,635.43
(99.53%)		📈 view plot
🚷 view threshold		15,408.00
(+0.13%)		15,480.56
(99.53%)		📈 view plot
🚷 view threshold		76.00
(-6.36%)		89.44
(84.98%)		📈 view plot
🚷 view threshold		334.00
(-0.42%)		342.93
(97.40%)
client_sv2_open_channel📈 view plot
🚷 view threshold		4,383.00
(-0.55%)		4,640.57
(94.45%)		📈 view plot
🚷 view threshold		1,461.00
(-0.03%)		1,478.84
(98.79%)		📈 view plot
🚷 view threshold		2,158.00
(-0.07%)		2,187.43
(98.65%)		📈 view plot
🚷 view threshold		11.00
(+19.32%)		18.15
(60.60%)		📈 view plot
🚷 view threshold		62.00
(-1.44%)		69.10
(89.73%)
client_sv2_open_channel_serialize📈 view plot
🚷 view threshold		14,002.00
(-0.57%)		14,512.30
(96.48%)		📈 view plot
🚷 view threshold		5,064.00
(-0.01%)		5,081.84
(99.65%)		📈 view plot
🚷 view threshold		7,322.00
(-0.05%)		7,354.92
(99.55%)		📈 view plot
🚷 view threshold		41.00
(+16.49%)		43.32
(94.65%)		📈 view plot
🚷 view threshold		185.00
(-1.62%)		200.41
(92.31%)
client_sv2_open_channel_serialize_deserialize📈 view plot
🚷 view threshold		22,639.00
(-0.13%)		22,914.00
(98.80%)		📈 view plot
🚷 view threshold		8,027.00
(+0.12%)		8,075.69
(99.40%)		📈 view plot
🚷 view threshold		11,674.00
(+0.10%)		11,757.37
(99.29%)		📈 view plot
🚷 view threshold		79.00
(+6.72%)		84.31
(93.71%)		📈 view plot
🚷 view threshold		302.00
(-0.62%)		312.97
(96.50%)
client_sv2_setup_connection📈 view plot
🚷 view threshold		4,681.00
(-0.10%)		4,776.00
(98.01%)		📈 view plot
🚷 view threshold		1,502.00
(-0.03%)		1,519.84
(98.83%)		📈 view plot
🚷 view threshold		2,276.00
(-0.13%)		2,303.11
(98.82%)		📈 view plot
🚷 view threshold		12.00
(+31.62%)		15.59
(76.95%)		📈 view plot
🚷 view threshold		67.00
(-0.68%)		69.95
(95.78%)
client_sv2_setup_connection_serialize📈 view plot
🚷 view threshold		16,118.00
(-0.42%)		16,498.76
(97.69%)		📈 view plot
🚷 view threshold		5,963.00
(-0.01%)		5,980.84
(99.70%)		📈 view plot
🚷 view threshold		8,663.00
(-0.00%)		8,694.24
(99.64%)		📈 view plot
🚷 view threshold		42.00
(+4.23%)		53.72
(78.18%)		📈 view plot
🚷 view threshold		207.00
(-1.05%)		217.37
(95.23%)
client_sv2_setup_connection_serialize_deserialize📈 view plot
🚷 view threshold		35,480.00
(-0.21%)		35,737.00
(99.28%)		📈 view plot
🚷 view threshold		14,855.00
(+0.07%)		14,904.75
(99.67%)		📈 view plot
🚷 view threshold		21,825.00
(+0.09%)		21,915.35
(99.59%)		📈 view plot
🚷 view threshold		92.00
(-1.53%)		115.80
(79.45%)		📈 view plot
🚷 view threshold		377.00
(-0.64%)		384.81
(97.97%)
🐰 View full continuous benchmarking report in Bencher

@GitGab19 GitGab19 added this to the 1.1.0 milestone Oct 9, 2024
@GitGab19 GitGab19 linked an issue Oct 9, 2024 that may be closed by this pull request
framing-sv2 doc cmts do not conform to standard #1178
Open
@plebhash
Copy link
Collaborator

plebhash commented Oct 9, 2024

we should take this opportunity to add a README.md

@GitGab19 GitGab19 removed this from the 1.1.0 milestone Oct 11, 2024
@rrybarczyk rrybarczyk force-pushed the framing-sv2-doc-cmt-syntax-update branch from e93c055 to 4bdf140 Compare October 14, 2024 21:20
@codecov Codecov
Copy link

codecov bot commented Oct 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.36%. Comparing base (384df6c) to head (b01167b).
Report is 4 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1179   +/-   ##
=======================================
  Coverage   19.36%   19.36%           
=======================================
  Files         164      164           
  Lines       10811    10811           
=======================================
  Hits         2094     2094           
  Misses       8717     8717
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <ø> (ø)
binary_serde_sv2-coverage 3.65% <ø> (ø)
binary_sv2-coverage 5.46% <ø> (ø)
bip32_derivation-coverage 0.00% <ø> (ø)
buffer_sv2-coverage 25.02% <ø> (ø)
codec_sv2-coverage 0.01% <ø> (ø)
common_messages_sv2-coverage 0.13% <ø> (ø)
const_sv2-coverage 0.00% <ø> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage 0.29% <ø> (ø)
jd_client-coverage 0.00% <ø> (ø)
jd_server-coverage 8.13% <ø> (ø)
job_declaration_sv2-coverage 0.00% <ø> (ø)
key-utils-coverage 2.39% <ø> (ø)
mining-coverage 2.51% <ø> (ø)
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.70% <ø> (ø)
noise_sv2-coverage 4.35% <ø> (ø)
pool_sv2-coverage 1.38% <ø> (ø)
protocols 24.72% <ø> (ø)
roles 6.63% <ø> (ø)
roles_logic_sv2-coverage 8.06% <ø> (ø)
sv1-mining-device-coverage 0.00% <ø> (ø)
sv2_ffi-coverage 0.00% <ø> (ø)
template_distribution_sv2-coverage 0.00% <ø> (ø)
translator_sv2-coverage 9.61% <ø> (ø)
utils 25.13% <ø> (ø)
v1-coverage 2.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rrybarczyk rrybarczyk force-pushed the framing-sv2-doc-cmt-syntax-update branch 4 times, most recently from 28d8ae9 to 222829a Compare October 15, 2024 01:35
@rrybarczyk
Copy link
Collaborator Author

A couple notes of areas of improvement:

  • I do not think I captured the need for Header::encrypted_len() properly. I understand this to be used when the message is too big to fit into a single frame, defined by the SV2_FRAME_CHUNK_SIZE constant. But I see this method is only used in the codec_sv2::decoder::WithNoise struct. Why don't we use this constant in the Sv2Frame decoder WithoutNoise struct? Do Sv2 messages ever exceed this max size that can fit into an Sv2Frame? Or do only the handshake messages (since they are so much bigger) ever exceed the size? To be clear, this technically does not have to do with Noise, but rather message chunking. I am just trying to understand why this method only is used on the Noise codec logic and not for Sv2Frames.
  • I don't think my handshake.rs example does a great job of showcasing the handshake framing logic. Any feedback would be great, and I can always delete it if it is not helpful.
  • I would like to make sure the reader/user understands that the codec_sv2 crate is used directly after using this framing_sv2 crate to encode the framed message for transmission and then decode it on receipt. However, I never could find the right wording or right spot to explain this. Not sure if it matters or not?
  • In the README, I used a lot of language in the doc comments to stay consistent. Happy to modify if needed.

@rrybarczyk rrybarczyk marked this pull request as ready for review October 15, 2024 01:46
GitGab19
GitGab19 reviewed Oct 15, 2024
View reviewed changes
protocols/v2/framing-sv2/README.md Outdated Show resolved Hide resolved
protocols/v2/framing-sv2/README.md Outdated Show resolved Hide resolved
protocols/v2/framing-sv2/examples/handshake_frame.rs Outdated Show resolved Hide resolved
Shourya742
Shourya742 suggested changes Oct 15, 2024
View reviewed changes
protocols/v2/framing-sv2/README.md Outdated Show resolved Hide resolved
protocols/v2/framing-sv2/src/header.rs Show resolved Hide resolved
protocols/v2/framing-sv2/src/header.rs Outdated Show resolved Hide resolved
@rrybarczyk rrybarczyk force-pushed the framing-sv2-doc-cmt-syntax-update branch 3 times, most recently from e2edf20 to b2d1e41 Compare October 15, 2024 14:07
@rrybarczyk rrybarczyk force-pushed the framing-sv2-doc-cmt-syntax-update branch from b2d1e41 to 11282cc Compare October 15, 2024 15:40
@rrybarczyk rrybarczyk force-pushed the framing-sv2-doc-cmt-syntax-update branch 2 times, most recently from a327909 to 9677dff Compare October 16, 2024 01:14
plebhash
plebhash reviewed Oct 16, 2024
View reviewed changes
protocols/v2/framing-sv2/README.md Outdated
Comment on lines 54 to 55
Encode and decode standard Sv2 frames, detailing the message serialization and transmission
process for unencrypted communications.
Copy link
Collaborator

@plebhash plebhash Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we should avoid bringing up "unencrypted" here... sure, the example is unencrypted, but from a conceptual standpoint, a Sv2Frame can be serialized into bytes just like on this example, and later be transmitted over an encrypted channel

so drawing attention to "unencrypted" here is misleading

@rrybarczyk rrybarczyk force-pushed the framing-sv2-doc-cmt-syntax-update branch from 9677dff to 6fdeb5f Compare October 16, 2024 18:38
plebhash
plebhash reviewed Oct 16, 2024
View reviewed changes
protocols/v2/framing-sv2/examples/handshake_frame.rs
Copy link
Collaborator

@plebhash plebhash Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

#1179 (comment)

I don't think my handshake.rs example does a great job of showcasing the handshake framing logic. Any feedback would be great, and I can always delete it if it is not helpful.

indeed, I don't feel we're really showing anything useful to the reader here

when you really look at it, HandShakeFrame is nothing but a simple wrapper around payload: Slice, and its methods don't really do anything fancy:

  • get_payload_when_handshaking has a big name, while it's nothing but a getter for the inner payload
  • encoded_length which just returns len() for the inner payload
  • from_bytes and from_bytes_unchecked are a simple constructor (and both methods are actually redundant)

so I guess there's nothing that really justifies a dedicated example around HandShakeFrame

Copy link
Collaborator

@plebhash plebhash Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I remember we used to have a Frame trait on this crate, which has already been removed via #976 because while it aimed to unify Sv2Frame and HandShakeFrame under the same interface, the trait was never really used as a trait bound anywhere

By trying to unify Sv2Frame and HandShakeFrame under the same interface, we ended up with the worst of both worlds, with a rather wonky and unintuitive API for both

Sv2Frame is on its way to be improved via #1033

I wonder if we even need HandShakeFrame, but getting rid of it would have implications on enum Frame and StandardEitherFrame (from codec_sv2), so unfortunately it's non trivial.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for that context, very helpful. Let me think on it a bit more.

@plebhash
Copy link
Collaborator

plebhash commented Oct 17, 2024 •
edited
Loading

we should bump framing_sv2 MINOR version on this PR

see #1221 for more details

edit:

it was pointed out on #1222 that we should probably start bumping PATCH instead

I started a new Discussion on Standardization of Docs #970 (comment)

@rrybarczyk rrybarczyk force-pushed the framing-sv2-doc-cmt-syntax-update branch from 6fdeb5f to b01167b Compare October 17, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plebhash plebhash plebhash left review comments

@GitGab19 GitGab19 Awaiting requested review from GitGab19

@Shourya742 Shourya742 Awaiting requested review from Shourya742

At least 1 approving review is required to merge this pull request.

Assignees

@rrybarczyk rrybarczyk

Labels
documentation Improvements or additions to documentation protocols Lowest level protocol logic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

framing-sv2 doc cmts do not conform to standard
4 participants
@rrybarczyk @plebhash @GitGab19 @Shourya742