Refactor: align attestation containers with LeanSpec (Issue #26) #41
Conversation
ArtiomTr
left a comment
ArtiomTr
left a comment
There was a problem hiding this comment.
looks good, although few things need to be fixed
|
|
||
| // Type-level number for 3112 bytes | ||
| pub type U3112 = Sum<U3100, U12>; | ||
| pub type U3112 = Sum<Prod<U31, U100>, U12>; |
There was a problem hiding this comment.
No need to keep / export this type
| pub fn verify(&self) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
this probably should be replaced with actual implementation
| type Error = String; | ||
| fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> { | ||
| ByteVector::<U3112>::try_from(bytes) | ||
| .map(Signature) | ||
| .map_err(|e| format!("{:?}", e)) | ||
| } |
There was a problem hiding this comment.
why changing error type to string? you can just use the error type that ByteVector's try from provides for you
lean_client/containers/src/config.rs
Outdated
| pub genesis_time: u64, | ||
| } | ||
|
|
||
| #[serde(default = "default_seconds_per_slot")] |
There was a problem hiding this comment.
I believe there is no need to define function here, as you can just use constant. Also, because this is declared in one place, you can just directly hardcode value here, without additional functions/constants
lean_client/containers/src/config.rs
Outdated
| use std::path::Path; | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, Ssz, Default, Serialize, Deserialize)] | ||
| #[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq, Eq, Ssz)] |
There was a problem hiding this comment.
Is config should really be ssz-serializable?
lean_client/containers/src/config.rs
Outdated
| pub genesis_validators: Vec<String>, | ||
| #[ssz(skip)] | ||
| #[serde(default)] | ||
| pub genesis_validators: Vec<serde_json::Value>, |
There was a problem hiding this comment.
I think this should be more concrete type, not array of any values
lean_client/containers/src/types.rs
Outdated
| pub type Bytes32 = H256; | ||
| pub type Uint64 = u64; |
There was a problem hiding this comment.
these aren't necessary anymore I think
| _ => return curr, | ||
| }; | ||
|
|
||
| // Choose best child: most attestations, then lexicographically highest hash |
There was a problem hiding this comment.
why so many comments removed? I wouldn't say those were unnecessary. Better to have some explanation
| pub fn new() -> Self { | ||
| let justification_lookback_slots: u64 = 3; | ||
| let seconds_per_slot: u64 = 12; | ||
| let seconds_per_slot: u64 = 4; |
There was a problem hiding this comment.
shouldn't this be taken from config?
| source_slot = store.latest_justified.slot.0, | ||
| "Created attestation with zero signature" | ||
| ); | ||
| Signature::default() |
There was a problem hiding this comment.
don't think this is correct. but anyway - removing this info! is not correct, as it was signalling that something gone wrong. I would change this info! to warn! at least, but not remove it
|
looks like this needs some rebasing |
Overview
This PR completes the refactoring of the configuration system, addressing both #25 and #26. It removes redundancy by eliminating global constants and merging configuration structures into a single source of truth.
Changes
GenesisConfigandConfiginto a singleConfigstruct.SECONDS_PER_SLOT,INTERVALS_PER_SLOT, andSECONDS_PER_INTERVAL. Components now fetch these values fromstore.config.chain/src/lib.rsto exportChainConfigdirectly viapub use.camelCaserenaming and default values toConfigto maintain compatibility with existing JSON test vectors.Related Issues