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

Refactor: roles_logic_sv2 crate #1268

Open
plebhash opened this issue Dec 2, 2024 · 9 comments
Open

Refactor: roles_logic_sv2 crate #1268

plebhash opened this issue Dec 2, 2024 · 9 comments

Comments

@plebhash
Copy link
Collaborator

plebhash commented Dec 2, 2024

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
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

roles_logic_sv2 declares disable_nopanic feature flag:

# Code coverage tools may conflict with the nopanic logic, so we can disable it when needed
disable_nopanic = []

the only place this is used is on some code that was commented out for super_safe_lock:

pub fn super_safe_lock<F, Ret>(&self, thunk: F) -> Ret
where
F: FnOnce(&mut T) -> Ret,
{
//#[cfg(feature = "disable_nopanic")]
{
self.safe_lock(thunk).unwrap()
}
//#[cfg(not(feature = "disable_nopanic"))]
//{
// // based on https://github.com/dtolnay/no-panic
// struct __NoPanic;
// extern "C" {
// #[link_name = "super_safe_lock called on a function that may panic"]
// fn trigger() -> !;
// }
// impl core::ops::Drop for __NoPanic {
// fn drop(&mut self) {
// unsafe {
// trigger();
// }
// }
// }
// let mut lock = self.0.lock().expect("threads to never panic");
// let __guard = __NoPanic;
// let return_value = thunk(&mut *lock);
// core::mem::forget(__guard);
// drop(lock);
// return_value
//}
}

super_safe_lock is a desired but non-essential feature, as described on #476

IMO there's no point in keeping around:

  • an empty function
  • an unused feature flag

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

@plebhash
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

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
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

about channel_logic::channel_factory module:

  • ChannelFactory is private
  • ProxyExtendedChannelFactory and PoolChannelFactory are public, while both use ChannelFactory as an inner object

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
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

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

@plebhash
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

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
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

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

@plebhash
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

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

@plebhash
Copy link
Collaborator Author

plebhash commented Dec 2, 2024

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

@Fi3
Copy link
Collaborator

Fi3 commented Dec 4, 2024

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

roles-logic is supposed to be used by other impls directly so make sure that we really don't want to export the functions. Sometimes I feel that we should export more of them. Btw before removing ask me cause I use several of them.

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

No branches or pull requests

2 participants