connectApp() #135

bpierre · 2020-07-14T13:34:40Z

Fixes #109
Builds on #136, #137

App connection

This PR adds a new way to connect apps:

import { connect } from '@aragon/connect'
import connectVoting from '@aragon/connect-voting'

const org = await connect('myorg.aragonid.eth', 'thegraph')
const voting = await org.app('voting')
const votingConnected = await connectVoting(voting)
const votes = await votingConnected.votes()

connectVoting() accepts either an App or a promise resolving to an App, allowing this syntax:

import { connect } from '@aragon/connect'
import connectVoting from '@aragon/connect-voting'

const myorg  = await connect('myorg.aragonid.eth', 'thegraph')
const voting = await connectVoting(myorg.app('voting'))
const votes  = await voting.votes()

Usage from React

This is how the same task can get accomplished using the React library:

import { createAppHook, useApp } from '@aragon/connect-react'
import connectVoting from '@aragon/connect-voting'

// We need to create a hook corresponding to the app connector
const useVoting = createAppHook(connectVoting)

function Votes() {
  const [voting] = useApp('voting')
  const [votes] = useVoting(voting, app => app.votes())
  return (
    <ul>
      {votes ? (
        votes.map(vote => <li key={vote.id}>{formatVote(vote)}</li>)
      ) : (
        <li>Loading votes…</li>
      )}
    </ul>
  )
}

Note: this doesn’t support subscriptions yet. I plan to introduce a separate change in the app connectors that will allow to subscribe using the exact same syntax as above.

App connector authoring

This is how an app connector can get implemented in the most basic way:

import { createAppConnector } from '@aragon/connect-core'

export default createAppConnector(() => ({
  // This object is going to get extended by the original App object.
  total: () => fetchTotal()
}))

And this is how the Voting app connector is implemented:

import { createAppConnector } from '@aragon/connect-core'
import Voting from './entities/Voting'
import VotingConnectorTheGraph, {
  subgraphUrlFromChainId,
} from './thegraph/connector'

type Config = {
  subgraphUrl: string
}

// The app and Config types are optional but if passed,
// they will be properly forwarded to the returned function.

export default createAppConnector<Voting, Config>(
  ({ app, config, connector, network, verbose }) => {
    if (connector !== 'thegraph') {
      console.warn(
        `Connector unsupported: ${connector}. Using "thegraph" instead.`
      )
    }

    return new MyAppConnector(
      new VotingConnectorTheGraph(
        config.subgraphUrl ?? subgraphUrlFromChainId(network.chainId),
        verbose
      ),
      app.address
    )
  }
)

Status

Make the app connectors universal
- Refactor Voting to use the new API + support multiple connectors.
- Refactor Tokens to use the new API + support multiple connectors.
- Rename @aragon/connect-thegraph-voting to @aragon/connect-voting.
- Rename @aragon/connect-thegraph-tokens to @aragon/connect-tokens.
Update documentation.
Find a way to tell TS about the connected app type.
Provide a utility function that lets authors build an app connector by extending App.
@aragon/connect-react compatibility.

Other changes

Export connect as a default export of @aragon/connect.
Add a toNetwork() function that transforms Networkish into Network.
Organization now receives a single ConnectionContext object.
Prepare the ipfs resolver from the options, injects it into ConnectionContext.
provider => ethersProvider, readProvider => ethereumProvider.
Move the initialization steps into connect() rather than doing it in Organization.
Organization: remove the _connect() method.
Add new demos: list-votes-cli and list-votes-react.
Update the documentation of app connectors (and remove subgraph / internal documentation from it).

Changes: - Pass a ConnectionContext instance to Organization. - ConnectorInterface is now IOrganizationConnector. - IOrganizationConnector: add a name property. - Prepare the ipfs resolver and injects it into ConnectionContext. - provider => ethersProvider, readProvider => ethereumProvider. - Move the initialization into connect() rather than Organization. - Organization: accept a single ConnectionContext object. - Organization: remove the _connect() method. - Move everything to the SubscriptionHandler type. - Coding style.

codecov · 2020-07-14T13:34:52Z

Codecov Report

Merging #135 into master will decrease coverage by 6.85%.
The diff coverage is 37.97%.

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   23.92%   17.07%   -6.86%     
==========================================
  Files          59       59              
  Lines        1049     1183     +134     
  Branches      168      205      +37     
==========================================
- Hits          251      202      -49     
- Misses        798      981     +183

Flag	Coverage Δ
#unittests	`17.07% <37.97%> (-6.86%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ages/connect-core/src/connections/ConnectorJson.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/entities/App.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/entities/Organization.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/entities/Permission.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/entities/Repo.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/entities/Role.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/index.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/utils/app-connectors.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/utils/index.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/utils/metadata.ts	`0.00% <0.00%> (ø)`
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c9ec43...d154577. Read the comment docs.

It also now contains the name and network properties.

commit 6456de9 Author: Pierre Bertet <hello@pierre.world> Date: Tue Jul 14 16:08:04 2020 +0100 ConnectorInterface is now IOrganizationConnector It also now contains the name and network properties.

0xGabi

Awesome PR @bpierre 💪 Every package is improving a lot with the latest changes.

There is an important point I notice on the EhtersProvider initialization. We should figure out if we can override the ensAddress otherwise create a WebSocket provider like we are doing for xDai.

I think this solution is already improving the developer experience even it's still a two-step process.

I like your second suggestion about using a method at the core organization level. What you think about naming it: connectApp? Regardless I will go ahead with this solution first.

0xGabi · 2020-07-15T19:48:58Z

examples/list-votes-cli/src/index.ts

+  printOrganization(org)
+
+  const votingInfo = await org.app('voting')
+  const voting = await votingInfo.connect(votingConnector())


Boom ⚡️

packages/connect-core/src/connections/IAppConnected.ts

packages/connect-core/src/entities/App.ts

0xGabi · 2020-07-15T20:38:43Z

packages/connect-core/src/entities/Organization.ts

@@ -65,8 +64,8 @@ export default class Organization {
    return this.#connection.orgAddress
  }

-  get provider(): ethers.providers.Provider {
-    return this.#connection.ethersProvider
+  get _connection(): ConnectionContext {


0xGabi · 2020-07-15T20:45:37Z

packages/connect-thegraph-voting/src/connector.ts

      queries.ALL_VOTES('query'),
      { appAddress, first, skip },
-      parseVotes
+      result => parseVotes(result, this)


I like this syntax a lot more!

0xGabi · 2020-07-15T20:47:10Z

packages/connect-thegraph-voting/src/index.ts

+  if (chainId === 1) {
+    return 'https://api.thegraph.com/subgraphs/name/aragon/aragon-voting-mainnet'
+  }
+  if (chainId === 4) {


We should deploy the apps into xDai 😅. Made this issue: #141

examples/list-votes-cli/src/index.ts

packages/connect/src/connect.ts

So that connect-core can do as much as possible to simplify the implementation of an app connector. createAppConnector() creates a function ready to be exported by the app connector package, like connectVoting(). It passes to its callback an object providing contextual information about the connection, the current app, the ipfs provider, verbosity, etc. It waits for the app if a promise is provided, and emits an error if the type is incorrect. It handles the possibility to pass a connector name, or a connector name and its configuration, as a secondary parameter. It extends the passed object with the original App. Lastly, it forwards the types of the configuration and the extended app, if declared.

0xGabi

Looking really good, let's merge and try to release sonnish, I want people to start using this new way of working with Connectors! 🙌

0xGabi · 2020-07-24T20:43:26Z

docs/connectors/tokens-app.md


 To create a new instance of the connector, you need the specific Tokens app address and a Subgraph URL:

-```javascript
-import { TokenManager } from '@aragon/connect-thegraph-tokens'
+```js


The new way is much better! I like that you only interact with the Connect library.

0xGabi · 2020-07-24T20:49:38Z

examples/list-balances-cli/src/index.ts

@@ -0,0 +1,59 @@
+import TokenAmount from 'token-amount'


This isolated examples of how to use the connectors are great. Good idea! 💪

0xGabi · 2020-07-24T20:52:22Z

packages/connect-voting/src/__test__/casts.test.ts

@@ -1,25 +1,25 @@
 import { VotingConnectorTheGraph, Vote, Cast } from '../../src'

-const VOTING_SUBGRAPH_URL = 'https://api.thegraph.com/subgraphs/name/aragon/aragon-voting-rinkeby'
+const VOTING_SUBGRAPH_URL =


I wonder if we should refactor the tests to use the new API or just connect at a lower level like we are currently doing?

Yes I think we could definitely improve the way we test things now that objects are instantiated by the library itself. Maybe the JSON provider could be useful here too. Do you see things we could do now, or should we talk about it separately?

Yes, let's first release version 0.5 and refactor the Intent API and afterwards we can concentrate fully on tests.

0xGabi · 2020-07-24T20:54:34Z

packages/connect-voting/src/thegraph/connector.ts

+  if (chainId === 1) {
+    return 'https://api.thegraph.com/subgraphs/name/aragon/aragon-voting-mainnet'
+  }
+  if (chainId === 4) {


I'll publish the Tokens and Voting app subgraphs on xDai so we release with support for that network at the app connector level as well.

packages/connect/src/connect.ts

…onnection-context

Evalir

That was long, but it looks awesome! Just left some comments (most of them doubts):

Evalir · 2020-07-28T13:22:10Z

docs/connectors/voting-app.md

+const org = connect('myorg.aragonid.eth', 'thegraph')
+const voting = connectVoting(org.app('voting'))


It's so short now! 🏎️

Evalir · 2020-07-28T13:24:32Z

examples/connect-react-intro/tsconfig.json

@@ -1,11 +1,14 @@
 {
-  "extends": "../../tsconfig.json",
+  "extends": "@snowpack/app-scripts-react/tsconfig.base.json",


Snowpack 👁️ ❄️

Evalir · 2020-07-28T13:27:52Z

examples/nodejs/src/create-karma-template.ts

+  const provider = new ethers.providers.JsonRpcProvider(env.rpc, env.network)
+  const wallet = new ethers.Wallet(env.key)
+  const signer = wallet.connect(provider)


Should we print the public key of the wallet?

Evalir · 2020-07-28T13:29:55Z

examples/nodejs/src/helpers.ts

    name,
  })
+
+  return data.repos[0]


Hmmm, if this returns null (due to a network error or something of the sort), this may just explode. This is probably the reason that makes connectors break if we update our subgraphs as well. Should we prefix it with ?, or should we just let the user handle it?

(this was part of another PR sorry :s)

It seems like it yes. We should definitely improve the reliability of the parts where data is fetched, it’s something I want to do soon alongside providing better errors. I also think we should consider GraphQLWrapper to be an object used internally or by app connectors, but not by end users or demos.

Evalir · 2020-07-28T13:41:03Z

packages/connect-core/src/utils/misc.ts

+  }
+
+  if (!value) {
+    throw new Error(`Network: incorrect value provided.`)


Hmm, shouldn't we say something like You must provide a "value" argument to the network config?

Yes we should have something like this at an upper level, but in toNetwork() there is no concept of configuration yet. I plan to add custom errors for the next version, which will make it much easier to do things like this! 👍

Evalir · 2020-07-28T13:41:50Z

packages/connect-core/src/utils/misc.ts

+  if (value.chainId === undefined) {
+    throw new Error(`Network: no chainId provided.`)
+  }
+
+  if (value.name === undefined) {


This was probably for clarity :P But shouldn't we do !value.chainId / !value.name?

This is to accept a chainId being 0 or a name being "", just in case someone does that 😄

0xGabi

Awesome work!! Super happy to see this one merged 🙌

bpierre added 5 commits July 14, 2020 00:25

Types: add Address + SubscriptionHandler

a583954

connect-core: add ConnectionContext

ea59883

Add App#connect()

f72801e

Add a simple demo to list votes

beaa976

bpierre requested a review from 0xGabi July 14, 2020 13:34

Types: add SubscriptionHandler, Address

923e2c2

bpierre mentioned this pull request Jul 14, 2020

Types: add SubscriptionHandler, Address #136

Merged

Merge branch 'subscription-handler' into connection-context

b52cef2

bpierre changed the base branch from master to connection-context-extra July 14, 2020 14:24

bpierre added 3 commits July 14, 2020 16:22

ConnectorInterface is now IOrganizationConnector

6456de9

It also now contains the name and network properties.

Squashed commit of the following:

d908e3c

commit 6456de9 Author: Pierre Bertet <hello@pierre.world> Date: Tue Jul 14 16:08:04 2020 +0100 ConnectorInterface is now IOrganizationConnector It also now contains the name and network properties.

Merge branch 'connector-interface-renaming' into connection-context

370b46c

bpierre changed the base branch from connection-context-extra to master July 14, 2020 15:38

bpierre changed the base branch from master to connection-context-extra July 14, 2020 15:38

bpierre added 2 commits July 14, 2020 16:40

Merge branch 'connection-context-extra' into connection-context

395f60d

Pass ConnectionContext to every entity

0e51db9

0xGabi suggested changes Jul 15, 2020

View reviewed changes

bpierre added 11 commits July 17, 2020 12:16

New connectApp() API

841c1c8

Add toNetwork()

90eb80e

Update Ethers to 5.0.6 (fix the incorrect ENS address issue)

78c0c21

Types: let Networkish accept incomplete network objects

f066437

ConnectionContext: add verbose

3f90a60

connect-core: export @aragon/connect-types

e3c2393

connect-thegraph-voting: multiple connectors + inherit connection

5bf7560

Examples: improve the list-votes-cli output

dc05673

Update yarn.lock

417585f

App connectors: extend the App entity

3be00b5

bpierre added 6 commits July 21, 2020 14:39

createAppConnector(): async callback

965b83d

connect-thegraph-tokens: universal + new connectors API

e231a5d

New demo: list-balances-cli

1c7bbfa

Documentation: update app connectors

fdf1ea8

@aragon/connect: export connect() as a default

7b84648

Rename connect-thegraph-* into connect-*

ff5d975

bpierre mentioned this pull request Jul 21, 2020

Connectors: add a disconnect() method (fix hanging tests) #140

Merged

bpierre added 3 commits July 22, 2020 02:10

Merge branch 'master' into connection-context-extra

d521631

Merge branch 'connection-context-extra' into connection-context

b831ab5

Fix tests

2d3ab9e

0xGabi approved these changes Jul 24, 2020

View reviewed changes

0xGabi and others added 12 commits July 27, 2020 12:55

Connector: Add default url for xDai (#157)

f0dc64b

Merge branch 'master' into connection-context

098b551

Make examples private

bc9dec6

Bump ethers to 5.0.7

42beabc

connect-core: export types explicitly

5af702d

connect-react: add createAppHook()

f9f96a9

Voting: align .votes() and .onVotes() filters

a2dbb76

rebuild-packages.sh: re-enable the app connectors

fe6a481

Update yarn.lock

9281750

Add a new demo: list-votes-react

1e0d87c

Merge branch 'connection-context' of github.com:aragon/connect into c…

d00f610

…onnection-context

connect: use a constants.js file

efcce03

bpierre requested a review from 0xGabi July 28, 2020 12:43

Evalir approved these changes Jul 28, 2020

View reviewed changes

bpierre changed the base branch from connection-context-extra to master July 28, 2020 21:28

Merge branch 'master' into connection-context

d154577

bpierre merged commit 26c7c28 into master Jul 29, 2020

bpierre deleted the connection-context branch July 29, 2020 02:48

0xGabi reviewed Jul 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connectApp() #135

connectApp() #135

bpierre commented Jul 14, 2020 •

edited

Loading

codecov bot commented Jul 14, 2020 •

edited

Loading

0xGabi left a comment

0xGabi Jul 15, 2020

0xGabi Jul 15, 2020

0xGabi Jul 15, 2020

0xGabi Jul 15, 2020

0xGabi left a comment

0xGabi Jul 24, 2020

0xGabi Jul 24, 2020

0xGabi Jul 24, 2020

bpierre Jul 28, 2020

0xGabi Jul 29, 2020

0xGabi Jul 24, 2020

Evalir left a comment

Evalir Jul 28, 2020

Evalir Jul 28, 2020

Evalir Jul 28, 2020

Evalir Jul 28, 2020

bpierre Jul 29, 2020

Evalir Jul 28, 2020

bpierre Jul 29, 2020

Evalir Jul 28, 2020

bpierre Jul 29, 2020

0xGabi left a comment

		const org = connect('myorg.aragonid.eth', 'thegraph')
		const voting = connectVoting(org.app('voting'))

connectApp() #135

connectApp() #135

Conversation

bpierre commented Jul 14, 2020 • edited Loading

App connection

Usage from React

App connector authoring

Status

Other changes

codecov bot commented Jul 14, 2020 • edited Loading

Codecov Report

0xGabi 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

0xGabi 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

Evalir 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

0xGabi left a comment

Choose a reason for hiding this comment

bpierre commented Jul 14, 2020 •

edited

Loading

codecov bot commented Jul 14, 2020 •

edited

Loading