feat(storage): add CrdtType to entity metadata#1786
feat(storage): add CrdtType to entity metadata#1786
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
xilosada
force-pushed
the
feat/storage-add-crdt-type-to-metadata
branch
from
1d297ef to
9ab13c2
Compare
|
Bugbot Autofix resolved the bug found in the latest run.
|
|
Bugbot Autofix resolved the bug found in the latest run.
|
xilosada
force-pushed
the
feat/storage-add-crdt-type-to-metadata
branch
2 times, most recently
from
12772fb to
c36a469
Compare
|
Bugbot Autofix prepared fixes for 2 of the 2 bugs found in the latest run.
Or push these changes by commenting: Preview (b654d65a2e)diff --git a/apps/comprehensive-crdt-test/src/lib.rs b/apps/comprehensive-crdt-test/src/lib.rs
--- a/apps/comprehensive-crdt-test/src/lib.rs
+++ b/apps/comprehensive-crdt-test/src/lib.rs
@@ -285,13 +285,13 @@
/// Set a simple value for the current user
pub fn set_user_simple(&mut self, value: String) -> Result<(), String> {
let executor_id = calimero_sdk::env::executor_id();
+ self.user_storage_simple
+ .insert(value.clone().into())
+ .map_err(|e| format!("Insert failed: {:?}", e))?;
app::emit!(Event::UserSimpleSet {
executor_id: executor_id.into(),
value: &value
});
- self.user_storage_simple
- .insert(value.into())
- .map_err(|e| format!("Insert failed: {:?}", e))?;
Ok(())
}
@@ -390,6 +390,22 @@
}
}
+ /// Get a nested value for a specific user
+ pub fn get_user_nested_for(&self, user_key: PublicKey, key: &str) -> Result<Option<String>, String> {
+ let nested_data = self
+ .user_storage_nested
+ .get_for_user(&user_key)
+ .map_err(|e| format!("Get for user failed: {:?}", e))?;
+ match nested_data {
+ Some(data) => Ok(data
+ .map
+ .get(key)
+ .map_err(|e| format!("Map get failed: {:?}", e))?
+ .map(|v| v.get().clone())),
+ None => Ok(None),
+ }
+ }
+
// ===== FrozenStorage Operations =====
/// Add a value to frozen storage
diff --git a/apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml b/apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml
--- a/apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml
+++ b/apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml
@@ -434,8 +434,9 @@
node: comprehensive-node-2
context_id: "{{context_id}}"
executor_public_key: "{{public_key_comprehensive-node-2}}"
- method: get_user_nested
+ method: get_user_nested_for
args:
+ user_key: "{{member_public_key}}"
key: nested-key
outputs:
nested_value: result |
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
Or push these changes by commenting: Preview (2e7246543c)diff --git a/apps/kv-store/workflows/simple-store.yml b/apps/kv-store/workflows/simple-store.yml
--- a/apps/kv-store/workflows/simple-store.yml
+++ b/apps/kv-store/workflows/simple-store.yml
@@ -1,12 +1,12 @@
description: Simple Store Application Workflow (Rust)
name: Simple Store App Test
-force_pull_image: false
+force_pull_image: true
nodes:
chain_id: testnet-1
count: 2
- image: merod:local
+ image: ghcr.io/calimero-network/merod:edge
prefix: simple-store-node
steps:
diff --git a/crates/storage/src/action.rs b/crates/storage/src/action.rs
--- a/crates/storage/src/action.rs
+++ b/crates/storage/src/action.rs
@@ -219,4 +219,5 @@
// Include crdt_type in hash to prevent tampering without invalidating signatures
// This is critical for User storage actions where crdt_type affects merge behavior
+ hasher.update(borsh::to_vec(&metadata.crdt_type).unwrap_or_default());
}
diff --git a/crates/storage/src/collections.rs b/crates/storage/src/collections.rs
--- a/crates/storage/src/collections.rs
+++ b/crates/storage/src/collections.rs
@@ -64,17 +64,6 @@
Id::new(hasher.finalize().into())
}
-/// Compute a deterministic collection ID from parent ID and field name.
-/// This ensures the same collection gets the same ID across all nodes.
-fn compute_collection_id(parent_id: Option<Id>, field_name: &str) -> Id {
- let mut hasher = Sha256::new();
- if let Some(parent) = parent_id {
- hasher.update(parent.as_bytes());
- }
- hasher.update(field_name.as_bytes());
- Id::new(hasher.finalize().into())
-}
-
#[derive(BorshSerialize, BorshDeserialize)]
struct Collection<T, S: StorageAdaptor = MainStorage> {
storage: Element,
@@ -144,32 +133,6 @@
this
}
- /// Creates a new collection with a deterministic ID derived from parent ID and field name.
- /// This ensures collections get the same ID across all nodes when created with the same
- /// parent and field name.
- ///
- /// # Arguments
- /// * `parent_id` - The ID of the parent collection (None for root-level collections)
- /// * `field_name` - The name of the field containing this collection
- #[expect(clippy::expect_used, reason = "fatal error if it happens")]
- pub(crate) fn new_with_field_name(parent_id: Option<Id>, field_name: &str) -> Self {
- let id = compute_collection_id(parent_id, field_name);
-
- let mut this = Self {
- children_ids: RefCell::new(None),
- storage: Element::new(Some(id)),
- _priv: PhantomData,
- };
-
- if id.is_root() {
- let _ignored = <Interface<S>>::save(&mut this).expect("save");
- } else {
- let _ = <Interface<S>>::add_child_to(*ROOT_ID, &mut this).expect("add child");
- }
-
- this
- }
-
/// Inserts an item into the collection.
fn insert(&mut self, id: Option<Id>, item: T) -> StoreResult<T> {
self.insert_with_storage_type(id, item, StorageType::Public) |
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (a85564b572)diff --git a/tools/merodb/src/export.rs b/tools/merodb/src/export.rs
--- a/tools/merodb/src/export.rs
+++ b/tools/merodb/src/export.rs
@@ -1452,7 +1452,7 @@
UserStorage,
FrozenStorage,
Record,
- Custom,
+ Custom { type_name: String },
}
#[derive(borsh::BorshDeserialize)] |
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 117.4s
🟡 Issues pending resolution
✅ 12 Fixed | 🆕 5 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
High Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/entities.rs:55)Medium Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/collections/counter.rs:228)High Severity(crates/storage/src/action.rs:222)High Severity(crates/storage/src/entities.rs:408)Medium Severity(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:473)Low Severity(apps/comprehensive-crdt-test/src/lib.rs:315)High Severity(crates/storage/src/action.rs:221)Low Severity(crates/storage/src/collections.rs:76)Medium Severity(apps/kv-store/workflows/simple-store.yml:9)Medium Severity(tools/merodb/src/export.rs:1562)
🆕 New Issues
These issues were found in the latest changes:
🟡 Warning (2)
1. Unused fields added to KvStore struct
File: apps/kv-store/src/lib.rs (line 18-21) | Consensus: 1/1 agents ✓
The fields tags: UnorderedSet<String> and metadata: LwwRegister<String> are added to the KvStore struct but are never used in any of the methods shown in the diff. The set, remove, and clear methods don't interact with these fields, making them dead code that increases state size without providing functionality.
Suggested fix:
Either implement methods that use these fields (e.g., `add_tag`, `get_metadata`, `set_metadata`) or remove them until they're actually needed. Per the workspace's 'No Dead Code' policy in AGENTS.md, code should only be added when needed.
Found by: cursor-agent
2. RGA merge order assertion may be brittle
File: apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml (line 326-330) | Consensus: 1/1 agents ✓
The test asserts that RGA text equals 'WorldHello' after concurrent inserts at position 0 from two nodes. This assumes a deterministic merge order where Node 2's 'World' appears before Node 1's 'Hello'. If RGA merge ordering depends on node IDs, timestamps, or other factors that could vary, this test could become flaky.
Suggested fix:
Either: (1) Document why this merge order is guaranteed (e.g., based on node ordering/timestamps), (2) Assert that both strings are present without requiring specific order: check length is 10 and contains both 'Hello' and 'World', or (3) Insert at different positions to make order deterministic.
Found by: cursor-agent
💡 Suggestion (1)
1. Silent error suppression with unwrap_or(0)
File: apps/comprehensive-crdt-test/src/lib.rs (line 208-213) | Consensus: 1/1 agents ✓
In get_root_vector, the code uses .map(|c| c.value().unwrap_or(0)) which silently converts any error from c.value() to 0. This could mask real issues during testing, making debugging difficult if the Counter's value retrieval fails.
Suggested fix:
Consider propagating the error properly: `.map(|c| c.value().map_err(|e| format!("Counter value failed: {:?}", e))).transpose()?` or at minimum document why 0 is an acceptable fallback value.
Found by: cursor-agent
📝 Nitpick (2)
1. History trimming logic has no effect
File: apps/kv-store/src/lib.rs (line 79-85) | Consensus: 1/1 agents ✓
The while loop intended to trim history to 100 entries immediately breaks without doing anything: while self.operation_history.len()? > 100 { break; }. The comment explains Vector doesn't have remove, but the code structure is misleading - it suggests trimming will happen when it won't.
Suggested fix:
Remove the misleading while loop and comment, or implement actual trimming. If history should be limited, consider using a different data structure or implementing a proper circular buffer pattern. If unlimited history is intentional, document that decision.
Found by: cursor-agent
2. Silent suppression of wasm-opt failures
File: apps/comprehensive-crdt-test/build.sh (line 15-17) | Consensus: 1/1 agents ✓
The || true after wasm-opt silently ignores all failures, including legitimate optimization errors. While this handles the case where wasm-opt isn't installed, it also hides real issues like corrupted WASM or optimization bugs.
Suggested fix:
Check if wasm-opt exists before running, and only suppress the 'not found' case: `if command -v wasm-opt &> /dev/null; then wasm-opt -O2 res/comprehensive_crdt_test.wasm -o res/comprehensive_crdt_test.wasm; fi`
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-b2f78e26
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (c6251bc0ab)diff --git a/tools/merodb/src/abi.rs b/tools/merodb/src/abi.rs
--- a/tools/merodb/src/abi.rs
+++ b/tools/merodb/src/abi.rs
@@ -96,13 +96,18 @@
// Root ID depends on context:
// - If context_id is provided, root ID is that context_id (Id::root() returns context_id())
// - If no context_id, we can't determine root fields reliably, so use all zeros as fallback
- let root_id_bytes: [u8; 32] = context_id
- .map(|ctx_id| {
- let mut bytes = [0u8; 32];
- bytes.copy_from_slice(ctx_id);
+ let root_id_bytes: [u8; 32] = match context_id {
+ Some(ctx_id) => {
+ let bytes: [u8; 32] = ctx_id.try_into().map_err(|_| {
+ eyre::eyre!(
+ "context_id must be exactly 32 bytes, got {} bytes",
+ ctx_id.len()
+ )
+ })?;
bytes
- })
- .unwrap_or([0u8; 32]);
+ }
+ None => [0u8; 32],
+ };
// Scan State column for EntityIndex entries
let iter = db.iterator_cf(&state_cf, rocksdb::IteratorMode::Start);
@@ -131,7 +136,10 @@
eprintln!(
"[infer_schema] Found root-level entity: id={}, parent_id={:?}, field_name={:?}, crdt_type={:?}",
hex::encode(index.id.as_bytes()),
- index.parent_id.as_ref().map(|id| hex::encode(id.as_bytes())),
+ index
+ .parent_id
+ .as_ref()
+ .map(|id| hex::encode(id.as_bytes())),
index.metadata.field_name,
index.metadata.crdt_type
); |
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 249.3s
🔴 Critical issues require attention
✅ 18 Fixed | 🆕 4 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
High Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/entities.rs:55)Medium Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/collections/counter.rs:228)High Severity(crates/storage/src/action.rs:222)High Severity(crates/storage/src/entities.rs:408)Medium Severity(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:473)Low Severity(apps/comprehensive-crdt-test/src/lib.rs:315)High Severity(crates/storage/src/action.rs:221)Low Severity(crates/storage/src/collections.rs:76)Medium Severity(apps/kv-store/workflows/simple-store.yml:9)Medium Severity(tools/merodb/src/export.rs:1562)Unused fields added to KvStore struct(apps/kv-store/src/lib.rs:18)RGA merge order assertion may be brittle(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:326)Silent error suppression with unwrap_or(0)(apps/comprehensive-crdt-test/src/lib.rs:208)History trimming logic has no effect(apps/kv-store/src/lib.rs:79)Silent suppression of wasm-opt failures(apps/comprehensive-crdt-test/build.sh:15)Medium Severity(tools/merodb/src/abi.rs:105)
🆕 New Issues
These issues were found in the latest changes:
🔴 Critical (1)
1. Security fix incomplete - crdt_type not actually hashed
File: crates/storage/src/action.rs (line 218-220) | Consensus: 1/1 agents ✓
The PR description claims 'Security Fix: Added crdt_type to hash_metadata_for_payload to prevent tampering without invalidating signatures'. However, the actual code only adds a comment stating the intent but never actually hashes the crdt_type field. The function ends with just the comment: '// Include crdt_type in hash to prevent tampering without invalidating signatures'. This leaves the claimed security vulnerability unfixed - an attacker can still modify crdt_type without invalidating signatures.
Suggested fix:
Add the actual hashing code after the comment: `if let Some(ref ct) = metadata.crdt_type { hasher.update(borsh::to_vec(ct).unwrap_or_default()); }`
Found by: cursor-agent
🟡 Warning (1)
1. Fragile error detection in backward-compatible deserialization
File: crates/storage/src/entities.rs (line 445-475) | Consensus: 1/1 agents ✓
The BorshDeserialize implementation for Metadata relies on string matching against error messages ('UnexpectedEof', 'Not all bytes read', 'Unexpected length', 'Unexpected end of input') to detect EOF conditions. This is fragile and could break if borsh changes error messages in future versions. The primary check using ErrorKind::UnexpectedEof is correct, but the string-based fallbacks add maintenance burden.
Suggested fix:
Consider using only the ErrorKind check and documenting that borsh version is pinned, or add tests that verify the error detection works with the current borsh version.
Found by: cursor-agent
💡 Suggestion (1)
1. NestedUserData Default may not initialize storage properly
File: apps/comprehensive-crdt-test/src/lib.rs (line 66-70) | Consensus: 1/1 agents ✓
The NestedUserData struct derives Default, but UnorderedMap::new() and Counter::new() create new storage elements. When used in tests with actual storage backends, the Default implementation may not behave as expected since these CRDTs require proper storage initialization. This could cause issues in the comprehensive test workflows.
Suggested fix:
Verify that the Default implementation works correctly in the test context, or add explicit documentation about when Default should be used.
Found by: cursor-agent
📝 Nitpick (1)
1. RGA text merge assertion may be order-dependent
File: apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml (line 323-326) | Consensus: 1/1 agents ✓
The assertion json_equal({{rga_text}}, {"output": "WorldHello"}) assumes a specific merge order for concurrent RGA insertions at position 0. RGA merge semantics depend on unique IDs and could produce different orderings ('HelloWorld' vs 'WorldHello') depending on implementation details. This test could become flaky if RGA tie-breaking changes.
Suggested fix:
Either verify the RGA tie-breaking behavior is deterministic and documented, or change the assertion to check that both strings are present regardless of order.
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-da06fc33
…nitializers After rebase with master, new tests need the updated Metadata fields.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (c36878629b)diff --git a/apps/comprehensive-crdt-test/src/lib.rs b/apps/comprehensive-crdt-test/src/lib.rs
--- a/apps/comprehensive-crdt-test/src/lib.rs
+++ b/apps/comprehensive-crdt-test/src/lib.rs
@@ -136,17 +136,9 @@
impl ComprehensiveCrdtApp {
#[app::init]
pub fn init() -> ComprehensiveCrdtApp {
- ComprehensiveCrdtApp {
- root_counter: Counter::new(),
- root_map: UnorderedMap::new(),
- root_vector: Vector::new(),
- root_set: UnorderedSet::new(),
- root_rga: ReplicatedGrowableArray::new(),
- root_register: LwwRegister::new(String::new()),
- user_storage_simple: UserStorage::new(),
- user_storage_nested: UserStorage::new(),
- frozen_storage: FrozenStorage::new(),
- }
+ // Use the auto-generated Default implementation which uses field names
+ // for deterministic collection IDs across nodes
+ ComprehensiveCrdtApp::default()
}
// ===== Counter Operations ===== |
- Add crdt_type and field_name to test Metadata initializers (post-rebase) - Fix UpdatedAt comparison in save_internal to use dereferenced values UpdatedAt::PartialEq always returns true, causing incorrect branch - Use element.metadata.crdt_type = None for User storage tests to avoid CRDT merge path (Element::root() now sets CrdtType::Record)
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 177.5s
🔴 Critical issues require attention
✅ 43 Fixed | 🆕 5 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
High Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/entities.rs:55)Medium Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/collections/counter.rs:228)High Severity(crates/storage/src/action.rs:222)High Severity(crates/storage/src/entities.rs:408)Medium Severity(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:473)Low Severity(apps/comprehensive-crdt-test/src/lib.rs:315)High Severity(crates/storage/src/action.rs:221)Low Severity(crates/storage/src/collections.rs:76)Medium Severity(apps/kv-store/workflows/simple-store.yml:9)Medium Severity(tools/merodb/src/export.rs:1455)Unused fields added to KvStore struct(apps/kv-store/src/lib.rs:18)RGA merge order assertion may be brittle(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:326)Silent error suppression with unwrap_or(0)(apps/comprehensive-crdt-test/src/lib.rs:208)History trimming logic has no effect(apps/kv-store/src/lib.rs:79)Silent suppression of wasm-opt failures(apps/comprehensive-crdt-test/build.sh:15)Medium Severity(tools/merodb/src/abi.rs:105)Security fix incomplete - crdt_type not actually hashed(crates/storage/src/action.rs:218)NestedUserData Default may not initialize storage properly(apps/comprehensive-crdt-test/src/lib.rs:66)RGA text merge assertion may be order-dependent(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:323)High Severity(crates/storage/src/entities.rs:55)Local development image change will break CI/CD(apps/kv-store/workflows/simple-store.yml:4)Incomplete/truncated build.rs file(apps/state-visualization-test/build.rs:1)PR description claims struct variant but code uses tuple variant for CrdtType::Custom(crates/storage/src/collections/crdt_meta.rs:17)Deterministic head selection may conflict with merge scenario handling(crates/node/src/delta_store.rs:551)Low Severity(crates/sdk/macros/src/state.rs:461)Local Docker image breaks CI/CD and other developers(apps/kv-store/workflows/simple-store.yml:4)RGA concurrent insert order assertion may be non-deterministic(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:321)CrdtType uses tuple variant for Custom but PR claims struct variant(crates/storage/src/collections/crdt_meta.rs:17)README.md appears truncated(apps/state-visualization-test/README.md:54)Suppressed clippy warning for len_without_is_empty(apps/comprehensive-crdt-test/src/lib.rs:15)Workflow changed to use local image instead of published image(apps/kv-store/workflows/simple-store.yml:4)Silent error handling masks potential test failures(apps/comprehensive-crdt-test/src/lib.rs:218)Lint suppression without justification(apps/comprehensive-crdt-test/src/lib.rs:15)Error details discarded in hex decoding(apps/comprehensive-crdt-test/src/lib.rs:408)wasm-opt failures silently ignored(apps/comprehensive-crdt-test/build.sh:16)Local testing configuration committed - will break CI(apps/kv-store/workflows/simple-store.yml:4)README file appears truncated/incomplete(apps/state-visualization-test/README.md:47)Error handling loses type information(apps/comprehensive-crdt-test/src/lib.rs:139)Module-level clippy lint suppression is too broad(apps/comprehensive-crdt-test/src/lib.rs:15)Counter value 0 is indistinguishable from missing counter(apps/comprehensive-crdt-test/src/lib.rs:213)Low Severity(apps/comprehensive-crdt-test/src/lib.rs:150)
🆕 New Issues
These issues were found in the latest changes:
🔴 Critical (1)
1. Workflow uses local Docker image - will break CI
File: apps/kv-store/workflows/simple-store.yml (line 4-9) | Consensus: 1/1 agents ✓
The workflow was changed from using ghcr.io/calimero-network/merod:edge to merod:local with force_pull_image: false. This is a local development configuration that will fail in CI environments where merod:local doesn't exist. This appears to be an accidental commit of debugging changes.
Suggested fix:
Revert to the original configuration: `image: ghcr.io/calimero-network/merod:edge` and `force_pull_image: true`
Found by: cursor-agent
🟡 Warning (1)
1. state-visualization-test app not added to workspace
File: apps/state-visualization-test/Cargo.toml (line 1-23) | Consensus: 1/1 agents ✓
The apps/state-visualization-test directory is created with Cargo.toml and README.md, but it is not added to the workspace members in the root Cargo.toml. This means the app will not be built or included in the workspace, resulting in orphaned code.
Suggested fix:
Either add `"./apps/state-visualization-test"` to the workspace members in `Cargo.toml`, or remove the incomplete app directory if it's not intended for this PR.
Found by: cursor-agent
💡 Suggestion (2)
1. Silent error conversion with unwrap_or(0)
File: apps/comprehensive-crdt-test/src/lib.rs (line 224-224) | Consensus: 1/1 agents ✓
The get_root_vector method uses .map(|c| c.value().unwrap_or(0)) which silently converts any error from value() to 0. This could mask legitimate errors and make debugging difficult when counter values are unexpectedly zero.
Suggested fix:
Consider propagating the error or using `map_err` like the rest of the codebase: `.map(|c| c.value().map_err(|e| format!("Value failed: {:?}", e))).transpose()?.flatten()`
Found by: cursor-agent
2. Workflow uses merod:local - may not work in CI
File: apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml (line 5-12) | Consensus: 1/1 agents ✓
The new comprehensive-crdt-test workflow uses image: merod:local which assumes a locally built Docker image exists. While this may be intentional for integration tests that require local builds, it should be documented or use a CI-compatible image path.
Suggested fix:
Either document that this workflow requires `merod:local` to be built first, or use `ghcr.io/calimero-network/merod:edge` for CI compatibility (or parameterize the image).
Found by: cursor-agent
📝 Nitpick (1)
1. Security-critical code could use more prominent documentation
File: crates/storage/src/action.rs (line 196-198) | Consensus: 1/1 agents ✓
The inclusion of crdt_type in hash_metadata_for_payload is described in a comment, but given its security importance (preventing signature bypass attacks as noted in the PR description), this critical security property could benefit from more prominent documentation or even a safety comment.
Suggested fix:
Consider adding `// SECURITY:` prefix to the comment or adding it to module-level documentation explaining that crdt_type must always be included in the hash to prevent tampering.
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-3ae6b4a9
Revert accidental change from ghcr.io/calimero-network/merod:edge to merod:local which breaks CI runners that don't have the local image.
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 232.8s
🟡 Issues pending resolution
✅ 47 Fixed | 🆕 5 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
High Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/entities.rs:55)Medium Severity(crates/storage/src/entities.rs:408)Medium Severity(crates/storage/src/collections/counter.rs:228)High Severity(crates/storage/src/action.rs:222)High Severity(crates/storage/src/entities.rs:408)Medium Severity(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:473)Low Severity(apps/comprehensive-crdt-test/src/lib.rs:315)High Severity(crates/storage/src/action.rs:221)Low Severity(crates/storage/src/collections.rs:76)Medium Severity(apps/kv-store/workflows/simple-store.yml:9)Medium Severity(tools/merodb/src/export.rs:1455)Unused fields added to KvStore struct(apps/kv-store/src/lib.rs:18)RGA merge order assertion may be brittle(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:326)Silent error suppression with unwrap_or(0)(apps/comprehensive-crdt-test/src/lib.rs:208)History trimming logic has no effect(apps/kv-store/src/lib.rs:79)Silent suppression of wasm-opt failures(apps/comprehensive-crdt-test/build.sh:15)Medium Severity(tools/merodb/src/abi.rs:105)Security fix incomplete - crdt_type not actually hashed(crates/storage/src/action.rs:218)NestedUserData Default may not initialize storage properly(apps/comprehensive-crdt-test/src/lib.rs:66)RGA text merge assertion may be order-dependent(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:323)High Severity(crates/storage/src/entities.rs:55)Local development image change will break CI/CD(apps/kv-store/workflows/simple-store.yml:4)Incomplete/truncated build.rs file(apps/state-visualization-test/build.rs:1)PR description claims struct variant but code uses tuple variant for CrdtType::Custom(crates/storage/src/collections/crdt_meta.rs:17)Deterministic head selection may conflict with merge scenario handling(crates/node/src/delta_store.rs:551)Low Severity(crates/sdk/macros/src/state.rs:461)Local Docker image breaks CI/CD and other developers(apps/kv-store/workflows/simple-store.yml:4)RGA concurrent insert order assertion may be non-deterministic(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:321)CrdtType uses tuple variant for Custom but PR claims struct variant(crates/storage/src/collections/crdt_meta.rs:17)README.md appears truncated(apps/state-visualization-test/README.md:54)Suppressed clippy warning for len_without_is_empty(apps/comprehensive-crdt-test/src/lib.rs:15)Workflow changed to use local image instead of published image(apps/kv-store/workflows/simple-store.yml:4)Silent error handling masks potential test failures(apps/comprehensive-crdt-test/src/lib.rs:218)Lint suppression without justification(apps/comprehensive-crdt-test/src/lib.rs:15)Error details discarded in hex decoding(apps/comprehensive-crdt-test/src/lib.rs:408)wasm-opt failures silently ignored(apps/comprehensive-crdt-test/build.sh:16)Local testing configuration committed - will break CI(apps/kv-store/workflows/simple-store.yml:4)README file appears truncated/incomplete(apps/state-visualization-test/README.md:47)Error handling loses type information(apps/comprehensive-crdt-test/src/lib.rs:139)Module-level clippy lint suppression is too broad(apps/comprehensive-crdt-test/src/lib.rs:15)Counter value 0 is indistinguishable from missing counter(apps/comprehensive-crdt-test/src/lib.rs:213)Low Severity(apps/comprehensive-crdt-test/src/lib.rs:150)Workflow uses local Docker image - will break CI(apps/kv-store/workflows/simple-store.yml:4)state-visualization-test app not added to workspace(apps/state-visualization-test/Cargo.toml:1)Silent error conversion with unwrap_or(0)(apps/comprehensive-crdt-test/src/lib.rs:224)Workflow uses merod:local - may not work in CI(apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml:5)
🆕 New Issues
These issues were found in the latest changes:
🟡 Warning (3)
1. Fragile error detection in custom BorshDeserialize
File: crates/storage/src/entities.rs (line 502-535) | Consensus: 1/1 agents ✓
The custom BorshDeserialize implementation for Metadata detects EOF errors by checking error message strings (e.g., 'UnexpectedEof', 'Not all bytes read'). This is fragile because error messages may change between borsh versions or vary across platforms. If the error format changes, legitimate backward-compatible deserialization would fail.
Suggested fix:
Consider using a length-prefixed format, a version byte, or checking remaining bytes in the reader before attempting to deserialize optional fields. Alternatively, try reading a single byte first to check if more data exists.
Found by: cursor-agent
2. Removed force_root_hash may cause convergence issues
File: crates/node/src/delta_store.rs (line 92-111) | Consensus: 1/1 agents ✓
The PR removes force_root_hash calls that previously ensured deterministic root hash selection across nodes. The new approach relies entirely on CRDT merge logic producing identical results on all nodes. If any merge function has non-deterministic behavior (e.g., timestamp ordering, HashMap iteration order), nodes may diverge. This is a significant behavior change that requires all merge operations to be strictly deterministic.
Suggested fix:
Ensure all merge functions are provably deterministic. Consider adding integration tests that verify root hash convergence across nodes with different delta arrival orders. Document the invariant that all CRDT merges must be deterministic.
Found by: cursor-agent
3. RGA concurrent insertion test assumes deterministic merge order
File: apps/comprehensive-crdt-test/workflows/comprehensive-crdt-test.yml (line 309-311) | Consensus: 1/1 agents ✓
The test asserts {"output": "WorldHello"} after concurrent insertions at position 0 from two nodes. This assumes Node 2's text will always precede Node 1's in the merged result. RGA's tie-breaking mechanism typically uses unique node IDs or timestamps, which could produce different orders depending on node identity assignment. This test may be flaky if node IDs change.
Suggested fix:
Either verify both nodes have consistent deterministic IDs, or change the assertion to check that both 'Hello' and 'World' are present without asserting specific order: `rga_text.output contains 'Hello' AND rga_text.output contains 'World'`.
Found by: cursor-agent
💡 Suggestion (2)
1. Removal of #[non_exhaustive] from Metadata struct
File: crates/storage/src/entities.rs (line 386-386) | Consensus: 1/1 agents ✓
The #[non_exhaustive] attribute was removed from Metadata. While the PR notes this is intentional for prerelease, this makes it a breaking API change. Future field additions to Metadata will require another breaking change. Downstream code pattern-matching on Metadata fields may silently become incomplete.
Suggested fix:
Consider keeping `#[non_exhaustive]` to allow future field additions without breaking changes, especially since more sync protocol features are planned (issues #1769-#1785).
Found by: cursor-agent
2. Scope creep: field_name addition beyond issue #1768
File: crates/storage/src/entities.rs (line 411-416) | Consensus: 1/1 agents ✓
The PR adds field_name: Option<String> to Metadata, which wasn't part of the original issue #1768 (Add CrdtType to Entity Metadata). While useful for schema inference, bundling unrelated features in one PR makes review harder and increases risk. The field_name feature adds complexity to serialization/deserialization and affects the hash computation.
Suggested fix:
Consider splitting the `field_name` feature into a separate PR to keep changes focused and easier to review/rollback if issues arise.
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-b97f41cb
| } | ||
|
|
||
| #[app::logic] | ||
| impl VisualizationTest { |
There was a problem hiding this comment.
App missing required initialization function
High Severity
The VisualizationTest app lacks an #[app::init] function within its #[app::logic] impl block. Without an init function, the app state is never created, and any method call will panic with "Failed to find or read app state". All other apps in this repository define an init() method annotated with #[app::init] that returns the initial state.
| ); | ||
| ct | ||
| } | ||
| Err(e) => { |
There was a problem hiding this comment.
🟡 Fragile error detection in custom BorshDeserialize
The custom BorshDeserialize implementation for Metadata detects EOF errors by checking error message strings (e.g., 'UnexpectedEof', 'Not all bytes read'). This is fragile because error messages may change between borsh versions or vary across platforms. If the error format changes, legitimate backward-compatible deserialization would fail.
Suggested fix:
Consider using a length-prefixed format, a version byte, or checking remaining bytes in the reader before attempting to deserialize optional fields. Alternatively, try reading a single byte first to check if more data exists.
| @@ -92,22 +92,22 @@ impl DeltaApplier<Vec<Action>> for ContextStorageApplier { | |||
| ))); | |||
There was a problem hiding this comment.
🟡 Removed force_root_hash may cause convergence issues
The PR removes force_root_hash calls that previously ensured deterministic root hash selection across nodes. The new approach relies entirely on CRDT merge logic producing identical results on all nodes. If any merge function has non-deterministic behavior (e.g., timestamp ordering, HashMap iteration order), nodes may diverge. This is a significant behavior change that requires all merge operations to be strictly deterministic.
Suggested fix:
Ensure all merge functions are provably deterministic. Consider adding integration tests that verify root hash convergence across nodes with different delta arrival orders. Document the invariant that all CRDT merges must be deterministic.
| args: | ||
| position: 0 | ||
| text: World | ||
|
|
There was a problem hiding this comment.
🟡 RGA concurrent insertion test assumes deterministic merge order
The test asserts {"output": "WorldHello"} after concurrent insertions at position 0 from two nodes. This assumes Node 2's text will always precede Node 1's in the merged result. RGA's tie-breaking mechanism typically uses unique node IDs or timestamps, which could produce different orders depending on node identity assignment. This test may be flaky if node IDs change.
Suggested fix:
Either verify both nodes have consistent deterministic IDs, or change the assertion to check that both 'Hello' and 'World' are present without asserting specific order: `rga_text.output contains 'Hello' AND rga_text.output contains 'World'`.
| @@ -332,10 +386,7 @@ impl Default for StorageType { | |||
| } | |||
There was a problem hiding this comment.
💡 Removal of #[non_exhaustive] from Metadata struct
The #[non_exhaustive] attribute was removed from Metadata. While the PR notes this is intentional for prerelease, this makes it a breaking API change. Future field additions to Metadata will require another breaking change. Downstream code pattern-matching on Metadata fields may silently become incomplete.
Suggested fix:
Consider keeping `#[non_exhaustive]` to allow future field additions without breaking changes, especially since more sync protocol features are planned (issues #1769-#1785).
| pub crdt_type: Option<CrdtType>, | ||
|
|
||
| /// Field name for schema inference and migrations. | ||
| /// |
There was a problem hiding this comment.
💡 Scope creep: field_name addition beyond issue #1768
The PR adds field_name: Option<String> to Metadata, which wasn't part of the original issue #1768 (Add CrdtType to Entity Metadata). While useful for schema inference, bundling unrelated features in one PR makes review harder and increases risk. The field_name feature adds complexity to serialization/deserialization and affects the hash computation.
Suggested fix:
Consider splitting the `field_name` feature into a separate PR to keep changes focused and easier to review/rollback if issues arise.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (53bc43bcaa)diff --git a/apps/state-visualization-test/src/lib.rs b/apps/state-visualization-test/src/lib.rs
--- a/apps/state-visualization-test/src/lib.rs
+++ b/apps/state-visualization-test/src/lib.rs
@@ -53,6 +53,17 @@
#[app::logic]
impl VisualizationTest {
+ #[app::init]
+ pub fn init() -> VisualizationTest {
+ VisualizationTest {
+ items: UnorderedMap::new(),
+ operation_count: Counter::new(),
+ operation_history: Vector::new(),
+ tags: UnorderedSet::new(),
+ metadata: LwwRegister::new(String::new()),
+ }
+ }
+
// =========================================================================
// Item Operations (UnorderedMap)
// =========================================================================
@@ -61,7 +72,8 @@
pub fn set(&mut self, key: String, value: String) -> app::Result<()> {
app::log!("Setting key: {:?} to value: {:?}", key, value);
- self.items.insert(key.clone(), LwwRegister::new(value.clone()))?;
+ self.items
+ .insert(key.clone(), LwwRegister::new(value.clone()))?;
self.operation_count.increment()?;
self.operation_history
.push(LwwRegister::new(format!("Set: {} = {}", key, value)))?; |
SDK JS Workflows FailedThe following SDK JS workflow(s) failed:
Please check the workflow logs for more details. |
Merobox Workflows FailedThe following workflow(s) failed after retries:
Please check the workflow logs for more details. |
Combined feature from PR #1786 and #1787: - Add field_name and crdt_type to Metadata struct - Add Element::new_with_field_name and new_with_field_name_and_crdt_type - Update Collection::new_with_field_name to store metadata - Update all collection types to pass their CrdtType: - UnorderedMap -> CrdtType::UnorderedMap - UnorderedSet -> CrdtType::UnorderedSet - Vector -> CrdtType::Vector - Counter -> CrdtType::Counter - RGA -> CrdtType::Rga - Add BorshSerialize/Deserialize/Ord/PartialOrd to CrdtType - Add UserStorage and FrozenStorage to CrdtType enum - Custom Borsh de/serialization for backward compatibility This enables: - Deterministic collection IDs (from #1787) - Schema inference from database metadata (from #1786) - CRDT type-aware merge dispatch
Pull request was closed
Combined feature from PR #1786 and #1787: - Add field_name and crdt_type to Metadata struct - Add Element::new_with_field_name and new_with_field_name_and_crdt_type - Update Collection::new_with_field_name to store metadata - Update all collection types to pass their CrdtType: - UnorderedMap -> CrdtType::UnorderedMap - UnorderedSet -> CrdtType::UnorderedSet - Vector -> CrdtType::Vector - Counter -> CrdtType::Counter - RGA -> CrdtType::Rga - Add BorshSerialize/Deserialize/Ord/PartialOrd to CrdtType - Add UserStorage and FrozenStorage to CrdtType enum - Custom Borsh de/serialization for backward compatibility This enables: - Deterministic collection IDs (from #1787) - Schema inference from database metadata (from #1786) - CRDT type-aware merge dispatch
Combined feature from PR #1786 and #1787: - Add field_name and crdt_type to Metadata struct - Add Element::new_with_field_name and new_with_field_name_and_crdt_type - Update Collection::new_with_field_name to store metadata - Update all collection types to pass their CrdtType: - UnorderedMap -> CrdtType::UnorderedMap - UnorderedSet -> CrdtType::UnorderedSet - Vector -> CrdtType::Vector - Counter -> CrdtType::Counter - RGA -> CrdtType::Rga - Add BorshSerialize/Deserialize/Ord/PartialOrd to CrdtType - Add UserStorage and FrozenStorage to CrdtType enum - Custom Borsh de/serialization for backward compatibility This enables: - Deterministic collection IDs (from #1787) - Schema inference from database metadata (from #1786) - CRDT type-aware merge dispatch
1 participant
Implements #1768
Summary
Adds
crdt_type: Option<CrdtType>to entityMetadatato enable proper CRDT merge dispatch during state synchronization. This PR also includes critical fixes for root hash convergence during concurrent delta merges, adds built-in CRDT types for UserStorage and FrozenStorage, and includes comprehensive testing infrastructure.Changes
Core Implementation (#1768)
CrdtTypeenum toentities.rswith all required variants (struct variant:Custom { type_name: String })crdt_typefield toMetadatastructMetadata::with_crdt_type(),Metadata::is_builtin_crdt()CrdtTypedefinitions - removed duplicate fromcrdt_meta.rs, now re-exports fromentities.rsuser.rsandfrozen.rsto use struct variant syntaxCustom { type_name: ... }instead of tuple variantcrdt_typetohash_metadata_for_payloadto prevent tampering without invalidating signaturesExtended Scope - Root Hash Convergence Fix
Related to POC fixes in
test/tree_syncbranchDuring testing of UserStorage workflows, we discovered that concurrent delta merges were not converging correctly. The root hash forcing logic was overwriting merged state, causing sync failures.
is_merge_scenario()methodparent_hashestracking to detect concurrent branchesDELTA_APPLY_TIMING)Impact: UserStorage and FrozenStorage workflows now pass successfully. Root hashes converge correctly when concurrent writes occur.
Extended Scope - Built-in CRDT Types
CrdtType::UserStorageas built-in type (replacesCustom("UserStorage"))CrdtType::FrozenStorageas built-in type (replacesCustom("FrozenStorage"))UserStorageandFrozenStorageimplementations to use built-in typesExtended Scope - Comprehensive Testing Infrastructure
comprehensive-crdt-testapp that tests ALL CRDT types, UserStorage, FrozenStorage, and root-level concurrent modificationstest_user_storage.ymlworkflow with intrinsics tests:test_frozen_storage.ymlworkflow with intrinsics tests:Security Fix
Critical: The
crdt_typefield is now included inhash_metadata_for_payloadto prevent signature bypass attacks. Previously, an attacker could modifycrdt_typein User storage actions without invalidating the signature, potentially altering merge behavior once CRDT merge dispatch is implemented.Fixes Applied
CrdtTypedefinition inentities.rswith struct variantCustom { type_name: String }crdt_typeincluded in hash computation for User storage actionsAcceptance Criteria
crdt_typeloads successfully (None) - verifiedcrdt_typesetcrdt_typeincluded in signature hash to prevent tamperingCIP Reference
test/tree_syncbranch for root hash convergenceTesting
crdt_typefieldFiles Modified
crates/storage/src/entities.rs- AddedCrdtTypeenum andcrdt_typefield toMetadatacrates/storage/src/action.rs- Security fix: Addedcrdt_typetohash_metadata_for_payloadcrates/storage/src/collections/crdt_meta.rs- Removed duplicate enum, re-exports from entities, addedUserStorageandFrozenStoragevariantscrates/storage/src/collections/user.rs- Updated to useCrdtType::UserStoragecrates/storage/src/collections/frozen.rs- Updated to useCrdtType::FrozenStoragecrates/storage/src/collections.rs- Updated exports to use canonical definitioncrates/storage/src/lib.rs- ExportsCrdtTypeandMetadatafrom entitiescrates/node/src/delta_store.rs- Root hash fix: Added merge scenario detection, removed root hash forcing for mergesapps/kv-store-with-user-and-frozen-storage/workflows/test_user_storage.yml- Enhanced with intrinsics testsapps/kv-store-with-user-and-frozen-storage/workflows/test_frozen_storage.yml- Enhanced with intrinsics testsapps/comprehensive-crdt-test/- New: Comprehensive test app with workflowRelated Issues
test/tree_syncbranch (commit1f14b914- "fix(sync): remove force_root_hash for multiple DAG heads")Note
High Risk
Touches core storage serialization/metadata, CRDT merge dispatch, and node sync/root-hash handling; mistakes could cause state divergence, data loss, or signature/validation regressions despite the added test coverage.
Overview
Enriches storage entities with persisted
Metadataincludingcrdt_typeandfield_name, introducing a canonicalCrdtTypeenum and wiring it through collection constructors (e.g.,new_with_field_name) to produce deterministic IDs and support schema inference.Updates merge and sync behavior to rely on mergeable deltas and registered root merges (root state now merges as a Record CRDT; no longer forces root hashes on mismatches or multiple DAG heads), adds a
WasmMergeCallbackpath for custom CRDT types, and tightens user-action integrity by hashingcrdt_typeinto signed payload metadata.Adds new integration/test fixtures: a
comprehensive-crdt-testWASM app + workflow covering all CRDTs/UserStorage/FrozenStorage and concurrent root modifications, astate-visualization-testfixture and schema-inference docs, plus expanded UserStorage/FrozenStorage workflows and unit tests for serialization/backward-compat handling.Written by Cursor Bugbot for commit 7ec9721. This will update automatically on new commits. Configure here.