Conversation
CHr15F0x
force-pushed
the
chris/serialized-class-defs-newtypes
branch
5 times, most recently
from
5646390 to
2c0a166
Compare
CHr15F0x
changed the title
…Definition, SerializedClass into SerializedClassDefinition
CHr15F0x
force-pushed
the
chris/serialized-class-defs-newtypes
branch
from
defb65a to
3de8656
Compare
| compute_class_hash(SerializedOpaqueClassDefinition::from_slice( | ||
| CAIRO_0_8_NEW_ATTRIBUTES, | ||
| )) |
There was a problem hiding this comment.
could also use the hash helper
| #[tokio::test] | ||
| async fn cairo_0_11_with_decimal_entry_point_offset() { | ||
| let hash = compute_class_hash(CAIRO_0_11_WITH_DECIMAL_ENTRY_POINT_OFFSET).unwrap(); | ||
| let (hash, _) = compute_class_hash(SerializedOpaqueClassDefinition::from_slice( |
| fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
| serde_json::from_str(value) | ||
| impl CasmContractClass { | ||
| pub fn try_from_serialized_definition( |
There was a problem hiding this comment.
nit, I don't think it would be semantically wrong if we'd name those from_serialized_def, which is way shorter. Wdyt? also in ContractClass
| // Keeping the "experimental" list for backwards | ||
| // compatibility. Also, the names in | ||
| // BlockifierLibfuncs would have to be mapped to | ||
| // (audited|testnet|experimental)_v0.1.0 . |
There was a problem hiding this comment.
are those removals intentional? they kinda seem valid
| } | ||
|
|
||
| fn sierra_version_from_class( | ||
| fn try_sierra_version_from_class( |
There was a problem hiding this comment.
I'd drop the try prefix, but up to you
| include_bytes!( | ||
| "../../fixtures/contracts/\ | ||
| integration_class_0x5ae9d09292a50ed48c5930904c880dab56e85b825022a7d689cfc9e65e01ee7.\ | ||
| json" | ||
| ), |
There was a problem hiding this comment.
formatting looks bit weird now, also the comma
| .context("Fetching class definition")?; | ||
| let class_definition: crate::types::class::sierra::SierraContractClass = | ||
| serde_json::from_str(&String::from_utf8(class_definition)?) | ||
| serde_json::from_str(&String::from_utf8(class_definition.into_bytes())?) |
There was a problem hiding this comment.
ik this was just translated but couldn't it use from_slice?
| pub const CLASS_DEFINITION_MAX_ALLOWED_SIZE: u64 = 4 * 1024 * 1024; | ||
|
|
||
| #[derive(Clone, Debug, Default, PartialEq, Eq, Hash, Dummy)] | ||
| pub struct SerializedSierraDefinition(Vec<u8>); |
There was a problem hiding this comment.
tempting to just slap Bytes in all those, but should probably be another PR 😅 worth an issue?
| Self(bytes) | ||
| } | ||
|
|
||
| pub fn from_slice(bytes: &[u8]) -> Self { |
There was a problem hiding this comment.
I get that being verbose is kinda a purpose of this PR, but sometimes, while reading, I wished you've added impl<S: AsRef<[u8]>> From<S> for SerializedXyzDefinition 😅
not sure tho if that wouldn't introduce surface for similar bugs you experienced tho
2 participants
Fixes: #3326
Rationale: we have 4 distinct types which are all passed around as
Vec<u8>. Just until recently this seemed good enough (though absolutely against static typing and a "everything is a type approach" that leverages the compiler), until it didn't. I spent a good couple of days trying to find a regression that I introduced, which was a simple copy-pasta caused by using aVec<u8>Sierra where Casm was actually expected. That was enough.The PR introduces 4 types:
SerializedSierraDefinitionSerializedCasmDefinitionSerializedCairoDefinitionSerializedOpaqueClassDefinition, where the internalVec<u8>is either sierra or cairo, but we don't really care because it's all passed around to the end consumer without the need to figure out which variant it really is; this is also how classes are stored and served via the RPCSerializedClassDefinition, which is the opposite of the above - it's an enum, which distinguishes between the two variantsI tried to make the changes transparent and not modify the semantics of the APIs, with one notable exception:
compute_class_hash, where it actually made sense to consume the buffer and reinterpret it as either of the variants in a safe manner, so thatdownload_classitself does not have to do the reinterpreting in an unsafe manner.I left all the fixtures as is to keep the delta reasonably small (which it already isn't).