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.

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

remove serde feature from protocol crate #1455

Conversation

Shourya742
Copy link
Contributor

Part of: #1387

@Shourya742 Shourya742 force-pushed the 2025-01-31-remove-serde-feature-from-protocol branch from bec0425 to b39cc2f Compare February 5, 2025 07:19
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.34%. Comparing base (450204e) to head (f7b068a).
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1455      +/-   ##
==========================================
- Coverage   19.44%   19.34%   -0.10%     
==========================================
  Files         146      144       -2     
  Lines       10244    10963     +719     
==========================================
+ Hits         1992     2121     +129     
- Misses       8252     8842     +590     
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <ø> (ø)
binary_serde_sv2-coverage 3.57% <ø> (+0.17%) ⬆️
binary_sv2-coverage 5.36% <ø> (+0.25%) ⬆️
bip32_derivation-coverage 0.00% <ø> (ø)
buffer_sv2-coverage 25.02% <ø> (ø)
codec_sv2-coverage 0.01% <ø> (+<0.01%) ⬆️
common_messages_sv2-coverage 0.13% <ø> (+<0.01%) ⬆️
const_sv2-coverage 0.00% <ø> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage 0.28% <ø> (+0.01%) ⬆️
jd_client-coverage 0.42% <ø> (ø)
jd_server-coverage 9.02% <ø> (ø)
job_declaration_sv2-coverage 0.00% <ø> (ø)
key-utils-coverage 2.39% <ø> (ø)
mining-coverage 2.43% <ø> (+0.09%) ⬆️
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.82% <ø> (ø)
noise_sv2-coverage 4.46% <ø> (+0.21%) ⬆️
protocols 24.81% <ø> (+0.95%) ⬆️
roles 6.43% <ø> (+2.86%) ⬆️
roles_logic_sv2-coverage 9.08% <ø> (+0.38%) ⬆️
sv2_ffi-coverage 0.00% <ø> (ø)
template_distribution_sv2-coverage 0.00% <ø> (ø)
translator_sv2-coverage 9.53% <ø> (?)
utils 25.13% <ø> (ø)
v1-coverage 2.42% <ø> (+0.11%) ⬆️

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really need to delete the entire example?

isn't there a way to keep it, just not using serde?

Copy link
Collaborator

@plebhash plebhash Feb 5, 2025

Choose a reason for hiding this comment

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

writing this example was an outcome of the documentation effort

it would be a pity to lose it just because of the serde deprecation, as it is still valuable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am writing an example without the serde flag..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in: 32cbab3

Copy link
Collaborator

@plebhash plebhash Feb 7, 2025

Choose a reason for hiding this comment

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

thanks, but please note that this should have been fixed on the original commit that erased the file, not adding a new commit to re-add the file

doing this way effectively erases the git history of the original file

I'm rebasing the branch to fix it

@plebhash
Copy link
Collaborator

plebhash commented Feb 5, 2025

looks like we're going to have to bump MAJOR for all the crates where the with_serde feature is being removed

https://github.com/stratum-mining/stratum/actions/runs/13152202385/job/36701547815?pr=1455

edit: actually, not necessarily ALL

some have already bumped MAJOR (before being published, e.g.: network_helpers_sv2 #1345 (comment))), so @Shourya742 please check for the need of bumping MAJOR of each crate by manually running cargo semver-checks on each commit

@Shourya742 Shourya742 force-pushed the 2025-01-31-remove-serde-feature-from-protocol branch 2 times, most recently from 6a79345 to 2b89e2c Compare February 6, 2025 04:39
@Shourya742 Shourya742 force-pushed the 2025-01-31-remove-serde-feature-from-protocol branch from 2b89e2c to 189d72a Compare February 6, 2025 14:12
@Shourya742 Shourya742 force-pushed the 2025-01-31-remove-serde-feature-from-protocol branch from 189d72a to 32cbab3 Compare February 7, 2025 04:55
@plebhash plebhash force-pushed the 2025-01-31-remove-serde-feature-from-protocol branch 2 times, most recently from 7133582 to f9e4135 Compare February 7, 2025 11:34
@plebhash plebhash force-pushed the 2025-01-31-remove-serde-feature-from-protocol branch from f9e4135 to f7b068a Compare February 7, 2025 11:35
@plebhash plebhash merged commit 19f26e3 into stratum-mining:main Feb 7, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants