-
Notifications
You must be signed in to change notification settings - Fork 283
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
Adding inclusion_delay and inactivity to ideal_rewards object #7564
Conversation
@@ -69,7 +69,8 @@ public TotalAttestationReward(long validatorIndex, final RewardAndPenalty reward | |||
this.inactivity = | |||
detailedRewardAndPenalty | |||
.getReward(RewardComponent.INACTIVITY) | |||
.minus(detailedRewardAndPenalty.getPenalty(RewardComponent.INACTIVITY)); | |||
.minus(detailedRewardAndPenalty.getPenalty(RewardComponent.INACTIVITY)) | |||
.longValue(); |
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.
Now that we are using a long
value, we should match the previous calculation's formats above.
this.inactivity =
detailedRewardAndPenalty.getReward(RewardComponent.INACTIVITY).longValue()
- detailedRewardAndPenalty.getPenalty(RewardComponent.INACTIVITY).longValue();
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.
Actually, we should probably update the other calculations to have a single .longValue()
so we only do the unboxing once. I'll update the PR.
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.
Actually, I introduced a bug doing it that way because we would have issues when the penalties for a component is greater than the rewards and we end up with a negative value causing a UInt underflow.
...est/resources/tech/pegasys/teku/beaconrestapi/handlers/v1/rewards/getAttestationRewards.json
Show resolved
Hide resolved
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
PR Description
Added two new fields to the
ideal_rewards
object accordingly to ethereum/beacon-APIs#340.Fixed Issue(s)
partially addresses #7554
Documentation
doc-change-required
label to this PR if updates are required.Changelog