-
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 2636) show 0 value internal transactions #2785
(Bids 2636) show 0 value internal transactions #2785
Conversation
fd75dd8
to
0704d04
Compare
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.
There is also this issue:
- Go to the Internal Transactions tab of a tx that has itx
- Activate Advanced
You can see all itx - Refresh the page
=> Advanced is still active but you can only see the itx as if it wasn't.
To see all of them again you need to deactivate Advanced and activate it again.
utils/format.go
Outdated
if !strings.HasSuffix(amountStr, ".") { | ||
amountStr += "." | ||
} |
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 doesn't make sense to me. What are you trying to do here?
If you now e.g. call this method with:
- valIf: 100.12345
- digitsAfterComma: 10
you would get "100.12345.0000"
or with
- digitsAfterComma: 2
a "100.12." (with a correct tooltip)
Didn't it just work beforehand?
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.
Not quite, seems like I came across an edge case which isn't handled correctly. Call the method with
- valIf: 100.00001234
- digitsAfterComma: 4
result:
- 10000000
When the number has higher precision than digitsAfterComma
, Truncate
cuts away all decimal places. We're still adding the missingZeros
though, so we'll massively inflate the result 😅
My fix was also wrong of course, thanks for pointing it out; I've adjusted it.
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 see, good find.
Seems to work for every case now.
<div id="advanced-itx" class="hide mr-20 float-right"> | ||
<div class="float-right"> | ||
<span class="text-monospace mr-2">Advanced</span> | ||
<label class="advanced-itx-switch float-right" for="advanced-itx-toggle"> |
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.
question: Unlike Etherscan the advanced-itx-switch
is visible and works in both the Overview and Internal Transactions tabs.
When clicked in the Overview tab you only see the number in the Internal Transactions tab change.
Should this be how it works?
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.
Ah no, this was not intended. I assume you're using firefox, because on chrome it works for me; I adopted the same fix we're using for the tabBarSwitcher, this should fix it. The list of internal transactions in the overview tab should only show non-advanced subcalls at all times.
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.
Yes, I am normally using Firefox.
It looks fixed for both Firefox and Chrome now.
fece9a3
to
13a8667
Compare
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.
lgtm
When refreshing the page the Advanced
switch now resets back to "Off".
13a8667
to
dbec987
Compare
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.
lgtm, only a merge should have happened since the last approved review.
based on #2784
copilot:summary