-
Notifications
You must be signed in to change notification settings - Fork 434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(BIDS-2841) Displaying amounts in the currency that the user selected #2770
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the BIDS for an instruction on how to test this locally for an existing testnet.
One ETH value that was not yet changed but should be is the "Tx Reward" for the block/slot page, e.g. https://beaconcha.in/block/18784041
The tooltip behaviour was also discussed and it's fine that every value formatting we change for this BIDS shows the currency like the withdrawals, e.g. the linked block page "Tx Reward" currently doesn't show ETH in the tooltip but it should.
…e very unclear to the beginner
…conversion everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at my comments.
Additionally I wonder about whether pages use $ or USD.
I think all the changed pages show USD now but e.g. the withdrawals tab of an address shows $ which is strange.
A validator page on mainnet already shows a wild mix, e.g.
https://beaconcha.in/validator/1017027#deposits (set to USD)
Is there a consensus for the behaviour?
templates/layout.html
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Just a reminder that the changes to this file have to be reverted after the review is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can keep this change to see the currency drop-down when the explorer runs locally. The price of ETH is then arbitrary ($2000).
I am not sure why this was deactivated for local tests.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Just a reminder that changes to this file have to be reverted after the review is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I made theses changes clean and didn't remove them because I think that it is good to allow testers to select "USD" in the drop-down when the explorer runs locally. The price of ETH is then arbitrary ($2000).
What do you think?
@@ -455,7 +455,7 @@ <h3 class="h5 col-md-12 text-center"><b>Showing {{ .AttestationsCount }} Attesta | |||
{{ if .ExecutionData }} | |||
<div class="tab-pane fade" id="transactionsTabPanel" role="tabpanel" aria-labelledby="transactions-tab"> | |||
<div class="card block-card py-1"> | |||
{{ template "execution_transactions" .ExecutionData }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: We have this change in combination with
{{ with .Data.ExecutionData }} |
to get the currency into
execution_transactions
but this is also called here{{ template "execution_transactions" . }} |
for premerge blocks, e.g. Prater 7316000, and it cannot simply be adjusted since it has no
ExecutionData
part./block/7316000 currently crashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, I believe. Can you please check that it works? (I do not know how to connect my branch with Prater)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed this doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope it works now! I pray the god of BIDS2841 to stop bringing bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not, I still get the error
"...at <$data.ExecutionData>: can't evaluate field ExecutionData in type *types.Eth1BlockPageData""
when I try to open a block page for a premerge block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not, I still get the error when I try to open a block page for a premerge block.
Fixed.
I learnt that the template engine of Go cannot test the existence of fields. So, instead, now the template tests the value of $.Active
which is set to "blockchain" by Eth1Block()
in eth1Block.go for post-merge pages, whereas it is set to "block" for pre-merge pages.
I find this solution dirty because these strings don't sound explicit and I am not sure that they are definitive. For now everything works.
@@ -56,7 +56,7 @@ | |||
<div class="row border-bottom p-3 mx-0"> | |||
<div class="col-md-2"><span data-toggle="tooltip" data-placement="top" title="Transaction rewards from block {{ .Number }}">Tx Reward:</span></div> | |||
<div class="col-md-10"> | |||
{{ formatAmount .Reward config.Frontend.ElCurrency 5 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This is a fix for newer slots/blocks but does not change it for blocks premerge, e.g. Prater 7316000.
The page for those currently does not work, see the comment below, but the line that formats the value is around here
{{ formatAmount .Reward config.Frontend.ElCurrency 5 }} |
and is not changed.
Before you change it it should be clarified whether we even want that or if it should always remain ETH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, I believe.
Besides the rewards, I also changed the currency for the tx fees and the burned fees. This is consistent with what Butta suggested (see the end of this post).
Can you please check that it works? (I do not know how to connect my branch with Prater)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK for this, I will have a look once premerge blocks work again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder to check
This was implemented before me. To try, now everything is changed to USD for consistency. |
@Eisei24 I asked Butta. We keep my change (USD, EUR, AUD, etc. everywhere / No $, € and so on). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the premerge blocks page.
One line that I noticed that still showed $ instead of USD is the "ETH Value" in the "Overview" on the address page, e.g. https://beaconcha.in/address/0xF82aC5937A20dC862F9bc0668779031E06000f17
Again I can't tell you if it should be left like it is.
True, this has been missed. I think the goal of the BIDS has been reached: making the currency conversion work everywhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The premerge blocks page still needs to be fixed and the desired gnosis behaviour needs to be discussed.
…default on Gnosis
|
Displaying amounts in the currency that the user selected (drop-down menu at the top right of the website). Before, this choice was ignored.
To the tester:
The currency selection is impossible when the explorer runs locally in its normal state (because price feeds do not exist, for example).
You will need to test this branch with a real network.
The ignored-currency bug has been corrected on many pages. Fixes for two other currency-related bugs that we found have been included too. The Ethereum version looks ready now.
The version for Gnosis needs to be discussed with an open mind: before the BIDS, some data were displayed in GNO (mainly the CL data) and other data in xDAI (EL data). With this BIDS, the user can select a fiat currency and the question is:
If 1 is chosen, then why do some amounts remain in crypto whereas they are displayed in fiat with Ethereum?
I chose 2 for this BIDS, because it seems consistent with the desire of the user and what people experience with Ethereum. The users still have the possibility to see the GNO amounts of 1 when they select "GNO" in the drop-down.
==================================================
Displayed currencies for Ethereum as discussed with the product owner:
mempool:
txs:
blocks:
/block/{block}#transactions: