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

Handle unwrap when shutting down JDC server #1352

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Jan 16, 2025

1st commit(9a35baa) resolves #1189

2nd and 3rd resolves #1351

@jbesraa jbesraa force-pushed the 2025-01-16-avoid-unwrap-jdc branch from 1d8516c to cd958d0 Compare January 16, 2025 08:41
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.08%. Comparing base (150360c) to head (6deea02).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1352   +/-   ##
=======================================
  Coverage   19.08%   19.08%           
=======================================
  Files         166      166           
  Lines       11066    11066           
=======================================
  Hits         2112     2112           
  Misses       8954     8954           
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <ø> (ø)
binary_serde_sv2-coverage 3.55% <ø> (ø)
binary_sv2-coverage 5.34% <ø> (ø)
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.28% <ø> (ø)
jd_client-coverage 0.00% <ø> (ø)
jd_server-coverage 7.79% <ø> (ø)
job_declaration_sv2-coverage 0.00% <ø> (ø)
key-utils-coverage 2.39% <ø> (ø)
mining-coverage 2.43% <ø> (ø)
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.70% <ø> (ø)
noise_sv2-coverage 4.44% <ø> (ø)
pool_sv2-coverage 2.04% <ø> (ø)
protocols 24.57% <ø> (ø)
roles 6.54% <ø> (ø)
roles_logic_sv2-coverage 7.93% <ø> (ø)
sv2_ffi-coverage 0.00% <ø> (ø)
template_distribution_sv2-coverage 0.00% <ø> (ø)
translator_sv2-coverage 9.60% <ø> (ø)
utils 25.13% <ø> (ø)
v1-coverage 2.41% <ø> (ø)

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
Contributor

@Shourya742 Shourya742 left a comment

Choose a reason for hiding this comment

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

Looks good!!!!

protocols/v2/roles-logic-sv2/src/errors.rs Show resolved Hide resolved
roles/jd-client/src/lib/downstream.rs Outdated Show resolved Hide resolved
@jbesraa jbesraa force-pushed the 2025-01-16-avoid-unwrap-jdc branch from cd958d0 to 09b806d Compare January 16, 2025 11:29
@jbesraa
Copy link
Contributor Author

jbesraa commented Jan 16, 2025

Thanks @Shourya742, addressed your comments

@jbesraa jbesraa force-pushed the 2025-01-16-avoid-unwrap-jdc branch from 09b806d to 073bcd2 Compare January 17, 2025 08:50
@jbesraa
Copy link
Contributor Author

jbesraa commented Jan 17, 2025

Rebased without further changes

@plebhash plebhash added the ready-to-be-merged triggers auto rebase bot label Jan 18, 2025
@pavlenex pavlenex force-pushed the 2025-01-16-avoid-unwrap-jdc branch from 073bcd2 to 8a6a41a Compare January 18, 2025 19:00
@plebhash plebhash removed the ready-to-be-merged triggers auto rebase bot label Jan 18, 2025
@plebhash
Copy link
Collaborator

plebhash commented Jan 18, 2025

oops, looks like SemVer CI is going to force us to bump MAJOR on roles_logic_sv2 here

taking note on #1345

@jbesraa jbesraa force-pushed the 2025-01-16-avoid-unwrap-jdc branch from 8a6a41a to 6bbbd9b Compare January 21, 2025 13:25
@jbesraa
Copy link
Contributor Author

jbesraa commented Jan 21, 2025

oops, looks like SemVer CI is going to force us to bump MAJOR on roles_logic_sv2 here

taking note on #1345

Ha! Error enum might be exported and causing this because a new property was added to it.

@plebhash
Copy link
Collaborator

oops, looks like SemVer CI is going to force us to bump MAJOR on roles_logic_sv2 here
taking note on #1345

Ha! Error enum might be exported and causing this because a new property was added to it.

yes, Error is exported and used on some roles crates

can we add a new commit bumping MAJOR on roles_logic_sv2 here? that would unblock the SemVer CI and allow us to merge... I already took note of this on #1345

@jbesraa jbesraa force-pushed the 2025-01-16-avoid-unwrap-jdc branch 3 times, most recently from 91a2087 to 5fa3813 Compare January 22, 2025 08:52
@jbesraa jbesraa force-pushed the 2025-01-16-avoid-unwrap-jdc branch 2 times, most recently from 02243ea to 151befc Compare January 22, 2025 14:37
@plebhash plebhash added ready-to-be-merged triggers auto rebase bot refactor Implies refactoring code roles Pertains to all roles labels Jan 22, 2025
In practice we always keep all of our crates updated with latest code
changes in all of other internal crates, so instead of maintaing the
versioning of each one for internal use, this commit just links to the
latest code in each crate.

This would alllow us to catch errors that might appear in one crate but
are originated in other. While this would sometimes require more work,
this can save us error that previously would appear only in the release
process.

This approach fits the current directory/project structre, but
if crates are moved outside of this repo, they should be linked to the
published crate on crates.io.
..This should prevent the code from panicing if a signal
to shutdown is sent the JDC server

As a result, MAJOR is pumped for `roles-logic-sv2` crate as new property
was added to public `Error` struct.
@pavlenex pavlenex force-pushed the 2025-01-16-avoid-unwrap-jdc branch from 151befc to 6deea02 Compare January 22, 2025 17:03
@plebhash plebhash merged commit e228c19 into stratum-mining:main Jan 22, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-be-merged triggers auto rebase bot refactor Implies refactoring code roles Pertains to all roles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid using unwrap in JDC listener Consider using internal crates without versioning
3 participants