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: remove unmaintained surf for reqwest #1448

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Sep 13, 2024

What was wrong?

fixes #1446 surf has been unmaintained for over 2 years now, it is also using a old version of packages which prevents us from compiling Trin to Risc-v #1444

How was it fixed?

Just convert surf -> reqwest equivalents, which was pretty straight forward

Copy link
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but there is one big issue in portal-bridge (see comment).

Also, light-client crate also depends on reqwest, so we should update that dependency (should use workspace version)

portal-bridge/src/api/execution.rs Outdated Show resolved Hide resolved
portal-bridge/src/bridge/era1.rs Outdated Show resolved Hide resolved
src/bin/test_providers.rs Outdated Show resolved Hide resolved
.add_header("CF-Access-Client-Secret", client_secret)
.map_err(|_| "to be able to add client secret header")?
.set_base_url(url);
pub fn url_to_client(url: Url) -> Result<ClientWithMiddleware, String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The surf client supported setting base_url, while reqwest doesn't. Based on the name of this function, I think that's the expected behavior. In fact, all uses of this function (consensus.rs and execution.rs) now only pass relative path, and they won't work.

Since this function is doing more than just setting base_url and correct headers (in case of pandaops), I would say we should do following:

  • have fn create_client() -> Result<ClientWithMiddleware, String> that does everything that this function do, except setting pandaops headers.
  • have different function that accepts &mut Request. that adds pandaops headers if appropriate

Something like:

fn add_pandaops_headers(request: &mut Request) {
    let host = request.url().host_str().unwrap_or("");
    if !host.contains("pandaops.io") {
        return;
    }
    let client_id = ...;
    request.headers_mut().insert("CF-Access-Client-Id", ...);
    let client_secret = ...;
    request.headers_mut().insert("CF-Access-Client-Secret", ...);
}

// and in other places:
let client = create_client()?;
let mut request = client.post("some-url").build()?;
add_pandaops_headers(&mut request);
let result = client.execute().await?;

Alternatively, we can have a function combines last 3 lines into something like this:

async fn add_pandaops_headers_and_execute(request_builder: RequestBuilder) -> Result<Response, Error> {
    let (client, request) = request_builder.build_split();

    //
    // setting pandaops headers
    //

     client.execute().await
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I will just implement base_url for Reqwest

portal-bridge/src/cli.rs Outdated Show resolved Hide resolved
src/bin/test_providers.rs Outdated Show resolved Hide resolved
src/bin/test_providers.rs Outdated Show resolved Hide resolved
trin-execution/src/era/manager.rs Outdated Show resolved Hide resolved
@@ -51,7 +51,7 @@ pub struct ExecutionApi {
}

impl ExecutionApi {
pub async fn new(primary: Url, fallback: Url) -> Result<Self, surf::Error> {
pub async fn new(primary: Url, fallback: Url) -> Result<Self, reqwest_middleware::Error> {
// Only check that provider is connected & available if not using a test provider.
debug!(
"Starting ExecutionApi with primary provider: {primary} and fallback provider: {fallback}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on line 276 should be removed, and probably some testing to make sure that reqwest can handle the async load, though it's a very popular library so I expect it will

@KolbyML
Copy link
Member Author

KolbyML commented Sep 13, 2024

@njgheorghita @morph-dev ready for another review. I believe I resolved all the PR concerns
image
image
Here are a few pictures of history latest nothing seems wrong

Running backfill using infura I see what you mean nick when you said shisui uTP failures. It looks good as well

@njgheorghita
Copy link
Collaborator

Cool. I'm guessing this comment predates era1 files, so back then when in backfill mode, we'd be making quite a bit of parallel http requests. But, now that we have era1 files (and plan to use some version of era files for everything from merge -> latest?) I actually don't think this is much of an issue anymore. Unless there's somewhere else in the bridge that I'm missing where we'll have large amounts of parallel http requests....

@KolbyML
Copy link
Member Author

KolbyML commented Sep 13, 2024

large amounts of parallel http requests

I just ran backfill using infura as I said before, so it was making a large amount of parallel http requests. Reqwest's get 100 times more downloads a month than surf and is also built on hyper so I don't see why it wouldn't be able to handle large amounts of parallel http requests

Copy link
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

LGTM. But wait for Nick's approval as well.

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

LGTM

@KolbyML KolbyML merged commit 89ff040 into ethereum:master Sep 16, 2024
9 checks passed
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.

Remove surf and replace it with reqwest
4 participants