dcgtc / dgrants

Decentralized Grant platform that implements quadratic funding.
GNU Affero General Public License v3.0
84 stars 39 forks source link

fix logoURI split error #522

Closed metafraction closed 2 years ago

metafraction commented 2 years ago

resolves #521

metafraction commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

metafraction commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

apbendi commented 2 years ago

@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 commented 2 years ago

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

metafraction commented 2 years ago

try now @apbendi rebased as well