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

Advance settings - isRetired #754

Closed
jamiehewitt15 opened this issue Jul 23, 2021 · 13 comments
Closed

Advance settings - isRetired #754

jamiehewitt15 opened this issue Jul 23, 2021 · 13 comments
Assignees
Labels

Comments

@jamiehewitt15
Copy link
Member

jamiehewitt15 commented Jul 23, 2021

Ocean OEPs indicate that there should be an isRetired attribute in the status object.

We therefore need a component for setting this isRetired status from within the market. It should go in the advanced settings section and can look visually similar to the isDisabled component:

isDisabled

We also need to ensure that assets that have the isRetired status can't be consumed

@krisliew krisliew self-assigned this Jul 23, 2021
@kremalicious
Copy link
Contributor

kremalicious commented Jul 23, 2021

and what should that status do exactly? Setting it is easy but defining how an asset marked as isRetired should behave across the app is still missing. It would be also great to think this in context of existing known problems and not just tailor this for one party, e.g. #209

@jamiehewitt15
Copy link
Member Author

@kremalicious Yeah that's a good point that you make. I think the main objective here is to create an option to "soft delete" assets.

We could say that the isRetired status means the asset won't be visible in the market anymore. If that's the case though it raises a question about how permanent this status is mean to be... if the asset isn't viewable in the market anymore it would be hard for the user to change the status. In the example that you mention (where the dateset has been posted twice by mistake) that should be fine.

@kremalicious
Copy link
Contributor

kremalicious commented Jul 23, 2021

I think that option is valuable for everybody if it behaves almost the same as purgatory assets but with different messaging, as we are seeing requests for assets to be put in purgatory which are not fitting its intention as @DMATS & @realdatawhale encountered.

So isRetired assets would then just get different messaging. As a reminder the rules for purgatory in the UI are:

  • won't be returned in any searches, asset lists, filtering is done on market in all search queries
  • their details page stays accessible for everyone if they know the link
  • they stay in user's bookmarks
  • on asset details they get a warning banner informing about its state
  • all actions are removed/disabled, except for removing pool liquidity

We have to be careful to not introduce any new rugpull attack surface here though as publisher could retire asset shortly after people have added lots of liquidity making liquidity providers then believe the asset is completely gone so they don't know how to remove their liquidity if they have not bookmarked the asset.

Then we need to decide how deep in the stack the restrictions should be implemented. Can't be restricted on contracts level but provider & libraries could implement checks too. For Aquarius we might want to make it simple and just exclude isRetired assets from all query results by default. And only return it when requested by DID which should be enough to make the UI still work.

@jamiehewitt15
Copy link
Member Author

That sounds workable, I think updating Aquarius to exclude isRetired assets from all query results by default sounds like a good idea. And then, like you say, the asset will still be returned if it is requested directly with it's DID.

In terms of reducing the potential for this to be used for a rug pull attacks, we could help avoid that by encouraging everyone to bookmark the asset after they have added liquidity.

For example, the success message could be adapted to say:
"Successfully added liquidity! We strongly recommend bookmarking this asset or saving the DID. If the asset is ever retired you will need the DID to remove your liquidity"

bookmark asset

Another option would be to automatically bookmark the asset on the user's behalf if they have added liquidity to it.

@realdatawhale
Copy link

Hi everyone, thank you for your discussion here. Indeed, we are looking to retire the following datasets. Where can we submit a PR for the same? Thanks in advance! @kremalicious @jamiehewitt15

STUTUR-23
https://market.oceanprotocol.com/asset/did:op:312d24dE83610e351A1977EbA8851CeA8D020e1B

MARRAY-61
https://market.oceanprotocol.com/asset/did:op:7c0D0DC5bCACeDf8121047c34bFb0ABeB00d4b19

FIXED PRICE: SALOTT-11
https://market.oceanprotocol.com/asset/did:op:8b4045F37dB438F9761c615e9512c3Bf742e29cf

FIXED PRICE: EGRFIS-84
https://market.oceanprotocol.com/asset/did:op:0e36Caa04EbefC1B2A77620F41e6c343a1d9d660

Thanks a ton!

@mihaisc
Copy link
Contributor

mihaisc commented Jul 26, 2021

People will be able to see the asset in the history page in the pool shares tab so finding the asset will not be a problem.

@kremalicious
Copy link
Contributor

after some PM talks:

  1. we are going back to original issue intention of only adding a checkbox to advanced settings, and leave the exact implications of that checkbox to external UI, and do not deal with it in this repo.
  2. we are going pragmatic for retiring requests and use the purgatory for it, with a new status retired by owner. @realdatawhale this means you are free to open up PR with your assets on list-purgatory

@jamiehewitt15
Copy link
Member Author

@kremalicious I'm not exactly sure what you mean in your first point, as you say we should add a checkbox to advance settings but not deal with it in this repo (the market)? Do you mean that the checkbox should be implemented in the market but not Ocean.js?

@soonhuat
Copy link
Contributor

@kremalicious
with 2. then the effect wouldn't be immediately.
on top of that, anyone also can raise such request via PR for any asset, then request manual intervention to approve the PR after some review conducted which require some effort.

do you have other use case for status.isRetired flag?

@jamiehewitt15
Copy link
Member Author

jamiehewitt15 commented Jul 28, 2021

I think that's a valid point. As we scale up, it won't always be sustainable to do this by reviewing PRs. It requires the reviewer to verify that the GitHub account is associated with the owner of the Ethereum address. That will take time and also introduces the possibility of mistakes and attacks

@jamiehewitt15
Copy link
Member Author

@soonhuat I had a chat with @kremalicious about this and the decision is for this to not be implemented in Ocean Market so you will have to proceed to implement it in your fork. For the moment we will continue using purgatory for retiring assets

@realdatawhale
Copy link

@jamiehewitt15 I've opened a PR on purgatory as suggested. Thanks.

@mihaisc
Copy link
Contributor

mihaisc commented Feb 15, 2022

This is not relevant in v4 because we have the state field on the nft . oceanprotocol/docs#783
image

@mihaisc mihaisc closed this as completed Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants