Initial E2E Working Setup #1

W3stside · 2019-08-16T14:19:16Z

This PR is large. But can be visualised and broken down into parts:

This provides an E2E testable app. Lots of the commits are just setup related. The main files are explained below. All following PRs will be focused.

Please follow the README to test - but should run simply via npm i && npm run start:example-ts

src/api/web3.ts sets up Web3Api singleton able to be passed around
a. Tested inside ./tests/api/index.test.ts
src/components/Web3ConnectButton.tsx pulls web3connect button and uses the initialised Web3Api
src/components/DisplayWeb3.tsx takes the instantiated Web3Api and displays:
a. Account
b. ETH Balance
c. Network Name
d. Client (Provider) Version
./examples/ts-frontend is used to test the components/setup fns in points 1-3

1. added start:examples-ts script to launch example app 2. added postinstall to get examples/ts-frontend setup

anxolin · 2019-08-18T16:47:12Z

examples/ts-frontend/README.md

+
+Open app in `localhost:1234`
+
+For wallet connect, please use `test.walletconnect.org`


I tried to connect with the Gnosis Safe, it worked the connection. It didn't work the disconnection.

Also, after refreshing, if I try to connect again with wallet connect, it's not showing the QR, and instead I see:

Not sure if it's related, but the previous wallet in the local storage, but I cannot change it:

For trust wallet, is not detecting the address and network info either:

With test.walletconnect.org it worked the connection.

But it didn't work the disconnection.

Yeah I was also having Safe issue, think the bridge URI was down when testing. Walletconnect integration is buggy at best. Not sure how to proceed on that front but will test more

I think we should extract the wallet connect part from this PR.
We can create one PR that just adds Wallet Connect integration, then merge it when it's fully working?

If we have some specific case it's not working, like the ones I shared, we can make it easily reproducible and share with Wallet Connect team.

anxolin · 2019-08-18T17:57:59Z

package.json

+    "react-dom": "^16.8.6",
+    "react-hot-loader": "^4.12.10",
+    "styled-components": "^4.3.2",
+    "web3": "^2.0.0-alpha.1",


In the doc of web3, they recommend not using 2.0 for production.

https://web3js.readthedocs.io/en/v2.0.0-alpha.1/getting-started.html

Should we use 1.0 instead?

@Velenir thoughts here?

In my experience 2.0.0 is more stable than 1.0. Certainly has more fixes included. Remember they were both renamed from beta only a month ago. 1.0 is more stable in the sense that truffle and co. are using it, that's all

On a semi-related note, I think we may be better off not dealing with web3 at all, only provider. Leave web3 (or etherjs or whatever) instantiation to anyone using the library. Everything from accountsChange to WC session management can be done with provider only

Yes, right now is a copy of 1.0 but if they write that is not mainnet ready it's because they expect breaking changes.

I think the app we have to do is super basic, it should be ok to work with "not the last version". If there's a big reason to jump for the unstable, we could review it, otherwise I think is not worth it at this point. We can revisit this in a few months.

package.json

src/api/utils.ts

anxolin · 2019-08-18T18:12:14Z

src/api/utils.ts

@@ -0,0 +1,20 @@
+export const windowLoaded = new Promise((accept, reject) => {
+    if (typeof window === 'undefined') {
+        return accept()


Why is the promise resolved if there's no window object?
Shouldn't be this handled as an exception?

better as to not block the rest of the logic that isn't entirely window dependent - though tbh I haven't tested this logic with a non window exposing device

Me thinks, leave waiting on window to the library users. If they need it, like ens only ever set up react on window.onload

But if the method is "windowLoaded" and there's no window, it seems or to return a failed promise.

Also, note that you already fail if it doesn't allow you to subscribe the listener:

You don't block the user. The one using the library decides how to handle the exception. You shouldn't decide for the user of the util function what to do if there's something wrong, like "no window"

anxolin · 2019-08-18T19:28:42Z

src/api/web3.ts

+        } catch (error) {
+            if (error.readyState === 4 && (error.status === 400 || error.status === 200)) {
+                // the endpoint is active
+                console.debug('API/WEB3.ts catch block => Success')


What does this mean?
Will be provider = new Web3(url) always initialized in this case?

for readOnly mode, yes

I don't get:
~~1. What is a null provider provider = new Web3(null)~~ (it's asked already in other comment)

The logic: if the fetch fails, what is this branch doing. It seems complicated:

This logic. Are we always using metamask if it's installed or do we allow the user to choose. For example, if we want to use wallet connect, it will prompt in the metamask extension.
Also, do we really need to hardcode the development ganache as a fallback?

anxolin · 2019-08-18T19:30:26Z

src/api/web3.ts

+            } else {
+                console.debug('API/WEB3.ts =>  No web3 instance injected. Falling back to null provider.')
+                readOnly = true
+                provider = new Web3(null)


What is a null provider?
Also, what is "readOnly"? If there's no connection with the node you can't read, right?

App loads, no blocking, no thrown error, user can see the app, better UX

Mmm, I'm not sure about this. This getProvider returns a promise: Promise<Web3>

This means, that if something is wrong, we can handle the case.

One solution is to do something like:
resolve(null) signaling, there's no Web3 object cause we didn't connect the wallet yet. The app can react and handle that case. So you can still have a good UX

src/api/web3.ts

anxolin

I tried to do a full review before my trip, but I didn't manage. Anyways, I think it's enough review, we can review first the open conversations, since there are many already :D

It think it's a super good start. I understand that this PR got big and next iterations should be smaller in changes and scope, I think it's ok this one time exception, but it made the review much more complicated and I think for both sides, since we'll have a lot of things to discuss here.

Many of the discussions opened aim to open discussions about project structure, code style and so on, because I think it's good to do this exercise now so we are all in sync

src/api/web3.ts

anxolin · 2019-08-18T20:14:16Z

src/api/web3.ts

+
+    /* Provider Specific */
+    const setProvider = web3.setProvider.bind(web3)
+    const resetProvider = (): void => setProvider(web3.currentProvider)


Why do we need resetProvider?

I think if we don't use it, we should remove.
If it's a bug of a provider, we can review that specific case

src/components/App.tsx

src/components/DisplayWeb3.tsx

1. ethereum utils imported from Web3 2. misc utils 3. exported from index

1. global file with constants 2. types file (global) 3. removed typings.d.ts + old utils file

1. allow synthetic imports into examples ts 2. removed unused lines

1. moved types into global 2. clarified names of fns 3. removed async catching 4. fixed tests to naming changes

anxolin

All positive changes since last review!

I added several replies and comments.
One of the most important things to get this merged, is to separate wallet connect from this PR until it's stable.

anxolin · 2019-08-27T17:14:02Z

src/components/DisplayWeb3.tsx

+        const setter = async (): Promise<void> => {
+            const accountsProm = web3Api.getAccounts()
+            const networkNameProm = web3Api.getNetworkName()
+            const clientVersionProm = web3Api.web3.currentProvider.send('web3_clientVersion', [])


I think there was already a web3 method for this.

Anyways. How do you handle the null provider thing we are discussing in the other thread.
As I wrote in the other thead. If the wallet is not connected. I think it's better to have a null web3Api and handle that case.

If the wallet re-connects, this component function will be called again with a non-null value

No universal method for that. Won't work with some wallets, and this method will

But one of my comments is, I saw in other parts of the PR that web3Api.web3.currentProvider can be null in some cases.

This will crash if it's the case, right?

Anyways, I also think is nor right to do the new Web3(null), we should handle the case differently too (there's other conversation for this topic)

Also, I would argue that the web3Api.web3.currentProvider.send(...) is a bit coupled, and since you already have an interface for interacting web3Api it would be simpler and more readable to do sth like web3Api.sendRpc(...)

anxolin · 2019-08-27T17:14:35Z

src/components/DisplayWeb3.tsx

+
+export const DisplayWeb3: React.FC<DisplayWeb3Props> = ({ web3Api }) => {
+    const [account, setAccount] = useState<string>()
+


supernit: I would get rid of this line breaks

anxolin · 2019-08-27T17:15:54Z

src/components/DisplayWeb3.tsx

+
+    useEffect(() => {
+        const setter = async (): Promise<void> => {
+            const accountsProm = web3Api.getAccounts()


I think it's better not to shorten the word promise :)

anxolin · 2019-08-27T17:17:50Z

src/components/DisplayWeb3.tsx

+                clientVersionProm,
+            ])
+
+            ReactDOM.unstable_batchedUpdates(() => {


what is unstable_batchedUpdates?
I'm not super familiar, made me curious the word "unstable"

It's what they use internally in event handlers to avoid multiple rerenders.
React-redux also uses it, so even if it's called unstable everyone does it

anxolin · 2019-08-27T17:19:03Z

src/components/Web3ConnectButton.tsx

+    const [web3Api, setWeb3FrontendAPI] = useState<Web3Api>()
+
+    return (
+        <>


I think this is not needed

anxolin · 2019-08-27T17:23:21Z

src/components/Web3ConnectButton.tsx

+            <Web3Connect.Button
+                providerOptions={{
+                    walletconnect: {
+                        bridge: 'https://safe-walletconnect.dev.gnosisdev.com/',


Is this hardcoded?

What is this bridge?

anxolin · 2019-08-27T17:25:09Z

src/globals.ts

+    GOERLI: 'GOERLI',
+}
+
+export const NETWORK_ID_TO_NAME = {


Why is not enough a single object for the networks?

anxolin · 2019-08-27T17:26:01Z

src/index.tsx

+        return getWeb3Api(options)
+    } catch (error) {
+        console.error('launchDappUI Error!', error)
+        throw new Error(error)


Why do we rethrow. Shouldn't we handle this error?

anxolin · 2019-08-27T17:30:13Z

src/types.ts

+    getTransaction: (txHash: string) => Promise<Transaction>
+    getTransactionReceipt: (txHash: string) => Promise<TransactionReceipt>
+    // Utils
+    isReadOnly: () => boolean


I don't think we need this. I would think this only make sense if we allow users to "sign in" by entering an address, so we show a read only version.

I think what you were using this for is to display a limited UI when there's an error connecting the wallet. But I think we can take other approaches (there's other open discussions about it, so no need to add it here too )

anxolin · 2019-08-27T17:31:26Z

src/utils/ethereum.ts

@@ -0,0 +1,3 @@
+import { isAddress, toBN, isBN, isBigNumber, fromAscii, toHex, toWei, fromWei } from 'web3-utils'
+
+export { isAddress, toBN, isBN, isBigNumber, fromAscii, toHex, toWei, fromWei }


Good one! I think it was the right move to move these utils from the API to here.

anxolin · 2019-08-27T17:39:36Z

@W3stside I see it has the WIP tag. Is there anything else you want to include in this PR?

anxolin

Lest resolve the open conversations before this gets merged.

Velenir · 2019-11-11T15:15:24Z

superseded but the waterfall 💧

W3stside and others added 20 commits August 14, 2019 16:09

setup package info

29ec362

setup packages and scripts

4a0d90f

fix module path resolution order

d6aebb4

add web3 providers

410064d

packages + linting/babel rules

6099dee

removed unnecessary code

39f7e8c

renaming files + exporting components

39c3954

API: init web3 api + tests

54b1f13

API utils file

5a2bc38

global TS typings

4daab53

removed unused files

22a97fe

DisplayWeb3 to show web3 data

b327907

Web3ConnectButton

3ba5072

styles file (styled components)

e331d8e

package index file with exports

85e83ca

examples > ts-frontend added

2e3c27a

updated README + .gitignore

f0ce5c6

packagelock / package.json

74bcae9

1. added start:examples-ts script to launch example app 2. added postinstall to get examples/ts-frontend setup

removed html from UI project, not necessary

dafa502

packagelock

bd80fc9

W3stside added the WIP label Aug 16, 2019

W3stside requested review from anxolin and Velenir August 16, 2019 14:19

W3stside self-assigned this Aug 16, 2019

anxolin reviewed Aug 18, 2019

View reviewed changes

anxolin suggested changes Aug 19, 2019

View reviewed changes

W3stside added 4 commits August 20, 2019 10:53

added Utils folder

0930a4f

1. ethereum utils imported from Web3 2. misc utils 3. exported from index

ui >> dapp-ui rename

4a47abd

types + globals file creation

7d536db

1. global file with constants 2. types file (global) 3. removed typings.d.ts + old utils file

tsconfig changes

49ca82f

1. allow synthetic imports into examples ts 2. removed unused lines

W3stside added 3 commits August 20, 2019 11:00

web3.ts api page changes (from PR review)

67266fd

1. moved types into global 2. clarified names of fns 3. removed async catching 4. fixed tests to naming changes

Fixed files for web3 api name changes

cfde101

move react & react-dom to dev, remove rhr

9365067

This was referenced Aug 21, 2019

Feature/web3 connect manual #4

Closed

added TX sending into DisplayWeb3 (testing) #5

Closed

anxolin suggested changes Aug 27, 2019

View reviewed changes

anxolin suggested changes Aug 30, 2019

View reviewed changes

Update web3.ts

82cf82e

Velenir closed this Nov 11, 2019


		Open app in `localhost:1234`

		For wallet connect, please use `test.walletconnect.org`


		export const DisplayWeb3: React.FC<DisplayWeb3Props> = ({ web3Api }) => {
		const [account, setAccount] = useState<string>()

		@@ -0,0 +1,3 @@
		import { isAddress, toBN, isBN, isBigNumber, fromAscii, toHex, toWei, fromWei } from 'web3-utils'

		export { isAddress, toBN, isBN, isBigNumber, fromAscii, toHex, toWei, fromWei }

Initial E2E Working Setup #1

Initial E2E Working Setup #1

Conversation

W3stside commented Aug 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin Aug 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin Aug 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin commented Aug 27, 2019

anxolin left a comment

Choose a reason for hiding this comment

Velenir commented Nov 11, 2019

W3stside commented Aug 16, 2019 •

edited

anxolin Aug 27, 2019 •

edited

anxolin Aug 27, 2019 •

edited