Skip to content
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

How many uses of makeNotifierKit should use makeSubscriptionKit instead? #3784

Open
erights opened this issue Sep 1, 2021 · 6 comments
Open
Assignees
Labels
bug Something isn't working code-style defensive correctness patterns; readability thru consistency Core Economy OBSOLETE in favor of INTER-protocol

Comments

@erights
Copy link
Member

erights commented Sep 1, 2021

Outside of the notifier package, and test and demo directories, it looks like we have 34 calls to makeNotifierKit in agoric-sdk. With those same filters, we currently have 1 call to makeSubscriptionKit. This may or may not be a good split --- it depends on the semantics of the notifications being pushed. Fortunately, 35 is small enough to inspect. From

  • the general familiarity with notifiers
  • the general unfamiliarity with subscriptions
  • the very dangerous misfeature that a notifier will often happen to accidentally work when a subscription is called for

I suspect that some of these notifier uses really should be subscription uses. The example which made me aware of this is represented well by #3783 . The code it corrects happened to work with a notifier by accident. #3783 demonstrates how painless it is to convert code from using a notifier to using a subscription. Please take a look.

@erights erights added the bug Something isn't working label Sep 1, 2021
@Chris-Hibbert Chris-Hibbert self-assigned this Sep 1, 2021
@zarutian
Copy link
Contributor

zarutian commented Sep 2, 2021

Defenitely Notifiers on ERTP purses shoulde Subscriptions instead, specially on Zoe fee purses of running contract vats. The latter for fuel gauge alarm reasons alone.

@erights
Copy link
Member Author

erights commented Sep 2, 2021

#3756 (comment)

@dckc dckc added the code-style defensive correctness patterns; readability thru consistency label Oct 8, 2021
@Tartuffo Tartuffo added the Core Economy OBSOLETE in favor of INTER-protocol label Jan 31, 2022
@Tartuffo
Copy link
Contributor

@Chris-Hibbert This needs an area label to get picked up the right Wed planning meeting. This looked like Core Economy, please change if that is not right.

@dtribble
Copy link
Member

dtribble commented Feb 8, 2022

A general note is that notifiers should be used only to report states, not transitions. Thus dropping one update doesn't prevent getting the latest state update. To use notifiers to communicate events, the update for a getUpdateSince(x) must include all the events since the specified time x or the "state" should be the entire list of events.

@erights
Copy link
Member Author

erights commented Feb 8, 2022

To use notifiers to communicate events,

or, if you are communicating events, use subscriptions rather than notifiers. If subscriptions are not better at events than notifiers, then subscriptions should either be fixed or retired.

@erights
Copy link
Member Author

erights commented Dec 24, 2022

Hi @gibson042 , I just added you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working code-style defensive correctness patterns; readability thru consistency Core Economy OBSOLETE in favor of INTER-protocol
Projects
None yet
Development

No branches or pull requests

7 participants