Skip to content

refactor: define system-wide Event types and use in homeserver and SDK#302

Open
86667 wants to merge 6 commits intosdk_version_0.7.0from
common_event_types
Open

refactor: define system-wide Event types and use in homeserver and SDK#302
86667 wants to merge 6 commits intosdk_version_0.7.0from
common_event_types

Conversation

@86667
Copy link
Contributor

@86667 86667 commented Feb 5, 2026

This should have been like this from initial impl: Api consumer types in pubky-common module.

changes in SDK are due to:

  1. Wasm not liking Rust enums
  2. base64 decoding into Hash object instead of using String

@86667 86667 force-pushed the common_event_types branch 2 times, most recently from 061d89d to 5df5d11 Compare February 5, 2026 10:45
@86667 86667 requested review from SHAcollision, SeverinAlexB, afterburn and dzdidi and removed request for SeverinAlexB February 10, 2026 04:26
@86667 86667 force-pushed the common_event_types branch from c16587c to bf09632 Compare February 23, 2026 08:36
@86667 86667 changed the base branch from main to sdk_version_0.7.0 February 23, 2026 08:39
@dzdidi dzdidi requested a review from Copilot February 23, 2026 09:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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.rs module defining EventType (enum with Put { content_hash: Hash } and Delete variants) and EventCursor types
  • 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.

Copy link
Contributor

@dzdidi dzdidi left a comment

Choose a reason for hiding this comment

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

Some nits. But I would wait for SDK review from @SHAcollision

Copy link
Contributor

@SHAcollision SHAcollision left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I just left a few minor questions.

EventType::Put => "PUT".to_string(),
EventType::Delete => "DEL".to_string(),
}
pub fn event_type(&self) -> EventType {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the SDK using the types defined in pubky-common to brings consistency across codebases.

Copy link
Contributor

@SHAcollision SHAcollision Mar 4, 2026

Choose a reason for hiding this comment

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

It doesn't answer the question of whether JS consumer should see the complex types or not. I guess it's okay?

Copy link
Contributor Author

@86667 86667 Mar 4, 2026

Choose a reason for hiding this comment

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

Thanks for calling that out. Ive taken another look and agree that i was thinking too Rusty for JS. Ive removed these more complex types going back to string for cursor and eventType

@86667 86667 force-pushed the sdk_version_0.7.0 branch 2 times, most recently from dff7abe to 680b71b Compare March 4, 2026 09:38
@86667 86667 force-pushed the common_event_types branch from 2da8472 to 390af98 Compare March 4, 2026 11:57
@86667 86667 force-pushed the sdk_version_0.7.0 branch 4 times, most recently from a979acf to 789131e Compare March 4, 2026 14:04
@86667 86667 force-pushed the common_event_types branch from 2cb8e72 to 5bbb8e8 Compare March 4, 2026 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants