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

quick fix to gateway prober #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bibo7086
Copy link

Changes (I think):

  1. Gateway list URL argument:
    • Updated the gateway-list argument to support URLs for the JSON gateway list.
  2. Monitoring node API address parsing:
    • Improved parsing of the monitor_api_address to ensure compatibility with addresses both with and without the http:// prefix.
    • Automatically prepends http:// to the monitor_api_address if it’s missing.
  3. Gateway list processing:
    • improved the logic for loading the gateway list by normalizing entries without an http or https scheme, automatically converting them to https by default.
  4. Probing gateway URL construction (main):
    • Fixed an issue where the :hash placeholder in the gateway URL was not properly replaced with the actual CID (content identifier). Now, the CID is correctly inserted into the URL.

There are probably more "Rusty" ways to do these things.

accept non-standard gateway json
Copy link
Member

@mrd0ll4r mrd0ll4r left a comment

Choose a reason for hiding this comment

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

I made a few comments, looks mostly good, just a few small things :)

format!("http://{}", monitor_api_address)
};

let u = monitor_api_address.parse::<http::Uri>()
Copy link
Member

Choose a reason for hiding this comment

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

Does this whole construction work? I don't remember if the path_and_query in the original was important... probably to strip any path components already present? But should be fine like this.

@@ -199,6 +202,18 @@ async fn do_probing() -> Result<()> {
)))
}
};

gateway_list = gateway_list
.into_iter()
Copy link
Member

Choose a reason for hiding this comment

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

You can merge this with the part above by just appending everything from .map to the closing curly brace of the match expression. That avoids the mutable binding, which is always nice.
Alternatively, if that doesn't look nice, something like this also works:

let a: Vec<String> = {
    let mut a_mut = foo();
    a_mut.into_iter().do_stuff().collect()
}

This keeps the mutability inside the block.

@@ -554,8 +569,11 @@ async fn probe_gateway(
num_tries: u32,
timeout: Duration,
) -> Result<()> {
let mut url = Url::parse(gateway_url.replace(":hash", &cid).as_str())?;
let formatted_url = format!("{}/ipfs/{}", gateway_url, cid);
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, they removed the placeholders in the official list.
For backwards compatibility, could you check if the placeholder is there and only append this if it's not? I have a bunch of code relying on the old format...

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