Skip to content

Conversation

jrainville
Copy link
Collaborator

Infura problem

Fixes the use of Infura to connect to the ENS contracts. When
connecting directly to Infura, it would throw with rejected due to project ID settings, because it doesn't accept the VM as the domain
Instead, when passing from the proxy, it works. So I changed the
default when no dappConnection to ['$EMBARK']. I also added a
message when the error happens to help users fix it themselves

Testnet problem

When in the testnet, we don,t register because we already have the
addresses, which is fine, but we also didn't populate the ensConfig
object which contains the important information about the addresses
and ABI.

Linting

There was a lot of lint problems in a couple of files so I cleaned
that up

@jrainville jrainville requested a review from a team January 14, 2020 20:39
@ghost
Copy link

ghost commented Jan 14, 2020

DeepCode's analysis on #6b5360 found:

  • 0 critical issues. ⚠️ 0 warnings and 0 minor issues. ✔️ 2 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

Copy link
Collaborator

@emizzle emizzle left a comment

Choose a reason for hiding this comment

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

Nice job!

In general, it's nice to have Infura traffic go across the proxy first, which provides some nice features during development. For those dapp developers who want to test real-world dapp scenarios with metamask, they can still use $WEB3 in their dappConnections list to do so.

The Infura error (rejected due to project ID settings) sounds a lot like CORS issues I've experienced in the past. Did you try changing the CORS settings to see if this was indeed the case? To prevent the issue, is there any way to update the origin identifier that originates in the VM, maybe with some kind of header? Why does this only happen with ENS contracts and not other contracts?

I'm afraid the warning message added here may confuse some dapp developers who actually do receive a valid project settings error message from Infura. For example, an Infura project may have very restrictive CORS settings (let's say it's restricted to only talk to requests from 1.1.1.1), and a mainnet dapp is deployed out in the wild (ie no Embark running) with the code in this PR, connecting to Infura via the project ID with restrictive CORS settings. The EmbarkJS ENS code would receive the "project settings" error from Infura and then there would also be a warning in the console about changing dappConnection settings to $EMBARK, which would be completely off the mark as Embark would not even be running in this situation.

Out of curiosity, what would the dappConnections setting need to be in a mainnet situation (when not using embark at all)? Would it need to contain the Infura endpoint?

In summary, it makes sense, in my opinion, to connect through the proxy by default, even for external nodes. Infura project settings errors should not be succeeded by a warning that may be misleading, and in the case they a user does hit the edge case, it would be nice to find a fix for the VM instead.

@jrainville
Copy link
Collaborator Author

Did you try changing the CORS settings to see if this was indeed the case?

I tried to find a way to change them or even know what were the original ones, but since we go through Web3 to send the request (it's a getAccounts), I can't seem to find any configuration option.
As for Infura, usually, as their support staff states, this error comes when something is whitelisted in the project settings, eg: when you only want mydomain.com to be able to use your project ID. In my case, I had no whitelisted domains at all.

Why does this only happen with ENS contracts and not other contracts?

That's a good question, since ENS used to default to the contrat dappConnection, the contracts should have thrown the same error, so I went to look. and it seems like it "hardcodes" the proxy url for the connection for the blokchain: https://github.com/embark-framework/embark/blob/master/packages/stack/embarkjs/src/index.js#L121

So I guess, we should do the same, when in the console/vm, we should always use the proxy and not dappConnections, because $WEB3 will never exist for example.

I'll try to change that.

@jrainville
Copy link
Collaborator Author

@emizzle updated to always use the Proxy in the VM and removed the warning

@emizzle
Copy link
Collaborator

emizzle commented Jan 16, 2020

In my case, I had no whitelisted domains at all.

Did you try whitelisting *? Just for testing's sake.

An alternative solution

I believe I have figured out why this work when going through the proxy. The proxy sets an origin header to http://embark for requests sent to the node: https://github.com/embark-framework/embark/blob/9aa9e6cf5a573b75b557664207faeb897e948dfc/packages/stack/blockchain-client/src/index.js#L63-L74 This works without going through the proxy in the browser because the browser has an origin by default.

As you can see, this only works for WebSockets, so could you please confirm that this works with http connections as well?

Now that we know the root cause and fix, the question is: do we definitely want to force connection through the proxy for the VM?

I've thought about this since my last comment, and I think I may have a different opinion now. The VM is meant to replicate the dapp in terms of comms with the node. IOW, we want the responses for requests in the VM to exactly mirror what we'd get in our dapp, otherwise it will be a source of confusion. For example, let's assume our dapp is configured to connect to a testnet endpoint (and therefore EmbarkJS.Blockchain|ENS will connect directly to the testnet node from the browser). If we then forced requests from the VM to go through the proxy on their way to the testnet node, then, for example, a request for eth_accounts originating from the browser, might be different than a request originating from the VM, as the VM routes the request through the proxy which could modify the response (depending on the config obviously).

With all that in mind, an alternative approach to forcing a dappConnection to the proxy would be to modify EmbarkJS.Blockchain|ENS.setProvider, such that their web3 instances use an Origin header as per https://github.com/embark-framework/embark/blob/9aa9e6cf5a573b75b557664207faeb897e948dfc/packages/stack/blockchain-client/src/index.js#L63-L74

@emizzle
Copy link
Collaborator

emizzle commented Jan 16, 2020

Also, CI seems to be failing.

@jrainville
Copy link
Collaborator Author

As you can see, this only works for WebSockets, so could you please confirm that this works with http connections as well?

I was only using HTTP

With all that in mind, an alternative approach to forcing a dappConnection to the proxy would be to modify EmbarkJS.Blockchain|ENS.setProvider, such that their web3 instances use an Origin header as per
I understand your POV, but I don't think the console is meant to emulate the Dapp. I see it more as an easy and fast way to send calls and transactions to the contracts without having to sign transactions (handled by the dev account or the proxy).

As for using the Origin in the VM, it might work for WS, but it's with HTTP that I was having issues.
I'll try testing with WS to see if I have a different behaviour and if it works with WS, I'll try to check for HTTP if there is a way to add an Origin.

@jrainville
Copy link
Collaborator Author

@emizzle I just tested with WS using the http://embark in the VM and Infura still throws the error. I think it really dislikes requests coming from a VM, though I'm not sure how it manages to know, I don't know what's different.

Anyway, I feel like always going through the Proxy for the VM is acceptable.

Fixes the use of Infura to connect to the ENS contracts. When
connecting directly to Infura, it would throw with `rejected due to
project ID settings`, because it doesn't accept the VM as the domain
Instead, when passing from the proxy, it works. So I changed the
default when no dappConnection to ['$EMBARK']. I also added a
message when the error happens to help users fix it themselves

When in the testnet, we don,t register because we already have the
addresses, which is fine, but we also didn't populate the ensConfig
object which contains the important information about the addresses
and ABI.

There was a lot of lint problems in a couple of files so I cleaned
that up
@jrainville
Copy link
Collaborator Author

Just rebased and fixed the test

@iurimatias iurimatias merged commit f5849e0 into master Jan 16, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/infura-id-error branch January 16, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants