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

fix #6228: do not display eth price in cli for etc #6877

Merged
merged 4 commits into from
Oct 25, 2017

Conversation

nicolasochem
Copy link
Contributor

Current behaviour:

When the client is started with defaults, the miner's gas calibrator
periodically queries the price of ether in usd and uses it to adjust
the wei_per_gas variable, displaying an info message in the cli each
time calibration happens. The info message mentions the price of ETH.

When started with the --min-gas-price option, the calibrator is inactive
and the price is not displayed.

Problem:

When running on an alternate chain such as ethereum classic, the info
message mentioning the ETH price is present, unless the --min-gas-price
option is used.

Solution:

if chain != foundation and --min-gas-price is not set,
don't use GasPricerConfig::Calibrated as default but rather fix
the minimum gas price to zero.

Current behaviour:

When the client is started with defaults, the miner's gas calibrator
periodically queries the price of ether in usd and uses it to adjust
the wei_per_gas variable, displaying an info message in the cli each
time calibration happens. The info message mentions the price of ETH.

When started with the --min-gas-price option, the calibrator is inactive
and the price is not displayed.

Problem:

When running on an alternate chain such as ethereum classic, the info
message mentioning the ETH price is present, unless the --min-gas-price
option is used.

Solution:

if chain != foundation and --min-gas-price is not set,
don't use GasPricerConfig::Calibrated as default but rather fix
the minimum gas price to zero.
@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M2-config 📂 Chain specifications and node configurations. M4-core ⛓ Core client code / Rust. labels Oct 24, 2017
@5chdn 5chdn added this to the 1.9 milestone Oct 24, 2017
@@ -649,6 +649,8 @@ impl Configuration {
return Ok(GasPricerConfig::Fixed(to_u256(dec)?));
} else if let Some(dec) = self.args.arg_min_gas_price {
return Ok(GasPricerConfig::Fixed(U256::from(dec)));
} else if self.args.arg_chain != "foundation" {
return Ok(GasPricerConfig::Fixed(U256::zero()));
Copy link
Collaborator

@arkpar arkpar Oct 24, 2017

Choose a reason for hiding this comment

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

Should be something like

if !Spec_type::from_str(self.chain()).map_or(false, |s| s == SpecType::Foundation)

because there are other strings that match the foundation chain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth to wait for #6878 (or integrate same changes) where self.chain() returns a SpecType

@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 24, 2017
@nicolasochem
Copy link
Contributor Author

@arkpar @tomusdrw I have updated PR to use the updated self.chain() that returns a SpecType, please review.

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Oct 25, 2017
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 25, 2017
@tomusdrw
Copy link
Collaborator

Closes #6228

@arkpar arkpar merged commit 542cee9 into openethereum:master Oct 25, 2017
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. M2-config 📂 Chain specifications and node configurations. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants