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

[3][sschiessl-bcp][.5][svk31] Exchange not loading for new, zero-balance accounts #1481

Closed
sschiessl-bcp opened this issue May 3, 2018 · 7 comments
Assignees
Labels
[3] Bug Classification indicating the existing implementation does not match the intention of the design [4c] High Priority Priority indicating significant impact to system/user -OR- workaround is prohibitivly expensive
Milestone

Comments

@sschiessl-bcp
Copy link
Contributor

When a new account is created via cloud login, the exchange tab doesnt load anymore for some markets.

prominent example:

does not load
https://wallet.bitshares.org/#/market/USD_BTS

does load
https://wallet.bitshares.org/#/market/BTS_USD

The issue is in

that the feeStatus does not get loaded, which causes a return null in the render method.

@sschiessl-bcp
Copy link
Contributor Author

So far I think it's due to the promise

asyncCache[key].queue.forEach(promise => {

not being resolved properly

@sschiessl-bcp
Copy link
Contributor Author

sschiessl-bcp commented May 3, 2018

So I probably was digging around for three hours now, learned quite some of the exchange mechanics, still no clue though.

Anyways, it seems that the issue was that there were multiple fee loading promises for BTS in the above case.

Adding this above

did resolve it for me

        if (assets[0] === assets[1]) {
            assets.splice(1, 1);
        }

Any insights form anyone as to why this is connected to zero balance accounts? It feels like this is treating the symptom, not the cause.

@wmbutler wmbutler added [3] Bug Classification indicating the existing implementation does not match the intention of the design [4c] High Priority Priority indicating significant impact to system/user -OR- workaround is prohibitivly expensive labels May 3, 2018
@wmbutler wmbutler added this to the 180501 milestone May 3, 2018
@wmbutler wmbutler changed the title Exchange not loadeding for new, zero-balance accounts [3] Exchange not loading for new, zero-balance accounts May 3, 2018
@svk31
Copy link
Contributor

svk31 commented May 4, 2018

That does fix the issue, I'm guessing the reason is that checkFeePoolAsync fails silently at some point because it tries to fetch and use balance objects, but the concerned accounts do not have any balances to fetch.

@svk31
Copy link
Contributor

svk31 commented May 4, 2018

Ok found it: c6cdd4a

@sschiessl-bcp
Copy link
Contributor Author

@svk31

do you think my initial fix with

assets[0] === assets[1]

is also necessary (I think it could be, because I often hear "my exchange is not loading" in telegram")? The code removes double entries if they occur on index 0,2 or 1,2. Is there a reason double entries occuring on 0,1 should be kept?

@svk31
Copy link
Contributor

svk31 commented May 8, 2018

I seem to recall that the original reason for removing duplicate assets was simply to make sure the asset info was not fetched twice in parallel. With my recent changes to trxHelper adding request queueing and caching, that's no longer a concern so we can actually just remove that duplicate check in Exchange.jsx.

@svk31
Copy link
Contributor

svk31 commented May 8, 2018

@wmbutler Not sure how to handle hours on this issue, @sschiessl-bcp's investigations allowed me to find the root cause quite quickly, and it took me about 30 minutes to fix the issue. Stefan should also be assigned some time for his investigations imo.

@svk31 svk31 closed this as completed May 8, 2018
svk31 added a commit that referenced this issue May 9, 2018
@wmbutler wmbutler changed the title [3] Exchange not loading for new, zero-balance accounts [3][sschiessl-bcp][.5][svk31] Exchange not loading for new, zero-balance accounts May 15, 2018
Cryptolero pushed a commit to CryptoBridge/cryptobridge-ui that referenced this issue Nov 2, 2018
Cryptolero pushed a commit to CryptoBridge/cryptobridge-ui that referenced this issue Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[3] Bug Classification indicating the existing implementation does not match the intention of the design [4c] High Priority Priority indicating significant impact to system/user -OR- workaround is prohibitivly expensive
Projects
None yet
Development

No branches or pull requests

3 participants