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

Use TryFrom instead of From+panic for Builtin#11140

Merged
dvdplm merged 4 commits into
openethereum:masterfrom
rokob:builtin-try-from
Oct 9, 2019
Merged

Use TryFrom instead of From+panic for Builtin#11140
dvdplm merged 4 commits into
openethereum:masterfrom
rokob:builtin-try-from

Conversation

@rokob
Copy link
Copy Markdown
Contributor

@rokob rokob commented Oct 8, 2019

Fixes #11134

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
Copy link
Copy Markdown

parity-cla-bot commented Oct 8, 2019

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
Copy link
Copy Markdown
Contributor Author

rokob commented Oct 8, 2019

[clabot:check]

@parity-cla-bot
Copy link
Copy Markdown

parity-cla-bot commented Oct 8, 2019

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

Many thanks,

Parity Technologies CLA Bot

@sorpaas 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 niklasad1 self-requested a review October 8, 2019 09:04
@niklasad1
Copy link
Copy Markdown
Collaborator

niklasad1 commented Oct 8, 2019 •
edited
Loading

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

niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Comment thread ethcore/builtin/src/lib.rs Outdated
niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Comment thread ethcore/builtin/src/lib.rs Outdated
niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Comment thread ethcore/builtin/src/lib.rs Outdated
niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Comment thread ethcore/builtin/src/lib.rs Outdated
niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Comment thread ethcore/builtin/src/lib.rs Outdated
niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Comment thread ethcore/builtin/src/lib.rs Outdated
@niklasad1 niklasad1 removed the A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. label Oct 8, 2019
niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

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.

@niklasad1 niklasad1 added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Oct 8, 2019
niklasad1
niklasad1 reviewed Oct 8, 2019
View reviewed changes
Comment thread ethcore/machine/src/test_helpers.rs
dvdplm
dvdplm approved these changes Oct 8, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, and thank you for the PR!

Comment thread ethcore/builtin/src/lib.rs Outdated
Comment thread ethcore/builtin/src/lib.rs Outdated
@rokob rokob force-pushed the builtin-try-from branch from b500fc7 to f949bd1 Compare October 9, 2019 03:11
@rokob rokob force-pushed the builtin-try-from branch from f949bd1 to 9601f54 Compare October 9, 2019 03:14
@rokob
Copy link
Copy Markdown
Contributor Author

rokob commented Oct 9, 2019

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

@niklasad1 niklasad1 added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Oct 9, 2019
@niklasad1 niklasad1 added the A8-looksgood 🦄 Pull request is reviewed well. label Oct 9, 2019
@dvdplm dvdplm merged commit a404dd5 into openethereum:master Oct 9, 2019
dvdplm added a commit that referenced this pull request Oct 10, 2019
@dvdplm
Merge branch 'master' into dp/chore/upgrade-jsonrpc-to-v14
240ccd1 
* 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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@dvdplm dvdplm dvdplm approved these changes

@sorpaas sorpaas sorpaas approved these changes

@niklasad1 niklasad1 niklasad1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ethcore-builtin]: use TryFrom instead of From

5 participants

@rokob @parity-cla-bot @niklasad1 @dvdplm @sorpaas