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

Add Authereum provider #82

Merged
merged 1 commit into from
Oct 28, 2019
Merged

Conversation

miguelmota
Copy link
Contributor

@miguelmota miguelmota commented Sep 16, 2019

Add Authereum web3 provider

src/providers/index.ts Outdated Show resolved Hide resolved
@pedrouid
Copy link
Member

Looking good, something wrong with the build, going to check it out

@miguelmota miguelmota force-pushed the authereum branch 2 times, most recently from 421a2b1 to c72cb1b Compare September 16, 2019 23:29
@miguelmota
Copy link
Contributor Author

Regarding the build, looks like it's running out of memory. You can use GENERATE_SOURCEMAP=false for the build since generating sourcemaps is memory intensive. More info here

@pedrouid
Copy link
Member

pedrouid commented Sep 17, 2019

BTW @miguelmota I just tested the example locally with Authereum and I got the following error:

Uncaught DOMException: Failed to read the 'localStorage' property from 'Window': Access is denied for this document.

@pedrouid
Copy link
Member

Also I tested the production build for the example locally too and disabling GENERATE_SOURCEMAP worked but I honestly never had to do this for any app I've built before and this is a very small one.

I also noticed that the app bundle size increased considerably with the authereum dependency from 799.35kb (master branch) to 1.19mb (authereum branch).

You should look into your dependencies and most importantly your webpack config because otherwise you will have to ask all apps to disable GENERATE_SOURCEMAP to integrate Authereum

@miguelmota
Copy link
Contributor Author

@pedrouid yeah you're right it's absurdly large. We'll work on removing dependencies to get it to a normal size. Agree that disabling sourcemaps shouldn't be required because of this. Thanks for trying it out and providing suggestions and I'll get back to you when it's improved

@miguelmota miguelmota force-pushed the authereum branch 2 times, most recently from 86d2e31 to 32ea7b5 Compare September 17, 2019 18:06
@miguelmota
Copy link
Contributor Author

Deployment looks good now with removed dependencies

@pedrouid
Copy link
Member

Nice 👍 However I just tested again and I still get the missing localStorage issue running locally in development.

@miguelmota
Copy link
Contributor Author

I'll test it out; what browser were you using? and were you using incognito mode?

@miguelmota
Copy link
Contributor Author

I wonder if you were using an old version because the latest version doesn't use localStorage at all. You should try pulling the latest changes and wiping node_modules before trying it again

@pedrouid
Copy link
Member

Hi @miguelmota, I'm happy to merge this PR but I keep getting this issue. I cloned again from authereum/web3connect repo and I got the same with localstorage. The version installed was 0.0.4-beta.8.

Screenshot 2019-09-27 14 44 53

Can you try it yourself on your machine? Fresh clone to try to replicate it.

@miguelmota
Copy link
Contributor Author

@pedrouid ah I see now, it must be because your browser has cookies and localStorage disabled in the settings. I'll try to figure out a way to work around that.

@miguelmota
Copy link
Contributor Author

miguelmota commented Oct 3, 2019

Hey @pedrouid I'm curious to how you're able to interact with other providers that rely on localStorage (ie like Portis) with the recommended browser setting turned off

nausettings

We aren't able to maintain sessions or store the ephemeral keys if there's no way to store them in the browser by blocking all site data. The best we can do is to alert the user to enable site data to access the provider (like portis does)

@miguelmota
Copy link
Contributor Author

miguelmota commented Oct 4, 2019

@pedrouid the authereum provider should no longer crash like that and it displays an error toastr if persistent storage is not available which is required. Many other providers won't also work with 3rd party storage disabled which is default in brave

Related web3/web3.js#3031

@miguelmota
Copy link
Contributor Author

Hey @pedrouid is there anything we can do to help get this merged? The error you were seeing no longer exists (tested on brave with shield enabled). Would really like to start promoting web3connect to dapps but first need it merged!

@pedrouid
Copy link
Member

Hey @miguelmota, I just resolved merge conflicts and run the example again but now Im getting a compile error for some reason.

Screenshot 2019-10-16 13 32 52

Any idea what this might be?

@miguelmota
Copy link
Contributor Author

miguelmota commented Oct 16, 2019

@pedrouid thanks for reporting. On macOS some files seem to be lowercased in the dist directory although the source files are capitalized. Debugging issue..

@crisgarner
Copy link
Contributor

@miguelmota I'm trying to test the Authereum integration and I'm getting the following error:

Screenshot from 2019-10-21 23-50-53

@miguelmota
Copy link
Contributor Author

miguelmota commented Oct 22, 2019

@crisgarner I filed an issue with the dependency provider causing the error blocknative/notify#22

Might need to find a workaround if the issue on their is not resolve soon enough

@miguelmota miguelmota force-pushed the authereum branch 2 times, most recently from 77623d2 to 87c23ec Compare October 25, 2019 22:20
@miguelmota miguelmota force-pushed the authereum branch 6 times, most recently from 210ba6f to 583c0c4 Compare October 28, 2019 02:26
@miguelmota
Copy link
Contributor Author

@pedrouid @crisgarner thanks for the patience; the dependency issues have been resolved and the latest build is working

@crisgarner crisgarner merged commit 60e9ad4 into WalletConnect:master Oct 28, 2019
@crisgarner
Copy link
Contributor

Tested and merged @miguelmota 👍

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.

None yet

3 participants