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

Client notifications/warnings #6838

Closed
karlb opened this issue Feb 22, 2021 · 8 comments · Fixed by #6871
Closed

Client notifications/warnings #6838

karlb opened this issue Feb 22, 2021 · 8 comments · Fixed by #6871
Assignees

Comments

@karlb
Copy link
Contributor

karlb commented Feb 22, 2021

Describe how the raiden client could pass notifications and warnings to the user.

Some details in #6548

@karlb karlb self-assigned this Feb 22, 2021
@karlb
Copy link
Contributor Author

karlb commented Feb 22, 2021

I suggest a single endpoint where notification messages can be delivered from the Raiden client to the UI. Pushing the messages would be ideal (websockets? long-polling?) but frequent polling should also be sufficient. Each message would either create a notification

{
  "notification_id": "udc-too-low",
  "summary": "Your UDC balance is too low",
  "body": "You should increase your balance in the User Deposit Contract to allow monitoring of your channels",
  "urgency": "normal",  // either "low", "normal" or "critical"
  "expire_timeout": null  // in seconds, null means "does not expire"
}

or remove an existing notification

{
  "close_notification_id": "udc-too-low"
}

If a notification with the same notification_id already exists, the new notification replaces the old one. This allows updating notifications without spamming the user.

This suggestion is loosely modeled after the XDG desktop notifications, so that document can be consulted for inspiration if extensions to the notification mechanism are necessary (e.g. specifying rich text, actions).

@karlb karlb removed their assignment Feb 22, 2021
@manuelwedler
Copy link
Contributor

I'm all for adding an asynchronous API. That would remove a lot of overhead and solve some problems in the WebUI. But I think it would be more consistent with the current API and be less effort to just use frequent polling. If we want to take the effort this could also be the start for a new version of the API, where we move more functionalities to an asynchronous API. I would prefer WebSockets over long-polling, because we could use one connection for all the communication, also for the client making payment requests for example.

The message format looks good to me. I'm just not sure in what cases we need an expire_timeout. For the WebUI I think it is better to keep the notifications in the notification panel until the user clears it.

Is this also meant to be used for informing about finalized transactions or other actions initialized by the user?

Related WebUI issues:

I closed the old issue #5410 .

@karlb
Copy link
Contributor Author

karlb commented Mar 1, 2021

The message format looks good to me. I'm just not sure in what cases we need an expire_timeout. For the WebUI I think it is better to keep the notifications in the notification panel until the user clears it.

I removed it from my suggestion. We can add it back later, if needed.

Is this also meant to be used for informing about finalized transactions or other actions initialized by the user?

I was thinking only of human readable notifications for this suggestion. For some transactions, we will need machine readable return values (e.g. channel_id after opening a channel). I lean towards leaving that out, but feel free to suggest what that should look like.

@netcriptus netcriptus self-assigned this Mar 3, 2021
@ulope
Copy link
Collaborator

ulope commented Mar 3, 2021

One more thing: I would suggest to not tightly couple the notification system with the API but instead keep the notifications in a central place (for example RaidenService) and only delegate access from the REST-API. That way the notifications can also be accessed from other places, for example the CLI.

@manuelwedler
Copy link
Contributor

manuelwedler commented Mar 3, 2021

I was thinking only of human readable notifications for this suggestion. For some transactions, we will need machine readable return values (e.g. channel_id after opening a channel). I lean towards leaving that out, but feel free to suggest what that should look like.

That makes sense to me. I was just thinking if we start using an asynchronous api for other functionalities we need some way to inform for example about finished transactions. But this should definitely be machine readable and not part of this issue.

So the question is still what notifications / warnings we want to have here. There should be at least these two warnings:

  • UDC balance low
  • ETH balance low

I also suggest to first implement this using the simple polling model as we have it in the other endpoints. Implementing an asynchronous API needs more thorough thoughts. There is this issue for it: #5455

For a simple polling model with GET requests, there cannot be a close message. The notification should just be removed when it is not valid anymore.

@karlb
Copy link
Contributor Author

karlb commented Mar 4, 2021

So the question is still what notifications / warnings we want to have here

Another one is the "Please upgrade" message.

@ezdac
Copy link
Contributor

ezdac commented Mar 4, 2021

Should configuration warnings also be available at the endpoint?
e.g. "Monitoring disabled"

@ulope
Copy link
Collaborator

ulope commented Mar 4, 2021

Should configuration warnings also be available at the endpoint?

I'd say yes.

The question is whether we should allow users to silence certain warnings. But that is probably a feature that can be added later

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 a pull request may close this issue.

5 participants