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

Refactor to make react native friendly #63

Closed

Conversation

@paullinator
Copy link

paullinator commented Nov 2, 2018

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

  • Create new MyMoneroApi class for easy API use
  • Add separate entry point into repo specifically for RN vs web/nodejs
  • Change API to require initialization before use to setup the CoreBridge
  • Change entire monero_utils API to be async to be friendly to async bridges like RN
  • Add tests for MyMoneroApi class
  • Put all tests in testsrc/ and use flow-copy-src to copy to test/ which is used by mocha
  • Use Flow for all new MyMoneroApi class
  • Changes to use fetch instead of xhr/request for cross platform compatibility
Paul Puey added 5 commits Oct 30, 2018
* Make entire API async to be friendly to async bridges such as React Native
* Implement no MyMoneroApi class
* Add flow and eslint to MyMoneroApi
* Add HostedMoneroApiClient and BackgroundResponseParser for MyMoneroApi to use
* Add tests for new API
* Move to testsrc/
* Use flow-copy-src to copy/strip code to test/
* Add missing await to async functions in tests
This is a breaking change to the API to now requires the entire library to be initialized before use via initMonero().
* initMonero() is the only thing returned by doing a `require('mymonero-core-js')`
* Doing an `await initMonero()` will initialize the monero_utils and inject it into all files that need monero_utils
* After initMonero(), user can then use all the normal exports in the mymonero_core_js object which is returned by initMonero()
* Export indexMoneroCore.rn.js from the repo for react native packager
* Use monero_utils_rs.js instead of monero_utils.js for RN as this avoids multi, incompatible requires of wasm/asmjs files
* Implement the MyMoneroCoreRN.js calls which expect a global.moneroCore bridge to be provided by the parent application
* Change to use fetch instead of request for cross compatibility between nodejs and RN
@paulshapiro

This comment has been minimized.

Copy link
Member

paulshapiro commented Nov 2, 2018

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

This comment has been minimized.

Copy link
Member

paulshapiro commented Nov 2, 2018

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

This comment has been minimized.

Copy link
Member

paulshapiro commented Nov 2, 2018

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

This comment has been minimized.

Copy link
Member

paulshapiro commented Nov 2, 2018

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

@@ -0,0 +1,9 @@
const MyMoneroConfig =
{
API__authority: "edge.mymonero.com:8443",

This comment has been minimized.

Copy link
@paullinator

paullinator Nov 6, 2018

Author

change to api.mymonero.com

const monero_config = require('../monero_utils/monero_config.js')
const net_service_utils = require('../hostAPI/net_service_utils.js')
//
const config__MyMonero = require('./config__MyMonero')

This comment has been minimized.

Copy link
@paullinator

paullinator Nov 6, 2018

Author

Remove config file and use values from MyMoneroApi.js class

if (!global.crypto) {
global.crypto = {}
}
if (!global.crypto.randomBytes) {

This comment has been minimized.

Copy link
@paullinator

paullinator Nov 6, 2018

Author

Can be removed as using C++ native random for RN. For browser and nodejs, WASM/ASMJS already call JS crypto libraries for random()

const out: BalanceResults = {
blockHeight: parsedAddrInfo.blockchain_height,
totalReceived: parsedAddrInfo.total_received_String,
totalSent: parsedAddrInfo.total_sent_String

This comment has been minimized.

Copy link
@paullinator

paullinator Nov 6, 2018

Author

add locked balance

@@ -45,7 +43,7 @@ async function t1()
}

try {
var fee = new mymonero.JSBigInt(await (await mymonero.monero_utils_promise).estimated_tx_network_fee(
var fee = new mymonero.JSBigInt(mymonero.monero_utils.estimated_tx_network_fee(

This comment has been minimized.

Copy link
@paullinator

paullinator Nov 6, 2018

Author

make async

* Remove config__MyMonero.js. Add apiServer to options instead
* Remove global.randomBytes. This is done in platform specific code
* Add await to index.node.js test
* Add contrib/ to .flowconfig ignore section
* Check for err_msg from monero_utils.send_step1__prepare_params_for_get_decoys and throw error if so
@paulshapiro

This comment has been minimized.

Copy link
Member

paulshapiro commented Jan 7, 2019

Relevant discussion indicates people would like to have a sync version

#80

@paulshapiro paulshapiro force-pushed the mymonero:develop branch from 969b59a to 7472d03 Jan 18, 2019
@paulshapiro

This comment has been minimized.

Copy link
Member

paulshapiro commented Mar 26, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.