stratum-mining / stratum

stratum
https://stratumprotocol.org
Other
224 stars 130 forks source link

Refactor: `roles_logic_sv2` crate #1268

Open plebhash opened 1 day ago

plebhash commented 1 day ago

Background

This task is an outcome of the protocols Rust docs issues tracked in #845.

While documenting protocols::v2::roles_logic_sv2 in #1014, areas of potential code debt were identified.

This issue servers as a place to list out these items to then be addressed in an organized manner. The initial Rust documentation effort was an immediate action, while a refactoring (which implies breaking API changes) is not so urgent priority, so for now we should leave this in the backlog for an appropriate moment in the future.

plebhash commented 1 day ago

roles_logic_sv2 declares disable_nopanic feature flag:

https://github.com/stratum-mining/stratum/blob/67a3f008597d33e789f5e715ccde16df6e22e441/protocols/v2/roles-logic-sv2/Cargo.toml#L45-L46

the only place this is used is on some code that was commented out for super_safe_lock: https://github.com/stratum-mining/stratum/blob/67a3f008597d33e789f5e715ccde16df6e22e441/protocols/v2/roles-logic-sv2/src/utils.rs#L82-L112

super_safe_lock is a desired but non-essential feature, as described on https://github.com/stratum-mining/stratum/issues/476

IMO there's no point in keeping around:

we should clean those up and simply leave #476 as a tracker for this potential improvement

plebhash commented 1 day ago

routing_logic is only useful for advanced proxy implementations with multiplexing of Standard Channels across different upstreams.

It is only consumed by mining_proxy crate, no other roles use any of it.

We should consider migrating this module away from roles_logic_sv2 crate into dedicated mining_proxy lib/bin crate.

plebhash commented 23 hours ago

about channel_logic::channel_factory module:

as discussed here, this architecture is suboptimal

a better approach could be inspired by the handlers module, which defines traits to be implemented with custom logic by the application layer

this trait-based approach would also make it easier to leverage roles_logic_sv2 to create re-usable libs on the application layer (roles becoming libs, not only bin)

this should also cover channel_logic::proxy_group_channel

plebhash commented 23 hours ago

similarly to channel_logic::channel_factory described above, we should also consider migrating job_creator and job_dispatcher into a trait-based approach

plebhash commented 23 hours ago

as discussed here, parsers::PoolMessages is a misleading name

we should rename it into AnyMessage (or something similar), and drop the current parsers::AnyMessage alias

plebhash commented 21 hours ago

utils has many public functions that are only used internally, therefore they should be made private

plebhash commented 21 hours ago

utils::get_target is not used anywhere, therefore it should be cleaned up from the codebase

plebhash commented 21 hours ago

utils::Id and utils::GroupId feel like they belong to modules related to channels and job logic