search

mymonero / mymonero-core-js

The JS library containing the Monero crypto plus lightwallet functions behind the official MyMonero apps
BSD 3-Clause "New" or "Revised" License
101 stars 103 forks source link

Refactor to make react native friendly #63

Closed paullinator closed 5 years ago

paullinator commented 5 years ago

This PR refactors mymonero-core-js to make it React Native friendly and adds an easy to use MyMoneroApi class

paulshapiro commented 5 years ago

Hi Paul,

Thanks for PRing.

You're going in the general direction of some improvements I've been thinking carefully about. However there are a few big-ish issues that prevent me from merging your changes. I'll have to describe my feedback rather than do a line-by-line PR review.

  1. This PR tries to accomplish far too many tasks at once - each such category of improvement can stand on its own merit and does usually need to be broken into a few PRs. But I can see you spent a lot of time on this as a single branch so let's ignore this one for this PR in the interest of merging.

  2. Your changes cause those who do not already accept a JS compilation pipeline to have to compile down to vanilla JS to use the MyMonero core js lib (this breaks backwards compatability for a number of long-standing Monero community users who want to be able to embed the JS directly in a site). async/await and arrow function syntax are finally gaining support in the most recent versions of browsers and environments, but there are tons of users in browsers where those things are not natively supported. And, once you use an async function, everything connected to that must be async, which means we need a fallback option. But none of this is really relevant at this moment because per #3 it's not necessary to these functions async, and we can separate such syntax into the files which truly need it so that index files can be constructed which match the specific build requirements for targeted environments.

  3. You've made foundational synchronous functions into async where they don't need to be just so that a higher level function can call them asynchronously. However, this isn't necessary due to the nature of promises. (All async functions are built out of sync primitives. The lower level utility functions can serve both purposes without breaking the ability to do synchronous calls to e.g. monero_utils and the response_parser for those whose environment or architecture does not require them to make calls with "async" only.) From this standpoint, the code is already friendly to an async bridge that is RN friendly, so can we rework your foundational changes into a layer on top that you can hook into? We don't need to have a monolith myMoneroApi.js as the only way to access mymonero from a library consumer's standpoint. (all utils modules can be require'd on their own). Any of the API improvements to "myMoneroApi.js" like an object for params should have or could have been made to the specific code modules rather than us putting a new layer on top that gives the API we want but now introduces the requirement that the underlying API never change. As a corrollary, please see Monero src/wallet/wallet2 vs src/wallet/api. It's been acknowledged in the Monero dev community that creating src/wallet/api was a mistake and that the wallet object interface itself must serve as the comsumable API. We might for example simply create a monero_utils_async which talks* to monero_utils.

  4. Bringing the MyMonero desktop app's hosted API client implementation into this repository with all its application specific parameters and implementations, as a separate file, named and organized as it is, is not quite right - nor is bringing in the MyMonero "lite" version (for the My Monero web wallet), as that's quite MyMonero specific. I have on my todo list the introduction of a solid api client implementation to this repo but it can be much more lightweight than all this. I'm adding a monero_fetch_sendingFunds_utils which borrows themes from your PR but still allows people to use either option. e.g. it seems worthwhile from my pov to allow those who don't want to bundle a fetch polyfill the option to still do so if they don't need the networking.

All in all, a great direction, with a number of nice improvements (like passing in fetch and upgrading to fetch-like API) though a lot more work is needed before the whole collection is ready and so it might be best for us to keep in touch over the next couple weeks so we can work in all of the necessary capabilities while maintaining existing support.

I'm also interested in looking at whether the whole monero_utils exception-generating / -bridging behavior is necessary and can be done away with in favor of returning err_msgs and err_codes. In fact, I've just convinced myself to do so! (However, it's a breaking change.)

paulshapiro commented 5 years ago

Paul – I just realized you're applying async to the Module calls themselves. I didn't know emscripten supported that already. Does RN require these functions to be async, and/or are there any performance wins in doing so? In any case, adding a way to call them with async now makes more sense to me. I'd still want to find a way to support sync calls as it changes the API for all these calls into promises in vanilla when they don't have to be. What this all means though is we'd have to have a separate bridge codepath for sync calls. We wouldn't want to reproduce the JS->JSON bridging API for any of these so it'd make sense to factor that stuff into functions which could be called by two instances, selected preferentially.. MyMoneroCoreBridge_sync.js and MyMoneroCoreBridge_async.js. This might in fact actually be the right way to go as it happens because I am as of an hour ago planning now on moving networking to C++ / core-cpp given the fact emscripten supports the fetch API (https://kripken.github.io/emscripten-site/docs/api_reference/fetch.html), our need to obtain testing (edit: data) automatically, and a need to integration test servers. However I can see the merit of maintaining only one MyMoneroCoreBridge and would like to solicit feedback from the community. A lot of websites would have to fundamentally and sweepingly change their architecture.

I do still agree with my earlier comments about the desirability of integrating API improvements directly into the existing modules.

paulshapiro commented 5 years ago

One more comment to add... we may end up needing to make this new hypothetical C++ function send_funds an async function anyway because there will probably be a C++ future waiting on the network request C++ promise, which will probably be called by an emscripten waitable fetch (https://kripken.github.io/emscripten-site/docs/api_reference/fetch.html#waitable-fetches), and we wouldn't want to block the main thread. In fact, unless we use a waitable fetch, we'd have to use userData to match requests up, which is the same (edit: ok, similar) complexity as building a general C++ -> JS bridge as we need to do for Ledger integration anyway. And we're probably going to move away from http requests for some of this anyway. So, changed my mind, not yet convinced it's worth investing the effort in writing the fetch integration itself in C++. And without that, it wouldn't make sense to port the relatively trivial send_funds/SendFunds yet.

paulshapiro commented 5 years ago

By the way, can you find any documentation (edit: or code) on async calls into Module functions, especially under wasm?

paulshapiro commented 5 years ago

Relevant discussion indicates people would like to have a sync version

https://github.com/mymonero/mymonero-core-js/issues/80

paulshapiro commented 5 years ago

I'm going to close this for now since we've had some side-channel discussions with Edge and they have opted to go with a native plugin for core-cpp for React Native integration. Whatever async solution we arrive at needs to support sync functions as well, as far as I can tell.