fix logoURI split error #522

metafraction · 2021-11-18T17:10:27Z

resolves #521

app/src/views/GrantRegistryNewGrant.vue

metafraction · 2021-11-23T17:58:15Z

@apbendi @mds1 I think this is a good solution

{
  protocol: 1,
  pointer: cid ? cid : 'placeholder_grants.svg',
}

what this does is that it inserts the placeholder image if no logo has been uploaded, and it also resolves #532

just one question is about protocol, I believe 1 is fine now since it won't have an empty cid field

mds1 · 2021-11-23T18:39:46Z

@mk1r That solution doesn't really make sense with our MetaPtr spec—the snippet you have would end up trying to load a hash of placeholder_grants.svg from IPFS (because protocol is 1), and of course that is not a valid IPFS hash.

The fact that we fallback to placeholder_grants.svg when there is no image should be independent of that metadata object. Is there a reason we can't go with Ben's suggested approach?

metafraction · 2021-11-23T18:43:55Z

Hmm interesting you mention we are loading a hash, because I tested it out and it was retrieving the placeholder_grants.svg image - could be that if it returns incorrect data, it falls back (but then not sure why it doesn't fall back if there is an empty cid as stated in #532)

mds1 · 2021-11-23T18:53:10Z

Huh interesting, that sounds like a bug then because it should be resolving the pointers using ptrToURI which uses getMetaPtr.

Either way, even though it does work we don't want to conflate the protocol-level spec (the MetaPtr format) with the app behavior (fallback to placeholder_grants.svg). Those should be independent of each other and not coupled in the way they are here

Is there a reason we don't want to use this approach?

metafraction · 2021-11-23T19:01:02Z

@mds1 alright sounds good, and no that approach should work - just thought the approach I mentioned was cleaner and would solve #532 also

apbendi

@mk1r, I created a grant without a logo, but viewing the homepage with this branch, I still get the error "Unsupported Protocol ID 0". The app should not throw an error if there is an unsupported protocol, it should gracefully degrade and show the placeholder image instead.

So to recap:

When creating a grant, if no image is provided, the logo metaptr should be set to 0/''
When loading a grant, if the logo metaptr has a protocol value of anything other than 1, the app should load the placeholder image.

Does this make sense?

metafraction · 2021-11-24T17:49:36Z

@apbendi interesting, I'm not able to reproduce that error - for me, it's doing what you describe in the recap

I'll also double check the code in homepage (which I haven't modified) - which network are you using btw?

apbendi · 2021-11-24T17:52:57Z

@mk1r I'm on Polygon.

for me, it's doing what you describe in the recap

Maybe I'm missing something, but it looks like we're still throwing here, so I don't see how this would be the case: https://github.com/dcgtc/dgrants/pull/522/files#diff-e87dc6bd5d221441ff0d6a39efdce1b52d884442b20b73c3457e68aca79b29dcR18

metafraction · 2021-11-24T17:57:53Z

@apbendi but it shouldn't throw when protocol is 0 right? https://github.com/dcgtc/dgrants/pull/522/files#diff-e87dc6bd5d221441ff0d6a39efdce1b52d884442b20b73c3457e68aca79b29dcR17

apbendi · 2021-11-24T18:01:43Z

@mk1r Ahh yes that's true, but we shouldn't be special casing protocol 0, we should be failing gracefully for all non-1 values.

As for why this isn't working for me, it's not clear. If you test against Polygon right now (where my no-logo grant exists) what are you able to reproduce the error?

metafraction · 2021-11-24T18:07:47Z

@apbendi think I found the issue, it's in utils - will push out an update, good find

metafraction · 2021-11-24T18:16:18Z

try now @apbendi rebased as well

mds1 · 2021-11-24T18:17:34Z

app/src/utils/data/ipfs.ts

@@ -54,7 +55,7 @@ export const getMetaPtr = ({ cid }: { cid: string | undefined }) => {
 */
 export const formatMetaPtr = (cid: string): MetaPtr => {
  return {
-    protocol: 1, // IPFS has a protocol ID of 1
+    protocol: cid ? 1 : 0, // IPFS has a protocol ID of 1


Why is this needed? We shouldn't have to special case protocol 0 within the ipfs (protocol 1) file

isn't this where we want to make the adjustment to the object?

i.e

{ "protocol": 0, "pointer": "" }

when no logo is uploaded

I think that would be better handled in the utils method that wraps around this method, similarly to help keep the app vs. protocol separation clean

Can you point me to the file / line? This is what's being returned to be uploaded to ipfs, right? I assumed that's where this issue was #522 (comment)

I’m on mobile right now about to take off on a flight, but I think I see what’s happening

You’re using this in the upload logo step, which is coupled to IPFS, which is why you put it here. I thought this was for parsing the logo

It still feels a little dirty IMO to have it here instead of a createEmptyPtr() method that those files fallback to if no image is uploaded

ah I see - I'll take a look at the method, but is there a different place we want to adjust the object? Isn't this where we want to make the change to the object if no logo is uploaded?

(btw, do appreciate you chiming in as I know you're pretty much on holiday lol)

If we’re setting the logo pointer of metadata JSON to be empty and point to nothing, that is entirely independent of IPFS, which is why I don’t think it should be handled in this file. You can do that in utils or just generate it directly in the file(s?) that needs it

I do think there is a way around modifying the object - is that what you're suggesting? i.e your option 2 #522 (comment)

app/src/utils/utils.ts

apbendi · 2021-11-24T18:30:47Z

app/src/utils/utils.ts

@@ -103,6 +103,7 @@ export function decodeMetadataId(id: string): MetaPtr {
 export function ptrToURI(logoPtr: MetaPtr | undefined | null) {
  if (!logoPtr) return null;
  const { protocol, pointer } = logoPtr;
+  if (protocol === 0) return 'placeholder_grant.svg';
  if (protocol === 1) return getMetaPtr({ cid: pointer });
  throw new Error(`Unsupported protocol ID ${protocol}`);


This method should never throw. Even if loading the metaPtr fails, we should log to console but always degrade gracefully and show the placeholder.

apbendi · 2021-11-24T19:29:13Z

app/src/utils/data/ipfs.ts

@@ -14,7 +14,8 @@ function assertIPFSPointer(logoPtr: MetaPtr | undefined) {
    logoPtr = decodeMetadataId(logoPtr);
  }
  const protocol = BigNumber.from(logoPtr.protocol).toString();
-  if (protocol !== '1') throw new Error(`assertIPFSPointer: Expected protocol ID of 1, found ${protocol}`);
+  if (protocol !== '1' && protocol !== '0' && !logoPtr.pointer)


Hmm, so as written, if anyone were to upload/update a grant with a metaPtr that had a value other than 1 or 0, or with an empty pointer, this method would throw. Since we call this method in getMetaPtr, and use that method to fetch metadata throughout the app, then if anyone were to do this, the app would start throwing errors.

I don't believe an unsupported protocol value, or empty/invalid metaPtr value, should result in the app throwing errors at any point, since users could put anything in there, and we can't control that. I think it should always degrade gracefully in some way, for example by simply excluding the grant if content fails to load. This might require a different PR though. Curious to hear what you think @mds1.

Generally agreed! Though this file isn't the right place to handle that graceful degradation based on how I was thinking of the architecture.

For context: getMetaPtr should primarily only be called by the ptrToURI wrapper method, with an exception for new grant / image upload code paths that call it directly because they're currently tightly coupled to IPFS. My intent with this assertIPFSPointer was to make sure only IPFS pointers are being passed into the methods in this file, and this file should be limited to handling cases where protocol is 1. Therefore if we throw in this method that's a bug and in that case I think it's ok to fail loudly. Given that, this line should only be if (protocol !== '1') throw new Error(...) and this entire file should not care about protocol being any non-1 value

Given all that, and if you agree with that architecture decision of isolating this file to strictly deal with IPFS, then ptrToURI and other methods in utils.ts are where we should gracefully handle the failure.

app/src/utils/data/ipfs.ts

app/src/views/GrantRegistryGrantDetail.vue

mds1 · 2021-11-30T22:03:27Z

app/src/utils/data/ipfs.ts

+  if (!['0', '1'].includes(protocol))
+    throw new Error(`assertIPFSPointer: Expected protocol ID of 0 or 1, found ${protocol}`);


Pushed f1fd499 to revert this because this file shouldn't be aware of protocol ID 0

apbendi

Seems to be working 👍

- @dgrants/app@0.2.19-cc1120d.0 - @dgrants/contracts@0.2.16-cc1120d.0 - @dgrants/dcurve@0.2.16-cc1120d.0 - @dgrants/types@0.2.16-cc1120d.0 - @dgrants/utils@0.2.14-cc1120d.0

$metafraction$

$@metafraction$ metafraction changed the title ~~fix logoURI split error and 401 auth required error~~ fix logoURI split error Nov 18, 2021

$@metafraction$ metafraction marked this pull request as ready for review November 18, 2021 18:24

$@metafraction$ metafraction requested a review from mds1 November 18, 2021 18:24

mds1 reviewed Nov 18, 2021

View reviewed changes

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

$@metafraction$ metafraction force-pushed the split-error-fix branch from e08e860 to 2a801b8 Compare November 23, 2021 20:03

apbendi requested changes Nov 24, 2021

View reviewed changes

$@metafraction$ metafraction force-pushed the split-error-fix branch from 2a801b8 to 381cb8c Compare November 24, 2021 18:14

mds1 reviewed Nov 24, 2021

View reviewed changes

$@metafraction$ metafraction force-pushed the split-error-fix branch 2 times, most recently from bc88eea to 9504b58 Compare November 24, 2021 18:37

apbendi reviewed Nov 24, 2021

View reviewed changes

thelostone-mc reviewed Nov 25, 2021

View reviewed changes

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

thelostone-mc reviewed Nov 25, 2021

View reviewed changes

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

$@metafraction$ metafraction force-pushed the split-error-fix branch 2 times, most recently from acea1df to 2e08c03 Compare November 29, 2021 20:59

mds1 reviewed Nov 30, 2021

View reviewed changes

$@metafraction$ metafraction force-pushed the split-error-fix branch from f1fd499 to 31d7540 Compare November 30, 2021 22:09

apbendi approved these changes Nov 30, 2021

View reviewed changes

metafraction and others added 10 commits November 30, 2021 17:42

$@metafraction$

check if logoURI is defined before split

956e1c4

$@metafraction$

requested change

16058bc

$@metafraction$

handle for editing

4972a9f

$@metafraction$

fix error + load placeholder for home and grants page

dfc29d7

$@metafraction$

adj error msg

3d55809

$@metafraction$

more requested changes

d1ebabb

$@metafraction$

move no logo object to constants

9414e04

$@metafraction$

protocol err adj

c20fcc1

fix: remove awareness of protocol 0 from ipfs file

cf17b74

fix: ptrToURI never throws and allows a fallbackURI to be passed in

cc1120d

apbendi force-pushed the split-error-fix branch from 175a768 to cc1120d Compare November 30, 2021 22:44

Publish

61335b7

- @dgrants/app@0.2.19-cc1120d.0 - @dgrants/contracts@0.2.16-cc1120d.0 - @dgrants/dcurve@0.2.16-cc1120d.0 - @dgrants/types@0.2.16-cc1120d.0 - @dgrants/utils@0.2.14-cc1120d.0

apbendi merged commit 61335b7 into main Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix logoURI split error #522

fix logoURI split error #522

$@metafraction$ metafraction commented Nov 18, 2021

metafraction commented Nov 23, 2021 •

edited

Loading

mds1 commented Nov 23, 2021 •

edited

Loading

metafraction commented Nov 23, 2021 •

edited

Loading

mds1 commented Nov 23, 2021

metafraction commented Nov 23, 2021

apbendi left a comment

metafraction commented Nov 24, 2021

apbendi commented Nov 24, 2021

metafraction commented Nov 24, 2021

apbendi commented Nov 24, 2021

metafraction commented Nov 24, 2021

metafraction commented Nov 24, 2021

mds1 Nov 24, 2021

$@metafraction$ metafraction Nov 24, 2021 •

edited

Loading

mds1 Nov 24, 2021

$@metafraction$ metafraction Nov 24, 2021

mds1 Nov 24, 2021

$@metafraction$ metafraction Nov 24, 2021 •

edited

Loading

mds1 Nov 24, 2021

$@metafraction$ metafraction Nov 24, 2021

apbendi Nov 24, 2021

apbendi Nov 24, 2021 •

edited

Loading

mds1 Nov 24, 2021

mds1 Nov 30, 2021

apbendi left a comment

		if (!['0', '1'].includes(protocol))
		throw new Error(`assertIPFSPointer: Expected protocol ID of 0 or 1, found ${protocol}`);

fix logoURI split error #522

fix logoURI split error #522

Conversation

metafraction commented Nov 18, 2021

metafraction commented Nov 23, 2021 • edited Loading

mds1 commented Nov 23, 2021 • edited Loading

metafraction commented Nov 23, 2021 • edited Loading

mds1 commented Nov 23, 2021

metafraction commented Nov 23, 2021

apbendi left a comment

Choose a reason for hiding this comment

metafraction commented Nov 24, 2021

apbendi commented Nov 24, 2021

metafraction commented Nov 24, 2021

apbendi commented Nov 24, 2021

metafraction commented Nov 24, 2021

metafraction commented Nov 24, 2021

Choose a reason for hiding this comment

metafraction Nov 24, 2021 • edited Loading

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

metafraction Nov 24, 2021 • edited Loading

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 Nov 24, 2021 • edited Loading

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

$@metafraction$ metafraction commented Nov 18, 2021

metafraction commented Nov 23, 2021 •

edited

Loading

mds1 commented Nov 23, 2021 •

edited

Loading

metafraction commented Nov 23, 2021 •

edited

Loading

$@metafraction$ metafraction Nov 24, 2021 •

edited

Loading

$@metafraction$ metafraction Nov 24, 2021 •

edited

Loading

apbendi Nov 24, 2021 •

edited

Loading