IPFS metadata storage #86

wildmolasses · 2021-08-04T20:08:38Z

This PR resolves #67

Important note: This PR uses a trick to accomplish the metaPtr resolution. Instead of resolving the metaPtr directly, it splits the URL, takes the CID, and resolves that with our fleek ipfs connection instance. Even if a metaPtr specified the cloudflare gateway, it would still use fleek to resolve. If a metaPtr did not have a CID in the last position of the URL, the fetch would completely fail.

The metadata fetching isn't robust. If the fetch fails, the metadata status will be 'error' and it will not reattempt a fetch.

There are no UI states yet corresponding to the metadata fetch status states of "pending", "resolved", "error"

apbendi · 2021-08-05T20:03:07Z

Great start @wildmolasses! Before making any changes to the frontend, be sure to rebase to main

thelostone-mc

left a couple of comments

app/index.html

app/src/shims.d.ts

thelostone-mc · 2021-08-17T07:33:08Z

app/src/utils/ipfs.ts

+  return ipfsFetch({
+    cid: url.split('/').reverse()[0],
+  });
+};


^ would it be worth adding comments to the functions on

what it does

what are the arguments

It would be useful if we decide to add in auto doc generation

Agreed, more comments would be useful!

Re docgen, personally I have not been using NatSpec everywhere, so we'll need some comments cleanup before we do docgen, but you're right that is probably best practice going forward

app/src/config.ts

thelostone-mc · 2021-08-17T07:38:43Z

app/src/store/data.ts

@@ -159,6 +161,28 @@ export default function useDataStore() {
    lastBlockTimestamp.value = (timestamp as BigNumber).toNumber();
    grants.value = grantsList as Grant[];
    grantRounds.value = grantRoundsList as GrantRound[];
+    const metaPtrs = grants.value.map((grant) => grant.metaPtr);
+    const newMetadata = metaPtrs
+      .filter((metaPtr) => !grantMetadata.value[metaPtr]) // only want new metaPtrs


Just double checking -> it looks like a grant can have multiple metadata but we want only the which as status as pending ?
not super clear on what we are doing here

i'll try to add some comments!

thelostone-mc · 2021-08-17T07:39:35Z

app/src/utils/utils.ts

@@ -28,6 +28,10 @@ export function isValidAddress(val: string | undefined) {
  return val && isAddress(val);
 }

+export function isDefined(val: unknown) {
+  return !!val;
+}


I'd recommend moving this to utils package

I think we should hold off on this for now per #114.

Though it also feels like this function is simple enough that it can be removed which might even be more readable (e.g. people know what Boolean() or !! does, but would have to look up what isDefined does

i'm using it for form validation; would Boolean() work just as well? I like how "isDefined" makes things a little more explicit but could defer to you guys!

app/src/store/data.ts

app/src/utils/ipfs.ts

app/src/views/GrantRegistryGrantDetail.vue

mds1 · 2021-08-17T14:52:54Z

app/src/views/GrantRegistryNewGrant.vue

+  const form = ref<{ owner: string; payee: string; name: string; description: string }>({
    owner: '',
    payee: '',
-    metaPtr: '',
+    name: '',
+    description: '',


Should we define a local Form type, since I imagine we'll be adding more fields later too?

If it's only used once I don't think it's necessary, but would defer to you guys

mds1 · 2021-08-17T14:54:36Z

app/src/views/GrantRegistryNewGrant.vue

+    const metaPtr = await ipfs
+      .createGrant({ name, description })
+      .then((cid) => ipfs.createMetaPtr({ cid: cid.toString() }));


Just curious why you use a .then instead of

Suggested change

const metaPtr = await ipfs

.createGrant({ name, description })

.then((cid) => ipfs.createMetaPtr({ cid: cid.toString() }));

const cid = await ipfs.createGrant({ name, description })

const metaPtr = await ipfs.createMetaPtr({ cid: cid.toString() }));

my general rule is that if something is used once there's no reason to declare it, i.e. since we don't use cid elsewhere there's no reason to declare it. i break this sometimes for readability's sake, but then clauses are pretty readable to me. Thoughts?

apbendi

Overall looking and working great, @wildmolasses. Nice job getting this going.

My only comment is on the way we're processing the metaPtr URI. Instead of hacking off the CID from the end— and assuming it's there in the first place— I think we should just have a whitelisted set of endpoints from which the app will fetch content from. This should be separate from the concern of whether the endpoint is an IPFS gateway or not.

In other words, we should have a hardcoded list of valid endpoints, which can include just https://ipfs-api.dev.fleek.cool for now (or also https://cloudflare-ipfs.com if we want) and when the metaPtr has a url at one of those endpoints, we should fetch the contents. If it doesn't, it shouldn't fetch, and should probably have an error message like "Unsupported source for grant metadata" or something like that.

Curious to hear what others think of this approach cc @thelostone-mc @mds1 @gdixon

wildmolasses · 2021-08-18T17:48:50Z

Overall looking and working great, @wildmolasses. Nice job getting this going.

My only comment is on the way we're processing the metaPtr URI. Instead of hacking off the CID from the end— and assuming it's there in the first place— I think we should just have a whitelisted set of endpoints from which the app will fetch content from. This should be separate from the concern of whether the endpoint is an IPFS gateway or not.

In other words, we should have a hardcoded list of valid endpoints, which can include just https://ipfs-api.dev.fleek.cool for now (or also https://cloudflare-ipfs.com if we want) and when the metaPtr has a url at one of those endpoints, we should fetch the contents. If it doesn't, it shouldn't fetch, and should probably have an error message like "Unsupported source for grant metadata" or something like that.

Curious to hear what others think of this approach cc @thelostone-mc @mds1 @gdixon

In principle I know that's correct, however different endpoints have different auth models -- well, really, fleek is the one where we need an api key in order to fetch. So if we're going to use the fleek endpoint as our metaPtr, do we want to write conditional logic where if the metaPtr is fleek (which is currently the case for any grant created by this app) then use an api key, and otherwise proceed without auth token? it starts to feel a little weird, but maybe this is acceptable.

apbendi · 2021-08-18T20:24:41Z

In principle I know that's correct, however different endpoints have different auth models -- well, really, fleek is the one where we need an api key in order to fetch. So if we're going to use the fleek endpoint as our metaPtr, do we want to write conditional logic where if the metaPtr is fleek (which is currently the case for any grant created by this app) then use an api key, and otherwise proceed without auth token? it starts to feel a little weird, but maybe this is acceptable.

Per our discussion offline, I think the right approach here is to use keep metaptr fetching dumb, so an http endpoint that requires an API key should not be used. For the immediate term, we can use a whitelisted cloudflare (or another open IPFS gateway) as the metaPtr URL, therefore allowing us to fetch without any authentication.

wildmolasses force-pushed the ipfs-poc branch 2 times, most recently from fd048be to 76262b4 Compare August 4, 2021 20:27

apbendi mentioned this pull request Aug 10, 2021

Implement Cart Checkout (spec) #94

Closed

17 tasks

wildmolasses force-pushed the ipfs-poc branch 3 times, most recently from cbb988c to 1e8218d Compare August 14, 2021 03:07

wildmolasses marked this pull request as ready for review August 14, 2021 03:44

wildmolasses requested review from mds1, gdixon, phutchins, apbendi and thelostone-mc August 15, 2021 22:15

wildmolasses force-pushed the ipfs-poc branch 2 times, most recently from b174305 to bdd8f0d Compare August 16, 2021 15:30

mds1 mentioned this pull request Aug 16, 2021

Cart page setup #111

Merged

thelostone-mc reviewed Aug 17, 2021

View reviewed changes

mds1 reviewed Aug 17, 2021

View reviewed changes

app/src/store/data.ts Outdated Show resolved Hide resolved

mds1 reviewed Aug 17, 2021

View reviewed changes

app/src/utils/ipfs.ts Outdated Show resolved Hide resolved

mds1 reviewed Aug 17, 2021

View reviewed changes

app/src/views/GrantRegistryGrantDetail.vue Outdated Show resolved Hide resolved

mds1 reviewed Aug 17, 2021

View reviewed changes

wildmolasses force-pushed the ipfs-poc branch from 0f34267 to 49299c3 Compare August 18, 2021 04:53

apbendi reviewed Aug 18, 2021

View reviewed changes

This was referenced Aug 19, 2021

Grant Round Minimal Metadata Implementation #118

Closed

Add Grant Logo To Metadata #119

Closed

thelostone-mc force-pushed the ipfs-poc branch from 5762326 to 8189da3 Compare August 19, 2021 18:06

ipfs poc

79a93e7

wildmolasses added 4 commits August 19, 2021 23:37

integrate ipfs into grant create

15ffe96

resolve grant metadata in data store and adjust UI

4674618

excludeDeps in vite config

0789a5a

PR feedback

0299e5f

thelostone-mc force-pushed the ipfs-poc branch from 8189da3 to 0299e5f Compare August 19, 2021 18:10

thelostone-mc added 2 commits August 20, 2021 00:08

remove duplicate

ebc46e0

make logo URI optional

27abe91

thelostone-mc approved these changes Aug 19, 2021

View reviewed changes

mds1 merged commit 422e669 into main Aug 19, 2021

This was referenced Aug 20, 2021

feat: dcurve #91

Merged

feat: fetch GrantRoundDetails from ipfs #128

Merged

thelostone-mc deleted the ipfs-poc branch September 6, 2021 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IPFS metadata storage #86

IPFS metadata storage #86

wildmolasses commented Aug 4, 2021 •

edited

Loading

apbendi commented Aug 5, 2021

thelostone-mc left a comment

thelostone-mc Aug 17, 2021

mds1 Aug 17, 2021

thelostone-mc Aug 17, 2021

wildmolasses Aug 19, 2021

thelostone-mc Aug 17, 2021

mds1 Aug 17, 2021

wildmolasses Aug 19, 2021

mds1 Aug 17, 2021

wildmolasses Aug 18, 2021

mds1 Aug 17, 2021

wildmolasses Aug 18, 2021

apbendi left a comment

wildmolasses commented Aug 18, 2021 •

edited

Loading

apbendi commented Aug 18, 2021

IPFS metadata storage #86

IPFS metadata storage #86

Conversation

wildmolasses commented Aug 4, 2021 • edited Loading

apbendi commented Aug 5, 2021

thelostone-mc 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

apbendi left a comment

Choose a reason for hiding this comment

wildmolasses commented Aug 18, 2021 • edited Loading

apbendi commented Aug 18, 2021

wildmolasses commented Aug 4, 2021 •

edited

Loading

wildmolasses commented Aug 18, 2021 •

edited

Loading