Increase test coverage & Fix for goerli setup #853
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #853 +/- ##
==========================================
+ Coverage 71.20% 73.10% +1.90%
==========================================
Files 30 38 +8
Lines 1330 1517 +187
Branches 0 52 +52
==========================================
+ Hits 947 1109 +162
- Misses 383 388 +5
- Partials 0 20 +20
☔ View full report in Codecov by Sentry. |
yrong
changed the title
yrong
changed the title
yrong
requested review from
claravanstaden,
vgeddes,
doubledup and
alistair-singh
There was a problem hiding this comment.
Awesome update @yrong! I made some minor comments and asked some questions. The new tests in this PR are particularly useful, nice!
| // sync for the next period | ||
| periodsToSync := []uint64{latestSyncedPeriod + 1} |
There was a problem hiding this comment.
What happens if the sync committee processing fell behind? Or will this not happen with the new beacon client checks?
There was a problem hiding this comment.
Not sure fully get the point here. FinalityUpdate changes every epoch(i.e. 6.4 mins) and SyncCommitteeUpdate every 27.3 hours, so plenty of time for our light client to catch up.
There was a problem hiding this comment.
Say the latestSyncedPeriod is 50, but the current sync period is 60, then sync period 50 and 51 would be synced only. Please correct me if I'm wrong, I'm wrapping my head around the changes. 😄
There was a problem hiding this comment.
Only period 51 will be synced in this case, we won't sync period 50 unless latestSyncedPeriod is also initialPeriod when checkpoint initialized. And we also won't start syncing period 52 unless executionUpdate in between catch up.
Actually the change above is mainly for coordinating with Vincent's improvement recently, to follow ALC spec more closely we do not store historical sync committees anymore. Instead only current&next which also means we can not verify arbitrary historical update as we did before.
@claravanstaden Please let me know if any concern.
|
Beacon client with goerli setup work as expected expect for one issue found in lodestar, add ChainSafe/lodestar#5613 |
There was a problem hiding this comment.
Looks great @yrong! I think it's nearly ready to merge. I just have one last question here: #853 (comment)
|
Ron, you can exclude coverage reports for the benchmarking and weight stuff by those code paths into codecov.yml. This should make the PR pass the checks. |
ExecutionUpdate