-
Notifications
You must be signed in to change notification settings - Fork 128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a large PR and going forward, we really should have much smaller incremental PRs. There's also a lot of mocking going on here (which makes me feel extremely un-easy). The maintenance burden for this is going to be non-trivial. Not exactly sure how to deal with that in the context of this PR though.
yield takeLatest('NETWORK_ID_FETCHING', getNetworkId) | ||
yield takeEvery('SEND_WEB3_TX', callSendTx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure where this went? Looks like it got removed, but shouldn't it have been moved to another place? What was the reason for its removal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Changed "it's" to "its".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@honestbonsai The SEND_WEB3_TX
action led to a code path that wasn't being utilized. I removed it because it touched the tests for web3Saga. I'd like to consider this resolved and leave it up to you to comment further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more change requests relating to style and naming.
merge it! after fixing the conflicts |
For some reason the mocked function's instrumentation isn't updated when passed into a generator. The work around it so monitor the generator yielded values to determine if the mocked Function is utilized as a `call` (vs `put`) saga side-effect.
4a7be5d
to
f211093
Compare
Built on @DiscRiskandBisque's testing work.
It should merge automatically after we zap
package-lock.json
on masterN.B. I removed web3Saga's
callSendTx
andcreateTxChannel
. Please do a manual integration test to make sure all remains copacetic.