Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding ipfs / arweave support #1765

Merged
merged 43 commits into from Nov 25, 2022

Conversation

EnzoVezzaro
Copy link
Contributor

@EnzoVezzaro EnzoVezzaro commented Nov 1, 2022

Changes proposed in this PR:

  • created storage type dropdown (Url / IPFS / Arweave)
  • added storage type selection on publish
  • added storage type selection on edit
  • updated @oceanprotocol/lib@2.5.2

WIP:

TO TEST:

  • IPFS: you can upload a file in IPFS using web3.storage (to get CID)
  • Arweave: you can use this little app to upload a file into Arweave (to get the transactionID)

Publish

Before validation:
Screenshot 2022-11-11 at 10 44 47

After validation:
Screenshot 2022-11-14 at 11 16 57

Edit

When entered in edit form:
Screenshot 2022-11-14 at 10 50 13

After clicking on "x":
Screenshot 2022-11-14 at 10 50 19

@vercel
Copy link

vercel bot commented Nov 1, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
market Ready (Inspect) Visit Preview Nov 25, 2022 at 1:39PM (UTC)

src/@utils/provider.ts Outdated Show resolved Hide resolved
src/@utils/provider.ts Outdated Show resolved Hide resolved
@EnzoVezzaro
Copy link
Contributor Author

Mmmm... weird 🤔 just tried both IPFS / Aweave without issues when publishing. The only think that I found was the same issue we currently have on Priced Assets. Not sure if it's related. Do you know anything else about this issue @bogdanfazakas ?

IPFS free, ok:
Screenshot 2022-11-23 at 07 48 07

Arweave (ignore title, I used an Arweave file here) priced, not ok:
Screenshot 2022-11-23 at 07 48 04

@jamiehewitt15
Copy link
Member

I ran another test published on IPFS and it worked fine this time. I'll let you know if I'm able to reproduce it.

@jamiehewitt15
Copy link
Member

One thing we should do is explain to people how to go about publishing with Arweave or IPFS. I've created an issue for this in the docs. When that's done we should add a link to the docs section in the tooltip.

@jamiehewitt15
Copy link
Member

Also tested publishing a Arweave asset and it worked well 👍

It would be great if we could add a test to this to keep the overall test coverage going up.

@EnzoVezzaro
Copy link
Contributor Author

It would be great if we could add a test to this to keep the overall test coverage going up.

We already have a test for both IPFS and Arwaeve. I'm kind of rushing a bit this because I need to keep going with the integration of graphql and on-chain data which depends on this PR. Maybe we can add more tests afterwards.

@jamiehewitt15
Copy link
Member

We already have a test for both IPFS and Arwaeve. I'm kind of rushing a bit this because I need to keep going with the integration of graphql and on-chain data which depends on this PR. Maybe we can add more tests afterwards.

Yeah, fair enough. I was just seeing that the overall test coverage is going down.

@jamiehewitt15
Copy link
Member

Why is it only possible to use a URL for the sample file? People may also want to use IPFS/ Arweave for that

@EnzoVezzaro
Copy link
Contributor Author

Why is it only possible to use a URL for the sample file? People may also want to use IPFS/ Arweave for that

Maybe it'll be implemented in the future, but for this task is out of scope.

@jamiehewitt15
Copy link
Member

Maybe it'll be implemented in the future, but for this task is out of scope.

Ok sure, seems fair enough

Copy link
Member

@jamiehewitt15 jamiehewitt15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have done quite a few tests now and can't find any faults. Works well, good job

package.json Outdated Show resolved Hide resolved
src/components/Publish/_utils.ts Outdated Show resolved Hide resolved
src/components/Publish/_validation.ts Outdated Show resolved Hide resolved
Copy link
Member

@bogdanfazakas bogdanfazakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of functionality all good from my tests, good job with the PR @EnzoVezzaro 👍

@codeclimate
Copy link

codeclimate bot commented Nov 25, 2022

Code Climate has analyzed commit d273388 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 18.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 21.4% (-0.1% change).

View more on Code Climate.

@EnzoVezzaro EnzoVezzaro merged commit f6d11e5 into main Nov 25, 2022
14 of 16 checks passed
@EnzoVezzaro EnzoVezzaro deleted the feature/issue-1711-add-ipfs-arwave-support branch November 25, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ipfs and arweave storage type support
5 participants