Feat: Add IPFS fallback logic to fetch metadata by 0xGabi · Pull Request #89 · aragon/connect

0xGabi · 2020-06-29T23:01:15Z

This PR includes a new async method create() for the entities that need metadata (App, Repo, and Role). This a requirement when creating the entity. We handle this during creation at the connector.

If we can't get the metadata from The Graph we use ethers to fetch with a GET request constructing the URL from the contentUri pointing to the default Aragon IPFS node.

Evalir

❤️ Asking some questions!

Evalir · 2020-06-30T00:31:16Z

-    }
-
    this.address = data.address
+    this.contentUri = data.contentUri ?? undefined


ES2020 🤩 wouldn't it be better to return null?

Thank for the suggestions. I think in this particular case the problem is that we are implementing the RepositoryData interface and the contentUri has those types. Regardless I agree there is something a bit odd here, and I believe it's a consequence of allowing data on The Graph to be undefined / null.

Anyways I like to know why you think null would be better than undefined?

It's probably due to my mental model regarding null & undefined; for me, null is the proper thing to use when there's no value. It indicates the absence of it. undefined, for me, indicates problems with fetching some data, a function that returns nothing (void) or some other anomalous development in processing data.

Agreed on the mental model, I see null as “empty”, while undefined is “nothing”. I also like to keep using the same type when it makes sense, e.g. here we could use "", or if it was a list we could use []: it removes the need to treat null / undefined as an exception for the consumer.

…I wish JS only had null 😆

Now that you mention about using an empty object of the particular type. I was wondering if we should do that at the subgraph level and always asure that at least we have an empty object. At least for the most part of the attributes.

Now that you mention about using an empty object of the particular type. I was wondering if we should do that at the subgraph level and always asure that at least we have an empty object. At least for the most part of the attributes.

I think it could be nice yes. 👍

codecov · 2020-06-30T13:55:47Z

Codecov Report

Merging #89 into master will decrease coverage by 0.70%.
The diff coverage is 19.84%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   28.82%   28.12%   -0.71%     
==========================================
  Files          53       54       +1     
  Lines         836      889      +53     
  Branches      137      145       +8     
==========================================
+ Hits          241      250       +9     
- Misses        595      639      +44

Flag	Coverage Δ
#unittests	`28.12% <19.84%> (-0.71%)`	⬇️

Impacted Files	Coverage Δ
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/params.ts	`0.00% <0.00%> (ø)`
packages/connect-core/src/utils/metadata.ts	`0.00% <0.00%> (ø)`
...ckages/connect-thegraph/src/core/GraphQLWrapper.ts	`43.39% <25.00%> (ø)`
packages/connect-thegraph/src/parsers/roles.ts	`68.96% <80.00%> (-1.41%)`	⬇️
packages/connect-thegraph/src/parsers/apps.ts	`93.33% <100.00%> (+1.33%)`	⬆️
... and 3 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 d12ca93...079ed25. Read the comment docs.

bpierre

I left some comments, let me know what you think!

bpierre · 2020-07-01T12:28:11Z

+  abi?: Abi
+  appName?: string
+  author?: string
+  contractPath?: string
+  deprecatedIntents?: { [version: string]: AppIntent[] }
+  description?: string
+  icons?: { src: string; sizes: string }[]
+  intents?: AppIntent[]
+  htmlUrl?: string
+  sourceUrl?: string


Ah I didn’t think about that… I’m wondering if it’s worth using readonly at all then?

If we are going to use a method _init() / create() always required probably not worth it. I can't think other alternatives to try to have readonly otherwise.

bpierre · 2020-07-01T12:29:42Z

+    const {
+      appName,
+      path,
+      functions,
+      deprecatedFunctions,
+      abi,
+    }: AragonArtifact = await resolveMetadata(
+      'artifact.json',
+      this.contentUri!,
+      this.#artifact
+    )
+
+    const {
+      author,
+      description,
+      start_url: htmlUrl,
+      icons,
+      source_url: sourceUrl,
+    }: AragonManifest = await resolveMetadata(
+      'manifest.json',
+      this.contentUri!,
+      this.#manifest
+    )


We should probably try / catch these two, and emit contextual errors.

I decide to handle it in the resolveMetadata function. What you think?

0xGabi · 2020-07-02T04:08:40Z

As we discussed with Pierre I move the logic of _init() to create() instead. Then I move the method call to the connector so we have both the instantiation of a new object and the call to create() method together.

0xGabi · 2020-07-02T14:55:11Z

Note I split the PRs to keep the changes more clear.

sohkai · 2020-07-03T10:13:45Z

We use ethers to fetch with a GET request constructing the URL from the contentUri pointing to the default Aragon IPFS node

I was thinking about this (moreso in the context of a CLI/frontend using connect to connect to generic apps that may or may not be hosted by our default IPFS node), and I think it's important to provide a way to override the default IPFS gateway used.

bpierre · 2020-07-03T16:00:06Z

+    return this.#created
+  }
+
+  async create({ artifact, manifest, ...data }: AppData): Promise<void> {


For create(), I was more thinking of a static method, that would fetch everything needed and instantiate, so we can do this:

const app = await App.create(/* … */)

Rather than this:

const app = new App(/* … */) await app.create()

That way, App would never be in a state where it is instantiated, but not initialized.

Oh, I didn't think about that. I'll include this change as a new PR.

bpierre · 2020-07-03T16:00:57Z

+      this.htmlUrl = htmlUrl
+      this.kernelAddress = data.kernelAddress
+      this.name = data.name
+      this.registry = data.registry || undefined


Suggested change

this.registry = data.registry || undefined

this.registry = data.registry

Co-authored-by: Pierre Bertet <hello@pierre.world>

…allback

0xGabi added 2 commits June 29, 2020 19:55

Feat: Add IPFS fallback logic to fetch metadata

abaa729

Update docs, example and incude params values

3088780

0xGabi requested review from bpierre and eternauta1337 June 29, 2020 23:01

0xGabi commented Jun 29, 2020

View reviewed changes

Comment thread packages/connect-core/src/entities/Organization.ts Outdated

Evalir reviewed Jun 30, 2020

View reviewed changes

0xGabi added 2 commits June 30, 2020 10:45

Add Promise.all & fix tests

ea5da37

Update token connector test

6a4be50

0xGabi marked this pull request as ready for review June 30, 2020 14:00

bpierre reviewed Jul 1, 2020

View reviewed changes

0xGabi added 5 commits July 1, 2020 22:21

Merge remote-tracking branch 'origin/master' into ipfs-fallback

f864f16

Merge remote-tracking branch 'origin/master' into ipfs-fallback

a6307ee

Feat: Make main connector parsers async

a872af6

Feat: Update core library to new connector changes

8b2fc57

Fix tests & handle metada errors

d25b70d

0xGabi requested review from Evalir and bpierre July 2, 2020 03:58

bpierre reviewed Jul 3, 2020

View reviewed changes

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

bpierre reviewed Jul 3, 2020

View reviewed changes

0xGabi and others added 5 commits July 3, 2020 13:24

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

28b48e3

Co-authored-by: Pierre Bertet <hello@pierre.world>

Merge remote-tracking branch 'origin/master' into ipfs-fallback

7e2f8ee

Merge branch 'ipfs-fallback' of github.com:aragon/connect into ipfs-f…

f1366de

…allback

Update yarn.lock

2ee4a98

Fix type check

079ed25

0xGabi merged commit 12ec1da into master Jul 3, 2020

0xGabi deleted the ipfs-fallback branch July 3, 2020 16:38

	this.registry = data.registry \|\| undefined
	this.registry = data.registry

Conversation

0xGabi commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xGabi Jun 30, 2020 • edited by Evalir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bpierre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

0xGabi commented Jul 2, 2020

Uh oh!

0xGabi commented Jul 2, 2020

Uh oh!

sohkai commented Jul 3, 2020

Uh oh!

bpierre Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0xGabi commented Jun 29, 2020 •

edited

Loading

0xGabi Jun 30, 2020 •

edited by Evalir

Loading

codecov Bot commented Jun 30, 2020 •

edited

Loading

bpierre Jul 3, 2020 •

edited

Loading