Confirmation polling on transactions

sparrowDom commented 5 years ago

First pull request? Read our guide to contributing

Checklist:

[x] Code contains relevant tests for the problem you are solving
[ ] Ensure all new and existing tests pass
~[ ] Update any relevant READMEs and docs~

TODO:

in case we decide to merge this, tests still need to be written

Description:

Metamask 4.12.0 introduced some breaking changes where confirmation and receipt events are not always triggered. And sometimes an error is thrown in spite of blockchain transaction succeeding.

The solution this PR proposes:

for easier handling refactor so that contract function calls and contract deploys are handled by the same function
keep confirmation and receipt events present and stop fallback solution in case those events are triggered
when submitting transaction start a fallback pooling function that:
- continually pools for transaction receipt. If it finds it and confirmation event has not been triggered. Manually call confirmation callback
- if transaction receipt has not been returned as promise resolution, mock it and resolve promise with it.
when we receive an error from Metamask that we know is not breaking don't reject the promise. Let fallback function handle resolution.

Related issues:

sparrowDom commented 5 years ago

@micahalcorn the logic was to get some safe margin over the DApps setting of 12 confirmations that are needed for transaction to be confirmed. Perhaps the margin makes no sense and we can set it to 12 here also?

micahalcorn commented 5 years ago

@sparrowDom I would probably assume 24 since thats how many web3 supplies.

franckc commented 5 years ago

Thanks for working on this ! Will review shortly. In the meantime, do you mind looking at those build failures ? it's just the linter complaining.

franckc commented 5 years ago

Adding @tyleryasaka for an extra pair of eyes on this origin-js core change...

sparrowDom commented 5 years ago

@franckc @micahalcorn @tyleryasaka did a bit of refactoring:

send & deploy events are handled by the same function
on("confirmation") event is still registered & alongside fallback pooling function is spawned
if on("confirmation") is received the pooling is stopped since it is not needed

sparrowDom commented 5 years ago

@franckc @micahalcorn @tyleryasaka @wanderingstan have updated the PR description, to help understand what the code does. The change is complex, since it handles a lot of the cases. But on the upside it solves both DApp issues mentioned in description.

I have done testing with Metamask 4.12.0 on Rinkeby and it worked for ListingCreation, both profile saving cases (contractDeployment, contract transaction)

I will do more testing tomorrow, and would really appreciate feedback. 🙏

tyleryasaka commented 5 years ago

I'm generally a fan of underscore/lodash, but for security and file size reasons we've been trying to limit dependencies. I think it's worth asking, do we really want to introduce lodash here? Any thoughts @DanielVF ?

DanielVF commented 5 years ago

Hah. I'm the king of hating dependencies. (the majorhammer.com site has zero dependencies outside the language and the database.)

On the other hand, underscore/lodash is comparatively small, ultra-stable, and can make for much cleaner code. The code here is cleaner than it would be otherwise. Underscore/lodash often one of the few libraries I us -it has the least downside of all possible dependencies.

That said, I think our project's current vibe has been to use the advanced javascript features that babel gives, but not more than that. This has made for a fairly consistent javascript codebase. I think staying consistent and unsurprising is probably worth it. ( That said, I feel like I'm hitting a favorite child by saying so. :( )

DanielVF commented 5 years ago

That was fast!

sparrowDom commented 5 years ago

@DanielVF don't worry I am no super hero coder 🤸‍♂️ Was just finishing with reimplementation of the 2 lodash functions that we require. I feel like in college again when implementing basic mapping functions :)

Otherwise I totally agree with your point. Maybe a good middle ground would be to implement the mapping functions that we need and put them in utils. This way we should have best of both worlds: clean code in contract-service and no additional dependancies.

tyleryasaka commented 5 years ago

I'll leave it to @franckc to determine if/when to merge this, but LGTM

franckc commented 5 years ago

Thanks @sparrowDom for all your work ! Let's sync up on Mon and decide next steps. We've got a demo coming up on Monday so I don't want to deploy risky changes until then... :)

sparrowDom commented 5 years ago

Turns out there is a way to detect Metamask version ( https://github.com/MetaMask/metamask-extension/issues/5433#issuecomment-427387179 ). This gives us another possibility to keep a list of broken Metamask versions and not support them inside the DApp. If we were to chose that option, there is probably no need to merge this Pull Request?

cuongdo commented 5 years ago

I think that detecting MetaMask version could be useful in terms of displaying an appropriate warning or using some sort of workaround to the user if an unsupported or partially unsupported version of MetaMask is detected.

However, I think we need to think carefully about what that means for the user. MetaMask 4.12 was out for over a week. That was the only version that could easily be downloaded through the Chrome extension store during that time. "Not supporting" MetaMask 4.12 without your fix or some other workaround would've been the same as "not supporting users who wouldn't or couldn't circumvent the Chrome extension store for a week." I don't think that's the way we want to go.

A possible compromise is to keep the normal code and only use your new code if an unsupported version of MetaMask is detected.

sparrowDom commented 5 years ago

I don't fully agree with that for the following reasons:

Metamask is totally DApp environment specific dependancy and for implementing one way for non 4.12 users an another for others would mean we need to either:
- inform origin-js what version of Metamask is being used
- extend origin-js api so that DApp can issues different function calls for different Metamask versions (both of these ugly up the code and documentation)
Displaying warning on specific Metamask versions + supporting different code paths for them is IMHO bad for maintainability. This is a good way to introduce regressions. Also for each release instead of supporting the latest Metamask version, our QA needs to test for regressions with different Metamasks in environment.
Metamask 4.12.0 is so broken that most DApps won't function with it. Should we really take the burden + extra time/development costs for their mistakes.

cuongdo commented 5 years ago

I agree that separate code paths makes regressions more likely. However, I don't think that allowing DApps built on top of Origin.js to be broken for > 1 week with the latest available MetaMask is the right experience for users either. We will sometimes need to make things less clean to deal with the reality of immature infrastructure. We should provide a better UX than most DApps.

I'm not clear on what the best set of tradeoffs here is, or whether we just need to make progress on a mobile wallet.

sparrowDom commented 5 years ago

It would probably be beneficial for other to weigh in (@micahalcorn @wanderingstan @franckc @tyleryasaka). So the current options are:

Show a warning when users are using non fully supported version of Metamask. Keep implemented workaround to mitigate Metamask errors.
Show a warning that no write operations are permitted with specific versions of Metamask. Prompt users to upgrade.

tyleryasaka commented 5 years ago

Let's not make our codebase cluttered to patch metamasks's mistakes. If metamask continues to be a problem for us, let's get Yu Pan's mobile app working or something like that so that people don't need to use metamask.

I think we should keep a list of broken metamask versions, detect the user's current version, and see if it is one of the broken ones. If it is a broken version, we simply display a generic warning to the user that their version of metamask is broken.

This would be a very simple piece of code and easy to maintain over time (we would simply need to add new broken metamask versions to a hard-coded array - hopefully not very often). I much prefer a simple and generalized approach such as this rather than introducing complexity to our codebase to support a broken version of a browser extension.

DanielVF commented 5 years ago

I think we've seen in the past that the combination of web sockets and metamask (or maybe websockets and ethereum in general) is just not reliable. It's not just this issue, we've seen fairly wonky behavior in the past. I think we switched our main web3 provider away from using web sockets to HTTP(s) requests because of this. A polling fallback makes sense if we lose or are unable to make a web sockets connection for this.

sparrowDom commented 5 years ago

I guess we have achieved a stalemate and both sides have good arguments.

If I try to sum up @cuongdo and @DanielVF lean more towards merging functionality from this PR and @tyleryasaka and me more against doing it. Should we flip a coin or shake the magic 8️⃣ 🏀 ? 🤷‍♂️

micahalcorn commented 5 years ago

🎱

sparrowDom commented 5 years ago

Aaaa so there is an 🎱emoticon! 🎉

micahalcorn commented 5 years ago

I can’t let that be my only contribution here. 😬

Perhaps the course of action depends on the nature of the bug. As Daniel points out, there may be other reasons for adding the polling code, in particular. I don’t think that there is any question that the mobile wallet is a priority for Q4. I also don’t think that we should go out of our way to support anything but the current version of MetaMask. So I guess my opinion is a bit nuanced:

merge this PR
have a general DApp warning that we don’t fully support MetaMask (if it is the current provider)
encourage use of our mobile app
only further clutter origin-js when we feel like the fix is not just just a short-term patch for the then-current version of MetaMask

tyleryasaka commented 5 years ago

I've stated my opinion but I also haven't looked at the specifics too closely here - do what you gotta do, I won't be heartbroken either way. 😃

sparrowDom commented 5 years ago

Closing since work is continued here: https://github.com/OriginProtocol/origin/pull/803

OriginProtocol / origin-js

Confirmation polling on transactions #599

Checklist:

Description: