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

Remove the dapps system #9017

Merged
merged 12 commits into from
Jul 11, 2018
Merged

Remove the dapps system #9017

merged 12 commits into from
Jul 11, 2018

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jun 29, 2018

The UI has been removed, therefore we no longer need the dapps service.

Please let me know if I did something stupid by removing too much.

@tomaka tomaka added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M7-ui M6-rpcapi 📣 RPC API. labels Jun 29, 2018
@5chdn 5chdn added this to the 1.12 milestone Jun 30, 2018
@5chdn 5chdn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Jun 30, 2018
@5chdn
Copy link
Contributor

5chdn commented Jun 30, 2018

Test v1::tests::mocked::parity_set::rpc_parity_set_dapps_list failed.

@axelchalon
Copy link
Contributor

Nice! Let's wait for Parity UI network dapps PR to be merged before we merge this

@5chdn 5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jun 30, 2018
@debris debris requested a review from tomusdrw July 2, 2018 07:37
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.

Looks good, I wonder if we should leave the /api endpoints thought. There are two cases were those are useful:

  1. /api/ping is useful to detect if the node is running (sometimes you might not have access to RPC due to CORS / origins validation, but still be able to be sure the node is running)
  2. /api/health was used to detect if the node is running correctly (200 if all good, 500 in case of any error) - this was quite useful for automated monitoring without hassle (most tools automatically handle health endpoint)

Ok(builder.start_http(addr)?)
/// Same as `start_http`, but takes an additional `middleware` parameter that is introduced as a
/// hyper middleware.
pub fn start_http_with_middleware<M, S, H, T, R>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about the duplication here, maybe pass a function that can add something to the builder: F: FnOnce(&mut Builder) function instead of Option<R>? (Cause I beleive the reason for duplication is that in case we are passing None the type cannot be inferred)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cause I beleive the reason for duplication is that in case we are passing None the type cannot be inferred

Indeed.

Theoretically I prefer the duplication, because with a closure the user of the API is able to modify more of the builder than if we just pass a middleware.

@5chdn 5chdn added the B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. label Jul 2, 2018
@5chdn 5chdn added the P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. label Jul 5, 2018
@5chdn 5chdn removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 9, 2018
@5chdn
Copy link
Contributor

5chdn commented Jul 9, 2018

Ok this is ready

DappsConfiguration {
enabled: self.dapps_enabled(),
dapps_path: PathBuf::from(self.directories().dapps),
extra_dapps: if self.args.cmd_dapp {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to remove the corresponding CMD and ARG in parity/cli/mod.rs? I think it's basically those:

And we probably also need to add them to parity/deprecated.rs.

Copy link
Collaborator

@sorpaas sorpaas Jul 10, 2018

Choose a reason for hiding this comment

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

@5chdn
Copy link
Contributor

5chdn commented Jul 10, 2018

I'm sorry for all the conflicts. This has top priority now and I will make sure nobody else merges anything else :D

@tomaka
Copy link
Contributor Author

tomaka commented Jul 10, 2018

I'm sorry for all the conflicts. This has top priority now and I will make sure nobody else merges anything else :D

Thank you!

@5chdn 5chdn requested review from tomusdrw and sorpaas July 10, 2018 17:45
result.push(Deprecated::Removed("--no-dapps"));
}

if args.arg_dapps_path != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the default is "$BASE/dapps" so this deprecation might be always printing?

My suggestion would be to change arg_dapps_path in parity/cli/mod.rs to be Option<String> with default to None. In that case, we can detect whether there's any attempt to set that arg and reliably print out deprecation warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, true. I put it as an empty string because the deprecated test builds the config with Default::default() (which sets the value to empty).
Fixed with an Option.

Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM. Just a final grumble on #9017 (comment)

@5chdn 5chdn added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 11, 2018
@sorpaas sorpaas 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 Jul 11, 2018
@5chdn 5chdn merged commit 494eb4a into openethereum:master Jul 11, 2018
@tomaka tomaka deleted the rm-dapps branch July 11, 2018 10:51
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. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants