Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions apps/state-schema-conformance/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,20 +111,20 @@ impl StateSchemaConformance {
#[app::init]
pub fn init() -> StateSchemaConformance {
StateSchemaConformance {
string_map: UnorderedMap::new(),
int_map: UnorderedMap::new(),
record_map: UnorderedMap::new(),
nested_map: UnorderedMap::new(),
counter_list: Vector::new(),
register_list: Vector::new(),
record_list: Vector::new(),
nested_list: Vector::new(),
map_of_counters: UnorderedMap::new(),
map_of_lists: UnorderedMap::new(),
list_of_maps: Vector::new(),
string_set: UnorderedSet::new(),
visit_counter: Counter::new(),
profile_map: UnorderedMap::new(),
string_map: UnorderedMap::new_with_field_name("string_map"),
int_map: UnorderedMap::new_with_field_name("int_map"),
record_map: UnorderedMap::new_with_field_name("record_map"),
nested_map: UnorderedMap::new_with_field_name("nested_map"),
counter_list: Vector::new_with_field_name("counter_list"),
register_list: Vector::new_with_field_name("register_list"),
record_list: Vector::new_with_field_name("record_list"),
nested_list: Vector::new_with_field_name("nested_list"),
map_of_counters: UnorderedMap::new_with_field_name("map_of_counters"),
map_of_lists: UnorderedMap::new_with_field_name("map_of_lists"),
list_of_maps: Vector::new_with_field_name("list_of_maps"),
string_set: UnorderedSet::new_with_field_name("string_set"),
visit_counter: Counter::new_with_field_name("visit_counter"),
profile_map: UnorderedMap::new_with_field_name("profile_map"),
status: LwwRegister::new(Status::Inactive),
user_id: LwwRegister::new(UserId32([0; 32])),
counter: LwwRegister::new(0),
Expand Down
9 changes: 8 additions & 1 deletion crates/sdk/macros/src/logic/method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,15 @@ impl ToTokens for PublicLogicMethod<'_> {

// todo! when generics are present, strip them
let init_impl = if init_method {
// Wrap init call to assign deterministic IDs after creation
call = quote_spanned! {name.span()=>
::calimero_storage::collections::Root::new(|| #call)
::calimero_storage::collections::Root::new(|| {
let mut state = #call;
// Assign deterministic IDs to all collection fields based on field names
// This ensures CIP Invariant I9: all nodes generate identical entity IDs
state.__assign_deterministic_ids();
state
})
};

quote_spanned! {name.span()=>
Expand Down
148 changes: 132 additions & 16 deletions crates/sdk/macros/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ impl ToTokens for StateImpl<'_> {
// Generate registration hook
let registration_hook = generate_registration_hook(ident, &ty_generics);

// Generate deterministic ID assignment method
let assign_ids_impl = generate_assign_deterministic_ids_impl(ident, generics, orig);

quote! {
#orig

Expand All @@ -68,6 +71,9 @@ impl ToTokens for StateImpl<'_> {

// Auto-generated registration hook
#registration_hook

// Auto-generated deterministic ID assignment
#assign_ids_impl
}
.to_tokens(tokens);
}
Expand Down Expand Up @@ -337,8 +343,8 @@ fn generate_mergeable_impl(
// Only merge fields that are known CRDT types
let merge_calls: Vec<_> = fields
.iter()
.filter_map(|field| {
let field_name = field.ident.as_ref()?;
.enumerate()
.filter_map(|(idx, field)| {
let field_type = &field.ty;

// Check if this is a known CRDT type by examining the type path
Expand All @@ -360,21 +366,41 @@ fn generate_mergeable_impl(
return None;
}

// Generate merge call for CRDT fields
Some(quote! {
::calimero_storage::collections::Mergeable::merge(
&mut self.#field_name,
&other.#field_name
).map_err(|e| {
::calimero_storage::collections::crdt_meta::MergeError::StorageError(
format!(
"Failed to merge field '{}': {:?}",
stringify!(#field_name),
e
// Handle both named fields and tuple struct fields
if let Some(field_name) = &field.ident {
// Named field
Some(quote! {
::calimero_storage::collections::Mergeable::merge(
&mut self.#field_name,
&other.#field_name
).map_err(|e| {
::calimero_storage::collections::crdt_meta::MergeError::StorageError(
format!(
"Failed to merge field '{}': {:?}",
stringify!(#field_name),
e
)
)
})?;
})
} else {
// Tuple struct field
let field_index = syn::Index::from(idx);
Some(quote! {
::calimero_storage::collections::Mergeable::merge(
&mut self.#field_index,
&other.#field_index
).map_err(|e| {
::calimero_storage::collections::crdt_meta::MergeError::StorageError(
format!(
"Failed to merge field {}: {:?}",
#idx,
e
)
)
)
})?;
})
})?;
})
}
})
.collect();

Expand Down Expand Up @@ -436,3 +462,93 @@ fn generate_registration_hook(ident: &Ident, ty_generics: &syn::TypeGenerics<'_>
}
}
}

/// Generate method to assign deterministic IDs to all collection fields.
///
/// This method is called by the init wrapper to ensure all top-level collections
/// have deterministic IDs based on their field names, regardless of how they were
/// created in the user's init() function.
fn generate_assign_deterministic_ids_impl(
ident: &Ident,
generics: &Generics,
orig: &StructOrEnumItem,
) -> TokenStream {
let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();

// Extract fields from the struct
let fields = match orig {
StructOrEnumItem::Struct(s) => &s.fields,
StructOrEnumItem::Enum(_) => {
// Enums don't have fields
return quote! {};
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum state types missing required method causing compile failure

Medium Severity

The generate_assign_deterministic_ids_impl function returns an empty quote! {} for enum types, generating no impl block and no __assign_deterministic_ids method. However, the init wrapper in method.rs unconditionally calls state.__assign_deterministic_ids(). This inconsistency causes compile failures when enum types are used as app state with #[app::init]. The code comment on line 541-542 claims the method is "always generated (even if empty)" but this is incorrect for enums.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved - This issue has been addressed in the latest changes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum state types missing required method generation

Medium Severity

The generate_assign_deterministic_ids_impl function returns an empty TokenStream for enums (line 483: return quote! {}), which means no __assign_deterministic_ids method is generated. However, the init wrapper in method.rs unconditionally calls state.__assign_deterministic_ids() on line 168. This causes a compilation error for any enum state type, as the method doesn't exist. The comment on lines 541-542 even states "This method is always generated... because the init wrapper unconditionally calls it" but this invariant is violated for enums.

Additional Locations (1)

Fix in Cursor Fix in Web

};

// Helper function to check if a type is a collection that needs ID assignment
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 String-based type detection is fragile and could match unintended types

The is_collection_type function uses contains() to match type names, which could incorrectly match user-defined types that happen to contain these substrings. For example, a type named CounterWidget, VectorDisplay, or MyUnorderedMapWrapper would all be incorrectly identified as collections and have reassign_deterministic_id called on them, which would fail at runtime since they don't implement that method.

Suggested fix:

Consider using more precise matching such as checking for exact type names or matching against the full path (e.g., `calimero_storage::collections::Counter`). Alternatively, document this limitation clearly and recommend users avoid type names containing these substrings.

fn is_collection_type(type_str: &str) -> bool {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Fragile string-based type detection for collection types

The is_collection_type function uses type_str.contains() to detect collection types. This could incorrectly match user-defined types containing these substrings (e.g., a type named MyCounterWrapper would match 'Counter'). It also won't work correctly with fully qualified paths or type aliases that don't contain the original type name.

Suggested fix:

Consider using syn's type parsing to extract the actual type path segments, or at minimum check for word boundaries (e.g., type_str ends with the type name or is followed by '<').

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acceptable. Compile-time macro check, works fine.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved - This issue has been addressed in the latest changes.

type_str.contains("UnorderedMap")
|| type_str.contains("Vector")
|| type_str.contains("UnorderedSet")
|| type_str.contains("Counter")
|| type_str.contains("ReplicatedGrowableArray")
|| type_str.contains("UserStorage")
|| type_str.contains("FrozenStorage")
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Macro type detection matches wrapped types incorrectly

Medium Severity

The is_collection_type function uses string-contains matching (type_str.contains("UnorderedMap") etc.) which incorrectly matches wrapper types containing collection names. For example, a field typed Option<UnorderedMap<String, String>> would match because the type string contains "UnorderedMap", causing the macro to generate self.field.reassign_deterministic_id(...) on the Option, which doesn't have that method. This results in a compilation error for valid user code patterns like optional collections, boxed collections, or tuple fields containing collections.

Fix in Cursor Fix in Web


// Generate reassign calls for each collection field
let reassign_calls: Vec<_> = fields
.iter()
.enumerate()
.filter_map(|(idx, field)| {
let field_type = &field.ty;
let type_str = quote! { #field_type }.to_string();

if !is_collection_type(&type_str) {
return None;
}

// Handle both named fields and tuple struct fields
if let Some(field_name) = &field.ident {
// Named field: use field name for both access and ID
let field_name_str = field_name.to_string();
Some(quote! {
self.#field_name.reassign_deterministic_id(#field_name_str);
})
} else {
// Tuple struct field: use index for access, index string for ID
let field_index = syn::Index::from(idx);
let field_name_str = idx.to_string();
Some(quote! {
self.#field_index.reassign_deterministic_id(#field_name_str);
})
}
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuple struct collection fields silently skip ID assignment

Medium Severity

The generate_assign_deterministic_ids_impl function uses field.ident.as_ref()? which returns None for tuple struct fields (e.g., struct MyState(UnorderedMap<K, V>)), silently skipping them. If a tuple struct is used as app state with collection fields created via new(), those collections would retain random IDs instead of getting deterministic ones. This violates the documented behavior that "users can use UnorderedMap::new() in init() while still getting deterministic IDs" and could cause sync failures between nodes without any compile-time or runtime warning.

Fix in Cursor Fix in Web

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved - This issue has been addressed in the latest changes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved - This issue has been addressed in the latest changes.

.collect();

quote! {
// ============================================================================
// AUTO-GENERATED Deterministic ID Assignment
// ============================================================================
//
// This method is called after init() to ensure all top-level collections have
// deterministic IDs. This allows users to use `UnorderedMap::new()` in init()
// while still getting deterministic IDs for proper sync behavior.
//
// CIP Invariant I9: Deterministic Entity IDs
// > Given the same application code and field names, all nodes MUST generate
// > identical entity IDs for the same logical entities.
//
// Note: This method is always generated (even if empty) because the init wrapper
// unconditionally calls it. For apps without CRDT collections, this is a no-op.
//
impl #impl_generics #ident #ty_generics #where_clause {
/// Assigns deterministic IDs to all collection fields based on their field names.
///
/// This is called automatically by the init wrapper. Users should not call this directly.
#[doc(hidden)]
pub fn __assign_deterministic_ids(&mut self) {
#(#reassign_calls)*
}
}
}
}
Loading
Loading