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

EIP-7251: Flatten get_active_balance #3949

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

ppopth
Copy link
Member

@ppopth ppopth commented Sep 29, 2024

We have both get_active_balance and get_total_active_balance which have totally different meanings, since get_active_balance uses the balance field while get_total_active_balance uses the effective_balance field.

The names suggest that get_total_active_balance is the total of get_active_balance which is not true.

The name of get_active_balance doesn't quite make sense and it's used only in one place, so this commit flattens the logic of get_active_balance to the place where it's used.

@ppopth ppopth force-pushed the flatten-get-active-balance branch 2 times, most recently from 7b06294 to fb486c8 Compare September 29, 2024 05:52
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

weak +1 on Justin's comment, but otherwise love that we are shrinking the spec :)

We have both get_active_balance and get_total_active_balance which have
totally different meanings, since get_active_balance uses the balance
field while get_total_active_balance uses the effective_balance field.

The names suggest that get_total_active_balance is the total of
get_active_balance which is not true.

The name of get_active_balance doesn't quite make sense and it's used
only in one place, so this commit flattens the logic of get_active_balance
to the place where it's used.
@ppopth ppopth force-pushed the flatten-get-active-balance branch from fb486c8 to cd10274 Compare September 30, 2024 16:21
Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for making that change 😄

@jtraglia jtraglia merged commit 0c8645e into ethereum:dev Sep 30, 2024
26 checks passed
@ppopth ppopth deleted the flatten-get-active-balance branch October 1, 2024 15:08
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.

4 participants