client/core: hide order form and display registration status message #353

fernandoabolafio · 2020-05-10T11:25:45Z

This PR modifies the code so the order form is hidden while the registration payment has not been confirmed. Instead, a message is shown to communicate the status of the confirmation. See the previews for more info.

The full list of changes:

Core changes:

Adds the dex URL to all feepayment notifications. This is particularly useful in this case to determine to either show or not the confirmation message in the current market.
The registration update notification uses now the regupdated subject.
Successful registration note uses now the regcompleted subject. So it server to multiple purposes in the frontend side. See 'Site changes' for more info.
Refresh the user after completed registration.

Site changes:

Added an override handler in the app.js which enables the overriding of notifications subject. This is useful to have a notification serving multiple purposes, such as the 'regcompleted' in this case where it is both displayed in the notifications dropdown and it is used to display the order form once the registration payment has been successful.
The ~~market~~ app page now handles feepayment notifications and update the exchanges confirmations as well the feePending value.
Hide 'order form' and display the 'registration status' while the fee payment has not been confirmed. Once it is confirmed, a success message is shown for 5 seconds and the order form is set as visible again.

UI Preview:

closes #337

buck54321

Tests well.

client/core/core.go

client/webserver/site/src/css/market.scss

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

client/webserver/site/src/js/app.js

client/core/core.go

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

chappjc · 2020-05-14T15:35:26Z

@fernandoabolafio Minor conflict to resolve please.

fernandoabolafio · 2020-05-14T15:58:41Z

@chappjc thanks for the heads up I am still working on some changes. I will fix all conflicts once I am done.

fernandoabolafio · 2020-05-14T17:12:45Z

@buck54321 I addressed all of your comments. The code looks much simpler now. Major changes are:

Instead of tracking the registration status with a new object, I am just using the user.exchanges. The tracking happens only in the App level and the loaded page is notified using a new method called registrationStatusUpdated.
I added confsRequired to the exchange object so now it doesn't need to be provided with notifications and the app is aware of this information from the start.

buck54321

A lot cleaner this way, but I think we can make it even better. See what you think of these recommendations.

client/webserver/site/src/js/basepage.js

client/webserver/site/src/js/app.js

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

client/webserver/site/src/js/doc.js

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

fernandoabolafio

@buck54321 thanks for your review! I addressed your changes and left two comments/questions for you.

client/core/core.go

buck54321

Looking good. Just some touchups.

client/rpcserver/handlers.go

client/rpcserver/handlers_test.go

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

buck54321 · 2020-05-18T21:10:26Z

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

+    const dexUrl = note.dexurl
+    if (dexUrl !== this.market.dex.url) return
+    // update local dex
+    this.market.dex = app.exchanges[dexUrl]


Looking around now, I'm noticing that this.market.dex is only ever used for the dex.url. Maybe we should just set the field as this.market.dexAddr, which won't change, so we don't have to update the dex object after fetchUser is called.

hmm. I don't think I follow you on this one. We also use this.market.dex to get the confs and confsRequired and that's the major reason we update the dex value.
See here.

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

client/rpcserver/handlers.go

client/core/notification.go

fernandoabolafio · 2020-05-20T20:47:41Z

@buck54321 thanks again for the comments! All the comments were addressed with the exception of this one: #353 (comment) where I left a question for you.

chappjc · 2020-05-21T14:36:32Z

url -> host rename created a couple very minor conflicts.

Also master broke some js stuff. Fixed in
(note "excecuted" typo is fixed in #399) though so that should probably go in first. merged

chappjc · 2020-05-21T16:03:31Z

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

+   */
+  handleFeePayment (note) {
+    const dexAddr = note.dex
+    if (dexAddr !== this.market.dex.url) return


Note that dex.url is now dex.host, with some related fixes in the now-merged PR #399 .

chappjc · 2020-05-22T02:59:58Z

PR #399 fixing some url/host variable name issues now merged. Should be able to resolve and update to use dex.host instead of dex.url in your new code. Thanks and sorry for the repeated conflicts! Lots of concurrent work on the same files right now.

fernandoabolafio · 2020-05-23T20:04:46Z

client/core/notification.go

 	return &FeePaymentNote{
-		Notification: db.NewNotification("fee payment", subject, details, severity),
+		Notification: db.NewNotification("feepayment", subject, details, severity),
+		Dex:          addrHost(dexAddr),


@chappjc thanks for the heads up. I updated the code to use the dex host instead. The feepayment is also sending the host by making sure to transform dexAddr using the addrHost helper.

buck54321

Looking good, but there may be an issue having to do with the recent work in #322. When I first create my account, the screen is all good. If I then click on a market with an unsupported asset, It's also still okay, though the styling could use some work

Then, if I click on the dcr-btc market again, both the the confirmations message and the order form are shown

buck54321 · 2020-05-26T12:16:44Z

client/webserver/site/src/js/app.js

+  }
+
+  /*
+   * handleFeePaymentNote is the handler for the 'feepayment'-type notificaetion, which


notificaetion

fernandoabolafio · 2020-05-26T17:27:43Z

@buck54321 I addressed the changes you requested:

Added a method called resolveOrderFormVisibility which is used every time another methods wants to update the orderForm visibility. The conditions which changes the form visibility are defined inside resolveOrderFormVisibility.
Simplified a bit the code at the end of setMarket and merged the 'loaderMsg' functions (hide/show) into a single one.
Add to 'query methods' hasFeePending and assetsAreSupported which are used to ease the readability and encapsulate logic we are reusing.
Style tweak:

chappjc

Looks like there is a data race on dexConnection.regConfirms. Also, I don't care much for it being a pointer in dexConnection, although I see why it's needed that way in the Exchange type, so that's OK I suppose.

chappjc · 2020-05-27T14:38:39Z

client/core/core.go

+			FeePending:    dc.acct.feePending(),
+			Connected:     dc.connected,
+			ConfsRequired: uint32(dc.cfg.RegFeeConfirms),
+			RegConfirms:   dc.regConfirms,


refreshUser=>Exchanges reading dc.regConfirms can happen in many places, but regConfirms is not guarded (connMtx is for Core.conns, not for these fields of dexConnection).

chappjc · 2020-05-27T14:39:25Z

client/core/core.go

+		return
+	}
+
+	dc.regConfirms = &regConfs


Possible concurrent write if refreshUser or Exchanges is called elsewhere.

chappjc · 2020-05-27T14:39:41Z

client/core/core.go

 			} else {
 				details := fmt.Sprintf("You may now trade at %s", dc.acct.url)
-				c.notify(newFeePaymentNote("Account registered", details, db.Success))
+				dc.regConfirms = nil


Possible concurrent write.

chappjc · 2020-05-27T14:43:44Z

client/core/types.go

+	FeePending    bool                  `json:"feePending"`
+	Connected     bool                  `json:"connected"`
+	ConfsRequired uint32                `json:"confsrequired"`
+	RegConfirms   *uint32               `json:"confs,omitempty"`


Only needs to be a pointer in Exchange for purposes of omitempty, but it's a bit awkward as a pointer in dexConnection. The alternative would be to have -1 or 0 be special values in dexConnection.regConfirms that indicate to write nil to Exchange.RegConfirms in (*Core).Exchanges.

Track registration status in the application level rename 'url' -> 'dexAddr' Make sure to pass the exchange host in the fee payment note

… a few more tweaks to prevent conflicts while controlling the order form visibility state

…and access methods

fernandoabolafio

@chappjc thanks for the review. I addressed your comments. Explanation of the changes below:

fernandoabolafio · 2020-06-01T10:04:11Z

client/core/core.go

+	regConfMtx  sync.RWMutex
+	regConfirms uint32


Added a sync.RWMutex to be used when writing/reading regConfirms

fernandoabolafio · 2020-06-01T10:08:00Z

client/core/core.go

+// getRegConfirms returns the number of confirmations received for the
+// dex registration or nil if the registration is completed
+func (dc *dexConnection) getRegConfirms() *uint32 {
+	dc.regConfMtx.RLock()
+	defer dc.regConfMtx.RUnlock()
+	if dc.regConfirms == regConfirmationsPaid {
+		return nil
+	}
+	return &dc.regConfirms
+}


The getRegConfirms returns an uint32 pointer that can be used in the core.Exchanges. It will return the number of confirmations or nil if the registration has been completed.
Notice that different from your suggestion I used a high number called regConfirmationsPaid to indicate that the confirmation is completed.
I could not use 0 because it just means that the registration is ongoing and no confirmation has been received so far. Using -1 requires converting the dexConnection.regConfirms to int32 which hurts the semantic of the code and would also for me to convert the Exchange field to int at some point.

fernandoabolafio · 2020-06-01T10:08:34Z

client/core/core.go

+
+// setRegConfirms sets the number of confirmations received
+// for the dex registration
+func (dc *dexConnection) setRegConfirms(confs uint32) {


Simple method to update regConfirms which deals with the synchronization internally.

fernandoabolafio · 2020-06-01T10:09:21Z

client/core/core.go

@@ -36,6 +36,9 @@ const (
 	keyParamsKey      = "keyParams"
 	conversionFactor  = 1e8
 	regFeeAssetSymbol = "dcr" // Hard-coded to Decred for registration fees, for now.
+	// high number to be assigned to 'regConfirms' in 'dexConnection'
+	// when the registration is completed
+	regConfirmationsPaid uint32 = 9999


The 'PNOOMA' number used to represent that the confirmation is completed.

OK, although note this is actually realistic for many ccs. e.g. ETH: https://blog.coinbase.com/announcing-new-confirmation-requirements-4a5504ba8d81

chappjc · 2020-06-01T13:56:50Z

client/core/core.go

@@ -497,7 +520,7 @@ func (c *Core) Exchanges() map[string]*Exchange {
 			FeePending:    dc.acct.feePending(),
 			Connected:     dc.connected,
 			ConfsRequired: uint32(dc.cfg.RegFeeConfirms),
-			RegConfirms:   dc.regConfirms,
+			RegConfirms:   dc.getRegConfirms(),


When code dereferences this Exchange.RegConfirms pointer, it's accessing dexConnection.regConfirms becausegetRegConfirms does return &dc.regConfirms. So this is basically sidestepping the locking as when another goroutine calls dc.setRegConfirms it is not locked anymore.

You need to either have getRegConfirms return a plain uint32 or make a copy inside the function and return a pointer to the copy (confs := dc.regConfirms; return &confs).

Makes sense! I changed it to return the copy instead.

The field was changed to a reference in a later commit of decred#353, but that meant that it was recognized as waiting for confirmations on startup, since it would be zero. Pointer works better in this case.

The field was changed to a reference in a later commit of #353, but that meant that it was recognized as waiting for confirmations on startup, since it would be zero. Pointer works better in this case.

buck54321 requested changes May 11, 2020

View reviewed changes

fernandoabolafio force-pushed the 337_hide-order-forms branch from 2c7ef0f to e1f1dca Compare May 14, 2020 17:06

buck54321 requested changes May 15, 2020

View reviewed changes

fernandoabolafio commented May 16, 2020

View reviewed changes

client/core/core.go Show resolved Hide resolved

fernandoabolafio requested a review from buck54321 May 18, 2020 19:59

buck54321 requested changes May 18, 2020

View reviewed changes

fernandoabolafio force-pushed the 337_hide-order-forms branch from 0d61544 to cd35880 Compare May 20, 2020 20:41

fernandoabolafio requested a review from buck54321 May 20, 2020 20:48

chappjc reviewed May 21, 2020

View reviewed changes

fernandoabolafio force-pushed the 337_hide-order-forms branch 2 times, most recently from cb5888a to 01525f4 Compare May 23, 2020 19:57

fernandoabolafio commented May 23, 2020

View reviewed changes

fernandoabolafio requested a review from chappjc May 23, 2020 20:04

buck54321 requested changes May 26, 2020

View reviewed changes

fernandoabolafio force-pushed the 337_hide-order-forms branch from 01525f4 to aaa0c7b Compare May 26, 2020 17:22

fernandoabolafio requested a review from buck54321 May 26, 2020 17:27

buck54321 approved these changes May 26, 2020

View reviewed changes

chappjc requested changes May 27, 2020

View reviewed changes

fernandoabolafio added 5 commits June 1, 2020 10:38

Display registration status while waiting for payment confirmations

e715cbe

Track registration status in the application level rename 'url' -> 'dexAddr' Make sure to pass the exchange host in the fee payment note

Create query methods for 'hasFeePending' and 'assetsAreSupported' and…

fed74ce

… a few more tweaks to prevent conflicts while controlling the order form visibility state

fix typo notificaetion -> notification

bbc5768

improve styling

063b8c1

fix unreachable code

abb0b0f

add synx mutex for registration confirms field under 'dexConneciton' …

e21fe29

…and access methods

fernandoabolafio force-pushed the 337_hide-order-forms branch from aaa0c7b to e21fe29 Compare June 1, 2020 10:01

fernandoabolafio commented Jun 1, 2020

View reviewed changes

fernandoabolafio requested a review from chappjc June 1, 2020 10:10

chappjc reviewed Jun 1, 2020

View reviewed changes

fernandoabolafio added 2 commits June 1, 2020 21:27

copy confs value before returning

503ab41

use highest uint32 for regConfirms when registration is completed

1640d7b

chappjc approved these changes Jun 1, 2020

View reviewed changes

chappjc merged commit 7654f9d into decred:master Jun 1, 2020

fernandoabolafio deleted the 337_hide-order-forms branch June 1, 2020 19:59

buck54321 mentioned this pull request Jun 9, 2020

core: fix issue with regConfirms field of dexConnection #458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/core: hide order form and display registration status message #353

client/core: hide order form and display registration status message #353

fernandoabolafio commented May 10, 2020 •

edited

Loading

buck54321 left a comment

chappjc commented May 14, 2020

fernandoabolafio commented May 14, 2020

fernandoabolafio commented May 14, 2020

buck54321 left a comment

fernandoabolafio left a comment

buck54321 left a comment

buck54321 May 18, 2020

fernandoabolafio May 20, 2020

fernandoabolafio commented May 20, 2020

chappjc commented May 21, 2020 •

edited

Loading

chappjc May 21, 2020 •

edited

Loading

chappjc commented May 22, 2020

fernandoabolafio May 23, 2020

buck54321 left a comment

buck54321 May 26, 2020

fernandoabolafio commented May 26, 2020

chappjc left a comment

chappjc May 27, 2020

chappjc May 27, 2020

chappjc May 27, 2020

chappjc May 27, 2020

fernandoabolafio left a comment

fernandoabolafio Jun 1, 2020

fernandoabolafio Jun 1, 2020 •

edited

Loading

fernandoabolafio Jun 1, 2020

fernandoabolafio Jun 1, 2020

chappjc Jun 1, 2020

chappjc Jun 1, 2020 •

edited

Loading

fernandoabolafio Jun 1, 2020

client/core: hide order form and display registration status message #353

client/core: hide order form and display registration status message #353

Conversation

fernandoabolafio commented May 10, 2020 • edited Loading

buck54321 left a comment

Choose a reason for hiding this comment

chappjc commented May 14, 2020

fernandoabolafio commented May 14, 2020

fernandoabolafio commented May 14, 2020

buck54321 left a comment

Choose a reason for hiding this comment

fernandoabolafio left a comment

Choose a reason for hiding this comment

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio commented May 20, 2020

chappjc commented May 21, 2020 • edited Loading

chappjc May 21, 2020 • edited Loading

Choose a reason for hiding this comment

chappjc commented May 22, 2020

Choose a reason for hiding this comment

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio commented May 26, 2020

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

fernandoabolafio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio Jun 1, 2020 • 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

chappjc Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio commented May 10, 2020 •

edited

Loading

chappjc commented May 21, 2020 •

edited

Loading

chappjc May 21, 2020 •

edited

Loading

fernandoabolafio Jun 1, 2020 •

edited

Loading

chappjc Jun 1, 2020 •

edited

Loading