Remove the dapps system #9017
Remove the dapps system #9017
Conversation
tomaka
added
A0-pleasereview 🤓
5chdn
added
the
B7-releasenotes 📜
|
Test |
|
Nice! Let's wait for Parity UI network dapps PR to be merged before we merge this |
5chdn
added
the
A1-onice 🌨
There was a problem hiding this comment.
Looks good, I wonder if we should leave the /api endpoints thought. There are two cases were those are useful:
/api/pingis 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)/api/healthwas 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>( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
added
the
B9-blocker 🚧
5chdn
added
the
P2-asap 🌊
5chdn
removed
the
A1-onice 🌨
|
Ok this is ready |
| DappsConfiguration { | ||
| enabled: self.dapps_enabled(), | ||
| dapps_path: PathBuf::from(self.directories().dapps), | ||
| extra_dapps: if self.args.cmd_dapp { |
There was a problem hiding this comment.
Do we need to remove the corresponding CMD and ARG in parity/cli/mod.rs? I think it's basically those:
- https://github.com/tomaka/parity/blob/8a612b66b5918e704dbb0e5484d58638867f9ae4/parity/cli/mod.rs#L29
- https://github.com/tomaka/parity/blob/8a612b66b5918e704dbb0e5484d58638867f9ae4/parity/cli/mod.rs#L542
And we probably also need to add them to parity/deprecated.rs.
There was a problem hiding this comment.
There's also the config serialization struct to be removed: https://github.com/tomaka/parity/blob/8a612b66b5918e704dbb0e5484d58638867f9ae4/parity/cli/mod.rs#L1223
For that, we probably want to do something similar to https://github.com/tomaka/parity/blob/8a612b66b5918e704dbb0e5484d58638867f9ae4/parity/cli/mod.rs#L1150 to not break current config files.
|
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! |
parity/deprecated.rs
Outdated
| result.push(Deprecated::Removed("--no-dapps")); | ||
| } | ||
|
|
||
| if args.arg_dapps_path != "" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
5chdn
added
A6-mustntgrumble 💦
sorpaas
added
A8-looksgood 🦄
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.