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

P 491 - url checking #2511

Merged
merged 9 commits into from
Feb 23, 2024
Merged

P 491 - url checking #2511

merged 9 commits into from
Feb 23, 2024

Conversation

BillyWooo
Copy link
Collaborator

  • fix the url hardcode typo
  • add url sanity check at initialization phase
  • remove build_client, replace with build_client_with_cert

@BillyWooo BillyWooo requested review from kziemianek and a team February 21, 2024 00:20
Copy link

linear bot commented Feb 21, 2024

@BillyWooo BillyWooo changed the title P 491 P 491 - url checking Feb 21, 2024
@@ -28,7 +28,7 @@ pub mod files {

// used by enclave
/// Path to the light-client db for the Integritee parentchain.
pub const LITENTRY_PARENTCHAIN_LIGHT_CLIENT_DB_PATH: &str = "integritee_lcdb";
Copy link
Member

Choose a reason for hiding this comment

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

will this require --clean-state for currently deployed bc-worker or some manual work during deployment for renaming dirs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you want to keep history data, just rename the previous folder from integritee_lcdb to litentry_lcdb.

let http_client = HttpClient::new(DefaultSend {}, true, Some(TIMEOUT), Some(headers), None);
RestClient::new(http_client, base_url)
}

pub fn build_client_with_cert(
base_url: &str,
Copy link
Member

Choose a reason for hiding this comment

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

Can this function accept already parsed url Url instead of str ?

}
pub fn set_twitter_official_url(&mut self, v: String) {
pub fn set_twitter_official_url(&mut self, v: String) -> Result<(), Error> {
check_url(&v)?;
debug!("set_twitter_official_url: {:?}", v);
self.twitter_official_url = v;
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered changing twitter_official_url type to Url ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Had similar comment above :D

log::debug!("Initializing data providers config");

// default prod config
let mut config = DataProviderConfig {
twitter_official_url: "https://api.twitter.com".to_string(),
twitter_litentry_url: "http://127.0.0.1:9527".to_string(),
twitter_litentry_url: "http://127.0.0.1:9527".to_string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If these URL is going to be used asUrl anyway, can we declare it as Url type?
Same applies to setters

}
pub fn set_twitter_official_url(&mut self, v: String) {
pub fn set_twitter_official_url(&mut self, v: String) -> Result<(), Error> {
check_url(&v)?;
debug!("set_twitter_official_url: {:?}", v);
self.twitter_official_url = v;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Had similar comment above :D

Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment

Choose a reason for hiding this comment

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

We could get it merged first but we'll need a follow-up to take Url as param

@BillyWooo BillyWooo enabled auto-merge (squash) February 23, 2024 14:05
@BillyWooo BillyWooo merged commit 655fc19 into dev Feb 23, 2024
31 checks passed
@BillyWooo BillyWooo deleted the p-491 branch February 23, 2024 15:00
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.

None yet

3 participants