search

ssvlabs / ssv

Secret-Shared-Validator(SSV) for ethereum staking
https://ssv.network
GNU General Public License v3.0
179 stars 92 forks source link

is `convert.RunnerRole` really needed? #1561

Open moshe-blox opened 1 month ago

moshe-blox commented 1 month ago

convert.RunnerRole seems like a bit of a hack, as if we're adapting Alan to the existing code rather than the other way around.

For example in QBFT storage: is there a good reason to store QBFT instances by BeaconRole at post-fork? At post-fork we perform consensus per RunnerRole not per BeaconRole, so intuitively they should be stored by RunnerRole.

This issue exists to discuss this and come up with a PR to fix if necessary.

y0sher commented 1 month ago

@moshe-blox, in exporter, we want to save consensus results (qbft storage..) in the 'old' format, so convert.RunnerRole basically enables using RunnerRole but also saving Attester and Sync committee duties. Since exporter doesn't share anything about committee consensus now and only about post-consensus.

moshe-blox commented 1 month ago

@y0sher i see, but still i think the QBFT storage should use the spectypes role, and only in exporter we should see usage of the convert function (and not in other packages)