client/core: refactor preregister and register methods #328

itswisdomagain · 2020-05-03T10:56:27Z

Key changes:

core.PreRegister renamed to core.GetFee

Close ws connections after retrieving fee, delete connection-caching code.

core.Register

Return error for registration attempts where a connection already exists for the url in the conns map. This tallies with the check made in ~~core.PreRegister~~core.GetFee.
Resolves first part of app: show better messaging when waiting for fee confirmations #306 by rewording the notification title displayed when fee is paid but not confirmed from ~~Fee paid~~ to Fee payment in progress. Though the notification subtext shows Waiting for N confirmations before trading at URL, that might easily get missed because of the bolder Fee paid title.

core.connectDEX

Attempt to connect to urls without scheme instead of using empty ws conn string and erroring.
~~Check cert before attempting connection to return better error message for missing certs scenarios; currently returns x509: certificate signed by unknown authority~~
Undone as per client/webserver,dexcctl: replace new wallet config file path with cfg text #346 (comment)

buck54321

Lots of nice improvements here, but a couple of questions.

client/core/core.go

dex/runner.go

client/db/bolt/db.go

client/core/core.go

chappjc · 2020-05-08T15:11:34Z

client/core/core.go

+	uri := parsedURL.Host // empty for urls without scheme
+	if uri == "" {
+		uri = parsedURL.String()


Happy to see this, but I'd like to see a change at some point that stops calling it a URL at all, and requires only a host:port.

client/core/core.go

chappjc · 2020-05-08T15:20:30Z

Some conflicts too with the just-merged fee notification tweaks PR.

core.PreRegister - Don't (re-)connect if there is a pending connection for same url, only reset timer. - Close pending connection if a new connection to a new url is established and temporarily cache the new connection. core.Register - Return error for registration attempts where a connection already exists for the url in the conns map. - If there is no cached connection (either from a previous PreRegister call or from a previous failed Register call), create a new connection and cache it (in case Register fails and user wants to try again). - Save new account info to db after registration to the dex server completes successfully but before paying fees so that user will not need to re-register with dex server if dexc is restarted. core.connectDEX - Attempt to connect to urls without scheme instead of using empty string. - Check cert before attempting connection to return better error message for missing certs scenarios.

chappjc · 2020-05-13T14:12:10Z

I know this PR is a "refactoring" job as stated by the title, but going forward let's try to be conservative with how many changes go into a PR. Part of it - and I feel like a complete hypocrite saying this - is large changes during review like the migration of code into dexconnection.go and routehandlers.go that make it difficult to follow diffs, and at a point when we had already reviewed most of the PR. Seems like everything was just copied over, but since the diff between two different files isn't shown it is hard to know. I did something like like this with server/market/epump.go in #287, but the motivation there was a deadlock bug that unfortunately necessitated significant changes to the types.

I support changes that solve bugs or make code less error prone, don't get me wrong. But we should try to keep the scope of the changes in check. Each notable change requires the tester to go back to their simnet harnesses and setup everything from scratch. That's where I'm at. I don't have any more specific comments, but I also just noticed the last commit and I still need to retest previous commits.

chappjc · 2020-05-13T14:22:29Z

Actually, there are a ton of open client/core PRs right now and I'm pretty certain it's going to create a bunch of conflict headaches because the resolution involves a new file. Can you revert b2e17e9. except the dexConn->dc rename? Could also propose the core.authDEX(dc) to dc.auth() change in a separate commit since we've already discussed it a bit, but I more inclined to say let's just draw the line and consider that change in a follow up, with clear justification and a separate merge to track it.

chappjc

Retested and working. Ready to go after final commit is reworked without the new file creation. I generally support that move, but when there are far fewer conflicting PRs up.

buck54321 · 2020-05-13T13:21:56Z

client/core/types.go

+	if privKey != nil {
+		// no need to generate a new privKey, just decrypt.
+		return privKey, a.unlock(crypter)
+	}


A properly initialized but locked dexAccount will still have a nil privKey. Did you mean to check dexAccount.encKey instead?

buck54321 · 2020-05-13T13:27:13Z

client/core/core.go

-
-// signAndRequest signs and sends the request, unmarshaling the response into
-// the provided interface.
-func (dc *dexConnection) signAndRequest(signable msgjson.Signable, route string, result interface{}) error {


You've made this a standalone function, but are then passing the dexConnection as the first argument every time you use it. Why not leave it as a method then?

Weird. I think there was a case where I needed to sign and send a request but did not have a dexConnection instance. Looking at it now, ALL references to the standalone function pass a reference to a dexConnection object. Will revert this change.

itswisdomagain · 2020-05-14T00:59:19Z

Retested and working. Ready to go after final commit is reworked without the new file creation. I generally support that move, but when there are far fewer conflicting PRs up.

Done.

chappjc · 2020-05-14T15:18:14Z

client/core/core.go

-	parsedURL, err := url.Parse(uri)
+	parsedURL, err := url.Parse(acctInfo.URL)
 	if err != nil {
-		return nil, fmt.Errorf("error parsing account URL %s: %v", uri, err)
+		return nil, fmt.Errorf("error parsing account URL %s: %v", acctInfo.URL, err)
+	}
+	uri := parsedURL.Host // empty for urls without scheme
+	if uri == "" {
+		uri = parsedURL.String()


This only seems to help with ""//127.0.0.1:7232", which isn't much of an improvement over a random shceme. Doesn't seem to work. If you give it "127.0.0.1:7232" without the scheme, then url.Parse errors. Seems like you'd want to try net.SplitHostPort in the event that Parse fails. e.g. https://play.golang.org/p/_iJOLLmeyzV But let's address this later.

$  ./dexcctl -u u -P p getfee "http://127.0.0.1:7232" ~/.dcrdex/rpc.cert { "fee": 100000000 } $  ./dexcctl -u u -P p getfee "127.0.0.1:7232" ~/.dcrdex/rpc.cert error parsing account URL 127.0.0.1:7232: parse "127.0.0.1:7232": first path segment in URL cannot contain colon $  ./dexcctl -u u -P p getfee "//127.0.0.1:7232" ~/.dcrdex/rpc.cert { "fee": 100000000 }

iirc, I tried with localhost:35 and that worked.

Oh, I think's that's because with localhost:35 it interprets that in the format scheme:opaque?query#fragment so that URL.String is printing it as scheme+":"+opaque.

Yup, it thinks localhost is the scheme in an opaque url that way https://play.golang.org/p/2KxZxqeMOTN

Let's fix this in another PR though.

itswisdomagain force-pushed the client-touchups branch from 898810c to a20e6b6 Compare May 3, 2020 16:16

itswisdomagain changed the title ~~client: refactor preregister and register ops in core and webserver~~ client/core: refactor preregister and register methods May 3, 2020

buck54321 reviewed May 4, 2020

View reviewed changes

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

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

dex/runner.go Outdated Show resolved Hide resolved

client/db/bolt/db.go Outdated Show resolved Hide resolved

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

itswisdomagain force-pushed the client-touchups branch from c3ee8c7 to b132cd5 Compare May 4, 2020 18:28

JoeGruffins mentioned this pull request May 5, 2020

client: authDEX on reconnect. #330

Merged

itswisdomagain marked this pull request as ready for review May 5, 2020 19:49

itswisdomagain force-pushed the client-touchups branch from b132cd5 to 751a29e Compare May 5, 2020 19:54

itswisdomagain commented May 5, 2020

View reviewed changes

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

chappjc requested changes May 8, 2020

View reviewed changes

itswisdomagain added 8 commits May 9, 2020 09:01

nuke dex connection caching, rename PreRegister to GetFee

a9eb52e

undo ConnectionMaster rename

604c61b

core.Register: save account only after fee payment

4a86804

rpcserver: rename preregister to getfee

220ce9f

remove last references to preregister

0eb6d5b

apiRegister -> pass cert in call to core.Register

0ef4a41

core: rename isDEXRegistered, remove cert check in connectDEX

b950279

itswisdomagain force-pushed the client-touchups branch 2 times, most recently from 64ffbd5 to b2e17e9 Compare May 9, 2020 16:10

chappjc self-requested a review May 12, 2020 22:29

chappjc approved these changes May 13, 2020

View reviewed changes

buck54321 reviewed May 13, 2020

View reviewed changes

itswisdomagain added 2 commits May 14, 2020 01:29

rename dexConn->dc, increment c.wg before go c.listen()

2effa3d

restore dc.signAndRequest, fix check for existing dexAccount privkey

848b533

itswisdomagain force-pushed the client-touchups branch from b2e17e9 to 848b533 Compare May 14, 2020 00:57

chappjc reviewed May 14, 2020

View reviewed changes

chappjc approved these changes May 14, 2020

View reviewed changes

chappjc merged commit 7ec2b8c into decred:master May 14, 2020

itswisdomagain deleted the client-touchups branch May 15, 2020 01:54

chappjc mentioned this pull request May 15, 2020

dexc: no warning or error if dex url lacks entry in certs.json #225

Closed

buck54321 mentioned this pull request May 15, 2020

core: drop scheme from DEX ID #374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/core: refactor preregister and register methods #328

client/core: refactor preregister and register methods #328

itswisdomagain commented May 3, 2020 •

edited

Loading

buck54321 left a comment

chappjc May 8, 2020

chappjc commented May 8, 2020

chappjc commented May 13, 2020 •

edited

Loading

chappjc commented May 13, 2020

chappjc left a comment

buck54321 May 13, 2020

buck54321 May 13, 2020

itswisdomagain May 14, 2020

itswisdomagain commented May 14, 2020

chappjc May 14, 2020 •

edited

Loading

itswisdomagain May 14, 2020

chappjc May 14, 2020

chappjc May 14, 2020

client/core: refactor preregister and register methods #328

client/core: refactor preregister and register methods #328

Conversation

itswisdomagain commented May 3, 2020 • edited Loading

buck54321 left a comment

Choose a reason for hiding this comment

chappjc May 8, 2020

Choose a reason for hiding this comment

chappjc commented May 8, 2020

chappjc commented May 13, 2020 • edited Loading

chappjc commented May 13, 2020

chappjc left a comment

Choose a reason for hiding this comment

buck54321 May 13, 2020

Choose a reason for hiding this comment

buck54321 May 13, 2020

Choose a reason for hiding this comment

itswisdomagain May 14, 2020

Choose a reason for hiding this comment

itswisdomagain commented May 14, 2020

chappjc May 14, 2020 • edited Loading

Choose a reason for hiding this comment

itswisdomagain May 14, 2020

Choose a reason for hiding this comment

chappjc May 14, 2020

Choose a reason for hiding this comment

chappjc May 14, 2020

Choose a reason for hiding this comment

itswisdomagain commented May 3, 2020 •

edited

Loading

chappjc commented May 13, 2020 •

edited

Loading

chappjc May 14, 2020 •

edited

Loading