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

Common EngineSigner #4189

Merged
merged 7 commits into from
Jan 20, 2017
Merged

Common EngineSigner #4189

merged 7 commits into from
Jan 20, 2017

Conversation

keorn
Copy link

@keorn keorn commented Jan 17, 2017

Reusable EngineSigner for signing consensus messages. Goes after #4184.

@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 17, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 20114c8 on engine-signer into ** on master**.

@rphmeier
Copy link
Contributor

Documentation on all public items would be a plus

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 17, 2017
@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jan 17, 2017
@keorn keorn mentioned this pull request Jan 18, 2017
@rphmeier
Copy link
Contributor

Why not take it a step further and make EngineSigner a trait with its current interface? Would serve to decouple Engine from implementation details like AccountProvider and make it more testable via mocked signers.

@keorn
Copy link
Author

keorn commented Jan 18, 2017

Its a very simple struct and is tested via multiple Engine tests. I dont see much reason for different implementations for now, once we need them it can be easily turned into a trait. imho this is simpler

@gavofyork
Copy link
Contributor

need resolving.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1ae411d on engine-signer into ** on master**.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 20, 2017
@gavofyork
Copy link
Contributor

i was also imagining a trait + trait object, but this is a decent first step.

@gavofyork gavofyork merged commit 97a60ce into master Jan 20, 2017
@gavofyork gavofyork deleted the engine-signer branch January 20, 2017 12:25
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.

5 participants