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

Implement Clone for LightsparkClient and Requester to Leverage Actix Web Data Extractor #25

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Nsandomeno
Copy link

@Nsandomeno Nsandomeno commented Dec 20, 2023

Hi, team... I'm blown away by your pace and so excited about what you're up too. I'm curious if there has been a decision against implementing Clone for LightsparkClient and Requester, to avoid a global client instance.

If Clone is implemented for these two structs than LightsparkClient can be injected as a Data Extractor in the Web Actix framework, preventing the need to initialize per request.

I've been pondering security considerations that would warrant this idea dumb, but the implementations of OperationSignKey are also Clone, which I believe should cover not only signing ops that recover keys from Lightspark but, manually loaded and remote signing operations as well... so I wanted to raise it.

My lightspark-rs fork commit is here which enables something like this:

#[get("/health-check")]
async fn health_check(
    client: web::Data<LightsparkClient<Secp256k1SigningKey>>,
) -> impl Responder {
    info!("Checking Lightspark connection...");
    let response = client.get_current_account().await; 

    match response {
        Ok(_) => {
            info!("API is healthy.");
            return HttpResponse::Ok().finish()
        },
        Err(_) => return HttpResponse::InternalServerError().finish()
    }
}


#[actix_web::main]
async fn main() -> std::io::Result<()> {
    let subscriber = FmtSubscriber::builder()
        .with_max_level(Level::DEBUG)
        .finish();

    tracing::subscriber::set_global_default(subscriber).expect("Failed to set global tracing subscriber.");

    let config = config::Config::new_from_env();
    let port = config.api_port;

    info!(config = format!("{:?}", config.clone()), "Booting server...");
    // TODO this could be wrapped as app_data if there is, to save the ammortize the cost of the client boot
    // across all client requests:
    // (1) no terrible reason to impl Clone on LightsparkClient and Requester

    let account_auth: AccountAuthProvider = AccountAuthProvider::new(
        config.api_client_id.clone(), config.api_client_secret.clone()
    );
    let app_config: web::Data::new(config);
    let client: web::Data<LightsparkClient<Secp256k1SigningKey>> = web::Data::new(LightsparkClient::new(account_auth).unwrap());

    HttpServer::new(move || {

        App::new()
            .app_data(app_config.clone())
            .app_data(client.clone())
            .wrap(middleware::NormalizePath::trim())
            .service(health_check)
    })
    .bind(("0.0.0.0", port))?
    .run()
    .await
}

I would greatly appreciate your thoughts!

P.S.

I'm an active candidate for your Backend Engineer role, building a lot of personal momentum within the Bitcoin tech stack, and would be over the moon to contribute to your mission.

@jklein24 jklein24 requested a review from zhenlu December 22, 2023 19:25
Copy link
Contributor

@zhenlu zhenlu left a comment

Choose a reason for hiding this comment

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

Hey @Nsandomeno, thanks for the contribution.

The code looks good, however please remove the submodule from repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this from the repo

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