Use provided usd-per-eth value if an endpoint is specified by rakanalh · Pull Request #11209 · openethereum/parity-ethereum

rakanalh · 2019-10-27T18:10:23Z

This PR will use GasPriceConfig::Calibrated when the user provides an endpoint to an API endpoint which provides the Eth->USD price for a custom network.

The change is inspired by previous work of Baranowski

parity-cla-bot · 2019-10-27T18:10:26Z

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

Many thanks,

Parity Technologies CLA Bot

dvdplm

First of all, thank you for taking this on. This is old code and it's not pretty. I think the "real fix" would be to include the oracle in the chainspec somehow but that seems like it could be a rather big change. What we have here is certainly an improvement and I'm inclined to accept it.
Added a few comments/nits.

/cc @niklasad1 thoughts on this?

niklasad1 · 2019-10-28T15:09:39Z

 		match *self {
 			GasPricerConfig::Fixed(u) => GasPricer::Fixed(u),
-			GasPricerConfig::Calibrated { usd_per_tx, recalibration_period, .. } => {
+			GasPricerConfig::Calibrated { usd_per_tx, recalibration_period, ref api_endpoint } => {


api_endpointis needed here because it is used to create PriceInfoClient

niklasad1 · 2019-10-28T17:12:49Z

--usd-per-eth=[SOURCE]                                                                                                                                                                                  
        USD value of a single ETH. SOURCE may be either an amount in USD, a web service or 'auto' to use each web                                                                                           
        service in turn and fallback on the last known good value. (default: auto)

Well, it doesn't seem that auto means that falling back to the last good value anymore, just use default web service (etherscan)?!

Can somebody explain what's the difference between --usd-per-eth auto and --usd-per-eth https://api.etherscan.io/api\?module\=stats\&action\=price ?

/cc @joshua-mir @debris

rakanalh · 2019-10-28T19:51:00Z

Thanks for reviewing @dvdplm and @niklasad1. I addressed your comments.

rakanalh · 2019-10-28T19:53:39Z

Can somebody explain what's the difference between --usd-per-eth auto and --usd-per-eth https://api.etherscan.io/api\?module\=stats\&action\=price ?

According to the previous code, they are both the same. Because the previous client was being instantiated using the default endpoint defined in the client itself.

niklasad1

LGTM, thanks

niklasad1 · 2019-10-29T08:21:52Z

According to the previous code, they are both the same. Because the previous client was being instantiated using the default endpoint defined in the client itself.

Yeah, I was referring to that the documentation seems to be incorrect but not related to your changes. Initially, I think auto was meant to use the last known value if for example etherscan or the internet connection was down. I don't think auto is good name anymore we should probably rename it to etherscan or update the documentation

rakanalh · 2019-10-29T09:18:02Z

@niklasad1 seems like the CI docker is having issues. I'll amend the commit and force push again once it's working again.

niklasad1 · 2019-10-29T09:29:58Z

seems like the CI docker is having issues. I'll amend the commit and force push again once it's working again.

Don't bother we squash merge anyway, I'll restart the CI jobs for you

The change will try to check if the specified value is an endpoint. If the value of `auto` is specified, the default endpoint URL will be used otherwise, the user-provided value will be taken as-is for an endpoint.

1. auto = use etherscan 2. value = use fixed pricer 3. endpoint = use the provided endpoint as-is

* Fix `invalid transaction price` error message * Setup Calibrated GasPriceConfig when usd-per-eth is an endpoint The change will try to check if the specified value is an endpoint. If the value of `auto` is specified, the default endpoint URL will be used otherwise, the user-provided value will be taken as-is for an endpoint. * Use if-let and check for usd-per-eth arg: 1. auto = use etherscan 2. value = use fixed pricer 3. endpoint = use the provided endpoint as-is * Fix typo in to_pricce error message * Correct whitespace indentation * Use arg_usd_per_eth directly

* master: (21 commits) ropsten #6631425 foundation #8798209 (#11201) Update list of bootnodes for xDai chain (#11236) ethcore/res: add mordor testnet configuration (#11200) [chain specs]: activate `Istanbul` on mainnet (#11228) [builtin]: support `multiple prices and activations` in chain spec (#11039) Insert explicit warning into the panic hook (#11225) Snapshot restoration overhaul (#11219) Fix docker centos build (#11226) retry on gitlab system failures (#11222) Update bootnodes. (#11203) Use provided usd-per-eth value if an endpoint is specified (#11209) Use a lock instead of atomics for snapshot Progress (#11197) [informant]: `MillisecondDuration` -> `as_millis()` (#11211) Step duration map configuration parameter ported from the POA Network fork (#10902) Upgrade jsonrpc to latest (#11206) [export hardcoded sync]: use debug for `H256` (#11204) Pause pruning while snapshotting (#11178) Type annotation for next_key() matching of json filter options (#11192) Crypto primitives removed from ethkey (#11174) Made ecrecover implementation trait public (#11188) ...

* Fix `invalid transaction price` error message * Setup Calibrated GasPriceConfig when usd-per-eth is an endpoint The change will try to check if the specified value is an endpoint. If the value of `auto` is specified, the default endpoint URL will be used otherwise, the user-provided value will be taken as-is for an endpoint. * Use if-let and check for usd-per-eth arg: 1. auto = use etherscan 2. value = use fixed pricer 3. endpoint = use the provided endpoint as-is * Fix typo in to_pricce error message * Correct whitespace indentation * Use arg_usd_per_eth directly

* ropsten #6631425 foundation #8798209 (#11201) * [stable] builtin, istanbul and mordor testnet backports (#11234) * ethcore-builtin (#10850) * [builtin]: support `multiple prices and activations` in chain spec (#11039) * [chain specs]: activate `Istanbul` on mainnet (#11228) * ethcore/res: add mordor testnet configuration (#11200) * Update list of bootnodes for xDai chain (#11236) * ethcore: remove `test-helper feat` from build (#11047) * Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159) * [stable]: backport #10691 and #10683 (#11143) * Fix compiler warning (that will become an error) (#10683) * Refactor Clique stepping (#10691) * Add Constantinople eips to the dev (instant_seal) config (#10809) * Add cargo-remote dir to .gitignore (?) * Insert explicit warning into the panic hook (#11225) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Add new line after writing block to hex file. (#10984) * Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…) * Upgrade jsonrpc to latest (#11206) * [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11086) * Fix block detail updating (#11015) * Switching sccache from local to Redis (#10971) * Made ecrecover implementation trait public (#11188) * [dependencies]: jsonrpc `14.0.1` (#11183) * [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Cleanup stratum a bit (#11161) * Upgrade to jsonrpc v14 (#11151) * SecretStore: expose restore_key_public in HTTP API (#10241)

* [CI] check evmbin build (#11096) * Correct EIP-712 encoding (#11092) * [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086) * Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053) * Add cargo-remote dir to .gitignore (?) * Update light client headers: ropsten 6631425 foundation 8798209 (#11201) * Update list of bootnodes for xDai chain (#11236) * ethcore/res: add mordor testnet configuration (#11200) * [chain specs]: activate Istanbul on mainnet (#11228) * [builtin]: support multiple prices and activations in chain spec (#11039) * [receipt]: add sender & receiver to RichReceipts (#11179) * [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) * Made ecrecover implementation trait public (#11188) * Fix docker centos build (#11226) * Update MIX bootnodes. (#11203) * Insert explicit warning into the panic hook (#11225) * Use provided usd-per-eth value if an endpoint is specified (#11209) * Cleanup stratum a bit (#11161) * Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported) * util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) * ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) * Type annotation for next_key() matching of json filter options (#11192) * Upgrade jsonrpc to latest (#11206) * [dependencies]: jsonrpc 14.0.1 (#11183) * Upgrade to jsonrpc v14 (#11151) * Switching sccache from local to Redis (#10971) * Snapshot restoration overhaul (#11219) * Add new line after writing block to hex file. (#10984) * Pause pruning while snapshotting (#11178) * Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127) * Fix block detail updating (#11015) * Make InstantSeal Instant again #11186 * Filter out some bad ropsten warp snapshots (#11247)

Fix invalid transaction price error message

d8cb31c

rakanalh commented Oct 27, 2019

View reviewed changes

Comment thread parity/configuration.rs Outdated

jam10o-new added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 27, 2019

dvdplm reviewed Oct 28, 2019

View reviewed changes

Comment thread miner/price-info/src/lib.rs Outdated

Comment thread parity/helpers.rs Outdated

Comment thread parity/configuration.rs Outdated

dvdplm added the M2-config 📂 Chain specifications and node configurations. label Oct 28, 2019

niklasad1 reviewed Oct 28, 2019

View reviewed changes

Comment thread parity/helpers.rs Outdated

niklasad1 reviewed Oct 28, 2019

View reviewed changes

Comment thread parity/configuration.rs Outdated

niklasad1 reviewed Oct 28, 2019

View reviewed changes

Comment thread parity/configuration.rs Outdated

niklasad1 reviewed Oct 28, 2019

View reviewed changes

Comment thread parity/configuration.rs Outdated

rakanalh requested review from dvdplm and niklasad1 October 28, 2019 19:44

niklasad1 reviewed Oct 29, 2019

View reviewed changes

Comment thread parity/configuration.rs Outdated

niklasad1 approved these changes Oct 29, 2019

View reviewed changes

niklasad1 added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 29, 2019

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Oct 29, 2019

niklasad1 added this to the 2.7 milestone Oct 29, 2019

dvdplm suggested changes Oct 29, 2019

View reviewed changes

Comment thread parity/params.rs Outdated

rakanalh added 2 commits October 29, 2019 11:09

Setup Calibrated GasPriceConfig when usd-per-eth is an endpoint

ef4083e

The change will try to check if the specified value is an endpoint. If the value of `auto` is specified, the default endpoint URL will be used otherwise, the user-provided value will be taken as-is for an endpoint.

Use if-let and check for usd-per-eth arg:

5e2395f

1. auto = use etherscan 2. value = use fixed pricer 3. endpoint = use the provided endpoint as-is

rakanalh added 3 commits October 29, 2019 11:10

Fix typo in to_pricce error message

e4d149d

Correct whitespace indentation

e695b7c

Use arg_usd_per_eth directly

00e24b5

dvdplm approved these changes Oct 29, 2019

View reviewed changes

dvdplm added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Oct 29, 2019

debris approved these changes Oct 29, 2019

View reviewed changes

debris merged commit 6993ec9 into openethereum:master Oct 29, 2019

rakanalh deleted the eth-usd-price branch October 29, 2019 11:25

This was referenced Nov 5, 2019

v2.5.10 stable #11239

Merged

v2.6.5 beta #11240

Merged

Conversation

rakanalh commented Oct 27, 2019 • edited by niklasad1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Oct 27, 2019

Uh oh!

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasad1 Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasad1 commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rakanalh commented Oct 28, 2019

Uh oh!

rakanalh commented Oct 28, 2019

Uh oh!

Uh oh!

niklasad1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 commented Oct 29, 2019

Uh oh!

rakanalh commented Oct 29, 2019

Uh oh!

niklasad1 commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rakanalh commented Oct 27, 2019 •

edited by niklasad1

Loading

niklasad1 commented Oct 28, 2019 •

edited

Loading

niklasad1 left a comment •

edited

Loading

niklasad1 commented Oct 29, 2019 •

edited

Loading