Repository navigation
refactor: Added initial stub contracts - #9561
rasswanth-s wants to merge 4 commits into
Conversation
pjwerneck
left a comment
pjwerneck
left a comment
There was a problem hiding this comment.
Left a few comments. Main points:
- The
Mailboxcontract has a shape that V2 replaces. A contract with the V2 shape now prevents a second rewrite of every caller. OwnerStorageandWatcherStorageare wrappers that only forward calls toPrivateStore. Two abstract classes give the same separation with one less layer.- Some abstract methods are compositions of other methods. A small set of primitives, plus helper methods on the base class, makes a backend easier to write.
DeliveryTrackerdiffers from the ticket on the first run and on persistence to Drive.
| from syft.sync.version.version_info import VersionInfo | ||
|
|
||
|
|
||
| class Mailbox(ABC): |
There was a problem hiding this comment.
Mailbox here uses list->fetch->ack, but the v2 ticket replaces this with send->receive->ack->delivered. Maybe we can define the v2 shape now and move the callers only once, doing the list and fetch inside received.
| """ | ||
|
|
||
| @abstractmethod | ||
| def list( |
There was a problem hiding this comment.
Why do we have different behavior for PROPOSALS and EVENTS, in the same method? Can't both use the same rules from the start?
| """ | ||
|
|
||
|
|
||
| class PrivateStore(ABC): |
There was a problem hiding this comment.
Why are we combining both peer registry, owner, and watcher in a single PrivateStore class, while OwnerStorage and WatcherStorage only forward each call here? It feels like there's an unnecessary layer here. With two OwnerStore and WatcherStore classes instead of a single PrivateStore, there's no need for the intermediary layer.
| """Every events message in my event log.""" | ||
|
|
||
| @abstractmethod | ||
| def fetch_events_since(self, since_timestamp: float) -> list[bytes]: |
There was a problem hiding this comment.
There's some redundancy in these event log methods. Three primitive methods are enough: append_event, list_event_handles(since) and fetch(handle). The others can be concrete implementations on the base class instead of abstracts to be implemented by the concrete class.
fetch_all_events()isfetch_events_since(None)fetch_events_since(t)isfetchfor each handle fromlist_event_handles(t).count_events_since(t)islen(list_event_handles(t)).
| def delete_rolling_state(self) -> None: | ||
| """Remove the rolling state.""" | ||
|
|
||
| # -- peer registry (SYFT_peers.json) --------------------------------------- |
There was a problem hiding this comment.
The v1 ticket has the peer registry methods in PeeringConnection. It's fine to be in PrivateStore, but the ticket should be updated to avoid confusion.
| """The bytes of one collection file.""" | ||
|
|
||
|
|
||
| class PeerState(ABC): |
There was a problem hiding this comment.
The name PeerState is already used by the enum at syft.sync.peers.peer.PeerState. The docstring warns about it, but it should have a different name.
| """Let ``peer`` read the key bundles I publish.""" | ||
|
|
||
| @abstractmethod | ||
| def read_peer_key_bundle(self, peer: str) -> str | None: |
There was a problem hiding this comment.
This can read the key bundle of a peer, but it cannot read the bundle of this client. The auto encryption settings work (branch pjwerneck/auto-encryption-settings) adds read_own_encryption_bundle to the Drive connection. Please add read_own_key_bundle(peer) here, so the two branches merge without a change to the contract.
| """Let ``peer`` read my version document.""" | ||
|
|
||
|
|
||
| class Transport(BaseModel): |
There was a problem hiding this comment.
The ticket calls this Connections not Transport. Transport is a better name, but the ticket should be updated.
| next sync. | ||
|
|
||
| Open decisions (see research/merged_transport_plan.md): | ||
| - First run with no views: assume current peers hold what they may read |
There was a problem hiding this comment.
The ticket says: Lost view → PeerReset → resend everything. This docstring proposes that if no view exist, the tracker assumes that each peer already holds all files it can read.
This prevents a full resend after the upgrade, but a peer that does not have a file never gets it.
| - First run with no views: assume current peers hold what they may read | ||
| (proposed), or resend everything. | ||
| - A rejected peer approved again: reset its view and resend (proposed). | ||
| - Enclaves lose the views at every boot until they are also kept on Drive. |
There was a problem hiding this comment.
The ticket keeps the views on Drive when persist_owner_state is true. It adds deliveries and peer_resets to RollingState and IncrementalCheckpoint, and peer_views to Checkpoint. Deliveries also count toward the upload threshold of the rolling state.
This stub keeps the views only in a local file (line 32). Is persistence to Drive in v2, or later? If it is later, an enclave loses the views at each boot. Then it sends all files again to each peer, or it sends nothing, as decided for line 21.
This branch has not been deployed
2 participants
Summary
Changes
Testing
Asana task
PR naming convention
Your PR title must follow this format or merging will be blocked:
Example titles:
feat: add retry logic to job approvalfix: handle timeout in notification senderdocs: update syft-bg READMEchore: bump dependenciesrefactor: split init flow into helperstest: add criteria validation testsci: add release train workflowAllowed types:
featfeat: add DS rejection emailsfixfix: handle empty peer listdocsdocs: update syft-bg READMEchorechore: bump dependenciesrefactorrefactor: split init flowtesttest: add approval criteria testscici: add release train workflowperfperf: cache Drive API responsesbuildbuild: pin syft-bg>=0.2.0Just edit the PR title to fix any errors — the check re-runs automatically.
Auto-labeling
Labels are applied automatically — you don't need to add them manually:
feat:addsfeature,fix:addsbugfix)packages/syft-bg/addspkg:syft-bg)These labels are used to auto-generate categorized release notes.