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

Graphical gas price selection #2898

Merged
merged 14 commits into from
Nov 1, 2016
Merged

Graphical gas price selection #2898

merged 14 commits into from
Nov 1, 2016

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Oct 26, 2016

Closes #2142

The description text might need to be edited...

parity-ui-gas-select

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M5-ui labels Oct 26, 2016
@keorn
Copy link

keorn commented Oct 26, 2016

Its not really octile since there are 9 sets. Is y axis the price for a particular quantile?

@jacogr
Copy link
Contributor

jacogr commented Oct 26, 2016

It is based on what we get back from ethcore_gasPriceStatistics - but yes, it looks very scewed, we should adjust it somewhat so people go way below and way above as well.

@jacogr
Copy link
Contributor

jacogr commented Oct 26, 2016

Think it is quite close, have a couple small nigglies I'll plop next to you for just in terms of layout.

Also want @gavofyork 's feedback on this one - he originally asked for it.
Probably @KenKappler as well...

@gavofyork
Copy link
Contributor

gavofyork commented Oct 27, 2016

@keorn and i always through you were good at maths! :-P

it's octile, because these are the quantiles which divide the distribution into 8 equal portions:

image

http://wikidiff.com/decile/octile

@gavofyork
Copy link
Contributor

gavofyork commented Oct 27, 2016

this is correct, fwiw (most people use sensible gas price, one or two people use 10x on that), though we may want to place Y on a log scale and/or remove max/min (first and last elements) from the graph.

@keorn
Copy link

keorn commented Oct 27, 2016

Does 9 divisions make 8 sets (no, 7)? Not sure where that screenshot is from :P, here is a source.

@ngotchac
Copy link
Contributor Author

Well, N+1 values makes N sets : [1, 2, 3] makes two sets: [1 ; 2] and [2 ; 3]

@keorn
Copy link

keorn commented Oct 27, 2016

Yes, but quantiles divide the total population so only the internal divisions are meaningful. So does this mean that the graphic shows the most extreme values as the leftmost and rightmost bars?

@ngotchac
Copy link
Contributor Author

Yes I think so.

So here are 3 different version, based on your comments:

  1. Default version
    parity-ui-gas-default
  2. Log scale
    parity-ui-gas-log
  3. Sliced (without min and max)
    parity-ui-gas-sliced

@keorn
Copy link

keorn commented Oct 27, 2016

3 looks about right (often 90% of transactions have the same price) and is not very nice. It might be better to have price on the x axis (where you do the sliding) and y axis would show the proportion of transactions you are ahead of.

@gavofyork
Copy link
Contributor

the distribution is different to the original screenshot - all the bands are equal so it's rather dysfunctional. the data for the graph are indeed octile points, so the visualisation as bands is a little misleading. that said, 99% of users will not care at all.

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 27, 2016
@jacogr
Copy link
Contributor

jacogr commented Oct 27, 2016

Moved to in-progress, different APIs for display to be added.

@ngotchac
Copy link
Contributor Author

Current UI : https://youtu.be/0kINeUtmMJ8

@keorn keorn mentioned this pull request Oct 28, 2016
2 tasks
@ngotchac
Copy link
Contributor Author

ngotchac commented Oct 31, 2016

Now using the new gasPriceHistogram RPC call : displays the gas price selection from this histogram.

screenshot_2016-10-31_18-29-04

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 31, 2016
@keorn
Copy link

keorn commented Oct 31, 2016

As @rphmeier mentioned, might be good to say that these are included transactions. Also more outliers can be cut or more buckets used to make the histogram a bit more interesting, but the range now is usually between reasonable 20 and 50 Gwei.

@rphmeier
Copy link
Contributor

(as a test, I submitted a tx yesterday with 15Gwei price. It took 12 hours to mine -- if you're comfortable with typical bank transfer times you might prefer the option to select outside of the range.)

@ngotchac
Copy link
Contributor Author

The value can also be modified from the text input field on the right of the graph.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1fdcf70 on ng-gas-price-selection into * on master*.

@jacogr
Copy link
Contributor

jacogr commented Oct 31, 2016

We are entering the "this dialog is way too high" territory - cut 15% of the graph and combine the 2 paragraphs at the bottom. (It would make sense to do these 0.75em as well, just to fit in with "labelling")

Apart from that, looks almost usable now... We may want to try log scale again as well, just since there are massive jumps with the numbers.

@jacogr jacogr added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 31, 2016
@ngotchac
Copy link
Contributor Author

ngotchac commented Nov 1, 2016

So this is how it looks like with a log scale (far left is for 111 transactions, the second highest is 4 transactions)

parity-gas-select-log

As for the legend font size, 0.75em is really unreadable to me, so I put it at 0.9

@keorn
Copy link

keorn commented Nov 1, 2016

imho
Can't this section be made optional (like advanced settings)? Default gas price from median is an alright estimator for most people who do not need to be shown all this complexity. Then you could have 3 steps + optional advanced settings.

@ngotchac
Copy link
Contributor Author

ngotchac commented Nov 1, 2016

@keorn Yep this is already the case. You have to check advanced settings (or equivalent) to access this gas price selector

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 43dc939 on ng-gas-price-selection into * on master*.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Nov 1, 2016
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Nov 1, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 1, 2016

Lint issues, check GitLab/Travis.

@ngotchac
Copy link
Contributor Author

ngotchac commented Nov 1, 2016

Ok, fixed

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Nov 1, 2016
@jacogr jacogr merged commit 84ca3d7 into master Nov 1, 2016
@jacogr jacogr deleted the ng-gas-price-selection branch November 1, 2016 14:04
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 803543d on ng-gas-price-selection into * on master*.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants