re-work register flow to accept new assets fee #1223

vctt94 · 2021-09-28T18:52:54Z

This PR is the frontend part of (#1202)

chappjc

Thanks for taking this on @vctt94!

Here's a quick first pass with some change I think can be made, but I've not actually tested it yet.
Overall, looking nice.

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

client/webserver/locales/en-us.go

client/webserver/http.go

client/webserver/locales/en-us.go

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

chappjc · 2021-09-29T16:44:04Z

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

+      const setupWallet = tr.querySelector('#setupWallet')
+      if (haveWallet) {
+        setupWallet.innerText = intl.prep(intl.ID_CHOOSE_WALLET)
+        Doc.bind(setupWallet, 'click', () => this.setWalletFee(fee.id))


I think this action should also update something to indicate the selection was made and that clicking "Register" will in fact use the intended asset and amount.

For this I had envisioned either or both of: (1) modify the table row's style or just the clicked this button to show it is selected (sorry if this is already done, I haven't tested), which would obviously require undoing this styling change if the user clicks another row, or (2) Update a new sentence just above the Password field to say something like "This will pay a fee of [X amount] [Y asset symbol]."

Also, the styling of the button would ideally be different (perhaps color) between Choose and Setup

I'm pretty happy now with how solution (1) is implemented. With a boundary around the selected asset's row, it's quite clear what is going to happen when you submit the form.

Maybe another reviewer will think the text proposed in (2) is also valuable, but I can live without it.

vctt94 · 2021-09-29T19:50:04Z

Thanks for your review chapp.

I changed the table row now for being clickable instead of the button, and made them with low oppacity, so when they are selected they fade in.

chappjc

I like the direction this is going. I think it could look more like this:

Selected:

Note also the text edits.

client/webserver/locales/en-us.go

chappjc · 2021-09-29T22:41:46Z

client/webserver/locales/en-us.go

@@ -131,8 +131,7 @@ var EnUS = map[string]string{
 	"Set App Password":          "Set App Password",
 	"reg_set_app_pw_msg":        "Set your app password. This password will protect your DEX account keys and connected wallets.",
 	"Password Again":            "Password Again",
-	"reg_dcr_required":          "Your Decred wallet is required to pay registration fees.", // TODO
-	"reg_dcr_unlocked":          "Unlock your Decred wallet to pay registration fees.",      // TODO
+	"reg_dcr_required":          "A wallet is required to pay registration fees. You can set one with this form.",


The key name and text I think should change.

Suggested change

"reg_dcr_required": "A wallet is required to pay registration fees. You can set one with this form.",

"reg_wallet_required": "A wallet is required to pay registration fees.",

Although I'm thinking this text is not even required on the form at all since the user just got here from the register form by clicking the Setup button, so they know why they are on this form. Before this PR the form came up on its own and it wasn't obvious why, but that's changed.

Since we went with getting rid of this as an argument to the "newWalletForm" template, it's the right time to clean up that template definition because it no longer gets an input from any callers and the template only has one possible rendering now.

That is, when called like {{template "newWalletForm" "reg reg reg"}}, we see:

When called like {{template "newWalletForm"}}, as it is now:

The two differences are obviously this text at the top, but also the text about the wallet password goes into a tooltip:

So in forms.html where we have {{define "newWalletForm"}}, there are a bunch of ifs using . that will never be true anymore since only this register form was ever executing this template with an arg.

Regarding the wallet password texts, we could just go with the existing w_password_tooltip, or combine it with the text from w_password_helper. i.e. "w_password_tooltip": "This is the password you have configured with your wallet software. Leave the password empty if there is no password required for the wallet.",

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

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

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

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

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

client/webserver/site/src/js/register.js

chappjc · 2021-09-30T19:42:49Z

client/webserver/site/src/js/register.js

@@ -161,7 +140,7 @@ export default class RegistrationPage extends BasePage {
    this.auth()


A couple lines up there is if (seed) this.walletForm.setRegMsg('Your Decred wallet is required').
I'm struggling to recall why restoring from seed means the Decred wallet is required. Does that basically just mean "don't forget your Decred wallet, because you had it setup before"? In which case we could remove that line now.
@buck54321

It would make sense if wallet was required when not restoring from seed, because pay fee.

Well in any case, seems like this conditional message is no longer correct.

buck54321

First pass. Looking pretty good.

When I register from scratch, and choose to "remember password", the DEX address form shows the app password input.

That input used to be shown only when landing on the registration page with the app already initialized and a Decred wallet already connected, and no password cached.

If I select an asset with no wallet, and click the "Register" button, I just get an error that says "no wallet".

Maybe in this case we should just show the wallet setup form for the selected asset, as if they'd clicked the "Setup" button.

buck54321 · 2021-10-04T23:15:40Z

client/webserver/locales/en-us.go

@@ -27,8 +27,9 @@ var EnUS = map[string]string{
 	"Submit":                         "Submit",
 	"Confirm Registration":           "Confirm Registration",
 	"app_pw_reg":                     "Enter your app password to confirm DEX registration.",
-	"reg_confirm_submit":             `When you submit this form, <span id="feeDisplay"></span> DCR will be spent from your Decred wallet to pay registration fees.`, // TODO for multi-asset reg
+	"reg_confirm_submit":             `When you submit this form, it will spend from your chosen wallet to pay the registration fee.`,


it will spend from

How about , funds will be spent from ...

buck54321 · 2021-10-05T00:29:23Z

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

+  /*
+  * assetById returns the asset string by its id.
+  */
+  static assetById (assetId) {


How about assetSymbol(assetID)? But probably don't need this method anyway.

buck54321 · 2021-10-05T00:33:16Z

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

+    const feeAsset = Object.values(xc.regFees)
+    feeAsset.forEach((fee) => {


for (const [symbol, fee] of Object.entries(xc.regFees)) { ... Doc.tmplElement(tr, 'asseticon').src = Doc.logoPath(symbol) Doc.tmplElement(tr, 'asset').innerText = symbol.toUpperCase()

And then you don't need the assetById method.

buck54321 · 2021-10-05T00:35:38Z

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

+            <button id="setupWallet" type="button" class="justify-content-center w-100 selected"></button>
+            <span id="walletReady"></span>


Once there are two rows, these elements are no longer unique. Should not use the id attribute here. Use data-tmpl like the rest.

buck54321 · 2021-10-05T00:42:39Z

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

+      const setupWallet = tr.querySelector('#setupWallet')
+      const walletReady = tr.querySelector('#walletReady')


Use data-tmpl attributes instead of id.

buck54321 · 2021-10-05T00:48:43Z

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

@@ -60,8 +67,9 @@ export class NewWalletForm {
    else Doc.show(this.fields.newWalletAppPWBox)
  }

-  async setAsset (asset) {
+  async setAsset (assetId) {


Please use assetID instead of assetId. We're not perfectly consistent, but we try to use either id or ID, not Id. You'll find many instances of marketID and assetID around.

buck54321 · 2021-10-05T00:54:28Z

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

+      Doc.tmplElement(tr, 'confs').innerText = fee.confs
+      Doc.tmplElement(tr, 'fee').innerText = fee.amount / 1e8
+
+      const haveWallet = app.user.assets[fee.id] ? app.user.assets[fee.id].wallet : null


I would think you'd want to check that we support the asset earlier, and bail on this row if we don't. Also, the app.user.assets -> SupportedAsset gives you the symbol, which is another reason we don't need Doc.assetById.

buck54321 · 2021-10-05T00:54:32Z

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

+        })
+        tr.classList.add('selected')
+      })
+      const asset = Doc.assetById(fee.id)


As mentioned, you don't need this method to get the asset ID.

buck54321 · 2021-10-05T00:54:47Z

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

    const fields = this.fields
+    const symbol = Doc.assetById(assetId)
+    if (!assetId || !symbol) {


Can just do if (assetID === null)

chappjc · 2021-10-05T01:41:56Z

Maybe in this case we should just show the wallet setup form for the selected asset, as if they'd clicked the "Setup" button

Or even gray out the Register button if a row is highlighted that is not a set up wallet? Otherwise then you have to handle the completion of wallet setup differently depending on how you got to there (from the Setup button or from the Register button).

vctt94 · 2021-10-05T14:52:51Z

I went with graying the button cause I think it was a simpler solution

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

vctt94 · 2021-10-05T15:56:23Z

whoops. mb

chappjc · 2021-10-05T16:47:22Z

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

+      fields.feeTableRows.removeChild(fields.feeTableRows.firstChild)
+    }
+    for (const [symbol, fee] of Object.entries(xc.regFees)) {
+      const haveWallet = app.user.assets[fee.id].wallet


This is not easy to test, but if the user's dexc does not support an asset that the server does and does for fees, this will be an error of the sort "can't access property "wallet", ...user.assets[l.id] is undefined".

If app.user.assets[fee.id] is undefined, this loop could either: (1) continue to next entry in regFee and not show the row, or (2) show the row somehow with "N/A" and "unsupported" in appropriate places. For this iteration, let's go with (1).

chappjc

OK LGTM now. Will merge when @buck54321 circles back and it's ok.

buck54321

When trying to create a wallet from the markets view, I'm getting some errors

Same thing from the wallets view.

buck54321 · 2021-10-06T23:04:28Z

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

+      const haveWallet = app.user.assets[fee.id].wallet
+      const tr = fields.feeRowTemplate.cloneNode(true)
+      Doc.bind(tr, 'click', () => {
+        this.setWalletFee(fee.id)


See other comment about setFeeWallet, but here you can just do this.feeWallet = assetID, or this.feeAssetID = assetID if you go with my naming suggestion.

buck54321 · 2021-10-06T23:05:48Z

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

@@ -350,12 +414,22 @@ export class ConfirmRegistrationForm {
  /*
   * submitForm is called when the form is submitted.
   */
-  async submitForm () {
+  async submitForm (assetID) {


Don't need to pass in the assetID. Get it from this.feeWallet.

buck54321 · 2021-10-06T23:08:02Z

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

+  setWalletFee (assetID) {
+    this.feeWallet = assetID
  }


Lots going on with the naming here. Is it a fee or a wallet or an asset ID? How about renaming feeWallet to feeAssetID or similar, and getting rid of setWalletFee and just set the field directly.

buck54321 · 2021-10-06T23:19:17Z

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

    while (fields.marketsTableRows.firstChild) {
      fields.marketsTableRows.removeChild(fields.marketsTableRows.firstChild)
    }
+    while (fields.feeTableRows.firstChild) {
+      fields.feeTableRows.removeChild(fields.feeTableRows.firstChild)
+    }


Can you change both of these to use Doc.empty?

buck54321 · 2021-10-06T23:19:59Z

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

+      if (app.user.assets[fee.id] === undefined) {
+        continue
+      }


One line is good

if (app.user.assets[fee.id] === undefined) continue

buck54321 · 2021-10-06T23:20:54Z

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

+        rows.forEach(row => {
+          row.classList.remove('selected')
+        })


One line is good

rows.forEach(row => { row.classList.remove('selected') })

buck54321 · 2021-10-06T23:47:56Z

client/webserver/site/src/js/register.js

    this.walletForm.refresh()
-    await this.changeForm(page.appPWForm, page.newWalletForm)
+    await this.changeForm(page.appPWForm, page.dexAddrForm)


I think you need a this.dexAddrForm.refresh() here too. If I choose the "remember password" option, the password field is still shown on the dex address form.

chappjc · 2021-10-08T18:15:09Z

I was able to add a DCR wallet from the /markets page after registering with BTC

chappjc added this to the 0.3 milestone Sep 28, 2021

vctt94 changed the title ~~[wip] re-work register flow to accept new assets fee~~ re-work register flow to accept new assets fee Sep 29, 2021

vctt94 marked this pull request as ready for review September 29, 2021 15:30

vctt94 force-pushed the multi-fee-payment-front branch from 064efd1 to e0254f9 Compare September 29, 2021 15:52

chappjc requested changes Sep 29, 2021

View reviewed changes

chappjc linked an issue Sep 29, 2021 that may be closed by this pull request

ui: update for different reg fee assets #1220

Closed

chappjc requested changes Sep 29, 2021

View reviewed changes

chappjc reviewed Sep 30, 2021

View reviewed changes

client/webserver/site/src/js/register.js Outdated Show resolved Hide resolved

chappjc reviewed Sep 30, 2021

View reviewed changes

buck54321 requested changes Oct 5, 2021

View reviewed changes

chappjc reviewed Oct 5, 2021

View reviewed changes

client/webserver/site/src/html/forms.tmpl Show resolved Hide resolved

vctt94 added 3 commits October 5, 2021 12:31

Rework register flow to accept new assets fee

3b84f29

review fixes

674f16f

more review fixes

3a59393

vctt94 force-pushed the multi-fee-payment-front branch 2 times, most recently from 33707c7 to 48421b7 Compare October 5, 2021 15:55

buck's review and chapp's comments

4db5fbf

vctt94 force-pushed the multi-fee-payment-front branch from 48421b7 to 4db5fbf Compare October 5, 2021 16:05

chappjc reviewed Oct 5, 2021

View reviewed changes

skip case asset is not supported

d4cb803

chappjc approved these changes Oct 5, 2021

View reviewed changes

chappjc requested a review from buck54321 October 6, 2021 17:14

buck54321 requested changes Oct 6, 2021

View reviewed changes

buck's review

a003ee0

buck54321 approved these changes Oct 9, 2021

View reviewed changes

chappjc merged commit 0d4075a into decred:master Oct 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

re-work register flow to accept new assets fee #1223

re-work register flow to accept new assets fee #1223

vctt94 commented Sep 28, 2021

chappjc left a comment

chappjc Sep 29, 2021 •

edited

Loading

chappjc Sep 30, 2021

vctt94 commented Sep 29, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading

chappjc Sep 29, 2021 •

edited

Loading

chappjc Sep 30, 2021

chappjc Sep 30, 2021 •

edited

Loading

JoeGruffins Oct 4, 2021

chappjc Oct 4, 2021

buck54321 left a comment

buck54321 Oct 4, 2021

buck54321 Oct 5, 2021

buck54321 Oct 5, 2021

buck54321 Oct 5, 2021

buck54321 Oct 5, 2021

buck54321 Oct 5, 2021

buck54321 Oct 5, 2021

buck54321 Oct 5, 2021

buck54321 Oct 5, 2021

chappjc commented Oct 5, 2021 •

edited

Loading

vctt94 commented Oct 5, 2021

vctt94 commented Oct 5, 2021

chappjc Oct 5, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading

buck54321 left a comment

buck54321 Oct 6, 2021

buck54321 Oct 6, 2021

buck54321 Oct 6, 2021

buck54321 Oct 6, 2021

buck54321 Oct 6, 2021

buck54321 Oct 6, 2021

buck54321 Oct 6, 2021

chappjc commented Oct 8, 2021

	"reg_dcr_required": "A wallet is required to pay registration fees. You can set one with this form.",
	"reg_wallet_required": "A wallet is required to pay registration fees.",

		@@ -161,7 +140,7 @@ export default class RegistrationPage extends BasePage {
		this.auth()

		const feeAsset = Object.values(xc.regFees)
		feeAsset.forEach((fee) => {

		<button id="setupWallet" type="button" class="justify-content-center w-100 selected"></button>
		<span id="walletReady"></span>

		const setupWallet = tr.querySelector('#setupWallet')
		const walletReady = tr.querySelector('#walletReady')

re-work register flow to accept new assets fee #1223

re-work register flow to accept new assets fee #1223

Conversation

vctt94 commented Sep 28, 2021

chappjc left a comment

Choose a reason for hiding this comment

chappjc Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vctt94 commented Sep 29, 2021 • edited Loading

chappjc left a comment • edited Loading

Choose a reason for hiding this comment

chappjc Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

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 commented Oct 5, 2021 • edited Loading

vctt94 commented Oct 5, 2021

vctt94 commented Oct 5, 2021

chappjc Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

chappjc left a comment • edited Loading

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

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 Oct 8, 2021

chappjc Sep 29, 2021 •

edited

Loading

vctt94 commented Sep 29, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading

chappjc Sep 29, 2021 •

edited

Loading

chappjc Sep 30, 2021 •

edited

Loading

chappjc commented Oct 5, 2021 •

edited

Loading

chappjc Oct 5, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading