client/core: Refactor Login #1903

martonp · 2022-10-15T21:46:49Z

client/core:

Login is updated not return a LoginResult. It only returns an error if the password is incorrect. On the first login after startup or after a logout, the wallets are connected, active trades are resolved, and the dex connections are authed. On subsequent logins, only the password is checked.
Two new functions, GetNotifications and AuthDEXConnections are added.
loadDBTrades now adds all trades for which a walletSet is able to be generated to the dexConnection's trades map. A field readyToTick is added to trackedTrade, and tick returns early for trades where it is set to false. resumeTrades will set readyToTick to true for all trades where both wallets can be unlocked. resumeTrades is also called whenever a wallet is unlocked or connected in order to possibly resume trades that were previously unable to have both wallets unlocked.

client/webserver:

/api/getnotes and /api/authdexconns are added.

app:

/api/getnotes is called after login to retrieve existing notifications since this is now removed from login.
/api/authdexconns is called instead of /api/login after a dex account is imported.
Active trades that have readyToTick will have their header displayed in red on the market's page. Also a message asking the user to unlock their wallets is displayed.

Closes #1762

client/core/core.go

chappjc

Some quick thoughts about the webserver/middleware/frontend bits. Moving to backend...

client/core/core.go

client/webserver/api.go

client/webserver/site/src/js/forms.ts

client/core/core.go

chappjc · 2022-11-22T18:21:58Z

client/core/core.go

-				}
+
+		tracker.wallets = walletSet
+		dc.trades[tracker.ID()] = tracker


Is dc.tradeMtx locked when doing this?

No, I've locked it now.

I haven't digested the logic yet as it pertains to the resumeTrades bool, but we should be sure never to overwrite an existing map entry. If there are two trackedTrade instances for the same orders, bad things can and will happen.

I don't think an overwrite could happen because this function is only called at login, but I've added a check to make sure.

The resumeTrades bool arguments have anything to do with adding trades to the map. It's only to avoid recursive calls back to resumeTrades when wallets are being unlocked in resumeTrade.

In (*Core).resumeTrade, there is a large amount of work being done even for trades that don't need to be resumed i.e. are already running fine. We should really leave alone trades that are fine. Calling FundingCoins repeatedly, re-auditing contracts, messing with account-based wallet reserves, etc. Need to avoid all that somehow. It looks to me like all orders that would (re)load fine are going to go through all this, return true from resumeTrade, and emit an order note.

We talked about having the lame trades in the trades map for reasons, but that doesn't mean we can't add a tracking structure as a field to dexConnections to assist.

That's potentially separate, but I also think we should reconsider at the need for the bool because it does hint that something is amiss in the design.

A related thing that stands out is when wallet A is unlocked, we try to resume all trades even if they have nothing to do with wallet A.

Ohh, readyToTick blocks calling these again? OK. Seems like resumeTrade could check this too and return true.

Please consider the other points though.

Seems like resumeTrade could check this too and return true.

Good call, I'll add a check.

That's potentially separate, but I also think we should reconsider at the need for the bool because it does hint that something is amiss in the design.

I was considering putting the lame trades into a separate map, and then having cancelOrder search through those as well, but it seemed cleaner to do it with the bool. Why is this a bad design? The trades map has all of the active trades for a dex, but some of them could not yet complete pre-processing, so we mark this with a flag. It seems decently straightforward.

A related thing that stands out is when wallet A is unlocked, we try to resume all trades even if they have nothing to do with wallet A.

I considered this also.. we could pass an assetID filter to resumeTrades, but the max cost of not doing it is one failed call to per wallet that has active trades.
Maybe it's far fetched, but there could be a case where the user has an active trade with asset A-B, and one with A-C. Both A and B initially failed to connect. Then they start both the A and B RPC wallets, unlock the B wallet, and A is able to automatically unlock. If we only resume trades involving wallet B, then the A-C trade will not be resumed. Unless we allow recursive calls back to resumeTrades.

I just think it's such a minimal cost, and better to attempt and fail, than not attempt and miss a chance to resume a trade.

it seemed cleaner to do it with the bool. Why is this a bad design? The trades map has all of the active trades for a dex, but some of them could not yet complete pre-processing, so we mark this with a flag. It seems decently straightforward.

OK, I think it works well.
It's just a bit error prone (see last review for instance) and takes some more investigation to follow when it's used correctly.

client/core/core.go

client/rpcserver/handlers.go

buck54321 · 2022-11-24T09:29:33Z

client/webserver/site/src/js/markets.ts

@@ -1252,6 +1258,7 @@ export default class MarketsPage extends BasePage {
      })
      app().bindTooltips(div)
    }
+    Doc.setVis(unreadyOrders, page.unreadyOrdersMsg)


I wonder if we'll want a way to indicate a problem even if the user is on a different market. Don't have to do it now. Could be a db.ErrorLevel notification maybe. Or maybe just well-placed messaging. Either way, I'd hate it if my orders on one market were not processing but I'm unaware because I'm looking at a different market.

I was thinking to do a popup that shows up on the markets page after login in another PR.

buck54321 · 2022-11-24T09:32:42Z

client/core/core.go

+// status, emitting WalletStateNotes on each progress update. If resumeTrades
+// is set to true, an attempt to resume and trades that were unable to be
+// resumed at startup will be made.
+func (c *Core) connectAndUnlockResumeTrades(crypter encrypt.Crypter, wallet *xcWallet, resumeTrades bool) error {


One thing that smells is the resumeTrades bool arguments being passed around. My intuition is that this hides an underlying deficiency where we should be doing a more sophisticated form of resolution once new wallets come online, i.e. we should be able to tell if trades need to be resumed without the caller's input.

The reason for passing it around is to avoid recursive calls from happening when connectAndUnlockWallet is called from resumeTrade. I guess it's not completely necessary..

chappjc

Looking pretty good to me!

client/core/core.go

chappjc · 2022-11-28T22:16:42Z

client/core/core.go

-				}
+
+		tracker.wallets = walletSet
+		dc.trades[tracker.ID()] = tracker


it seemed cleaner to do it with the bool. Why is this a bad design? The trades map has all of the active trades for a dex, but some of them could not yet complete pre-processing, so we mark this with a flag. It seems decently straightforward.

OK, I think it works well.
It's just a bit error prone (see last review for instance) and takes some more investigation to follow when it's used correctly.

client/core/core.go

client/webserver/locales/en-us.go

client/webserver/site/src/js/settings.ts

chappjc · 2022-11-30T17:01:05Z

client/webserver/site/src/js/markets.ts

+    // - If an OrderLoaded notification is recieved, it means an order that was
+    //   previously not "ready to tick" (due to its wallets not being connected
+    //   and unlocked) has now become ready to tick. The active orders section


Doesn't this also happen when the user just logs in with active orders? Won't it emit one of these OrderLoaded topic notes for each active order on initial login, causing it to refresh active orders multiple times?

Yes, but those will be before login is complete, so the market page will not yet be loaded.

Yes, but those will be before login is complete, so the market page will not yet be loaded.

Ah, OK, perfect.

chappjc · 2022-11-30T21:02:23Z

I noticed a nil context panic in btc.externalFeeEstimator, but I'll get to that in #1967 right after this goes in. Saw a duplicate log statement too, but otherwise working as expected.

I had forgotten that "unfunded" orders get automatically canceled in authDEX. Since these failed-to-resume orders never get tracker.coinsLocked = true in resumeTrade, booked orders that don't have a connected or unlockable wallet get canceled. I think that's good, I just forgot. Yeah, very good... previously these orders wouldn't have even been loaded into the trades map. 😬

chappjc · 2022-12-01T02:07:02Z

The wild goroutine (go c.resumeTrades(nil)) is causing issues, particularly with tests where data races are caused with rapid repeated calls like in TestReconfigureWallet.

client/core: - Login is updated not return a LoginResult. It only returns an error if the password is incorrect. On the first login after startup or after a logout, the wallets are connected, active trades are resolved, and the dex connections are authed. On subsequent logins, only the password is checked. - The new Core method, Notifications is are added. - resolveActiveTrades is now called not only from login, but also after a wallet is unlocked. Trades that failed to resume on the initial call to resolveActiveTrades are cached on the dexConnection and when wallets are later unlocked, resolveActiveTrades is called again in order to attempt to resume those trades. client/webserver: - /api/getnotes is added. ui: - /api/getnotes is called after login to retrieve existing notifications since this is now removed from login.

martonp force-pushed the refactorLogin branch from cb2ec8a to 6c48d5e Compare October 16, 2022 10:45

martonp force-pushed the refactorLogin branch 2 times, most recently from 1c1e301 to 17a5e71 Compare November 1, 2022 23:16

martonp marked this pull request as ready for review November 1, 2022 23:16

buck54321 reviewed Nov 10, 2022

View reviewed changes

client/core/core.go Outdated Show resolved Hide resolved

client/core/core.go Show resolved Hide resolved

client/core/core.go Outdated Show resolved Hide resolved

chappjc self-requested a review November 10, 2022 19:52

martonp force-pushed the refactorLogin branch from 1d2dba8 to cd8ab1a Compare November 10, 2022 20:20

chappjc reviewed Nov 10, 2022

View reviewed changes

client/core/core.go Show resolved Hide resolved

martonp force-pushed the refactorLogin branch from 996b5a2 to df3c13b Compare November 22, 2022 14:31

chappjc requested changes Nov 22, 2022

View reviewed changes

chappjc reviewed Nov 22, 2022

View reviewed changes

martonp force-pushed the refactorLogin branch 2 times, most recently from 2d6ce52 to 66bca3e Compare November 23, 2022 14:05

buck54321 reviewed Nov 24, 2022

View reviewed changes

chappjc mentioned this pull request Nov 28, 2022

[dex] DEX-locked funds awareness decred/decrediton#3838

Open

chappjc requested changes Nov 28, 2022

View reviewed changes

chappjc reviewed Nov 30, 2022

View reviewed changes

chappjc force-pushed the refactorLogin branch from bc890f2 to 8e77ee9 Compare December 2, 2022 04:14

chappjc merged commit 13758c0 into decred:master Dec 2, 2022

martonp deleted the refactorLogin branch December 20, 2022 22:07

chappjc added this to the 0.6 milestone Dec 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/core: Refactor Login #1903

client/core: Refactor Login #1903

martonp commented Oct 15, 2022 •

edited

Loading

chappjc left a comment

chappjc Nov 22, 2022

martonp Nov 23, 2022

chappjc Nov 24, 2022

martonp Nov 26, 2022

chappjc Nov 26, 2022 •

edited

Loading

chappjc Nov 26, 2022 •

edited

Loading

martonp Nov 26, 2022 •

edited

Loading

chappjc Nov 28, 2022

buck54321 Nov 24, 2022

martonp Nov 24, 2022

buck54321 Nov 24, 2022

martonp Nov 24, 2022

chappjc left a comment

chappjc Nov 28, 2022

chappjc Nov 30, 2022

martonp Nov 30, 2022

chappjc Nov 30, 2022

chappjc commented Nov 30, 2022 •

edited

Loading

chappjc commented Dec 1, 2022

client/core: Refactor Login #1903

client/core: Refactor Login #1903

Conversation

martonp commented Oct 15, 2022 • edited Loading

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Nov 26, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc Nov 26, 2022 • edited Loading

Choose a reason for hiding this comment

martonp Nov 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Nov 30, 2022 • edited Loading

chappjc commented Dec 1, 2022

martonp commented Oct 15, 2022 •

edited

Loading

chappjc Nov 26, 2022 •

edited

Loading

chappjc Nov 26, 2022 •

edited

Loading

martonp Nov 26, 2022 •

edited

Loading

chappjc commented Nov 30, 2022 •

edited

Loading