-
Notifications
You must be signed in to change notification settings - Fork 20
SSZ 2: acct-types and ee-chain-types #1101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Commit: c41fd74 SP1 Execution Results
|
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1101 +/- ##
==========================================
+ Coverage 74.62% 74.80% +0.17%
==========================================
Files 510 514 +4
Lines 42807 43412 +605
==========================================
+ Hits 31946 32474 +528
- Misses 10861 10938 +77
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 32 files with indirect coverage changes 🚀 New features to boost your workflow:
|
5ee7052 to
64813cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to use pythonic SSZ schemas for all of these. I am not against having a acct-ssz-types crate and then reexporting the generated types from the acct-types crate.
crates/ee-chain-types/src/block.rs
Outdated
| /// Variable-length list for subject deposits (max 65536 deposits per block) | ||
| type SubjectDepositList = VariableList<SubjectDepositData, 65536>; | ||
|
|
||
| /// Variable-length list for output transfers (max 65536 transfers per block) | ||
| type OutputTransferList = VariableList<OutputTransfer, 65536>; | ||
|
|
||
| /// Variable-length list for output messages (max 65536 messages per block) | ||
| type OutputMessageList = VariableList<SentMessage, 65536>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These consts should be defined somewhere.
@delbonis : After implementing separate *-ssz-types crates, I hit an issue: due to Rust's orphan rule (can't implement traits on foreign types), I had to move all the implementation (types + business logic + errors) into the *-ssz-types crates, not just SSZ serialization. This creates confusion about where to add new functionality. I am considering consolidating everything back into the original crates (acct-types, ee-chain-types) with pythonic schemas in an ssz/ or ssz-schemas/ subdirectory. The trade-off is that acct-types and ee-chain-types will depend on SSZ libraries. Would this consolidated approach work? |
Yeah that is a little annoying, but a structure like this would work, wouldn't it? include!(".../whatever/ssz.rs");
impl Foo { // type defined in ssz.ssz, so this impl is able to be defined here
// ...
}
#[cfg(test)]
mod tests {
// ...
}
I'm in favor of just
The standard SSZ libraries being included in the dep tree is fine. The proc macros would be a dev-dependency so not actually make it into the actual built binaries.
Part of the motivation suggesting a un-consolidated But that being said, these crates are already broken down pretty far, so the above points maybe don't apply strongly. Not sure how I feel about it. |
|
@delbonis I have used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the "roundtrip" tests can be done using proptest!.
See https://proptest-rs.github.io/proptest/ and for examples:
| fn ssz_fixed_len() -> usize { | ||
| 2 | ||
| } | ||
|
|
||
| fn ssz_bytes_len(&self) -> usize { | ||
| 2 | ||
| } | ||
|
|
||
| fn ssz_append(&self, buf: &mut Vec<u8>) { | ||
| (*self as u16).ssz_append(buf); | ||
| } | ||
| } | ||
|
|
||
| impl ssz::Decode for AccountTypeId { | ||
| fn is_ssz_fixed_len() -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn ssz_fixed_len() -> usize { | ||
| 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move all these 2 into a const with proper docstrings?
| ssz_derive::Encode, | ||
| ssz_derive::Decode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we import these Encode/Decode in the top of the file with use?
| fn test_bitcoin_amount_tree_hash() { | ||
| use tree_hash::TreeHash; | ||
|
|
||
| let amount = BitcoinAmount::from_sat(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let amount = BitcoinAmount::from_sat(1000); | |
| let amount = BitcoinAmount::from_sat(1_000); |
| fn test_bitcoin_amount_ssz_roundtrip() { | ||
| use ssz::{Decode, Encode}; | ||
|
|
||
| let amount = BitcoinAmount::from_sat(12345); | ||
| let encoded = amount.as_ssz_bytes(); | ||
| let decoded = BitcoinAmount::from_ssz_bytes(&encoded).unwrap(); | ||
| assert_eq!(amount, decoded); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a property test proptest!. See https://github.com/alpenlabs/bitcoin-bosd/blob/21c257df5c45cac22b91b0c5b1ed78b9fd3dc25f/src/serde.rs#L62-L85
Check the intro in https://proptest-rs.github.io/proptest/intro.html
for a quick introduction on what are property tests if you want.
| fn test_account_id_ssz_roundtrip() { | ||
| let id = AccountId([42u8; 32]); | ||
| let encoded = id.as_ssz_bytes(); | ||
| let decoded = AccountId::from_ssz_bytes(&encoded).unwrap(); | ||
| assert_eq!(id, decoded); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proptest! as well
| } | ||
|
|
||
| #[test] | ||
| fn test_subject_deposit_data_ssz_roundtrip() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another roundtrip test the can be done with proptest!
| } | ||
|
|
||
| #[test] | ||
| fn test_block_inputs_ssz_roundtrip_with_deposits() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another roundtrip test the can be done with proptest!
| } | ||
|
|
||
| #[test] | ||
| fn test_output_transfer_ssz_roundtrip() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another roundtrip test the can be done with proptest!
| } | ||
|
|
||
| #[test] | ||
| fn test_block_outputs_ssz_roundtrip_with_data() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another roundtrip test the can be done with proptest!
| } | ||
|
|
||
| #[test] | ||
| fn test_exec_block_notpackage_ssz_roundtrip() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another roundtrip test the can be done with proptest!
Description
SSZ support for
acct-typesandee-chain-types.The PR was created with help from Claude Code.
Type of Change
Notes to Reviewers
Checklist
in the body of this PR.
Related Issues