client: Update server host #1605

martonp · 2022-05-05T06:06:26Z

DEX servers are able to advertise on multiple host names. This update gives users of the dex client the ability to switch between the different host names of the same dex.

client/core: Add UpdateDexHost method which takes
a current dex host and a new host name, and if the dex
at both host names use the same dex pub key, the host
name is updated. Also, adding a dex with the same pub
key as a dex which is already registered is now blocked.
ui: Adds a button on the dex settings page to allow updating
the dex host. The DEXAddressForm is reused for this purpose.
The DEXAddressPopup is also updated to not show known
exchanges that the user is already registered for.

buck54321 · 2022-05-25T11:04:13Z

client/core/account.go

+			atomic.StoreUint32(&newDc.reportingConnects, 1)
+			c.wg.Add(1)
+			go c.listen(newDc)
+			go newDc.subPriceFeed()
+
+			c.connMtx.Lock()
+			c.conns[newDc.acct.host] = newDc
+			c.connMtx.Unlock()


This section is a duplicate. Maybe a new upgradeConnection method?

buck54321 · 2022-05-25T11:51:41Z

client/core/account_test.go

+		newHost     string
+		feePending  bool
+		expectError bool
+		newPubKey   *secp256k1.PublicKey


Looks like you don't need the newPubKey and newHost fields. They are always the same. Just use vars?

newPubKey isn't always the same, but you're right about newHost.

buck54321 · 2022-05-25T11:57:16Z

client/webserver/api.go

@@ -418,6 +418,31 @@ func (s *WebServer) apiUpdateCert(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, simpleAck(), s.indent)
 }

+func (s *WebServer) apiUpdateDexHost(w http.ResponseWriter, r *http.Request) {
+	form := new(updateHostForm)


If updateHostForm is only used once here, how about an anonymous struct.

form := new(struct { Pass encode.PassBytes `json:"pw"` OldHost string `json:"oldHost"` NewHost string `json:"newHost"` Cert string `json:"cert"` })

I think it improves readability.

buck54321 · 2022-05-25T12:09:06Z

client/core/account.go

+// UpdateDexHost updates the host for a connection to a dex. The dex at oldHost
+// and newHost must be the same dex, which means that the dex at both hosts use
+// the same public key.
+func (c *Core) UpdateDexHost(oldHost, newHost string, appPW []byte, certI interface{}) (*Exchange, error) {


Noting the difference of the capitalization of DEX between UpdateDexHost with GetDEXConfig.

buck54321 · 2022-05-25T12:19:08Z

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

+    if (this.knownExchanges.length === 0) {
+      Doc.show(page.customBox)
+      Doc.hide(page.showCustom, page.knownXCs, page.pickServerMsg, page.addCustomMsg)
+    } else {
+      Doc.hide(page.customBox)
+      Doc.show(page.showCustom)
+    }


Something is off here and I can't submit the form. When bringing up the form to change the host, there is no button.

Compare that with the expanded custom dex form when going through the registration sequence, where the button is displayed.

Sorry about that, I think it must be because I forgot to update the cache buster. I've updated it now.

Still happening for me.

I see the problem. It's a pre-existing bug. The button won't be displayed when the password is cached, since we're hiding page.auth, which includes the submit button.

During the initial registration sequence, it doesn't matter, because we

Doc.bind(page.showCustom, 'click', () => { Doc.hide(page.showCustom) Doc.show(page.customBox, page.auth) })

But when the custom form is displayed by default (because we already registered at the only DEX), we never hit that code.

I think the easiest solution is to

if (this.knownExchanges.length === 0) Doc.show(page.auth)

buck54321 · 2022-05-31T11:00:10Z

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

+    if (this.knownExchanges.length === 0) {
+      Doc.show(page.customBox)
+      Doc.hide(page.showCustom, page.knownXCs, page.pickServerMsg, page.addCustomMsg)
+    } else {
+      Doc.hide(page.customBox)
+      Doc.show(page.showCustom)
+    }


Still happening for me.

I see the problem. It's a pre-existing bug. The button won't be displayed when the password is cached, since we're hiding page.auth, which includes the submit button.

During the initial registration sequence, it doesn't matter, because we

Doc.bind(page.showCustom, 'click', () => { Doc.hide(page.showCustom) Doc.show(page.customBox, page.auth) })

But when the custom form is displayed by default (because we already registered at the only DEX), we never hit that code.

I think the easiest solution is to

if (this.knownExchanges.length === 0) Doc.show(page.auth)

martonp · 2022-05-31T14:39:42Z

@buck54321 I see it now. I need to make a habit of testing with the password both cached and not cached.

JoeGruffins

Testing well for me.

client/webserver/locales/en-us.go

client/webserver/site/src/localized_html/en-US/bodybuilder.tmpl

server/cmd/dcrdex/main.go

client/webserver/api.go

client/core/account.go

client/core/core.go

client/webserver/api.go

buck54321 · 2022-06-11T01:20:14Z

client/core/account.go

+	err = c.AccountDisable(appPW, oldHost)
+	if err != nil {
+		return nil, fmt.Errorf("failed to disable old account: %w", err)
+	}
+
+	_, err = c.discoverAccount(newDc, crypter)
+	if err != nil {
+		return nil, err
+	}


I'm wondering if the order of these AccountDisable and discoverAccount should be switched.

I changed it so that it disconnects the dex first, then does discoverAccount, then finally removes the old dex from the db.

buck54321 · 2022-06-11T01:24:56Z

client/webserver/http.go

+// KnownUnregisteredExchanges returns all the known exchanges that
+// the user has not registered for.
+func (s *WebServer) KnownUnregisteredExchanges(registeredExchanges map[string]*core.Exchange) []string {


No need to export.

buck54321 · 2022-06-11T01:32:15Z

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

    if (this.pwCache) this.pwCache.pw = pw
    this.success(res.xc, cert)
  }

+  makeDexUpdater (dexToUpdate: string) {


Please comment here that this is a one way function. The changes made during makeDexUpdater are not reversed upon refresh. So the same DEXAddressForm can't do double duty for new registration and host name updates.

I would actually prefer that makeDexUpdater was used internally based on an argument to the constructor.

Remove makeDexUpdater now? I believe you have it done in if (dexToUpdate) { now.

buck54321 · 2022-06-11T01:48:25Z

client/webserver/site/src/html/dexsettings.tmpl

@@ -22,6 +22,9 @@
      <button id="updateCertBtn" class="bg2 selected">[[[Update TLS Certificate]]]</button>


Not from this PR, but from #1602 (comment), I think only part of solution made it into the final product.

Can you get rid of the <h4> up above and do something like

<div class="flex-center fs16 mb-2"> <span class="me-2 connection ico-connection d-hide" id="connectedIcon"></span> <span class="me-2 disconnected ico-disconnected d-hide" id="disconnectedIcon"></span> <span id="connectionStatus"></span> </div>

buck54321 · 2022-06-11T01:50:31Z

client/core/account.go

+			oldHost, newHost)
+	}
+
+	err = c.AccountDisable(appPW, oldHost)


If there are active orders, we'll fail here, but should we check that condition higher up to prevent making an unnecessary connection?

chappjc

A few little things left, but seems g2g.

client/core/core.go

client/core/account.go

chappjc · 2022-06-13T18:40:17Z

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

    if (this.pwCache) this.pwCache.pw = pw
    this.success(res.xc, cert)
  }

+  makeDexUpdater (dexToUpdate: string) {


Remove makeDexUpdater now? I believe you have it done in if (dexToUpdate) { now.

chappjc

LGTM. Working very well.

Might as well rebase while waiting for @buck54321 to circle back since your merge base is quite old. I tested a rebased branch though.

buck54321 · 2022-06-17T12:56:01Z

client/core/account.go

+	if err != nil {
+		return nil, err


This would be a weird place to end up, but I think the likelihood of getting here is virtually nil.

DEX servers are able to advertise on multiple host names. This update gives users of the dex client the ability to switch between the different host names of the same dex. - client/core: Add `UpdateDexHost` method which takes a current dex host and a new host name, and if the dex at both host names use the same dex pub key, the host name is updated. Also, adding a dex with the same pub key as a dex which is already registered is now blocked. - ui: Adds a button on the dex settings page to allow updating the dex host. The DEXAddressForm is reused for this purpose. The DEXAddressPopup is also updated to not show known exchanges that the user is already registered for.

martonp force-pushed the updateHost branch 4 times, most recently from 24c6f8a to 301a141 Compare May 25, 2022 04:04

martonp marked this pull request as ready for review May 25, 2022 04:04

buck54321 requested changes May 25, 2022

View reviewed changes

chappjc added this to the 0.5 milestone May 25, 2022

martonp force-pushed the updateHost branch from 06fa272 to e38431c Compare May 26, 2022 02:40

buck54321 requested changes May 31, 2022

View reviewed changes

martonp force-pushed the updateHost branch from 2d1a90e to 8f2de95 Compare May 31, 2022 14:55

JoeGruffins approved these changes Jun 2, 2022

View reviewed changes

chappjc requested changes Jun 7, 2022

View reviewed changes

buck54321 requested changes Jun 11, 2022

View reviewed changes

chappjc self-requested a review June 13, 2022 16:07

chappjc requested changes Jun 13, 2022

View reviewed changes

chappjc approved these changes Jun 14, 2022

View reviewed changes

martonp force-pushed the updateHost branch from 5b9620c to f2c6ced Compare June 16, 2022 00:51

buck54321 approved these changes Jun 17, 2022

View reviewed changes

chappjc mentioned this pull request Jun 17, 2022

ui: order history button (before server has been picked) doesn't work properly? #1649

Closed

chappjc force-pushed the updateHost branch from f2c6ced to b4cada4 Compare June 20, 2022 23:03

chappjc merged commit 1283356 into decred:master Jun 20, 2022

martonp deleted the updateHost branch January 20, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: Update server host #1605

client: Update server host #1605

martonp commented May 5, 2022 •

edited

Loading

buck54321 May 25, 2022

buck54321 May 25, 2022

martonp May 26, 2022

buck54321 May 25, 2022

buck54321 May 25, 2022

buck54321 May 25, 2022

martonp May 26, 2022

buck54321 May 31, 2022

buck54321 May 31, 2022

martonp commented May 31, 2022

JoeGruffins left a comment

buck54321 Jun 11, 2022

martonp Jun 13, 2022

buck54321 Jun 11, 2022

buck54321 Jun 11, 2022

chappjc Jun 13, 2022

buck54321 Jun 11, 2022

buck54321 Jun 11, 2022

chappjc left a comment

chappjc Jun 13, 2022

chappjc left a comment •

edited

Loading

buck54321 Jun 17, 2022

		@@ -22,6 +22,9 @@
		<button id="updateCertBtn" class="bg2 selected">[[[Update TLS Certificate]]]</button>

client: Update server host #1605

client: Update server host #1605

Conversation

martonp commented May 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented May 31, 2022

JoeGruffins 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

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

chappjc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented May 5, 2022 •

edited

Loading

chappjc left a comment •

edited

Loading