-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments
Also, I'll get the Pinata api key stuff, will let you know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
But I'm still waiting on our Pinata keys to test it end to end.
Will wait until I have that before approving
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
I also tested parsing the uploaded file in the backend, though the json format of your upload was not correct. We call the "innerst" referrer, address now.
Indeed, I mentioned that change on #1405 (review) Now that you mention it might be work addressing that as its own PR. Created new issue just for that #1412 |
@alfetopito that change you mentioned was addressed in #1190 |
Hey @matextrem I'm trying to test but I'm not sure how. |
@alfetopito , these are the steps I did to test it:
Add this inside About function component, before the return statement
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managed to test it locally with the provided instructions.
CID: QmYmxiQ4BxLudtQTBg7PhVQWeuqHQdATstg168ZcH9MmJh
Hash: 0x9b10bf7289e87734dd1385e836ad64c50f6c0e77ab53edf42add7b66a97f6f92
Contents: {"appCode":"CowSwap","metadata":{"referrer":{"address":"BLA BLA BLA","version":"0.1.0"}},"version":"0.1.0"}
@anxolin you had comments regarding the code in another PR.
Would you like to comment here?
Yep, i know i mention this in another PR, but lost track of that, and I see this PR goes to develop so I assume this other PR is merged? I see this PR adds the integration with |
Nope, they are in Ramiro's PR that depends on this one. |
Cool, just did, u recommended me back then to comment in this one. Anyways, comments are still unaddressed 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! looks good to me 👌
This is a soft approve, just to understand the error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve :)
Summary
This PR closes #942
A function was added in order to receive the metadata doc and upload this to IPFS (and pinning it using Pinata)
An example of a test metadata doc pinned with Pinata: https://gateway.pinata.cloud/ipfs/QmQVCKwriSzFpz5Ybjp3iumJ9HpMq7bS9aReYvxDBactew
To Test
It can be tested by calling
uploadMetadataDocToIpfs
and passing a metadata doc as a parameter (We could usegenerateReferralMetadataDoc
with a test address).REACT_APP_PINATA_API_KEY
andREACT_APP_PINATA_SECRET_API_KEY
enviroment variables to the.env
file.Node version was updated in order to use
ipfs-core
and@sdk/pinata
dependencies