Make Kitsune2MemoryOpStore generic over its op type
#277
Conversation
|
Oh yeah, I based this on a commit behind |
|
I will also sneak a totally unrelated question in here: is there a way to register a handler that gets called whenever new ops have been received via publish/gossip/fetch? Or must one simply poll the op store? |
|
We've gone back and forth a bit about whether to re-use the in-memory op store. It is useful for writing tests within Kitsune2 and it is valid as a reference implementation but not persisting ops is not usable outside tests, there is also no op location logic used in tests with the memory op store. We've found that the problems run deeper when you try to re-use it and it's worth the initial effort to create one. Even for Wind Tunnel testing, we needed different functionality and ended up defining a new op store https://github.com/holochain/wind-tunnel/blob/main/bindings/kitsune_client/src/op_store.rs.
That is entirely intentional, Kitsune2 needs to know very little about the data it is handling. Everything is done with
Yes and no. No, in the sense that the memory op store doesn't do this. Yes, in the sense that Kitsune2 notifies the host about newly arrived op data using the Kitsune2 could obviously use more documentation about which trait implementations from the core module are intended to be re-used, and which ones the host is supposed to implement. The op store is one the host is expected to implement. What I'd really like is some sample application that uses a real database to show how everything fits together. Is there something else about the memory op store that is useful, beyond getting started quicker? Is it that you'd like the storage logic separated from the op store logic somehow so that you can run unit tests in-memory and then only use a database for integration tests? |
No, but the value of getting started quicker can't be overstated. I'm trying to get a demo of an app ready in two weeks, and it doesn't have to be robust, it just has to have a p2p experience. Getting something working with a mem op store ASAP lets me have all of this without investing in building my own op store:
My plan was to set up an always-online node that just syncs with other people. Good enough for a demo, and even lets me test initial sync over and over. Also I don't have any need for location, all Spaces will be full-sync for a long time, and I'm not doing anything like holochain's authorities pattern.
It seems to me that in the ideal world, the host shouldn't be expected to implement an op store, but rather initially to select one from a list of quality first- and third-party op store implementations. Maybe implementing their own if needed. Most framework users are accustomed to picking a pre-made "backend" or "driver", which this basically is. Regardless of whether something is designed to be reused or not, people who are trying to get stuff done are certainly going to attempt to reuse and repurpose them anyway (especially when there are no other guides), so why not go the extra mile and make them reusable? :) I'm definitely not going to invest in building my own op store at this time, especially since I have one that already works for me. Depending how this goes I may invest further in making a persistent op store, if one doesn't already exist by then (or I may have to choose another framework for other reasons before that point). If you're not already thinking this way, I would encourage you to think of every op store as reusable, as I think that will be important for actual adoption of Kitsune. Last question, would you recommend I use the windtunnel op store you linked to instead of the one I'm currently using? |
I would like to have more example and/or ready-to-use code for Kitsune2 but we haven't had time to do that. Between the memory op store and the Holochain implementation on sqlite, the elements of implementing the trait are visible at least and that's the best we've been able to do so far. Even if we did implement a persistent op store for re-use, we then have to make decisions about the op type that is in use and how to process that. We'd end up defining another trait that can hide the common operations that need to be done to convert between bytes <-> op data <-> op id. Then there's the question of what database we use. My first preference would be to have a clean and simple implementation of a Rust app built on top of K2 that implements the traits we expect a host application to implement, rather than using the core implementations like our showcase app currently does. That would be a good starting point to having demonstrated how K2 works outside of Holochain.
I really don't want people to use the memory op store in a binary. I was thinking this PR was about using it in tests but with the op type being defined as something other than the memory op. Kitsune expects that op data is persistent and it's only in-memory for unit tests and so that Kitsune isn't opinionated about how you store your data.
If that computer has to restart because of a loss of power, or even just for software updates or perhaps a kernel issue - all the network data is gone. That might be acceptable for a demo but it certainly wouldn't be expected behavior by people in general.
I have already written one that uses the Fjall key-value database, which is why I'm defending the position that having a persistent store is practical outside of Holochain. I did run into the same serialization errors along the way that you're describing, it's not a trivial task, but it can certainly be done. That code isn't open source yet and I haven't finished figuring out how/when I'm going to do that - unfortunately it's unlikely that will be in the next two weeks though. If you're happy that you are getting what you need from the memory op store then I don't object to you using it for your demo of course - but that's a different story to making the functionality available to people who don't understand the system as well as you do.
That op store has code in that depends on Wind Tunnel so it won't work outside of that environment. I think the useful part of that one is the post step to receiving data where it records metrics. That is the point where in a custom op store you could do the step you were asking about which is notify the host about ops being received. |
Of course my ideal is to have a persistent op store, and I imagine it would be the ideal for just about anyone else. I'm only using the mem one because it's available and it works now.
Yes, most normal use cases would want a persistent store, including mine. Still, I wouldn't go so far as to say there are no valid reasons for using a mem op store in production. Off the top of my head: imagine IoT devices that don't have persistent storage of their own and/or are extremely limited in memory, and want to do some ephemeral syncing of small amounts of data and don't need to install a database for that. p2p syncing of data is a very general pattern and I can't imagine anticipating all real world use cases up front. Or even in the course of development of a custom op store, it's quite useful to have a simple-as-possible working reference implementation, both as guidance and also as something testable, so you can A/B test to find if the problem is in your own implementation. The more complex the reference implementation, the less useful it is.
When you do please let me know! Until then I'll keep using the only op store that exists. No further reply needed unless it's about wanting to use this, otherwise feel free to close. |
WalkthroughThis change refactors the in-memory operation store to use a generic, trait-based abstraction for operations, replacing the hardcoded Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Factory as MemOpStoreFactory<TestMemoryOp>
participant Store as Kitsune2MemoryOpStore<TestMemoryOp>
participant Op as TestMemoryOp
Test->>Factory: create()
Factory->>Store: instantiate generic store
Test->>Store: process_incoming_ops(Vec<Bytes>)
Store->>Op: TestMemoryOp::from_bytes
Op-->>Store: TestMemoryOp instance
Store-->>Test: operation processed
Test->>Store: retrieve_ops(...)
Store->>Op: TestMemoryOp::to_bytes
Store-->>Test: Vec<Bytes> (serialized ops)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/gossip/src/respond/accept.rs (1)
357-360: AvoidTimestamp::now()to keep tests deterministicUsing wall-clock time can introduce rare, timing-dependent failures (e.g., if two ops end up with identical timestamps on fast machines or when tests run close to a second boundary).
- let op = TestMemoryOp::new(Timestamp::now(), vec![i; op_size]); + let op = TestMemoryOp::new(Timestamp::from_micros(1_000 + i as i64), vec![i; op_size]);crates/core/src/factories/mem_op_store.rs (2)
109-132: Remove duplicate method implementationsThese methods duplicate the trait implementation without adding value. Since
TestMemoryOpalready implementsMemoryOptrait with public methods, these wrapper methods are redundant.Apply this diff to remove the duplication:
-impl TestMemoryOp { - /// Deserialize a [TestMemoryOp] from bytes. - pub fn from_bytes(bytes: Bytes) -> Self { - MemoryOp::from_bytes(bytes).unwrap() - } - - /// Serialize a [TestMemoryOp] to bytes. - pub fn to_bytes(&self) -> Bytes { - MemoryOp::to_bytes(self).unwrap() - } - - /// Compute the op id for this op. - /// - /// Note that this produces predictable op ids for testing purposes. - /// It is simply the first 32 bytes of the op data. - pub fn compute_op_id(&self) -> OpId { - MemoryOp::compute_op_id(self) - } - - /// Get the creation timestamp of the op. - pub fn created_at(&self) -> Timestamp { - self.created_at - } -} - impl TestMemoryOp {If these methods are needed for backward compatibility, consider documenting that rationale.
289-291: Update error message to reflect generic implementationThe error message references the old type name which no longer exists.
- K2Error::other("Failed to deserialize op data, are you using `Kitsune2MemoryOp`s?") + K2Error::other(format!("Failed to deserialize op data: {}", e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
crates/core/src/factories/core_bootstrap.rs(1 hunks)crates/core/src/factories/core_fetch/test.rs(2 hunks)crates/core/src/factories/core_fetch/test/incoming_request_queue.rs(3 hunks)crates/core/src/factories/core_fetch/test/incoming_response_queue.rs(4 hunks)crates/core/src/factories/core_fetch/test/outgoing_request_queue.rs(1 hunks)crates/core/src/factories/core_publish/test.rs(4 hunks)crates/core/src/factories/mem_op_store.rs(10 hunks)crates/core/src/factories/mem_op_store/test.rs(4 hunks)crates/core/src/lib.rs(2 hunks)crates/core/tests/fetch.rs(2 hunks)crates/dht/src/dht/tests.rs(15 hunks)crates/dht/src/dht/tests/harness.rs(3 hunks)crates/dht/src/hash.rs(2 hunks)crates/dht/src/test.rs(2 hunks)crates/dht/src/time.rs(13 hunks)crates/gossip/src/gossip.rs(8 hunks)crates/gossip/src/harness.rs(3 hunks)crates/gossip/src/harness/op_store.rs(3 hunks)crates/gossip/src/respond/accept.rs(2 hunks)crates/gossip/src/respond/disc_sector_details_diff.rs(2 hunks)crates/gossip/src/respond/disc_sector_details_diff_response.rs(2 hunks)crates/gossip/src/respond/initiate.rs(2 hunks)crates/gossip/src/respond/ring_sector_details_diff.rs(2 hunks)crates/gossip/src/respond/ring_sector_details_diff_response.rs(2 hunks)crates/gossip/src/respond/tests.rs(5 hunks)crates/gossip/tests/historical_load.rs(2 hunks)crates/gossip/tests/initial_sync.rs(3 hunks)crates/kitsune2/src/lib.rs(2 hunks)crates/kitsune2/tests/integration.rs(2 hunks)crates/kitsune2_showcase/src/app.rs(4 hunks)crates/kitsune2_showcase/src/app/file_op_store.rs(3 hunks)crates/transport_tx5/src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: cdunster
PR: holochain/kitsune2#290
File: crates/core/src/factories/mem_peer_store/test.rs:317-346
Timestamp: 2025-08-06T07:54:53.186Z
Learning: The `Inner::remove` method in `mem_peer_store` returns `Option<Arc<AgentInfoSigned>>`, while the `PeerStore::remove` trait method returns `BoxFut<'_, K2Result<()>>`. The `Inner::remove` method is called directly on `Inner` instances in tests.
📚 Learning: the `inner::remove` method in `mem_peer_store` returns `option>`, while the `pe...
Learnt from: cdunster
PR: holochain/kitsune2#290
File: crates/core/src/factories/mem_peer_store/test.rs:317-346
Timestamp: 2025-08-06T07:54:53.186Z
Learning: The `Inner::remove` method in `mem_peer_store` returns `Option<Arc<AgentInfoSigned>>`, while the `PeerStore::remove` trait method returns `BoxFut<'_, K2Result<()>>`. The `Inner::remove` method is called directly on `Inner` instances in tests.
Applied to files:
crates/dht/src/dht/tests.rscrates/gossip/src/respond/ring_sector_details_diff_response.rscrates/gossip/src/respond/disc_sector_details_diff_response.rscrates/gossip/src/respond/disc_sector_details_diff.rscrates/gossip/src/respond/accept.rscrates/gossip/src/respond/initiate.rscrates/core/src/factories/core_fetch/test/incoming_request_queue.rscrates/dht/src/hash.rscrates/kitsune2/src/lib.rscrates/gossip/src/respond/ring_sector_details_diff.rscrates/gossip/src/respond/tests.rscrates/gossip/src/gossip.rscrates/core/src/factories/core_publish/test.rscrates/dht/src/time.rscrates/kitsune2_showcase/src/app.rscrates/gossip/tests/initial_sync.rscrates/core/src/lib.rscrates/core/src/factories/core_fetch/test/incoming_response_queue.rscrates/dht/src/dht/tests/harness.rscrates/core/tests/fetch.rscrates/gossip/src/harness.rscrates/dht/src/test.rscrates/gossip/src/harness/op_store.rscrates/core/src/factories/mem_op_store/test.rscrates/kitsune2_showcase/src/app/file_op_store.rscrates/core/src/factories/mem_op_store.rs
📚 Learning: in the kitsune2 codebase, for test-only functionality that won't be exposed as public api, direct ac...
Learnt from: cdunster
PR: holochain/kitsune2#290
File: crates/core/src/factories/mem_blocks/test.rs:46-46
Timestamp: 2025-08-05T13:46:15.678Z
Learning: In the Kitsune2 codebase, for test-only functionality that won't be exposed as public API, direct access to internal implementation details (like accessing internal mutex with .0.lock()) is acceptable when the implementation is unlikely to change, rather than adding public methods solely for testing purposes.
Applied to files:
crates/dht/src/dht/tests.rscrates/gossip/src/respond/ring_sector_details_diff_response.rscrates/gossip/src/respond/disc_sector_details_diff_response.rscrates/gossip/src/respond/accept.rscrates/gossip/src/respond/initiate.rscrates/dht/src/hash.rscrates/kitsune2/src/lib.rscrates/gossip/src/respond/tests.rscrates/gossip/src/gossip.rscrates/kitsune2/tests/integration.rscrates/core/src/factories/core_fetch/test.rscrates/dht/src/time.rscrates/kitsune2_showcase/src/app.rscrates/gossip/tests/initial_sync.rscrates/core/src/factories/core_fetch/test/incoming_response_queue.rscrates/dht/src/dht/tests/harness.rscrates/core/tests/fetch.rscrates/gossip/src/harness.rscrates/dht/src/test.rscrates/core/src/factories/mem_op_store/test.rscrates/kitsune2_showcase/src/app/file_op_store.rscrates/core/src/factories/mem_op_store.rs
🧬 Code Graph Analysis (4)
crates/core/src/factories/core_fetch/test/outgoing_request_queue.rs (1)
crates/core/src/factories/core_fetch/test.rs (1)
make_op(16-18)
crates/kitsune2/tests/integration.rs (2)
crates/api/src/timestamp.rs (1)
from_micros(36-38)crates/test_utils/src/lib.rs (1)
random_bytes(32-37)
crates/dht/src/dht/tests/harness.rs (1)
crates/core/src/factories/mem_op_store.rs (4)
from(145-148)from(152-156)from(192-197)from(201-203)
crates/core/tests/fetch.rs (5)
crates/core/src/lib.rs (2)
default_test_builder(154-171)new(71-82)crates/kitsune2/tests/integration.rs (1)
create_op_list(25-36)crates/gossip/src/harness.rs (1)
ops(143-150)crates/core/src/factories/mem_op_store.rs (3)
new(136-141)new(181-188)new(237-242)crates/test_utils/src/lib.rs (1)
random_bytes(32-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: static
🔇 Additional comments (59)
crates/core/src/factories/core_bootstrap.rs (1)
83-98: Enhanced error messaging with dynamic URL context.This improvement includes the actual
server_urlvalue in error messages, making debugging URL parsing and validation failures much more informative.crates/transport_tx5/src/lib.rs (1)
36-39: Improved error context for SigUrl parsing failures.Including the actual input string in the error message provides better diagnostics for debugging URL parsing issues.
crates/gossip/src/respond/initiate.rs (2)
187-187: Updated import to use TestMemoryOp for test operations.This change aligns with the broader refactor where
MemoryOpwas generalized to a trait withTestMemoryOpas the concrete test implementation.
612-612: Consistent usage of TestMemoryOp in test code.The operation creation now uses
TestMemoryOp::newwhich is consistent with the new type system where test operations use the dedicated test implementation.crates/gossip/src/respond/ring_sector_details_diff_response.rs (2)
195-195: Updated import to use TestMemoryOp for test operations.Consistent with the refactor where
MemoryOpbecame a trait andTestMemoryOpserves as the concrete test implementation.
271-271: Consistent usage of TestMemoryOp in test code.The operation instantiation now uses
TestMemoryOp::newwhich aligns with the new generic operation store design.crates/dht/src/hash.rs (2)
456-456: Updated import to use TestMemoryOp for test operations.This change is consistent with the systematic refactor where
MemoryOpwas generalized to a trait withTestMemoryOpas the concrete test implementation.
651-651: Consistent usage of TestMemoryOp in test code.The operation creation now uses
TestMemoryOp::newwhich maintains consistency with the new generic operation store architecture.crates/gossip/src/respond/disc_sector_details_diff.rs (2)
200-201: Import rename aligns tests with new generic op abstractionSwitching to
TestMemoryOpkeeps the test compiling after the store-generic refactor. Nothing else to flag here.
247-249: Good use of deterministic timestamp in test op creationUsing
Timestamp::from_micros(100 + i)produces stable, reproducible test data – much better than relying onTimestamp::now().crates/gossip/src/respond/accept.rs (1)
325-326: Import rename acknowledged
kitsune2_core::factories::TestMemoryOpis the correct concrete type after the refactor.crates/gossip/src/respond/disc_sector_details_diff_response.rs (2)
188-189: Import update looks correctNo issues detected with bringing
TestMemoryOpinto the test module.
258-260: Deterministic timestamp – goodKeeping the timestamp calculation deterministic maintains test repeatability.
crates/gossip/src/respond/ring_sector_details_diff.rs (2)
237-238: Import update toTestMemoryOpis appropriateChange is consistent with the new generic op store.
301-305: 👍 deterministic, boundary-relative timestampsBasing the timestamp on
disc_boundaryrather thannow()keeps tests reproducible and meaningful.crates/core/src/factories/core_fetch/test/outgoing_request_queue.rs (1)
4-7: LGTM - Clean import reorganizationThe consolidation of factory-related imports improves code organization by grouping related modules together.
crates/dht/src/test.rs (2)
6-8: LGTM - Updated import for generic refactorThe addition of
TestMemoryOpto the imports aligns with the generic memory op store abstraction.
32-32: LGTM - Explicit generic type parameter specificationThe explicit specification of
TestMemoryOpas the generic parameter is required for the new generic factory design and is appropriate for test code.crates/gossip/tests/historical_load.rs (2)
6-6: LGTM - Updated import for test operation typeThe addition of
TestMemoryOpimport supports the transition to the generic memory op store abstraction.
59-59: LGTM - Consistent use of TestMemoryOpThe change from
MemoryOp::newtoTestMemoryOp::newaligns with the generic refactor while maintaining identical test behavior.crates/gossip/src/gossip.rs (2)
448-448: LGTM - Updated test importsThe addition of
TestMemoryOpimport supports the generic memory op store refactor while maintaining access to both types as needed.
527-728: LGTM - Systematic TestMemoryOp adoptionThe consistent replacement of
MemoryOp::newwithTestMemoryOp::newacross all test functions maintains identical test behavior while adapting to the new generic operation abstraction.crates/dht/src/dht/tests.rs (2)
6-6: LGTM - Updated DHT test importsThe addition of
TestMemoryOpimport enables the transition to the generic memory op store while maintaining compatibility.
24-515: LGTM - Comprehensive TestMemoryOp integrationThe systematic replacement of
MemoryOp::newwithTestMemoryOp::newacross all DHT test functions is well-executed and maintains identical test semantics while supporting the generic operation abstraction. The changes are consistent and comprehensive across the entire test suite.crates/gossip/src/respond/tests.rs (2)
19-19: LGTM! Import updated correctly.The import has been correctly updated to use
TestMemoryOpas part of the generic operation store refactor.
56-56: LGTM! Operation construction updated consistently.All instances of
MemoryOp::newhave been correctly replaced withTestMemoryOp::new. The test logic remains unchanged, which is appropriate for this refactor.Also applies to: 162-162, 280-280, 401-401
crates/core/src/factories/core_publish/test.rs (2)
4-4: LGTM! Import updated correctly.The import has been correctly updated to include
TestMemoryOpas part of the generic operation store refactor.
37-37: LGTM! Operation construction updated consistently.All instances of
MemoryOp::newhave been correctly replaced withTestMemoryOp::new. The test functionality remains unchanged, which is appropriate for this refactor.Also applies to: 39-39, 90-90, 92-92, 321-321
crates/dht/src/time.rs (2)
721-721: LGTM! Import updated correctly.The import has been correctly updated to use
TestMemoryOpin the test module as part of the generic operation store refactor.
900-910: LGTM! Test operation construction updated consistently.All test functions have been correctly updated to use
TestMemoryOp::newinstead ofMemoryOp::new. The test logic and functionality remain unchanged, which is appropriate for this refactor.Also applies to: 985-987, 1035-1047, 1098-1104, 1153-1163, 1182-1187, 1219-1221, 1247-1253, 1310-1325, 1396-1401, 1471-1477, 1580-1591
crates/core/src/factories/core_fetch/test.rs (2)
7-7: LGTM! Import updated correctly.The import has been correctly updated to use
TestMemoryOpas part of the generic operation store refactor.
16-17: LGTM! Test utility function updated consistently.The
make_opfunction has been correctly updated to returnTestMemoryOpand useTestMemoryOp::new. This maintains the same functionality while supporting the generic operation store refactor.crates/gossip/tests/initial_sync.rs (2)
6-6: LGTM! Import updated correctly.The import has been correctly updated to include both
MemoryOpandTestMemoryOpas part of the generic operation store refactor.
33-34: LGTM! Test operation construction updated consistently.All instances of
MemoryOp::newin the integration tests have been correctly replaced withTestMemoryOp::new. The sync test functionality and timing remain unchanged, which is appropriate for this refactor.Also applies to: 96-97
crates/core/src/lib.rs (2)
165-165: LGTM! Proper generic type specification.The explicit type parameter
<TestMemoryOp>correctly specifies the concrete operation type for the generic memory operation store factory, aligning with the refactoring to makeKitsune2MemoryOpStoregeneric over its op type.
178-178: LGTM! Import correctly added.The import of
TestMemoryOpfrom the factories module supports the usage on line 165 and maintains consistency with the generic abstraction changes.crates/kitsune2/tests/integration.rs (2)
10-10: LGTM! Import correctly updated.The import now includes both the
MemoryOptrait and the concreteTestMemoryOpimplementation, which is appropriate for test code that needs to create specific test operations.
29-31: LGTM! Operation creation updated correctly.The change from
MemoryOp::newtoTestMemoryOp::newcorrectly uses the concrete test implementation while maintaining the same functionality for creating test operations.crates/core/src/factories/core_fetch/test/incoming_response_queue.rs (3)
6-6: LGTM! Import updated correctly.The import now includes
TestMemoryOpalongside the trait import, supporting the usage in the test functions below.
96-99: LGTM! Test operation creation updated.The changes from
MemoryOp::newtoTestMemoryOp::newcorrectly use the concrete test implementation while maintaining the same test logic and functionality.
159-159: LGTM! Consistent operation creation updates.Both instances correctly change from
MemoryOp::newtoTestMemoryOp::new, maintaining consistency with the refactoring across all test functions in the file.Also applies to: 234-234
crates/core/src/factories/core_fetch/test/incoming_request_queue.rs (3)
5-5: LGTM! Import updated for generic factory usage.The import now includes
MemOpStoreFactoryandTestMemoryOp, which are needed for the explicit generic instantiation and type annotations used in the test code.
31-31: LGTM! Generic factory instantiation.The explicit type parameter
<TestMemoryOp>correctly specifies the concrete operation type for the generic memory operation store factory, ensuring type safety in the test setup.
223-223: LGTM! Explicit type annotation for clarity.The explicit type annotation
Vec<TestMemoryOp>makes the expected operation type clear and ensures type safety when processing fetch responses in the test code.crates/dht/src/dht/tests/harness.rs (3)
8-8: LGTM! Import updated correctly.The import change from
MemoryOptoTestMemoryOpaligns with the refactoring to use the concrete test implementation in the DHT test harness.
52-52: LGTM! Method parameter type updated.The parameter type change from
MemoryOptoTestMemoryOpcorrectly uses the concrete test implementation while maintaining the same functionality for injecting operations into the DHT test harness.
419-419: LGTM! Type conversion updated consistently.The change from
MemoryOp::from(op)toTestMemoryOp::from(op)correctly uses the concrete test implementation's conversion method while maintaining the same functionality for transferring operations between DHT instances.crates/gossip/src/harness.rs (1)
11-11: LGTM! Consistent refactor from MemoryOp to TestMemoryOp.The import, function signature, and conversion logic are all properly updated to use the new
TestMemoryOptype. This aligns with the PR's goal of making the memory op store generic.Also applies to: 120-120, 146-146
crates/gossip/src/harness/op_store.rs (1)
66-66: LGTM! Proper usage of TestMemoryOp in conversions.The conversions correctly use
TestMemoryOpfor deserializing operation data and reconstructingMetaOpinstances, consistent with the refactoring goals.Also applies to: 172-175
crates/kitsune2_showcase/src/app.rs (1)
7-10: LGTM! Consistent usage of TestMemoryOp throughout the application.The changes properly update the showcase app to use
TestMemoryOpfor operation creation, conversion, and deserialization. The import correctly includes bothMemoryOp(likely the trait) andTestMemoryOp(the concrete implementation).Also applies to: 278-278, 352-352, 394-394
crates/core/tests/fetch.rs (1)
2-3: LGTM! Proper test updates for the generic refactor.The test correctly updates to use
TestMemoryOpwhile maintaining the same test logic. The function signature and operation creation are consistent with the new type system.Also applies to: 21-21, 25-25
crates/core/src/factories/mem_op_store/test.rs (1)
1-2: Excellent demonstration of the generic memory op store refactor.The test file properly showcases the new generic
Kitsune2MemoryOpStore<TestMemoryOp>usage while maintaining all existing test logic. The updates are comprehensive and consistent:
- Helper functions updated to use
TestMemoryOp- Store instances properly parameterized with
TestMemoryOp- Deserialization correctly targets
TestMemoryOpThis demonstrates the successful transition from hardcoded types to the generic trait-based system.
Also applies to: 7-7, 12-12, 18-18, 20-20, 31-32, 39-39, 60-60, 90-90
crates/kitsune2_showcase/src/app/file_op_store.rs (3)
7-9: LGTM!The imports correctly reference the new generic types and
TestMemoryOpimplementation.
19-19: LGTM!The generic parameter is properly specified as
TestMemoryOpand initialization follows the new pattern correctly.Also applies to: 26-26
77-78: LGTM!The conversion correctly uses the generic
MemoryOpRecord::<TestMemoryOp>::new()constructor and the trait methodto_bytes().crates/core/src/factories/mem_op_store.rs (4)
25-35: LGTM!The generic implementation with
PhantomDatais the correct pattern for zero-cost type parameterization.
79-107: LGTM!The
TestMemoryOpcorrectly implements theMemoryOptrait with appropriate error handling usingInfalliblefor test scenarios.
206-224: Well-designed trait abstractionThe
MemoryOptrait provides a clean abstraction with appropriate bounds and flexibility through the associatedErrortype. This enables the generic implementation while maintaining type safety.
271-524: LGTM!The
OpStoreimplementation correctly uses trait methods throughout, properly handling serialization/deserialization and maintaining the generic abstraction.
| @@ -3,6 +3,7 @@ use kitsune2_api::{ | |||
| BoxFut, Builder, Config, DhtArc, DynOpStore, K2Error, K2Result, MetaOp, | |||
| OpId, OpStore, OpStoreFactory, SpaceId, Timestamp, | |||
| }; | |||
| use kitsune2_core::factories::MemoryOp; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify import consistency with the refactoring.
The import shows MemoryOp while the actual usage in lines 66 and 172 uses TestMemoryOp. Please confirm whether this import should be TestMemoryOp for consistency with the rest of the codebase.
🏁 Script executed:
#!/bin/bash
# Check what MemoryOp refers to in the factories module
rg "pub.*MemoryOp" crates/core/src/factories/
rg "pub.*TestMemoryOp" crates/core/src/factories/Length of output: 895
We need more context from op_store.rs to confirm whether the import of MemoryOp (trait) is intentional or a typo for TestMemoryOp (struct). Let’s inspect the file to see how MemoryOp is used versus where TestMemoryOp is constructed.
🏁 Script executed:
#!/bin/bash
set -e
# Show imports and usages in op_store.rs
echo "=== Imports and first 50 lines ==="
sed -n '1,50p' crates/gossip/src/harness/op_store.rs
echo -e "\n=== Usages of MemoryOp ==="
rg -n "MemoryOp" crates/gossip/src/harness/op_store.rs || true
echo -e "\n=== Usages of TestMemoryOp ==="
rg -n "TestMemoryOp" crates/gossip/src/harness/op_store.rs || trueLength of output: 3287
Remove the unused MemoryOp import in op_store.rs
The use kitsune2_core::factories::MemoryOp; on line 6 isn’t referenced by any trait bounds or code in this module—only TestMemoryOp is actually used (via its full path). To keep imports accurate:
- Delete line 6:
- use kitsune2_core::factories::MemoryOp; - (Optional) If you prefer, you can import
TestMemoryOpfor readability:and then update the two fully‐qualified uses at lines 66 and 172 to justuse kitsune2_core::factories::TestMemoryOp;
TestMemoryOp.
🤖 Prompt for AI Agents
In crates/gossip/src/harness/op_store.rs at line 6, remove the unused import
statement for MemoryOp since it is not referenced anywhere in the module.
Optionally, add an import for TestMemoryOp to improve readability and update its
fully-qualified usages at lines 66 and 172 to just TestMemoryOp.
| factories::{self, MemOpStoreFactory, TestMemoryOp}, | ||
| Ed25519Verifier, |
There was a problem hiding this comment.
Exporting test-only op type from public API is risky
TestMemoryOp lives in the factories test helpers; wiring it into the public default_builder() makes that test struct effectively part of the production API. Down-stream crates pulling Kitsune2 will now depend on a type that is explicitly “for tests”.
-use kitsune2_core::{
- factories::{self, MemOpStoreFactory, TestMemoryOp},
+use kitsune2_core::{
+ factories::{self, MemOpStoreFactory},And later:
- op_store: MemOpStoreFactory::<TestMemoryOp>::create(),
+ // Use the generic mem-op store with the default in-crate op type.
+ // Consider exposing a `ProdMemoryOp` or require callers to specify their own.
+ op_store: MemOpStoreFactory::create(),Alternatively, make default_builder() generic over the op type so production code can choose a suitable concrete type without importing test utilities.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| factories::{self, MemOpStoreFactory, TestMemoryOp}, | |
| Ed25519Verifier, | |
| // crates/kitsune2/src/lib.rs | |
| use kitsune2_core::{ | |
| factories::{self, MemOpStoreFactory}, | |
| Ed25519Verifier, | |
| }; | |
| pub fn default_builder() -> Builder { | |
| Builder::new() | |
| // Use the generic mem-op store with the default in-crate op type. | |
| // Consider exposing a `ProdMemoryOp` or require callers to specify their own. | |
| .op_store(MemOpStoreFactory::create()) | |
| .verifier(Ed25519Verifier::new()) | |
| } |
🤖 Prompt for AI Agents
In crates/kitsune2/src/lib.rs around lines 17 to 18, the issue is that the
test-only type TestMemoryOp is being exported and used in the public API via
default_builder(), which exposes test utilities to production code. To fix this,
remove TestMemoryOp from the public exports and refactor default_builder() to be
generic over the operation type, allowing production code to specify its own
concrete type without depending on test-only types.
| op_store: MemOpStoreFactory::<TestMemoryOp>::create(), | ||
| peer_meta_store: factories::MemPeerMetaStoreFactory::create(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Follow-up: require callers to supply an op type or provide a prod-ready default
Continuing from the above, consider:
- Adding
pub struct DefaultOpinsidekitsune2_coremeant for real usage. - Or make
default_builder()generic:
pub fn default_builder<O: MemoryOp + 'static>() -> Builder { … }This prevents accidental leakage of test-only code into production binaries.
🤖 Prompt for AI Agents
In crates/kitsune2/src/lib.rs around lines 50 to 51, the current code uses a
test-only operation type directly, which risks leaking test code into
production. To fix this, either define a public DefaultOp struct in
kitsune2_core for production use or make the default_builder function generic
over an operation type parameter constrained by the MemoryOp trait. This ensures
callers explicitly specify the operation type or use a proper production
default, preventing accidental inclusion of test-only types.
I'm going to try integrating kitsune2 into a project as a standalone, with no Holochain. So far so good, but here is a change I found helpful.
Kitsune2MemoryOpStoreis useful for testing other kitsune hosts, but it is written to be used withMemoryOpwhich is clearly for testing purposes -- itsOpIdimplementation is intentionally not robust, making it unsuitable for real-world situations. Aside from that, host implementers will have written their own custom op types, and having to convert toMemoryOpisn't great. A better pattern is that any reusable OpStore implementation should be abstract over its op data.I defined a trait that has the key elements necessary for working with your mem op store implementation:
created_attimestamp, op id calculation, and serialization. This lets users more quickly get up and running withKitsune2MemoryOpStore, which as far as I know is the only working OpStore implementation in the world aside from Holochain's. Without this, your first attempts at hooking up your own custom op type likely result in confusing deserialization errors, and in my case, discovering that I got the same OpId for every op due to the first 32 bytes being the same.If you don't want to use this that's fine, but I will be using this in my branch.
Summary by CodeRabbit
Bug Fixes
Refactor
Style
Documentation