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

Make the Engine hold AccountProvider #3499

Closed
wants to merge 4 commits into from
Closed

Make the Engine hold AccountProvider #3499

wants to merge 4 commits into from

Conversation

keorn
Copy link

@keorn keorn commented Nov 17, 2016

The Miner was holding AccountProvider and passing it on sealing generation, when it was used only by BasicAuthority and AuthorityRound. Now it will also be possible to use it in other situations when signing is necessary.

Not sure if the registering part is nice: Mutex<Option<Arc<AccountProvider>>>.

@keorn keorn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 17, 2016
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.

Seems that if #3491 gets in we will need accounts in miner anyway.

work_poster: Option<WorkPoster>,
gas_pricer: Mutex<GasPricer>,
}

impl Miner {
/// Creates new instance of miner.
fn new_raw(options: MinerOptions, gas_pricer: GasPricer, spec: &Spec, accounts: Option<Arc<AccountProvider>>) -> Miner {
fn new_raw(options: MinerOptions, gas_pricer: GasPricer, spec: &Spec) -> Miner {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The change is conflicting with #3491 - I'm actually using AccountProvider in miner to identify local transactions :/

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 18, 2016
@keorn
Copy link
Author

keorn commented Nov 18, 2016

I see, will just add it to the Engine when needed then.

@keorn keorn closed this Nov 18, 2016
@keorn keorn deleted the miner-no-ap branch December 5, 2016 11:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants