Closed ironng closed 4 years ago
@Shadowfiend I'm thinking about adding tests for the sagas. Thoughts? Not sure if it should be a part of this or in a separate PR.
Not @Shadowfiend, but I'd like it- though the dApp might change drastically the state model, contract interactions, and sagas shouldn't anymore, so it feels like the right time
I think the biggest question I have before we add tests for sagas is simply: are sagas the right tool here? We've discussed how problematic they are to actually debug. This PR adding some error handling helps a little---but also adds try/catch boilerplate in a bunch of places to do it, on top of the existing saga boilerplate. Do we want to continue down this path, or do we want to consider a different solution?
I think adding the redux dev tools has improved the general debugging experience and adding the try/catch blocks here have also helped a lot. We could also add more explicit actions tell a more complete story in the dev tools debugger. However, I'm not a fan of the amount of boilerplate either.
What do you have in mind for an alternative to sagas? redux-thunk? redux-observable?
On a side note, while testing, I noticed that there are some Metamask errors that still seem to elude the new catch blocks added here. I was wondering if if would help if we added some catches to some of the promises in Deposit/Redemption code in tbtc.js, but I haven't tried it yet.
@Shadowfiend @mhluongo, opinions on the above? I'd like to move this to ready to review if we don't plan to write tests just yet.
Oof sorry, I missed this yesterday---I have no opinion or research on alternatives, tbh. This is simply the moment to investigate them if we're going to. Migration later is going to be a big lift and is unlikely to get prioritized even if we're suffering. So I'm just making sure we're asking the question rather than letting momentum carry us forward, while we have the option.
I'm fine with punting on the tests; even if you're confident in sticking with sagas (in which case, let's start writing tests), I think tests can go live in their own PR. This one's sizable for review already.
@Shadowfiend it's true, if we were to switch this would be the right moment to consider it, and I'm glad you are wary of letting momentum carry us forward!
@Shadowfiend Thank you for your review!! Good point about creating a component for rendering error messages to the UI! Thank you for cataloguing each instance! I think I will address all of these in another PR, just because I don't want to block the regression fix by taking too long.
This PR tries to catch most of the errors that can emerge during both the deposit and redemption flows and surface these messages to the UI. While it doesn't tackle any new UX (like a button to retry) yet, hopefully making these errors visible can empower users maybe refresh the page. (New UX will be handled in a separate PR).
There is also a little bit of refactoring here to remove actions living in components that should be state driven. There is a new saga called
onStateRestored
that runs after the deposit state is successfully restored and triggers new actions based on the given deposit state.Lastly, there is a fix (984eb3c and 3176082) here for a regression introduced in #224.
Addresses part 2 of #218.
TODO
fundingTransaction
orbroadcastTransactionId
duringautoSubmit
?)