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

JSON RPC Engine #1236

Merged
merged 38 commits into from Jan 16, 2020
Merged

JSON RPC Engine #1236

merged 38 commits into from Jan 16, 2020

Conversation

brunobar79
Copy link
Contributor

@brunobar79 brunobar79 commented Dec 4, 2019

Replaces the whole provider implementation with https://github.com/metamask/metamask-mobile-provider which internally uses metamask-inpage-provider so we have no more differences between the extension provider and mobile

Fixes #1216 ( Parity with the extension)
Fixes #1209
Fixes #1232

@brunobar79 brunobar79 requested review from ibrahimtaveras00 and estebanmino and removed request for ibrahimtaveras00 December 5, 2019 02:27
@brunobar79 brunobar79 added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Dec 5, 2019

res.result = rawSig;
}),
web3_clientVersion: () =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing net_versioncall

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled by metamask-inpage-provider https://github.com/MetaMask/metamask-inpage-provider/blob/master/index.js#L346 . No need to go to the background for info that the page already has

engine.push(this.middlewares.wallet_watchAsset());

// Mobile specific methods
engine.push(this.middlewares.wallet_watchAsset());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comment

const isInitialized = !!vault;
const { network, selectedAddress } = Engine.datamodel.flatState;
return {
...{ isInitialized },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just isInitialized, ?

isEnabled,
selectedAddress: isReady ? selectedAddress.toLowerCase() : null,
networkVersion: network,
chainId: `0x${parseInt(chainId, 10).toString(16)}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't crash the app but I for rpc networks we'll be returning 0xNaN if that's ok 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a check and set it to null

@ibrahimtaveras00
Copy link
Contributor

ibrahimtaveras00 commented Dec 6, 2019

Issue 1:

On playstore/testflight when I go to uniswap and connect, pretty quickly I can see my balance on top of the ETH dropdown, however, on this branch, I don't see my balance until I reload the page

seen here = http://recordit.co/7ZDqImuJ5Q

Happens on both android + iOS

Issue 2:

Happening on both android + iOS; if I go to migrate.makerdao.com and connect, the next screen just shows an endless spinner = https://recordit.co/JAeGLqcZIf

here's comparison to testflight = http://recordit.co/7ZDqImuJ5Q

Issue 3:

I'm having issues getting the connect modal to pop up when navigating to https://3box.io/ and tapping on choose wallet or log in. For android, I have to reload the page, then it works; while on iOS reloading the page does nothing.

Seen here = http://recordit.co/Q3tLcbgzBv

Issue 4:

When going to peepeth, I can connect w/o any problem, however, when I try to proceed through the sign up flow, It's stopping me with the reason "Install web3 provider"

seen here:
Screen Shot 2019-12-06 at 4 52 07 PM

Issue 5:

Android only; It seems that when I navigate to mainnet.aragon.org and tap on enable account the connect modal doesn't pop up unless I reload the page and tap on enable account again

Issue 6:

MM Homepage > Explore Tab > Buy Crypto > Wyre > Connect with wallet > tap on MM

Endless spinner for "Waiting for signature..."

seen here = http://recordit.co/7varBCtOTQ

Issue 7:

I'm unable to connect to MM Wallet at https://trustless.ethfinex.com/ On iOS it just stays on pending with endless spinner, while on Android, nothing happens when I tap "CONNECT METAMASK"

Screen Shot 2019-12-09 at 3 47 32 PM

ISSUE 8:

I have to reload the page a few times in order to connect kyberswap to my MM wallet; seen here = http://recordit.co/Dg9IaeLyJr

steps:

  • MM Homepage > Explore Tab > Finance > Kyberswap > attempt to connect

Furthermore when I do connect, I am unable to go past this view:
Screen Shot 2019-12-09 at 4 02 30 PM

I basically tried to "SWAP" from KNC to ETH

@ibrahimtaveras00
Copy link
Contributor

ibrahimtaveras00 commented Dec 9, 2019

Good news, I just built latest develop and can knock some things off this list

Issue 1 happens on develop but not playstore (I'll create a different issue for this)

Created = #1240

Issue 6 same, but this time I can reproduce on playstore as well compared to last week (I'll create a different issue for this)

Created = #1241

Issue 7 Same

Created = #1242

I've deleted the above from this PR and updated the list 👍

@brunobar79
Copy link
Contributor Author

brunobar79 commented Jan 15, 2020

@ibrahimtaveras00 just pushed all the fixes.

@estebanmino Could you please take another look ?

Issue 1 - Uniswap - FIXED
Issue 2 - migrate.makerdao.com - FIXED
Issue 3 - 3box - FIXED
Issue 4 - That error is fixed, however it will fail in the very last step of the signup process saying "Error: Unprocessable entity" which is an issue with peepeth itself.
Issue 5 - Aragon - FIXED
Issue 6 - Wyre - FIXED
Issue 7 - ETHFinxes - FIXED
Issue 8 - KyberSwap - FIXED

const { approvedHosts, privacyMode, selectedAddress } = this.props;
const isEnabled = !privacyMode || approvedHosts[hostname];
if (isEnabled) {
res.result = [selectedAddress.toLowerCase()];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why toLowerCase? we have it on checksummed format

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better safe than sorry

Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good now on both OS's, QA Passed 👍

@brunobar79 brunobar79 merged commit b131002 into develop Jan 16, 2020
@estebanmino estebanmino deleted the json-rpc-engine-multi-bridge branch February 4, 2020 15:58
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* inpage => bg working

* eth_requestAccounts working

* publicConfig stream working

* clean up

* signing methods started to work

* all signing methods working

* fix warn

* added all the other wallet methods

* lots of fixes

* update prettierignore

* single iframes working

* break down bgBridge per frame

* progress

* works

* e2e passing

* clean up

* clean up

* lock deps

* use metamask-mobile-provider pkg

* fix e2e uniswap

* fix network/account change

* code review comments

* clean up

* fix

* fix ios build

* android support

* debugging

* fix stream disconnection

* uniswap working

* bump metamask-mobile-provider

* fix all the remaining issues

* Ignore warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Any New Features that needs a full manual QA prior to being added to a release.
Projects
None yet
3 participants