fix: use next epoch for pending balance/consolidations processing #7053
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## electra-fork-rebasejul30 #7053 +/- ##
============================================================
- Coverage 49.39% 49.37% -0.03%
============================================================
Files 589 589
Lines 39233 39233
Branches 2247 2243 -4
============================================================
- Hits 19378 19370 -8
- Misses 19814 19822 +8
Partials 41 41 |
| @@ -14,8 +13,8 @@ import {getCurrentEpoch} from "../util/epoch.js"; | |||
| * TODO Electra: Update ssz library to support batch push to `pendingBalanceDeposits` | |||
| */ | |||
| export function processPendingBalanceDeposits(state: CachedBeaconStateElectra, cache: EpochTransitionCache): void { | |||
| const nextEpoch = state.epochCtx.epoch + 1; | |||
There was a problem hiding this comment.
I really like the name upcomingEpoch made by @matthewkeil for processEpoch code in his async shuffling PR #6938
There was a problem hiding this comment.
yea name is good but i think good to be closer to naming used in specs, though not a strong opinion
There was a problem hiding this comment.
+1 on following spec naming, I think we should have a good reason if we wanna deviate from terms used in spec.
There was a problem hiding this comment.
Yeah, if we really have a problem with the naming, we should aim to update it in the spec instead. Deviating off standards create more context loss for new people reading our code IMO.
matthewkeil
force-pushed
the
electra-fork-rebasejul30
branch
from
aa653e2
to
ed18ff0
Compare
g11tech
dismissed stale reviews from twoeths, nflaig, and wemeetagain
via
b9f0b50
| ], | ||
| // TODO Electra: Review this test in the next spec test release | ||
| skippedTests: [/incorrect_not_enough_consolidation_churn_available/], | ||
| skippedTests: [/incorrect_not_enough_consolidation_churn_available/, /^deneb\/light_client\/sync\/.*electra_fork.*/], |
There was a problem hiding this comment.
I believe /incorrect_not_enough_consolidation_churn_available/ can be removed since it was fixed after alpha.3
| @@ -14,8 +13,8 @@ import {getCurrentEpoch} from "../util/epoch.js"; | |||
| * TODO Electra: Update ssz library to support batch push to `pendingBalanceDeposits` | |||
| */ | |||
| export function processPendingBalanceDeposits(state: CachedBeaconStateElectra, cache: EpochTransitionCache): void { | |||
| const nextEpoch = state.epochCtx.epoch + 1; | |||
There was a problem hiding this comment.
+1 on following spec naming, I think we should have a good reason if we wanna deviate from terms used in spec.
|
🎉 This PR is included in v1.22.0 🎉 |
devnet3 branch