trufflesuite / drizzle

Reactive Ethereum dapp UI suite
905 stars 235 forks source link

Drizzle does not initialize with Coinbase Wallet #40

Open cig1 opened 5 years ago

cig1 commented 5 years ago

I am trying to use Drizzle with Coinbase Wallet (iOS) and it gets stuck at initializing. Metamask and Cipher (iOS) work fine. I am using the Rinkeby test network and React with these versions:

    "drizzle": "^1.4.0"
    "drizzle-react": "^1.2.0"
    "drizzle-react-components": "^1.3.0"

Part of my drizzleOptions file:

const options = {
  web3: {
    block: false,
    fallback: {
      type: "ws",
      url: "wss://rinkeby.infura.io/ws",
    },

index.js

ReactDOM.render(
  <DrizzleContext.Provider drizzle={drizzle}>
      <div>
        <App />
      </div>
   </DrizzleContext.Provider>
, document.getElementById('root'));

app:

export default () => (
  <DrizzleContext.Consumer>
    {drizzleContext => {
      const { drizzle, drizzleState, initialized } = drizzleContext;
      if (typeof web3 === 'undefined') {
        return (
          <div className='App'>
            <AppNoWeb3/>
          </div>
          )}
      if (!initialized) {
         return (
         <div>
          <p>Loading... </p>
         </div>
       )}
      return (
        <div className='App'>
          <AppWeb3 drizzle={drizzle} drizzleState={drizzleState} />
        </div>
      );
    }}
  </DrizzleContext.Consumer>
)

I've tried the Coinbase Wallet Discord channel without success and found a post, https://spectrum.chat/trufflesuite/drizzle/drizzle-doesnt-initialize-with-coinbase-wallet~5e2ed9fe-1505-45de-9f11-a40aa1be85c8 describing the same problem.

Does anyone know a fix?

cig1 commented 5 years ago

I found a fix. In src/web3/SagaWeb3.js at initializing I put in a check to see if Coinbase Wallet is used. If so window.web3.currentProvider is used, else the script is run as before.

diff --git a/node_modules/drizzle/src/web3/web3Saga.js b/node_modules/drizzle/src/web3/web3Saga.js
index f2f745e..c176ff4 100644
--- a/node_modules/drizzle/src/web3/web3Saga.js
+++ b/node_modules/drizzle/src/web3/web3Saga.js
@@ -17,6 +17,15 @@ export function * initializeWeb3 ({ options }) {
       return web3
     }

+    if (typeof window.web3 !== 'undefined') {
+      const { currentProvider: cp } = window.web3
+      if (!!cp.isToshi) {
+        web3 = new Web3(window.web3.currentProvider)
+        yield put({ type: Action.WEB3_INITIALIZED })
+        return web3
+      }
+    }
+
     if (window.ethereum) {
       const { ethereum } = window
       web3 = new Web3(ethereum)
cds-amal commented 5 years ago

Hi @cig1,

Thanks for this! Is it possible to share a full repo that shows this issue? The diff shows a test inserted for a custom provider before the MetaMask's test. There is already a test for custom provider in the Saga.

I wonder if the tests ordering is the issue.

cig1 commented 5 years ago

Example repo: https://github.com/cig1/drizzle_react_example

To use type 'yarn' to install and 'yarn start' to run. The example site will be available on your local network which you can use on your iPhone to test with Coinbase Wallet.

adrianmcli commented 5 years ago

@cig1 Looks like this could be a good PR opportunity. Also, it looks like your link isn't working.

cig1 commented 5 years ago

@cig1 Looks like this could be a good PR opportunity. Also, it looks like your link isn't working.

The link works now

cds-amal commented 5 years ago

@cig1 Thanks for the repo link!

TIL coinbase injects a toshi provider into window.ethereum. I think the fix to this issue would be to invoke ethereum.enable only if ethereum.isMetaMask is truthy.

Edit: Learned more about this issue.

cds-amal commented 5 years ago

@cig1 FYI

I tested web3 resolution on IOS with the status and coinbase apps. Drizzle works with status but doesn't resolve with coinbase. I suspect the coinbase browser does something different when it comes to assigning context for redux-saga's Calleffect. I was able to get the code to work by explicitly declaring a context to the Call effect. Curious that it is the only dapp browser (so far) that has an issue.

  yield call(ethereum.enable) // change this line
  yield call([ethereum, ethereum.enable]) // to add context

I've also reached out to the coinbase dev team to verify this before I make the change.

cds-amal commented 5 years ago

@cig1 FYI, I'm working on this issue, which you can follow on the fix/coinbase-init-web3 branch, and you can test with this repo

elliotolds commented 5 years ago

We've been trying to get our app to work in Coinbase Wallet by using the fix/coinbase-init-web3 branch referred to above, and have been unsuccessful so far.

We created a super simple test app that uses Drizzle the same way our code does. We describe how to repro Drizzle failing to load in Coinbase Wallet in the README here: https://github.com/ethereumgeek/simple_drizzle

Our code is base on https://github.com/truffle-box/drizzle-box, which we notices uses Drizzle differently than the code in this repo (which we verified does work for us). Our code imports DrizzleProvider but the working code imports DrizzleContext.

Is there something wrong with how we're using Drizzle? Or should the fix in the drizzle branch be expanded to cover this style of use? We can try to change how we write our Drizzle code as a workaround, but I'm curious about the root cause of why the fix isn't working for us.

adrianmcli commented 5 years ago

@elliotolds @cds-amal's example repo uses the new Context API introduced in React 16.3 (with the Render Props pattern). Since React is deprecating the old Context API by v17, this is now the recommended way.

Admittedly, the Drizzle Box is a bit outdated. Thanks for bringing this to our attention. @cds-amal let's work on updating the Drizzle Box this or next week. And feel free to provide more comments to help our community members out.

ethereumgeek commented 5 years ago

We updated our project to use DrizzleContext and everything works great when running locally with npm run start.

However, UglifyJS fails in our production build (with npm run build) after updating from "drizzle-react": "^1.1.1" to "drizzle-react": "^1.3.0". It seems to be complaining that drizzle-react has some ES6 code.

The specific error we get when updating to drizzle-react": "^1.3.0 is:

Creating an optimized production build...
Failed to compile.

static/js/main.96a41f63.js from UglifyJs
SyntaxError: Unexpected token: punc ()) [./~/drizzle-react/dist/drizzle-react.js:1,2921]

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! drizzle-box@1.0.0 build: `node scripts/build.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the drizzle-box@1.0.0 build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

If I comment out new webpack.optimize.UglifyJsPlugin( ... ) then our production build works with drizzle-react": "^1.3.0.

Thanks!

ethereumgeek commented 5 years ago

I've updated our project to use the react-scripts instead of the webpack build scripts from https://github.com/truffle-box/drizzle-box. I also removed web3 from being an explicit dependency in our package.json file.

I'm now able to get our Drizzle project working with Coinbase Wallet by linking https://github.com/cds-amal/drizzle_react_example/tree/drizzle-220 after the above changes.

I think it would be helpful to build a new Drizzle Box that uses react-scripts build scripts and that also uses DrizzleContext.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 years ago

This issue has been closed, but can be re-opened if further comments indicate that the problem persists. Feel free to tag maintainers if there is no reply to further comments.