oceanprotocol / market

🧜‍♀️ THE Data Market
https://market.oceanprotocol.com
Apache License 2.0
189 stars 291 forks source link

Invalid DDOs can be published on-chain, and Aquarius won't index them #367

Closed trentmc closed 3 years ago

trentmc commented 3 years ago

Describe the bug In Rinkeby, asset doesn't show up in Ocean Market after publishing. Can't set price etc.

To Reproduce Steps to reproduce the behavior:

  1. Open browser.
  2. Have Metamask on, with wallet connected to Rinkeby. Make sure the account has enough ETH.
  3. Go to market.oceanprotocol.com.
  4. Click 'connect wallet' on top right
  5. Click on 'PUBLISH'
  6. Fill out the whole form with test info; check the "I agree to these Terms and Conditions" box. Name of asset is "branin2".
  7. Click 'SUBMIT' button
  8. The page will say "Publishing Data Set : 1/5 .." then "2/5".
  9. Metamask will pop up; click confirm. Metamask will pop up again; click confirm.
  10. Page will say "Publishing Data Set : 3/5". Then "Successfully published. Now create a price on your dataset". It has a button "Go to dataset"
  11. Click on the button "Go to dataset".
  12. It goes to a page that says "Could not retrieve asset. The DDO for did:op:297E5671E9EB802b3E4494f8BeF1d84209161dea was not found in MetadataCache. If you just published a new data set, wait some seconds and refresh this page."
  13. So I waited a minute, and refreshed. Go same error message
  14. So I waited 15 min, and refreshed. Still the same error message
  15. I opened a separate browser tab, went to market.oceanprotocol.com, and scrolled down to "New data sets" section. The asset ("Branin2") is not there. Nothing's there in the last hour, so it's easy to tell.

Expected behavior To be able to publish properly, including:

Desktop (please complete the following information):

Screenshots If applicable, add screenshots to help explain your problem.

Ocean Market 1 publish1

Ocean Market 2 publish2

Ocean Market 3 publish3

Ocean Market 4 - see the DDO error publish4 - ddo error

Ocean Market 5 - DDO error after waiting a few min publish4 - ddo error after waiting

Ocean Market 6 - DDO error after waiting many min publish4 - ddo error after waiting 15 min

Ocean Market - not shown in most recent assets publish6 - data asset not in ocean market new data sets

Metamask txs metamask txs

Etherscan txs. For this address, got tx 1 and tx 2

kremalicious commented 3 years ago

No clue how to reproduce. Seems like an Aquarius issue, in the last weeks it went down on Rinkeby almost daily. Asset simply does not exist there: https://aquarius.rinkeby.oceanprotocol.com/api/v1/aquarius/assets/ddo/did:op:297E5671E9EB802b3E4494f8BeF1d84209161dea

Multiple sets were published on Rinkeby in last hours: Screen Shot 2021-02-07 at 18.32.10.png

trentmc commented 3 years ago

Did you try reproducing with the steps given? It happened to me two times in a row.

And if it is Aquarius issues:

kremalicious commented 3 years ago

agree, user-friendly error handling is terrible but this is also on Aquarius side resulting in market having no reliable way of detecting specific errors. This is a shortcoming of Aquarius API implementation described a bit here https://github.com/oceanprotocol/aquarius/issues/273

trentmc commented 3 years ago

Thinking out loud, if we assume that it is Aquarius causing the market ux issues, then the easy way to reproduce it is to turn off Aquarius / make it act badly.

Then the Market ux can be improved to give the user better guidance (rather than an error message saying "please wait a few seconds and refresh").

kremalicious commented 3 years ago

As of right now we cannot differentiate if Aquarius is down or an asset is not found. All is a 500 from the market perspective cause that's what Aquarius tells us. If publishing fails somewhere in between, the UI shows a retry button but only based on transactions failing. After that we have to rely on Aquarius which we kinda can't. In the above screenshot the publish success screen comes up because ocean.js sends us back a DDO. But then it's actually not there. So not much room to come up with some better error flow here.

As I said, I cannot reproduce, just tried another test asset and all goes successful for me on Rinkeby https://market.oceanprotocol.com/asset/did:op:94590D392B91EA6Fbc166B69de705e989e1ED33d

trentmc commented 3 years ago

Here's perhaps the most robust solution, though it will take more work:

Don't use Aquarius to publish.

After all, it's only supposed to be a metadata cache. But when you're publishing you don't need a cache. You just have to get some data into ddo.sol contract.

This would likely require writing a good chunk of JS. But it would be far more robust. It also sets up nicely for v4 which will put metadata into the erc20 itself.

kremalicious commented 3 years ago

We use Aquarius to display an asset and there is no other way right now. As I said, nothing in the publish fails from the UI side, but when retrieving a single asset it fails. There is no way of retrieving a single asset with its metadata from somewhere else.

kremalicious commented 3 years ago

code example to show that Aquarius is already not used when publishing: https://github.com/oceanprotocol/ocean.js/blob/main/src/ocean/Assets.ts#L192-L199

So ocean.js tells us DDO is now on-chain so we show success, then we try to retrieve it cause we want to display it but Aquarius does not have it. So we pass through the error. Telling user to try again although they already paid for a successful publish would be weird regardless if we could detect this mismatch between on-chain and cache somehow

So guess the only thing left would be the logs of https://aquarius.rinkeby.oceanprotocol.com somewhere in K8 which might give a clue

kremalicious commented 3 years ago

I was able to reproduce this locally with #369 and this is the error Aquarius shows in its logs:

aquarius_1         | 2021-02-09 17:03:52,105 - aquarius.events.events_monitor - ERROR - New ddo has validation errors: [{'path': 'main/files/0/contentLength', 'message': "None is not of type 'string'"}]

So in my case Aquarius wants a contentLength on the files but provider did not detect it and just leaves it empty. This is a new bug and was not present before we switched to using the provider as a file checker 5 days ago in #352.

Again, we should not try to hack around this, but prevent any form submissions in case of validation errors. Then we do not need to workaround nonexistent error responses from Aquarius. If there are issues with publish they need to be fixed rather than trying to describe all potential issues to end users in the UI.

kremalicious commented 3 years ago

Solving https://github.com/oceanprotocol/provider/issues/91 will fix the probable root cause of this issue. We still need DDO validation before form submission to prevent users from running into similar issues for which we need to wait for resolve of https://github.com/oceanprotocol/ocean.js/issues/400 or simply hit https://docs.oceanprotocol.com/references/aquarius/#assetsddovalidate

@trentmc do you still remember which exact file URL you used for this data set?

trentmc commented 3 years ago

Good job working to get to the root of this!

I think I supplied this: https://github.com/trentmc/branin/branin.arff

I did not check then (my bad). I see now that it returns a "not found" message.

It's also possible that I supplied the proper url: https://github.com/trentmc/branin/blob/master/branin.arff

bogdanfazakas commented 3 years ago

Should we set a default value from the market if the contentLength is not received from provider when checking fileinfo ? We do the same think here https://github.com/oceanprotocol/market/blob/main/src/utils/provider.ts#L31-L32 with the contentType but that's a required parameter in the FIle interface.

kremalicious commented 3 years ago

yeah think so, then we're really safe and it is no overhead at all. Could you prepare a PR for it?

On principle, we should always only map what Aquarius' metadata validation wants into the library typings, and then in the clients. But hey, tech reality!

bogdanfazakas commented 3 years ago

sure , I'll handle this.

kremalicious commented 3 years ago

we will have this closed by #372, which will prevent the reason for the original issue from happening again, in tandem with changes on provider outlined in https://github.com/oceanprotocol/provider/issues/91.

At same time there are multiple other ways which would further help users in similar cases of "faulty" metadata. These ways are outlined in #373 and #366 and will be tackled individually there