Review electra type safety

[wemeetagain]

There are some patterns that lead to degraded type-safety.
We should review our codebase, especially things added in electra, for these types of patterns and refactor if possible.

Ideally we can more clearly articulate our findings here into something that can be added to the contributing doc or elsewhere.

is there a way to make this more typesafe?

I'll make an issue for later, but there are some patterns in our code that lead to this kind of bug (that we should clean up later).

Originally posted by @wemeetagain in #6788 (comment)

[philknows]

Is this in any way related to #6775 ?

[nflaig]

this should be addressed by single attestation changes we did, or more specifically this PR #7261

also the specific part of the code where this was commented is now type safe

lodestar/packages/beacon-node/src/chain/validation/attestation.ts

Lines 528 to 543 in 27301c3

	const attestation: SingleAttestation = attestationOrCache.attestation
	? attestationOrCache.attestation
	: !isForkPostElectra(fork)
	? {
	// Aggregation bits are already asserted above to not be null
	aggregationBits: aggregationBits as BitArray,
	data: attData,
	signature,
	}
	: {
	committeeIndex,
	attesterIndex: validatorIndex,
	data: attData,
	signature,
	};

phase0.Attestation vs. electra.SingleAttestation have a distinct type now and are not compatible, we have a lot of asserts in the code to make sure the correct type is used

lodestar/packages/beacon-node/src/chain/opPools/attestationPool.ts

Lines 139 to 146 in 27301c3

	if (isForkPostElectra(fork)) {
	// Electra only: this should not happen because attestation should be validated before reaching this
	assert.notNull(committeeIndex, "Committee index should not be null in attestation pool post-electra");
	assert.true(isElectraSingleAttestation(attestation), "Attestation should be type electra.SingleAttestation");
	} else {
	assert.true(!isElectraSingleAttestation(attestation), "Attestation should be type phase0.Attestation");
	committeeIndex = null; // For pre-electra, committee index info is encoded in attDataRootIndex
	}

I would consider this closed as it is not actionable

[nflaig]

closing as there doesn't seem to be an objection to my analysis #6799 (comment), can always re-open if someone thinks this is still a problem