Skip to content
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-2782) Remove duplicated code in two functions of eth1Account.go #2750

Merged
merged 4 commits into from
Dec 11, 2023

Conversation

thib-wien
Copy link
Contributor

@thib-wien thib-wien commented Dec 4, 2023

Removes identical code appearing in functions Eth1Address() and Eth1AddressWithdrawals() in eth1Account.go.
Instead, this duplicated logic is moved into GetAddressWithdrawals() in db.go.

I recommend the reviewer to study the modifications in Eth1Address() and Eth1AddressWithdrawals() before studying GetAddressWithdrawals(), it will help understanding.

🤖[deprecated] Generated by Copilot at 1caa68c

Refactored and simplified the code for fetching and displaying eth1 address withdrawals. Used the html/template package and a common response type for the withdrawals table. Improved the handling of no withdrawals and currency conversion.

Copy link
Collaborator

@Eisei24 Eisei24 left a 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.

The new withdrawals function should be kept as similar to the other address page tabs functions as possible.
E.g. the tmr warning they have at the start can also be added.

On Prater I used this address for testing purposes since it has enough withdrawals to require a few scroll loads to show all of them but not too many so that you can actually reach the end in a few loads.
0x2fd398fc58e7193a2410bf92e47e05e1e3e24c73

When I use this one with your changes I get a "Something went wrong fetching please try again another time." at the end.
I did not test this on current master.

handlers/eth1Account.go Outdated Show resolved Hide resolved
handlers/eth1Account.go Outdated Show resolved Hide resolved
handlers/eth1Account.go Outdated Show resolved Hide resolved
db/db.go Show resolved Hide resolved
db/db.go Outdated Show resolved Hide resolved
@thib-wien
Copy link
Contributor Author

@Eisei24
The bug seems to be fixed :)
I also did the little improvements that you suggested.

Copy link
Collaborator

@Eisei24 Eisei24 left a 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 new suggestions.

db/db.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Eisei24 Eisei24 left a comment

Choose a reason for hiding this comment

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

lgtm

You can still delete the setting of Draw and RecordsTotal in the emptyData if you want.

@thib-wien thib-wien merged commit 9c92c61 into master Dec 11, 2023
3 checks passed
@thib-wien thib-wien deleted the BIDS-2782/Remove_duplicated_code branch December 11, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants