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

Expose multi-step operations as observables #357

Open
2 tasks
andrevmatos opened this issue Sep 27, 2019 · 3 comments
Open
2 tasks

Expose multi-step operations as observables #357

andrevmatos opened this issue Sep 27, 2019 · 3 comments
Labels
8 discussion 💬 Issues that need discussion and decision taking refactor sdk 🖥

Comments

@andrevmatos
Copy link
Contributor

andrevmatos commented Sep 27, 2019

Description

Currently, the Raiden public class/API exposes all its methods as async methods/Promises. This is ok for one-shot async operations (like getTokenBalance), but not enough for methods which have multiple steps it may be useful to expose to the user.
Example is depositChannel method, which consists in at least 2 steps (approve + setTotalDeposit, possibly more, like mined/confirmed pattern), and more than one of those requires user interaction (tx prompts), which makes it very important ot expose this progression to the user, so an UI can reflect the current state of the operation (by showing the user what they're about to sign).
For these operations which may be relevant to inform the user about task progression, we should return a hot observable instead of a promise, which emits descriptive elements about each step, and completes in the end or errors on failure. Getting back the current behavior then is as simple as calling .toPromise() on the observable.
transfer and most on-chain methods also may make good use of this.

Acceptance criteria

  • Any relevant public method returns a hot Observable, the simplest ones are still Promise based, as both patterns complement each other and play together

Tasks

@nephix
Copy link
Contributor

nephix commented Nov 14, 2019

we should return a hot observable instead of a promise

Might make sense to consider giving users of the SDK an alternative to RxJS observables, by simply allowing them to provide a callback function.

Main concern is that devs get discouraged from trying out the SDK because they will have to learn RxJS first, which has a steep learning curve.

By providing an optional callback we keep it easy to get started, unopinionated and don't force users to implement RxJS, eventhough it has a lot of benefits.

An example of how this could be done can be seen in Google Firebase's onAuthStateChanged method

@christianbrb
Copy link
Contributor

  • We know better in which step the open channel stepper is
  • Hardest part is to decide what approach to take (technology decision)

@kelsos
Copy link
Contributor

kelsos commented Feb 10, 2020

Last week we had a long discussion with @nephix regarding this issue, so I would like to present some of my arguments on this matter.

The biggest problem at the moment is that the API is half-reactive, half promise-based. The functions are exposed as promises but the events etc are exposed as observables. I believe that exposing a fully reactive API should be easy for us to do, and it should not require a lot of extra effort to support while bringing all the benefits reactive programming provides.

However, I can also see the points @nephix raised. I have a reactive extensions background (.NET, Android, JavaScript) and using an API based on observables feels familiar. I can understand however that learning and understanding reactive programming is not easy and forcing the users to understand reactive programming if they want to use the SDK doesn't seem right.

For me, it also doesn't make sense to remove the observable/reactive API completely. It would feel like we are crippling the API. Rather I would argue about finding an elegant way to support both approaches.

Converting to and from promises it should not be hard.

One thing I would argue against is doing something like the onAuthStateChanged that was linked above. In my opinion, this API design seems to put the traditional JavaScript first, while the reactive approach seems mostly like an afterthought.

In my experience with reactive extensions, while subjects were used, their usage was frowned upon. Most of the proper APIs would return an Observable that you could subscribe to. I don't remember any API where the RX support was build by passing subjects instead.

Instead, I think that we should make sure that we have two equal approaches that users of either approach find satisfactory.

@taleldayekh taleldayekh added the 8 label Feb 20, 2020
@christianbrb christianbrb removed this from the Product Backlog milestone Feb 24, 2020
@christianbrb christianbrb added this to the Product Backlog milestone Mar 19, 2020
@taleldayekh taleldayekh added the discussion 💬 Issues that need discussion and decision taking label May 28, 2020
@taleldayekh taleldayekh modified the milestone: Product Backlog May 28, 2020
@christianbrb christianbrb removed this from the Product Backlog milestone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8 discussion 💬 Issues that need discussion and decision taking refactor sdk 🖥
Projects
None yet
Development

No branches or pull requests

5 participants