SSZ 1: SPS-62 checkpoint-types #1098
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1098 +/- ##
========================================
Coverage 74.69% 74.70%
========================================
Files 511 512 +1
Lines 42825 42942 +117
========================================
+ Hits 31988 32079 +91
- Misses 10837 10863 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Commit: 030301e SP1 Execution Results
|
There was a problem hiding this comment.
Should we be converting over to using types that look like this and then convert to SSZ or vice versa? Converting over the existing types I think is kinda a wasted effort since we know we'll be changing them.
There was a problem hiding this comment.
I updated the PR to add SSZ to SPS-62 checkpoint types. According to @prajwolrg, the work on SPS-62 types is not done yet. I will create other PRs for types that already exist.
| impl From<borsh::L1BlockCommitment> for crate::L1BlockCommitment { | ||
| fn from(value: borsh::L1BlockCommitment) -> Self { | ||
| Self { | ||
| height: value.height_u64(), | ||
| blkid: FixedVector::from(value.blkid().as_ref().to_vec()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl From<crate::L1BlockCommitment> for borsh::L1BlockCommitment { |
There was a problem hiding this comment.
Only use crate:: imports in use decls.
crates/identifiers-ssz/Cargo.toml
Outdated
There was a problem hiding this comment.
What is the rationale for introducing new crates just for the SSZ serialization?
My thought was that instead of making new crates for the SSZ definitions, we should convert the existing crates over to using the SSZ definitions, removing the borshy ones (which we want to eliminate anyways) and manually adding the extra conversion impls where needed. This way we won't need an additional conversion step before serialization / after deserialization that does what is essentially an extra copy.
There was a problem hiding this comment.
I did have an exchange with Claude Code about this. I felt the conversion was not necessary. The original rationale was to avoid introducing breaking changes and allow gradual migration. I agree with your suggestion. So will update the PR soon.
krsnapaudel
force-pushed
the
ssz-1
branch
4 times, most recently
from
1cb9b49 to
c33b6f7
Compare
krsnapaudel
changed the title
identifiers and checkpoint-types
Description
SSZ support for checkpoint types per SPS-62.
This PR was created with help from Claude Code.
Type of Change
Notes to Reviewers
Checklist
in the body of this PR.
Related Issues