-
Notifications
You must be signed in to change notification settings - Fork 35
Root WASM merge path #2001
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: feat/sync-wasm-merge-callback
Are you sure you want to change the base?
Root WASM merge path #2001
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,17 @@ pub fn merge_root_state( | |
| match try_merge_registered(existing, incoming, existing_ts, incoming_ts) { | ||
| MergeRegistryResult::Success(merged) => Ok(merged), | ||
| MergeRegistryResult::NoFunctionsRegistered => { | ||
| // No in-process merge function registered. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Duplicate WASM callback check creates dead code and incorrect error handling The first WASM callback check at line 109 returns immediately on both success AND failure, making the second check at line 136 unreachable dead code; if the callback returns an error, it propagates up instead of falling back to LWW as the second block's comment suggests. Suggested fix: |
||
| // Try WASM callback if available (RuntimeEnv provides this). | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| if let Some(wasm_callback) = crate::env::get_wasm_root_merge_callback() { | ||
| tracing::debug!( | ||
| target: "calimero_storage::merge", | ||
| "No in-process merge function registered, trying WASM callback" | ||
| ); | ||
| return wasm_callback(existing, incoming); | ||
| } | ||
|
|
||
| // I5 Enforcement: No silent data loss | ||
| // | ||
| // If the root entity contains CRDTs (Counter, etc.) and no merge function | ||
|
|
@@ -122,6 +133,25 @@ pub fn merge_root_state( | |
| // - The data type doesn't match any registered type (test contamination) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Duplicate WASM callback check is dead code The second Suggested fix: |
||
| // - Deserialization failed (corrupt data) | ||
| // | ||
| // Try WASM callback as a fallback before LWW. | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| if let Some(wasm_callback) = crate::env::get_wasm_root_merge_callback() { | ||
| tracing::debug!( | ||
| target: "calimero_storage::merge", | ||
| "All registered merge functions failed, trying WASM callback" | ||
| ); | ||
| match wasm_callback(existing, incoming) { | ||
| Ok(merged) => return Ok(merged), | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| target: "calimero_storage::merge", | ||
| error = ?e, | ||
| "WASM callback also failed, falling back to LWW" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fall back to LWW to maintain backwards compatibility. | ||
| // The incoming value wins if timestamps are equal or incoming is newer. | ||
| // | ||
|
|
||
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.
📝 Nit: Verbose type signature could use a type alias
The
Rc<dyn Fn(&[u8], &[u8]) -> Result<Vec<u8>, MergeError>>type is repeated across multiple files; a type alias would improve readability (DRY).Suggested fix: