Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Find Market only shows OPEN assets #1320

Closed
sschiessl-bcp opened this issue Mar 14, 2018 · 35 comments
Closed

Find Market only shows OPEN assets #1320

sschiessl-bcp opened this issue Mar 14, 2018 · 35 comments
Labels
[1c] Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks
Milestone

Comments

@sschiessl-bcp
Copy link
Contributor

sschiessl-bcp commented Mar 14, 2018

Can anyone confirm this? If I look for markets I only get open assets listed?

image

@grctest
Copy link
Contributor

grctest commented Mar 14, 2018

Can confirm this is occurring for me using the bitshares.org web wallet, if you start typing in the 'asset name' field it will proceed with the search, however I agree that openledger's UIA shouldn't be the absolute default for the reference wallet.

@diogogomes
Copy link

diogogomes commented Mar 14, 2018

This happened to me too on Desktop Client (linux).

My workaround until now was like @grctest mentioned: start write the asset's name (eg: BRIDGE, etc) to show other assets.

@wmbutler wmbutler changed the title Find Market only shows OPEN assets [2] Find Market only shows OPEN assets Mar 14, 2018
@wmbutler wmbutler added the [1c] Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks label Mar 14, 2018
@wmbutler wmbutler added this to the 180401 milestone Mar 14, 2018
@wmbutler
Copy link
Contributor

wmbutler commented Mar 14, 2018

Can we get this regex for the search to look for asset names:

  • from the start of the asset
  • from the beginning of the . on an asset

In this particular case, I'd expect none of the OPEN.* assets to display because none match the search criteria.

@svk31
Copy link
Contributor

svk31 commented Mar 15, 2018

It defaults to open.x assets because when this search was added they were the only ones we had. It also populates default assets because people didn't understand they had to type something to search.

The asset search api only allows searching from the start of the asset so we can't really change that.

@calvinfroedge calvinfroedge self-assigned this Mar 25, 2018
@calvinfroedge
Copy link
Contributor

Working on this issue...have identified problem but will take some time to resolve.

@calvinfroedge
Copy link
Contributor

calvinfroedge commented Mar 28, 2018

Open PR which resolves issue...

#1365

update

Would love second pair of eyes to ensure another issue was not introduced. Notes regarding implementation in PR.

@calvinfroedge calvinfroedge changed the title [2] Find Market only shows OPEN assets [7.5] Find Market only shows OPEN assets Mar 28, 2018
@calvinfroedge calvinfroedge modified the milestones: 180415, 180401 Mar 28, 2018
@svk31
Copy link
Contributor

svk31 commented Mar 29, 2018

I would prefer that we just remove the default assets here, making the user search for something manually. They were there for convenience back when we only had OPEN.X gateway assets, now we have many so the search shouldn't be biased.

Maybe add a gateway selector instead that will trigger a search for the assets of the selected gateway provider.

@wmbutler
Copy link
Contributor

wmbutler commented Mar 29, 2018

@svk31 the gateway selector is needed but I think it's a larger UX concern since we will want to allow users to group and search by gateway throughout. I've broached this with @ahdigital

@calvinfroedge
Copy link
Contributor

calvinfroedge commented Mar 29, 2018

@svk31 Is there a problem with the PR?

@calvinfroedge
Copy link
Contributor

calvinfroedge commented Mar 29, 2018

I think I resolved the issue in my PR as it was reported...user wanted to be able to see all assets when visiting "Find Markets". I made sure OPEN, RUDEX, and UIAs are all visible in find markets up front. That resolves the issue as reported. I also diagnosed and resolved performance issues with loading the additional market data (which was even an issue before when only showing OPEN assets). The majority of my time I spent on the performance issue (~5 hours). The original estimate was 2 hours and I increased as we have been doing when an issue takes more time than in the estimate.

When I took the issue on, my expectation was that the issue had already been curated and that the desired solution was to make all of the assets appear by default, as well as making search behave as expected (as in only assets which match search criteria should appear). My PR is here:

#1365

It seems as if @svk31 is changing the scope of the issue entirely rather than critiquing my solution to the reported issue:

I would prefer that we just remove the default assets here, making the user search for something manually. They were there for convenience back when we only had OPEN.X gateway assets, now we have many so the search shouldn't be biased.

Maybe add a gateway selector instead that will trigger a search for the assets of the selected gateway provider.

My hope is that any issue that makes it into a sprint does not get materially changed in scope after it has been placed in the sprint, as whoever does the work essentially loses their time investment.

If it's about actual work going beyond the estimate, I'm happy to first explore an issue and submit a revised estimate. My contention is simply that the desired end result from a user perspective should already be settled before the issue makes it into a sprint. Alternatively, if I can pick up on a new approach and not lose my existing hours that's great, too.

@wmbutler
Copy link
Contributor

@svk31 please review and let me know how to proceed. If there is a particular issue you have with Calvin's work, please voice it. It sounds as if Calvin dug a bit deeper and fixed some root issues.

@svk31
Copy link
Contributor

svk31 commented Mar 30, 2018

I don't have any issues with the work, technically it looks fine. I'm just pointing out that the original issue was just a misunderstanding: he was changing the quote currency instead of searching for an asset, and expecting the list of searched assets to change.

Adding more assets to the default search doesn't fix that, and as calvin himself pointed out doing so caused performance issues.

@calvinfroedge
Copy link
Contributor

I was able to work around the performance issues introduced by ignoring renders, though I'll agree it is not the most ideal solution. I do think it's a reasonable user expectation that anything under "Find Markets" is a market for the given quote currency. This was not the case before and is now.

So what is the plan here...simply not to show any assets by default? I am happy to make the change, just hoping to receive at least a partial payment for the work done to resolve the original issue. The original issue may have been a misunderstanding but it did make it into the sprint.

@wmbutler
Copy link
Contributor

@calvinfroedge It is definitely a problem if we try to list all assets before a user has typed anything in because of the network load.

@calvinfroedge
Copy link
Contributor

@wmbutler @svk31 We can show nothing until the user starts typing. That's an easy change to make.

@wmbutler
Copy link
Contributor

wmbutler commented Apr 1, 2018

Placing this in 180415. Is there any reason this didn't get merged into 180401?

@wmbutler wmbutler modified the milestones: 180401, 180415 Apr 1, 2018
@sschiessl-bcp
Copy link
Contributor Author

I think the reason was simply that we did not come to a conclusion yet.

If it matters, I would also prefer lazy loading if the user actually selects a base asset, with optional gateway selection (could also be done via settings). Exchange view is already slow enough.

@calvinfroedge I don't think anyone wants to deny you the payment, but please do keep in mind that working 5h more on it does not justify per se adding 5 quality hours to it, such an adjustment always needs to be confirmed by @wmbutler (my statement is not meant as a judgement of your work, simply as explanation)

@calvinfroedge
Copy link
Contributor

calvinfroedge commented Apr 1, 2018

I agree that no conclusion has been reached. Need @wmbutler and @svk31 to confirm final approach.

@sschiessl-bcp I have been working on the UI since last summer and we have always added time to estimates to adjust for overage. This has not been a problem up to now so that's simply what I'm accustomed to. My point was just that if there was no consensus on what the end result should be this issue should not have been placed in a sprint. Concerns like extra server load should have been taken into account prior to putting the issue into the sprint.

I think I resolved the issue (showing all assets) in the allotted time, but the performance issues made it unusable. The extra time was spent just diagnosing the performance issue. The actual fix was only a few lines of code. If you check out the branch, you'll see the UI loads the assets pretty quickly now without hanging. The only difference is no market stats show for 500-1500 ms.

@wmbutler
Copy link
Contributor

wmbutler commented Apr 2, 2018

Since it appears we need to not show assets when there is no search criteria entered, let's also show some helper text in that area that says.

Search assets above...

This will help prompt the user to know that a search must be initiated in order to show assets.

@calvinfroedge
Copy link
Contributor

calvinfroedge commented Apr 2, 2018

Ok, so...

  1. No assets shown initially

  2. Helper text

  3. Show all assets on search

@wmbutler
Copy link
Contributor

wmbutler commented Apr 2, 2018

  1. Show all assets on search

Show top 20 assets matching search regex on search. I think this will work to keep the list from being sluggish. Debounce of maybe 200ms?

@svk31
Copy link
Contributor

svk31 commented Apr 2, 2018

Like I mentioned in another comment the asset search API is very limited, unless we have all assets cached locally which is unlikely. It's a very literal search, no regex capability, so searching TC will not give you BTC, nor will searching BTC give you OPEN.BTC or RUDEX.BTC. That's another reason why I originally decided to load the OPEN.X assets by default, since searching BTC in that case would actually give you OPEN.BTC.

There are thousands of assets now so loading them all (100 at a time) each time we load the GUI is not an option imo, nor is storing that many asset names locally a good idea. Perhaps the core team can provide a better asset search API call with regex capabilities? @abitmore @pmconrad

@startailcoon
Copy link
Contributor

startailcoon commented Apr 2, 2018

When I did some work on the Asset page on the Explorer section I came across the very same issue.

I then opened an issue on the core section addressing it.

Elastic Search have been recommended for loading/searching assets.

@wmbutler
Copy link
Contributor

wmbutler commented Apr 2, 2018

ok, let's hit ES for searching assets then.

@sschiessl-bcp
Copy link
Contributor Author

Since ES does not need to be enabled in the connected node, should there be a two fold way of searching?
One using asset api with adore dumb search (like current one), one that replaces said logic with ES if present with proper indication.?

Or, should there be an option to configure separate ES connection?

@wmbutler
Copy link
Contributor

I think a fallback position is the smartest way to do it. Check for ES, if non responsive, use traditional method.

@calvinfroedge
Copy link
Contributor

Ok, so as ES option does not exist, this issue is on hold? Or should I just return all options on search until the ES option exists?

@sschiessl-bcp
Copy link
Contributor Author

There is an ES endpoint already provided for the GUI that you can use, I ll send you the URL later.

@calvinfroedge
Copy link
Contributor

@sschiessl-bcp Just following up with you on that ES endpoint.

@sschiessl-bcp
Copy link
Contributor Author

sschiessl-bcp commented Apr 20, 2018

Sorry for the delay...

#1150 (comment)

@wmbutler wmbutler modified the milestones: 180418, 180501 Apr 21, 2018
@sschiessl-bcp
Copy link
Contributor Author

@calvinfroedge

How is the status on this? Could you connect to the ES?

@wmbutler wmbutler removed this from the 180501 milestone May 12, 2018
@calvinfroedge calvinfroedge removed their assignment May 21, 2018
@calvinfroedge
Copy link
Contributor

@sschiessl-bcp Unclaiming this, you are free to close it out

@wmbutler wmbutler added this to the 180605 milestone May 22, 2018
@wmbutler wmbutler modified the milestones: 180615, 180701 Jun 20, 2018
@svk31
Copy link
Contributor

svk31 commented Jul 9, 2018

Fixed by @startailcoon in #1677, not sure what to do about the rewards here. @wmbutler ?

@wmbutler
Copy link
Contributor

wmbutler commented Jul 9, 2018

@startailcoon did you get paid fairly in #1677? If so, please remove the bounty and close this.

@wmbutler wmbutler modified the milestones: 180706, 180721 Jul 9, 2018
@startailcoon
Copy link
Contributor

@wmbutler I would say yes. Removing bounty on this issue.

@startailcoon startailcoon changed the title [7.5] Find Market only shows OPEN assets Find Market only shows OPEN assets Jul 9, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
[1c] Task Task for team member to perform. Description may contain a Task List and reference child Sub-Tasks
Projects
None yet
Development

No branches or pull requests

7 participants