Localnet validator testing #541
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #541 +/- ##
===========================================
- Coverage 58.65% 58.56% -0.10%
===========================================
Files 75 75
Lines 1253 1255 +2
Branches 148 150 +2
===========================================
Hits 735 735
- Misses 491 493 +2
Partials 27 27
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
integration/restValidation.ts
Outdated
| blockSlot: number, | ||
| ignoreBefore?: number, | ||
| ): Promise<Committees[]> => { | ||
| const url = new URL(`/eth/v1/beacon/states/${blockSlot}/committees`, baseUrl); |
There was a problem hiding this comment.
can we use beacon api client rather than re-implementing everywhere?
API will receive changes over time and it will be nightmare to update on bunch of places.
There was a problem hiding this comment.
what if the validator beacon API master class has something wrongly implemented?
There was a problem hiding this comment.
we override if there is non compliant api: #541 (comment)
integration/restValidation.ts
Outdated
| const validatorIndex = Number(validator.data.data.index); | ||
|
|
||
| const url = new URL(`/eth/v1/events?topics=attestation,head`, baseUrl); | ||
| const eventSource = new EventSource(url.href); |
There was a problem hiding this comment.
you already have eth2API instance why not use that to listen on events?
There was a problem hiding this comment.
probably is this problem? ChainSafe/ssz#23
I tried validator on Teku Beacon node and got some error while szz deserializing
There was a problem hiding this comment.
I think we should extend CgEth2Api with specific implementation for different clients until they all converge to be fully compliant with spec.
Like CgTekuEth2Api extends CgEth2Api and CgLighthouseEth2Api extends CgEth2Api where CgEth2Api is spec compliant implementation
There was a problem hiding this comment.
probably is this problem? ChainSafe/ssz#23
I tried validator on Teku Beacon node and got some error while szz deserializing
Nope, you shouldn't get empty beacon blocks over API. Send me json you get from Teku, there is probably some discrepancies with data we get.
There was a problem hiding this comment.
I think we should extend CgEth2Api with specific implementation for different clients until they all converge to be fully compliant with spec.
Like
CgTekuEth2Api extends CgEth2ApiandCgLighthouseEth2Api extends CgEth2Apiwhere CgEth2Api is spec compliant implementation
and this will cause the problem mentioned above #541 (comment)
then will be better to provide client and version to the client and if there is some non-compliant we can override for specific (or range) client/version
There was a problem hiding this comment.
But you know what client you will use for each testnet so you can provide that info in this PR and instantiate appropriate beacon api client instance. As for chainguardian, ideas was (and should still be) to query https://ethereum.github.io/eth2.0-APIs/#/Node/getNodeVersion and depending on that initialize either LighthouseBeaconApi instance or TekuBeaconApi instance or throw error if not supported.
There was a problem hiding this comment.
it is out of scope for this task
for Teku there is #529 where we will proceed by this #541 (comment)
There was a problem hiding this comment.
It makes no sense to merge this PR with all those new beacon api implementation. If this Pr is only for Lighthouse testing, than just use CgEth2BeaconApi everywhere and we can have separate PR with teku specific implementation.
There was a problem hiding this comment.
then what we gonna do with #529 ?
There was a problem hiding this comment.
Ok then, just use CgEth2BeaconApi here (in this PR) and add other api client implementations as part of #529
Log spec discrepancies so we can report it upstream.
integration/restValidation.ts
Outdated
| CGBeaconEventType.ATTESTATION, | ||
| ] as unknown) as BeaconEventType[]); | ||
| for await (const {type, message} of stream) { | ||
| if (((type as unknown) as CGBeaconEventType.ATTESTATION) === CGBeaconEventType.ATTESTATION) { |
There was a problem hiding this comment.
to make it more readable, could you move each case implementation to separate function?
There was a problem hiding this comment.
it's hard to do that, as this cases depends on the upper scope
There was a problem hiding this comment.
Can't you pass required params as method args?
There was a problem hiding this comment.
CGBeaconEventType.ATTESTATION just use Map attestations
but CGBeaconEventType.HEAD uses from uper scope
- slot
- proposers
- firstSlot
- startEpoch
- lastEpoch
- committees
- proposers
- attestations
pretty needy case 🙈
There was a problem hiding this comment.
pass everything as huge object. It's really hard to follow that huge piece of code 😅
There was a problem hiding this comment.
and then return as the very huge object to assign all variables to upper scope variables?
Short description of work done
PR Checklist
Changes
Issues
Closes #528