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

Add optional userdata to url storage type, add arweave storage type. #1082

Closed

Conversation

MantisClone
Copy link

@MantisClone MantisClone commented Jul 28, 2022

Fixes #1131

This is a continuation of PR #1019.

Changes proposed in this PR:

  • Add arweave storage type
  • Re-format the storage object section to use regular code blocks instead of code blocks embedded in tables.

Screenshot of the changes

TODO

@netlify
Copy link

netlify bot commented Jul 28, 2022

👷 Deploy Preview for docs-oceanprotocol processing.

Name Link
🔨 Latest commit 5429780
🔍 Latest deploy log https://app.netlify.com/sites/docs-oceanprotocol/deploys/62e2d2576ce7c30009919ca3

@MantisClone MantisClone marked this pull request as ready for review July 28, 2022 18:16
@MantisClone
Copy link
Author

@jamiehewitt15 I believe this PR is ready for review.

@jamiehewitt15
Copy link
Member

Thanks @MantisClone, is this dependant on oceanprotocol/provider#485?

Given that Arweave is completely new, it would be great to have a bit more explanation somewhere about it's benefits and how it can be used. Many people will still be unfamiliar with it.

@MantisClone
Copy link
Author

Thanks @MantisClone, is this dependant on oceanprotocol/provider#485?

@jamiehewitt15 Yes.

The complete list of related PRs includes:

Given that Arweave is completely new, it would be great to have a bit more explanation somewhere about it's benefits and how it can be used. Many people will still be unfamiliar with it.

That's a fair critique. Here are the pages I think will need edits:

  • "Publishing with hosting services" could use a new Arweave section - explaining benefits and how to upload
  • "Publish a data asset" needs to show and explain the new "Storage type" field on the publish page.

Do you agree? Is there any other documentation you believe is required?

@MantisClone MantisClone marked this pull request as draft July 29, 2022 20:38
@alexcos20
Copy link
Member

alexcos20 commented Aug 8, 2022

I would remove both headers & userdata for arweave type, since they are not applicable

alexcos20 pushed a commit to oceanprotocol/provider that referenced this pull request Sep 16, 2022
* Add test scaffold for arweave fileinfo

* first cut at arweave fileinfo tests

* Update get_download_url to support Arweave

* Add ARWEAVE_GATEWAY to README and pytest.ini

* Fix syntax, remove extra `:`

* Rearrange assertions, print result if status not "200 OK"

* Update FileInfoRequest validation to accept type=arweave

* Fix syntax

* Fix content length

* Fix test_check_arweave_bad

* Run pre-commit checks

* Add first cut at download test using arweave file

* Update validate_url_object to work with arweave

* Pass download_url to check_url_details() instead of url_object["url"]

* Remove is_safe_url check and `url` arg from `build_download_response`.

  * When calling download, `is_safe_url()` is called earlier by
    `check_url_details()` so calling again in `build_download_response`
    is redunant.
  * When calling computeResult, `validate_url=False` so `is_safe_url()`
    check is skipped anyway.

* Account for larger latency in GH Actions.

* Add compute test using dataset stored in arweave

* Fix build_download_response so filename doesn't leak arweave tx id

* Update fileinfo error logic to better bubble up Exception messages.

* Improve tests

* Fix syntax error

* Fix tests

* Generalize file dict: "value" instead of "url", "hash", "transactionId"

* Fix test

* Revert "Generalize file dict: "value" instead of "url", "hash", "transactionId""

This reverts commits:
  * deb5d69
  * 9dc6fa4

* Remove unnecessary patches for is_safe_url

* Simulate revert of nonce erroneous commit.

* Add try/except back into fileinfo

* Fixup merge

* Add Arweave file type

* Add arweave to list of acceptable types in fileinfo validate_and_create

* Rename test to better describe what it's testing

* Add malformed arweave url_object to test_validate_url_object

* Reinstate test_util.py::test_build_download_response_arweave

* Reinstate arweave related tests

* Update error message

* Remove unnecessary code

* Make Arweave capitalized

* Fix expected exception type

* Remove is_safe_url check from `build_download_response`.

  * When calling download, `is_safe_url()` is called earlier by
    `check_details()` so calling again in `build_download_response`
    is redunant.
  * When calling computeResult, `validate_url=False` so `is_safe_url()`
    check is skipped anyway.

* Fix build_download_response so filename doesn't leak arweave tx id or ipfs hash

* Upgrade IPFS test to check for CID leak

* Fix debug log statement and add missing error check after calling check_details

* Discard benign MismatchedABI warnings

* Remove Content-Length check from arweave fileinfo test

* Remove extra argument

* Fix method name

* Attempt to fix test_compute_arweave

* Try to fix the alg filesChecksum use in test_compute_arweave

* Add Asset type hints to improve method discovery in IDEs like VSCode.

* Fix test_compute_areweave

* Make error message more explicit

* Revert all formatting-only changes.

* Revert a few more formatting-only changes

* Move `magic` import above the `artifacts` import

* Update test to match error message when transactionId key is missing

* Add ALLOWED_FILE_TYPES to avoid duplicating list [ipfs,url,arweave]

* Adjust tests such that assets stored in arweave allow all algorithms

* Specify alg_diff arg name for clarity

* Add ARWEAVE_TRANSACTION_ID constant that points to branin.arff

* Remove the result check from `test_compute_arweave`

* Add assertion about the downloaded contents

* Remove try/except. Exceptions are caught by errorhandler

* Add test where ARWEAVE_GATEWAY is invalid

* Revert "Remove Content-Length check from arweave fileinfo test"

This reverts commit 0024c26.

* Add assertion to see what result is when ARWEAVE_GATEWAY is bad

* Empty commit to re-run CI.

* Fix test case where ARWEAVE_GATEWAY is not reachable

* Remove unnecessary snippet

* Fix content lenght (because using branin.arff file now)

* Add --verbose flag to pytest command

* Wait until test_compute_specific_algo_dids finishes compute job

  - Trying to fix error in test_compute (the following test):
    ```
	>       assert response.status == "200 OK", f"start compute job failed: {response.data}"
	E       AssertionError: start compute job failed: b'{"error": "`agreementId` already in use for other job."}'
	E       assert '400 BAD REQUEST' == '200 OK'
	E         - 200 OK
	E         + 400 BAD REQUEST
	```

* Fix contentType

* Revert "Wait until test_compute_specific_algo_dids finishes compute job"

This reverts commit 06327be.

* Revert "Add --verbose flag to pytest command"

This reverts commit f60f915.

* Separate compute test with auth token to ensure agreementId uniqueness.

* Revert "Remove is_safe_url check from `build_download_response`."

This reverts commit 29f85a9.

* Revert formatting-only changes

* Remove comment.

* Add newline

* Remove headers and userdata for arweave type per comment from @alexcos20

  - oceanprotocol/docs#1082 (comment)

* Revert more formatting only changes

* Add newline at end of file

* Revert "Remove headers and userdata for arweave type per comment from @alexcos20"

This reverts commit 61ff964.

* Fix test_validate_url_object

* Empty commit to re-run the CI checks with updated barge

* Fix test_compute_paid_env. Use `mint` instead of `transfer`

  - Fix error message:
  >       fee_token.functions.transfer(consumer_wallet.address, to_wei(80)).transact(
            {"from": deployer_wallet.address}
        )
E           web3.exceptions.ContractLogicError: execution reverted: VM Exception while processing transaction: revert ERC20: transfer amount exceeds balance

Co-authored-by: Calina Cenan <calina@cenan.net>
@jamiehewitt15
Copy link
Member

@MantisClone this is still marked as draft. Are you still actively working on it?

@MantisClone
Copy link
Author

Yeah I was planning to finish it off this week. Last week I was just trying to get my Ocean.py pr merged.

@jamiehewitt15
Copy link
Member

Yeah I was planning to finish it off this week. Last week I was just trying to get my Ocean.py pr merged.

Ok cool, fair enough

@MantisClone
Copy link
Author

@jamiehewitt15 I added Arweave upload instructions to core-concepts/asset-hosting.md, per your request.
https://github.com/oceanprotocol/docs/pull/1082/files#diff-e17aa10544eed5b37e241d4e4e30523e0040f2ca72f22afacaf7002a091ed3b3R20-R57

The instructions are missing a screenshot of the Storage type and Transaction ID fields on the Ocean Market publish page. These fields are implemented in oceanprotocol/market#1527 but it hasn't been merged yet.

Unfortunately, my latest commits seem to have added a bunch of unnecessary edits like image files that aren't actually used and needlessly changing image URLs to be absolute images/ instead of relative ../. 😩 Also, it looks like I'm out of date with main again. So I'll work on syncing and cleaning stuff up.

@MantisClone
Copy link
Author

This PR used to include changes to the DDO spec to add the Arweave file type. These changes were superseded by #1137

The remaining changes explain how to upload files to Arweave.

As such, the title of this PR no longer makes sense so I'm renaming it to better reflect its content.

@codeclimate
Copy link

codeclimate bot commented Oct 10, 2022

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

View more on Code Climate.

@AnaLoznianu
Copy link
Member

@MantisClone As we recently added Arweave support to the market, we can finish these changes.
I understand that it has been a while since you started working on this and perhaps your priorities have changed.
Is it ok for you if we take it over?

@MantisClone
Copy link
Author

Hi @AnaLoznianu. 👋 Yes, you can take over.

@AnaLoznianu
Copy link
Member

Thanks, I moved this to a new PR: #1165 as yours is coming from a fork with makes it a bit difficult. Thank you 🙏

Project Management automation moved this from In Progress to Done Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Add arweave data type
4 participants