-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: [MR-603] Typed canister queues and references #1697
Conversation
…eadlineExpired and ResponseDropped.
…ed-inbound-responses
…ed-inbound-responses
…lbacks_with_enqueued_response invariant in CanisterQueues doc comment.
…ed-inbound-responses
…ry reject response that will result in a critical eror anyway (and the response being dropped).
… as a dummy canister ID.
Assign type parameters to canister queues and references, to designate them as either input/inbound or output/outbound. Ensures that input queues can only hold inbound references; and output queues can only hold outbound references. Implements separate logic (for inbound and outbound references) for determining staleness, lookup and removal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a first quick pass to wrap my head around how things are intended to work together. I really like the change!
I left some preliminary comments and will follow up with a more detailed pass.
… the two specific `MessageStore<_>` implementations into `MessageStoreImpl`.
…nt when decoding CanisterQueue from proto.
…(but still CanisterInput by value for input queues).
…d to InboundReference / OutboundReference, to make it clear that the functions may panic (even though the two types are essentially private to the module).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this more robust. I did a first pass over the "meat", though not the tests yet. I wonder:
- can we make the context the property of the
T
type inReference<T>
. That would seem to get rid of a bunch ofasserts
that we currently have - do we even need
Reference::context
to be visible from outside of message_pool? I.e., outside ofTryFrom<QueueItem>
?
…e item types declare their own context (inbound vs outbound) and use that when constructing a new reference of the given type.
MR-603: Assign type parameters to canister queues and references, to designate them as either input/inbound or output/outbound.
Ensures that input queues can only hold inbound references; and output queues can only hold outbound references. Implements separate logic (for inbound and outbound references) for determining staleness, lookup and removal.