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

[POC] Refactor startup to a finite state machine #2130

Closed
wants to merge 9 commits into from

Conversation

vctt94
Copy link
Member

@vctt94 vctt94 commented May 31, 2019

This PR is a proposal to closes #2121

it is built on top of #2129.

right now our getStartedPage is a bunch of if and else which makes it really complicated to read and understand the logic behind it.

This PR switches how decrediton starts to use a finite state machine instead of if and else.

Right now it only works on advanced mode -> remote, and if approved I will conclude the other methods.

This is the finite state machine diagram I am building:

startdecreditonFSM

And this is the machine state repo I am using: https://github.com/davidkpiano/xstate

I am also adding context which was added at react 16.3: https://reactjs.org/docs/context.html so we don't need to pass props all the way down to others components.

If this gets approved, we can also use this concept to refactor how we are fetching our data. On this part of the doc there is an example of a fetch machine: https://xstate.js.org/docs/guides/states.html#state-meta-data

and with that we would not need to use isLoading all the time, as it is right now, but we can discuss this further on #1558

@matheusd
Copy link
Member

matheusd commented Jun 4, 2019

There's definitely potential in using the xstate lib for reducing complexity. And having it generate the transition diagrams is really nice.

I can see how using it can simplify the important bit in this PR (the GetStarted index page), so I believe the approach is sound. However I have a few things to point out.

  • I'm uncertain context is the right way to pass data downwards the component stack. Redux is there exactly for that, so maybe adding a couple of new connectors would be better than relying on context.

  • The state machine is doing too little at this point. It seems your current approach is to just use it as a representation of the underlying state versus having it actually drive the main app actions (side effects from the PoV of the state machine). You might wanna try moving some of the underlying logic that triggers changes to the app (e.g. onStartWallet, onRetryStartRPC) to the state machine itself and then have the components be really just the presentation layer.

@vctt94
Copy link
Member Author

vctt94 commented Jun 5, 2019

Thanks for reviewing my PR.

Actually I drew this diagram using https://www.draw.io/
I believe it is not possible to draw using xstate lib, but I may be wrong.

about your comments:

  • The reason I am using context, is because I can see react is going the direction to remove redux to control state (they already added context and hooks), but I believe we can left this for another PR to introduce, as this one already add a new concept to decrediton.

  • I agree with you, the reason I am doing this way is to try to keep the POC simple so you guys can see the idea behind it, but I will do as you suggested.

@vctt94 vctt94 force-pushed the refactorStartup branch 2 times, most recently from 199756f to edd0994 Compare July 2, 2019 19:04
@vctt94
Copy link
Member Author

vctt94 commented Jul 2, 2019

I removed context and moved most the logic to the state machine and the code looks cleaner, indeed.
The remote connection in advanced should be working now.

I believe it still has some code to touch, and this PR will get too big.

I can create a branch on my fork, so we can merge this refactors so we don't end with a huge diff. What do you think?

@matheusd
Copy link
Member

matheusd commented Jul 3, 2019

The end result seems reasonable enough for now, I don't think you need to merge into a staging branch.

@vctt94
Copy link
Member Author

vctt94 commented Jul 3, 2019

I am fine by merging here directly. My only concern is that it will break the others startup method till the refactor is finished

@matheusd
Copy link
Member

matheusd commented Aug 6, 2019

Is wallet removal and creation supposed to be broken on this (and on the following PR)? Just asking because I'm testing it out.

The code is fine by itself.

@vctt94
Copy link
Member Author

vctt94 commented Aug 7, 2019

I fixed the wallet removal.

Wallet creation I will fix at #2167 because it is a new state at choosing wallet.

I am also going to update the state machine diagram.

@matheusd
Copy link
Member

matheusd commented Aug 8, 2019

This seems gtg (with the caveat of the broken workflows) so we need to discuss with @alexlyp whether we want this merged right now or only after finishing all the other outstanding work in other PRs.

@vctt94
Copy link
Member Author

vctt94 commented Aug 8, 2019

The problem with having many open PRs at the same time, is that we will have many conflicts when starting merge them.

There is a branch startup_redesign which I believe we can use to merge the PRs in there. What do you think @alexlyp ?

It will only need to rebase against master and I can re-open this PR against it. I think it is better then breaking the startup workflow.

@alexlyp
Copy link
Member

alexlyp commented Aug 8, 2019

So issue being, since there are some outstanding PRs, this will cause conflicts on all of those?

@vctt94
Copy link
Member Author

vctt94 commented Aug 8, 2019

No, it probably doesn't have many conflicts with the outstandings PRs.

The problem is that I believe it will have some PR to be implemented until all the refactor being completed. It already has #2167 implemented against this one, and still have the regular mode and spv mode to be implemented.

@vctt94 vctt94 mentioned this pull request Aug 16, 2019
@vctt94
Copy link
Member Author

vctt94 commented Aug 20, 2019

closing because of #2177

@vctt94 vctt94 closed this Aug 20, 2019
@vctt94 vctt94 deleted the refactorStartup branch October 3, 2019 12:06
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 this pull request may close these issues.

[rpc] Code improvements
3 participants