Skip to content

Subgraph helpers for assetSelection list and previous orders#488

Merged
bogdanfazakas merged 9 commits into
feature/computefrom
feature/sugraph-util-methods
Apr 14, 2021
Merged

Subgraph helpers for assetSelection list and previous orders#488
bogdanfazakas merged 9 commits into
feature/computefrom
feature/sugraph-util-methods

Conversation

@bogdanfazakas

@bogdanfazakas bogdanfazakas commented Apr 9, 2021

Copy link
Copy Markdown
Member

Fixes # .

Changes proposed in this PR:

  • set price for each asset in AssetSelection from the graph
  • get previous orders for compute selceted assets from the graph

@vercel

vercel Bot commented Apr 9, 2021

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oceanprotocol/market/8mkDg1Mdm5GPHdYFCuzj3jMhQMYp
✅ Preview: https://market-git-feature-sugraph-util-methods-oceanprotocol.vercel.app

Comment thread src/utils/subgraph.ts Outdated
Comment thread src/utils/subgraph.ts Outdated
@bogdanfazakas bogdanfazakas marked this pull request as ready for review April 13, 2021 09:02
@bogdanfazakas bogdanfazakas requested a review from mihaisc as a code owner April 13, 2021 09:02
Comment thread src/utils/subgraph.ts Outdated
@kremalicious

Copy link
Copy Markdown
Contributor

awesome, confirmed all prices are in sync in this place now:

Screen Shot 2021-04-14 at 11 17 10

Getting this though, but I guess this is actually what we want now when checking for isOrderable?

Screen Shot 2021-04-14 at 11 18 44

Bit confusing to users as to what is wrong but at lest we don't send people into failed transactions so should not be a blocker. Ideally we have some visual pattern to signal to users right after selecting algo that the algo won't work, like dim it, or some tiny warning text on the row.

Case could be also made to hide algos where we know they are not orderable from the AssetSelection here altogether, or dim them, or group them in the list to the bottom with some group header Not Usable or something.

@bogdanfazakas

bogdanfazakas commented Apr 14, 2021

Copy link
Copy Markdown
Member Author

awesome, confirmed all prices are in sync in this place now:

Screen Shot 2021-04-14 at 11 17 10

Getting this though, but I guess this is actually what we want now when checking for isOrderable?

Screen Shot 2021-04-14 at 11 18 44

Bit confusing to users as to what is wrong but at lest we don't send people into failed transactions so should not be a blocker. Ideally we have some visual pattern to signal to users right after selecting algo that the algo won't work, like dim it, or some tiny warning text on the row.

Case could be also made to hide algos where we know they are not orderable from the AssetSelection here altogether, or dim them, or group them in the list to the bottom with some group header Not Usable or something.

yeah that error is displayed when asset is not orderable, in you case it's probably because your dataset is on other provider that the selected algorithm with service type compute
we can add some checks in AssetSelection and pass the dataset provider and not display algorithms with compute services that are using diferent providers in the list

@kremalicious

Copy link
Copy Markdown
Contributor

haha, this is a product design nightmare. If the restriction of our product is that dataset and algo need to be on the same provider I would be in favor of only showing algos of the same provider in the list. Or we prevent them from showing up when publisher edits compute settings, then we will not run into the issue on the start compute side

@mihaisc

mihaisc commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

So you can use an algo from another provider if that algo has an access service not a consume service. Since the filtering is done client side you can remove the algos that have compute service with a different provider

@kremalicious

kremalicious commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

@bogdanfazakas

Copy link
Copy Markdown
Member Author

can't order anything.

On Rinkeby, trying to combine these, both on same provider:

Still giving me:

Screen Shot 2021-04-14 at 13 26 10

Strange, i also got another error for this asset combo, but when trying to buy DT, an oceanjs error 'Not enough Ocean Tokens'

@mihaisc

mihaisc commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

@mihaisc

mihaisc commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

@mihaisc

mihaisc commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

but this issue has nothing to do with this pr

@kremalicious

Copy link
Copy Markdown
Contributor

ah ok, so just tried with that Row count, but get the same. If nothing to do with this PR then I'm fine just moving ahead and merging, and fixing in another PR

Screen Shot 2021-04-14 at 14 17 04

@bogdanfazakas

Copy link
Copy Markdown
Member Author

ah ok, so just tried with that Row count, but get the same. If nothing to do with this PR then I'm fine just moving ahead and merging, and fixing in another PR

Screen Shot 2021-04-14 at 14 17 04

can you try again now, for me started working suddenly maybe again some compute env issues

@kremalicious

Copy link
Copy Markdown
Contributor

nope, but this message seems interesting

ERROR: Algorithm [object Object] is not allowed by did:op:d5766cc73AE44977b4F7d72cDd3b33740101e830

Screen Shot 2021-04-14 at 14 27 01

@kremalicious

kremalicious commented Apr 14, 2021

Copy link
Copy Markdown
Contributor

I think my bad, testing locally and despite fresh npm install and all still failing. But was able to run successfully on preview version 🥳

Screen Shot 2021-04-14 at 14 34 04

Exciting!
Screen Shot 2021-04-14 at 14 35 08

Screen Shot 2021-04-14 at 14 42 55

Screen Shot 2021-04-14 at 14 44 32

@bogdanfazakas bogdanfazakas merged commit f531fea into feature/compute Apr 14, 2021
@bogdanfazakas bogdanfazakas deleted the feature/sugraph-util-methods branch April 14, 2021 13:06
@kremalicious kremalicious mentioned this pull request Apr 14, 2021
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.

3 participants