refactor: define system-wide Event types and use in homeserver and SDK#302
refactor: define system-wide Event types and use in homeserver and SDK#30286667 wants to merge 4 commits intosdk_version_0.7.0from
Conversation
86667
force-pushed
the
common_event_types
branch
2 times, most recently
from
061d89d to
5df5d11
Compare
86667
requested review from
SHAcollision,
SeverinAlexB,
afterburn and
dzdidi
and removed request for
SeverinAlexB
86667
force-pushed
the
common_event_types
branch
from
c16587c to
bf09632
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the event streaming system to consolidate event types (EventType and EventCursor) from individual implementations in the homeserver and SDK into a shared pubky-common module. The refactoring also improves type safety by replacing string-based content hashes with proper Hash objects and base64 encoding for transport.
Changes:
- Created new
pubky-common/src/events.rsmodule definingEventType(enum withPut { content_hash: Hash }andDeletevariants) andEventCursortypes - Updated SDK and homeserver to use the shared types from
pubky-common::events - Modified WASM bindings to wrap the Rust enum in a struct-based API that works with JavaScript
- Updated all tests and examples to use the new API
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pubky-common/src/events.rs |
New shared module defining EventType enum and EventCursor struct with comprehensive tests |
pubky-common/src/lib.rs |
Export the new events module |
pubky-sdk/src/actors/event_stream.rs |
Refactored to use pubky_common::events types, added base64 decoding for content hashes from SSE, removed duplicate type definitions |
pubky-sdk/bindings/js/src/wrappers/event_stream.rs |
WASM-compatible wrappers for EventType and EventCursor, converting Rust enums to struct-based API with helper methods |
pubky-sdk/bindings/js/pkg/tests/events.ts |
Updated TypeScript tests to use new API methods (isPut(), isDelete(), contentHash()) |
pubky-sdk/bindings/js/Cargo.toml |
Added base64 dependency for WASM bindings |
pubky-homeserver/src/persistence/files/events/*.rs |
Updated to import and use types from pubky_common::events instead of local definitions |
pubky-homeserver/src/persistence/files/events/events_entity.rs |
Fixed duplicate variable declaration bug, uses shared EventType and EventCursor |
examples/rust/7-events_stream/main.rs |
Updated to use new EventType.content_hash() API instead of direct field access |
Cargo.lock |
Added base64 dependency for WASM package |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dzdidi
left a comment
dzdidi
left a comment
There was a problem hiding this comment.
Some nits. But I would wait for SDK review from @SHAcollision
| /// Note: Uses `u64` internally, but Postgres BIGINT is signed (`i64`). | ||
| /// sea_query/sqlx binds `u64` values, which works correctly as long as | ||
| /// IDs stay within `i64::MAX` (~9.2 quintillion). Since event IDs are | ||
| /// auto-incrementing from 1, this is not a practical concern. |
There was a problem hiding this comment.
Maybe add validation / error handling so that if id is above i64 but bellow u64 code does not hit DB?
There was a problem hiding this comment.
Adding this is a lot of code. Ive created issue instead - #322
pubky-sdk/src/actors/event_stream.rs
Outdated
| } | ||
|
|
||
| /// Decode a base64-encoded content hash into a Hash. | ||
| /// If the hash is missing or invalid for a PUT event, falls back to zero hash. |
There was a problem hiding this comment.
i am not sure about this but hash is Option already, why not use None as a fallback instead of zero hash?
There was a problem hiding this comment.
Hmm true that this isnt ideal. Ive updated to always expect the content_hash and handle gracefully down the stack with an error log
SHAcollision
left a comment
SHAcollision
left a comment
There was a problem hiding this comment.
Overall LGTM. I just left a few minor questions.
| cross_log!(warn, "Failed to parse SSE event: {}", e); | ||
| Some(Err(e)) | ||
| cross_log!(error, "Failed to parse SSE event, skipping: {}", e); | ||
| None |
There was a problem hiding this comment.
I am not sure if I understand correctly what's going on here. This changes behavior from surfacing parse failures to silently dropping malformed SSE events. That can hide data loss from consumers. Let's either (a) preserve Err emission semantics, or (b) maybe this should be more explicitly documented, maybe we could drop counter/metric so operators can detect malformed upstream events.
If we keep the skip behavior, we could add an inline comment here explaining why malformed events are dropped instead of returned as errors, so future maintainers understand the choice.
| EventType::Put => "PUT".to_string(), | ||
| EventType::Delete => "DEL".to_string(), | ||
| } | ||
| pub fn event_type(&self) -> EventType { |
There was a problem hiding this comment.
eventType and cursor changed from string fields to object wrappers, just wondering if this is intentional and we confirmed this results on an ergonomic improvement rather than a hassle for JS devs.
| /// data: pubky://user_pubkey/pub/example.txt | ||
| /// data: cursor: 42 | ||
| /// data: content_hash: abc123... (optional) | ||
| /// data: content_hash: <base64-encoded-blake3-hash> (required for PUT events) |
There was a problem hiding this comment.
Nice improvement on requiring content_hash for PUT. Could we explicitly state this is base64 of raw 32-byte blake3 digest (not hex) in the public doc comment to prevent confusion?
This should have been like this from initial impl: Api consumer types in
pubky-commonmodule.changes in SDK are due to:
Hashobject instead of usingString