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

rewards api change for attestations #7414

Closed
rolfyone opened this issue Aug 6, 2023 · 5 comments
Closed

rewards api change for attestations #7414

rolfyone opened this issue Aug 6, 2023 · 5 comments

Comments

@rolfyone
Copy link
Contributor

rolfyone commented Aug 6, 2023

ethereum/beacon-APIs#340

@rolfyone
Copy link
Contributor Author

rolfyone commented Aug 6, 2023

we should flag this as breaking given we've no longer got the rewards api under experimental.

@courtneyeh courtneyeh self-assigned this Sep 5, 2023
@courtneyeh
Copy link
Contributor

The total_rewards change does not appear to be a massive change. This will require changing how the response is aggregated. The inactivity seems to be included in the RewardComponent, meaning it should not be too hard to get. I expect this task should take 1 or 2 days assuming no surprises.

The ideal_rewards change appears to be a bit more complex. It will also require more investigation and will need changes in the ideal calculation. This may take a couple of days and potentially an extra day for texting.

Estimate is a week for completing all. Could be broken up into two tickets for the two different tasks.

@rolfyone
Copy link
Contributor Author

just to clarify, the api PR said

This PR adds the following fields to the AttestationsRewards response:

A inactivity field to both ideal_rewards and total_rewards.
A inclusion_delay field to ideal_rewards

so we're saying the update to total_rewards is relatively easy meaning the inactivity field in just total_rewards? which bridges both the items in part 1 of ticket
And the bulk of unknowns is the changes (inactivity in ideal rewards, and inclusion_delay in ideal_rewards)

Is that correct?

@courtneyeh
Copy link
Contributor

That is what I expect based on the previous investigation.

@courtneyeh
Copy link
Contributor

This issue has been broken down into #7553 and #7554

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

No branches or pull requests

2 participants