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

Duplicate and misplaced logic in roles crates #997

Open
rrybarczyk opened this issue Jun 23, 2024 · 0 comments
Open

Duplicate and misplaced logic in roles crates #997

rrybarczyk opened this issue Jun 23, 2024 · 0 comments
Assignees

Comments

@rrybarczyk
Copy link
Collaborator

rrybarczyk commented Jun 23, 2024

Background

When reviewing @plebhash #813, I found a lot of general and/or duplicate logic within each role's crate.

Problem

This logic is misplaced (does not make sense to belong to a specific role), and there is a lot of duplicate logic that can lead to bugs.

For example:

  • fn duration_from_toml is repeated in jd-server::lib::mod and jd-client::lib::proxy_config.
  • CoinbaseOutput is repeated in jd-server::lib::mod, jd-client::lib::proxy_config, and pool::lib::mining_pool::mod. This is only needed because the roles_logic_sv2::CoinbaseOutput does not derive serde::Deserialize which is needed when using the toml crate to read and parse the config tomls.
  • fn get_coinbase_output is repeated in jd-server::lib::mod, jd-client::lib::proxy_config, and pool::lib::mining_pool::mod. It is used to read and parse the coinbase_outputs from the role's config tomls.
  • Duplicate Args logic in translator::args and jd-client::args

Solution

Identify all the duplicate and common logic within the roles crates and begin moving them into the common crate.

Proper Handling of CoinbaseOutput

Regarding the duplicate CoinbaseOutput, this logic can be completely removed if a custom deserialization on Config is added like:

#[derive(Debug, Deserialize, Clone)]
pub struct ProxyConfig {
    //
    // Other ProxyConfig fields
    //
    #[serde(deserialize_with = "deser_coinbase_output")]
    pub coinbase_outputs: Vec<CoinbaseOutput>,
    //
    // Other ProxyConfig fields
    //
}

fn deser_coinbase_output<'de, D>(deserializer: D) -> Result<Vec<CoinbaseOutput>, D::Error>
where
    D: serde::Deserializer<'de>,
{
    todo!()
}

In order to make the get_coinbase_output method working, the following implementation should be added to protocols::v2::roles_logic_sv2::utils:

impl TryFrom<&CoinbaseOutput> for Script {
    type Error = Error;

    fn try_from(value: &CoinbaseOutput) -> Result<Self, Self::Error> {
        // Same implementation as the existing `impl TryFrom<CoinbaseOutput> for Script` in this file
    }
}

Then all duplicate CoinbaseOutput structs in the roles crates can be removed.

@rrybarczyk rrybarczyk self-assigned this Jun 23, 2024
@rrybarczyk rrybarczyk changed the title Need a general utility crate in roles/roles-utils Duplicate and misplaced logic in roles crates Jun 23, 2024
@rrybarczyk rrybarczyk added this to the Roles Refactor milestone Jun 24, 2024
@rrybarczyk rrybarczyk removed this from the Roles Refactor milestone Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant