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

Jump to bottom

Arweave Integration Milestone 1: Publish using transaction ID #485

Merged
merged 96 commits into from Sep 16, 2022
Merged

Arweave Integration Milestone 1: Publish using transaction ID #485

merged 96 commits into from Sep 16, 2022

Conversation

MantisClone
Copy link
Contributor

@MantisClone MantisClone commented Jun 13, 2022 •
edited

Towards oceanprotocol/pm#151

Changes proposed in this PR:

  • Add new arweave files structure
"files": [
  {
    "type": "arweave",
    "transactionId": "a4qJoQZa1poIv5guEzkfgZYSAD0uYm7Vw4zm_tCswVQ",
  }
]

  • Update fileinfo and download endpoints to support files stored in Arweave.

  • Add ARWEAVE_GATEWAY envvar for resolving Arweave transaction ids

  • Add test_download_arweave to show that downloading an asset stored in Arweave works.

  • Add test_compute_arweave to show that a compute job that uses a dataset stored in Arweave works.

  • Add test_check_arweave_good and test_check_arweave_bad to show that fileinfo endpoint works with arweave transaction ids.

  • Fix bug in the download endpoint where the Arweave transaction ID or IPFS CID are leaked in the "Content-Disposition" response header.

  • Discard MismatchedABI warnings that occur when processing event logs - these are expected because a given transaction receipt will likely have many different Events, not just ones that match the Event ABI of interest.

  • Add ARWEAVE_TRANSACTION_ID constant that points to branin.arff stored permanently on Arweave.

Note
This does not include the ability to upload to Arweave. That is left for a future PR.

Changes that were implemented but then reverted:

@alexcos20 alexcos20 self-assigned this Jun 14, 2022
@MantisClone
Copy link
Contributor Author

MantisClone commented Jun 14, 2022 •
edited

The CI checks for this PR is failing due to a strange error: https://github.com/oceanprotocol/provider/runs/6887646381?check_suite_focus=true#step:10:8

Does anyone know what might be going wrong?

Run docker logs ocean_aquarius_1 && docker logs ocean_provider_1 && docker logs ocean_provider2_1 && docker logs ocean_computetodata_1
docker logs ocean_aquarius_1 && docker logs ocean_provider_1 && docker logs ocean_provider2_1 && docker logs ocean_computetodata_1
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.8.12/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.8.12/x64/lib
Error: No such container: ocean_aquarius_1
Error: Process completed with exit code 1.

MantisClone and others added 5 commits June 15, 2022 11:39
@MantisClone
Merge branch 'oceanprotocol:main' into arweave-integration
a2218f6
@MantisClone
Pass download_url to check_url_details() instead of url_object["url"]
55c4dc8
@MantisClone
Remove is_safe_url check and url arg from build_download_response.
e9d438b 
  * 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.
@calina-c @MantisClone
Account for larger latency in GH Actions.
ceae5f8
@MantisClone
Add compute test using dataset stored in arweave
b35ea2d
@MantisClone MantisClone marked this pull request as ready for review June 16, 2022 22:41
@MantisClone MantisClone marked this pull request as draft June 17, 2022 21:19
@MantisClone
Copy link
Contributor Author

Lol, I just realized while testing with ocean.py that when downloading an arweave file, the filename is the transaction id 🤦

So this PR is not ready. I converted it back to draft. Definitely have to fix it so that the downloaded filename doesn't leak the transaction id.

@MantisClone MantisClone mentioned this pull request Jun 19, 2022
Closed
@MantisClone MantisClone marked this pull request as ready for review June 19, 2022 12:02
@MantisClone
Wait until test_compute_specific_algo_dids finishes compute job
06327be 
  - 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
	```
@MantisClone
Fix contentType
019710f
@MantisClone
Revert "Wait until test_compute_specific_algo_dids finishes compute job"
04f1bb8 
This reverts commit 06327be.
@MantisClone
Revert "Add --verbose flag to pytest command"
2e8315d 
This reverts commit f60f915.
@MantisClone
Copy link
Contributor Author

@calina-c I have re-requested a review despite test_compute failing. I believe it is failing for reasons outside of this PR. This can be seen in PR #534. It is "empty", identical to main, but test_compute is failing in the same way.

@calina-c
Copy link
Contributor

calina-c commented Sep 2, 2022

You can probably fix the failing tests by cherry picking #533 (comment)

@MantisClone MantisClone mentioned this pull request Sep 2, 2022
Closed
@MantisClone
Copy link
Contributor Author

Thanks, @calina-c You were right, cherry-picking 6d22a6d fixed the failing test.

I believe this PR is now ready for review.

@MantisClone
Copy link
Contributor Author

@calina-c I have reverted the is_safe_url removal and I believe this is ready for your review again.

@alexcos20
Copy link
Member

Please merge main branch, a new type was added

@MantisClone
Merge branch 'main' into arweave-integration
cb1bbe6
@MantisClone
Fix test_validate_url_object
6711eb1
@MantisClone
Empty commit to re-run the CI checks with updated barge
802ea1d
@MantisClone
Fix test_compute_paid_env. Use mint instead of transfer
ee74864 
  - 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
@MantisClone
Copy link
Contributor Author

Hello @calina-c @alexcos20 I believe this PR is ready for review.

P.S. It seems I am unable to request additional reviewers, perhaps because I am an external contributor.

@alexcos20 alexcos20 self-requested a review September 16, 2022 08:34
alexcos20
alexcos20 approved these changes Sep 16, 2022
View reviewed changes
Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

lgtm

@alexcos20 alexcos20 merged commit e023e7c into oceanprotocol:main Sep 16, 2022
2 checks passed
@MantisClone MantisClone deleted the arweave-integration branch September 16, 2022 14:43
@alexcos20 alexcos20 mentioned this pull request Sep 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcos20 alexcos20 alexcos20 approved these changes

@calina-c calina-c

Assignees

@alexcos20 alexcos20

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MantisClone @calina-c @alexcos20