Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Inject parity script to all dapps // Expand dapps to any ZIP file #7260

Merged
merged 8 commits into from
Dec 13, 2017

Conversation

ngotchac
Copy link
Contributor

This PR injects too all HTML files served by the dapps server the parity.js script that instantiate a new window.ethereumProvider object.

It also adds the possibility to now link a dapp to a ZIP file (like a Github Release), instead of only the repo + commit tupple
To do that, one must set as the commit field in the GithubHint contract entry the value 0x00..001. This is to prevent the dapp server to extract zip files that should remain zip files, and not dapps.

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

LGTM, minor grumbles.

@@ -167,13 +167,15 @@ fn should_return_fetched_dapp_content() {
response1.assert_status("HTTP/1.1 200 OK");
assert_security_headers_for_embed(&response1.headers);
assert_eq!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

could use assert!(condition, format, params..) instead of assert_eq!(expression, true, format, params..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -85,14 +89,16 @@ pub struct Content {
/// MIME type of the content
pub mime: Mime,
/// Content owner address
pub owner: Address,
pub owner: Address
Copy link
Collaborator

Choose a reason for hiding this comment

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

In rust we try to have trailing commas after each line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't there be a linter for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd love to have one, we tried using rustfmt some time ago, but there were many places were the formatting was a bit weird. We should give it a go again soon though!

Content {
url: account_slash_repo,
mime: mime,
owner: owner
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be simplified to:

Content {
  url: account_slash_repo,
  mime,
  owner,
}

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. labels Dec 11, 2017
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 11, 2017
@5chdn 5chdn added this to the 1.9 milestone Dec 11, 2017
@tomusdrw tomusdrw closed this Dec 11, 2017
@tomusdrw tomusdrw reopened this Dec 11, 2017
@svyatonik svyatonik merged commit 82d7fc5 into master Dec 13, 2017
@svyatonik svyatonik deleted the ng-injected-dapps branch December 13, 2017 06:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants