Use TryFrom instead of From+panic for Builtin #11140

rokob · 2019-10-08T05:23:10Z

Currently there is a panic if a bad builtin name is entered in the chain spec. This PR replaces that panic by returning an Error instead of panicking in that situation and then using TryFrom instead of From to create the Builder type.

I'm not sure what compiler version you are using as I got errors and a ton of change from running rustfmt, so I finagled it to run over just my changes but there might still be some stylistic issues.

Also, the code is a bit convoluted but I wanted to put this up to make sure this is what you are looking for before cleaning it up a bit.

parity-cla-bot · 2019-10-08T05:23:13Z

It looks like @rokob hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

rokob · 2019-10-08T05:36:28Z

[clabot:check]

parity-cla-bot · 2019-10-08T05:36:29Z

It looks like @rokob signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

niklasad1 · 2019-10-08T15:19:38Z

I'm not sure what compiler version you are using as I got errors and a ton of change from running rustfmt, so I finagled it to run over just my changes but there might still be some stylistic issues.

We are not using rustfmt that is the reason why you get errors I guess and note that we are using tabs instead of spaces, please use the .editorconfig

Officially, we use the latest stable compiler

ethcore/builtin/src/lib.rs

niklasad1

Hi, thanks for your contribution.

It definitely is on the right track with some formating nits otherwise it looks good but it needs to be merged/rebased with master.

ethcore/machine/src/test_helpers.rs

dvdplm

lgtm, and thank you for the PR!

ethcore/builtin/src/lib.rs

rokob · 2019-10-09T03:19:59Z

I got rid of the extraneous lines that were just formatting noise and not part of this actual change.

* master: Secret store: fix Instant::now() related race in net_keep_alive (#11155) RPC method for clearing the engine signer (#10920) Use TryFrom instead of From+panic for Builtin (#11140)

sorpaas approved these changes Oct 8, 2019

View reviewed changes

sorpaas added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. M4-core ⛓ Core client code / Rust. labels Oct 8, 2019

niklasad1 self-requested a review October 8, 2019 09:04