client: hide order form if asset version is not supported #2054

vctt94 · 2023-01-17T14:37:26Z

closes #2039

martonp · 2023-01-17T20:30:39Z

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

+    if (!base || !quote) return false
+    const { baseCfg: b, quoteCfg: q } = this.market
+    // check if versions are supported
+    return base.info?.versions.includes(b.version) && quote.info?.versions.includes(q.version)


Could you test this with tokens? WalletInfo will not be defined for tokens, so I think this will throw an error.

It will not throw an error because of the optional chaining operator, but it will also not work. Fixed in the next commit, now checking if the parent support its version.

thanks for spotting it.

martonp · 2023-01-17T20:31:46Z

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

-    return b && q
+    const [base, quote] = [this.market.base, this.market.quote]
+    if (!base || !quote) return false
+    const { baseCfg: b, quoteCfg: q } = this.market


I would just leave this as baseCfg and quoteCfg to be able to more clearly differentiate in the check below.

vctt94 · 2023-01-18T13:33:57Z

Thanks for the review marton, addressed your comments.

ukane-philemon · 2023-01-18T14:50:13Z

Thanks @vctt94 for picking this up.

Don't we want display a message to user if we eventually hide the order form due to this new check? I think, setLoaderMsgVisibility() displays a message for unsupported assets but does not consider assets with unsupported version.

vctt94 · 2023-01-18T16:54:07Z

Hey Ukane, my pleasure.

I am not sure I understand what you mean, right now it does show a message if assets aren't supported, which is: {{ asset }} is not supported.

Do you mean it should be a more descreptive message?

ukane-philemon · 2023-01-18T17:20:16Z

Do you mean it should be a more descreptive message?

Yeah.

vctt94 · 2023-01-18T17:32:32Z

Thanks for the suggestion, but I am not sure I agree with you.

Right now the assetsAreSupported() method returns a boolean, which indicates if assets are supported or not. So to be more descriptive about the message we would need to change this method in order to know what is the reason of this asset not being supported so we can show the right message at setLoaderMsgVisibility().

Which I believe {{ asset }} is not supported already covers the point.

chappjc · 2023-01-18T17:37:16Z

Can you share a screenshot of current view when not supported?

vctt94 · 2023-01-18T17:44:03Z

This is the currently view:

chappjc · 2023-01-18T18:37:52Z

So is what's missing the unsupported version?

vctt94 · 2023-01-18T19:07:50Z

Yeah I think so. If everyone agrees I can change assetsAreSupported, to show the unsupported version as well.

vctt94 · 2023-01-18T22:30:29Z

Addressed @ukane-philemon comment on my last commit.

This is how it looks right now:

chappjc · 2023-01-19T14:01:24Z

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

@@ -111,6 +112,7 @@ export const enUS: Locale = {
  [ID_BUY]: 'Buy',
  [ID_SELL]: 'Sell',
  [ID_NOT_SUPPORTED]: '{{ asset }} is not supported',
+  [ID_VERSION_NOT_SUPPORTED]: 'version: {{ version }} of {{ asset }} is not supported',


Let's rephrase this. The "version: " prefix there is a bit odd. How about:

{{ asset }} version {{ version }} is not supported

e.g. "USDC.ETH version 0 is not supported"

or

{{ asset }} (v{{version}}) is not supported

e.g. "USDC.ETH (v0) is not supported"

Any preferences? I'm leaning towards the "v0" way because that can remain consistent across all languages, no need to translate "version" and hope the meaning comes across.

I think the point of the issue this PR is aiming to resolve is that without naming the version it might be confusing if the user can see that the server does indeed have a market for this asset, so we have to state the version so there's some kind of explanation for why it's not supported.

vctt94 · 2023-01-19T14:33:15Z

Addressed the suggested text change, this is how it looks:

martonp · 2023-01-19T19:31:14Z

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

+    if (base.token && quote.token) {
+      const bParent = app().assets[base.token.parentID]
+      const qParent = app().assets[quote.token.parentID]
+      if (!bParent.info?.versions.includes(baseCfg.version)) {


I don't think this is correct, you're checking the parent's version against the token version. The token also has its own version independent of the parent asset, but it doesn't seem to be exposed through the SupportedAsset type. I think the token wallet's version also needs to be exposed through the SupportedAsset type. @buck54321 Am I correct about this?

Sorry for the late response. Yes. We are not properly exposing supported versions for tokens. @vctt94, ~~check out buck54321/dcrdex@97a8cb2...buck54321:dcrdex:supported-versions-addon~~
Edit: scratch that. Gonna re-evaluate

Just opened #2094 too, so if that goes in first, you can just rebase to get the client/asset parts.

I've changed tacts on this. See the updated #2094, but I think we'll have the server maintain a single protocol version and token assets will share with the parent asset.both a wallet version and a set of supported server versions.

The code here is roughly correct, though I'd recommend the following refactor buck54321/dcrdex@97a8cb2...buck54321:dcrdex:parent-vers-pattern
The only difference is that it will show the token's symbol instead of the parent asset, but I think that makes sense anyway.

buck54321 · 2023-01-31T01:06:13Z

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

+    text: string;
+    } {
+    const { market } = this
+    const [base, quote] = [market.base, market.quote]


const { base, baseCfg, quote, quoteCfg } = market

buck54321 · 2023-01-31T12:34:03Z

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

+    const { market } = this
+    const [base, quote] = [market.base, market.quote]
+    if (!base || !quote) {
+      const symbol = market.base ? market.quoteCfg.symbol : market.baseCfg.symbol


const symbol = base ? ...

vctt94 · 2023-02-06T15:57:54Z

I rebased this PR against #2094 and added the suggested reffactor.

chappjc · 2023-02-07T19:49:19Z

#2094 is in, can rebase that commit out of your branch

check if assets are token Add ID_VERSION_NOT_SUPPORTED message on loaderMsg when version not supported lint change text message

vctt94 · 2023-02-08T14:48:16Z

rebased

martonp reviewed Jan 17, 2023

View reviewed changes

vctt94 force-pushed the hide-incompatible-form branch from 0b8240a to 491a509 Compare January 18, 2023 22:31

chappjc reviewed Jan 19, 2023

View reviewed changes

martonp reviewed Jan 19, 2023

View reviewed changes

chappjc added this to the 0.6 milestone Jan 30, 2023

buck54321 reviewed Jan 31, 2023

View reviewed changes

buck54321 mentioned this pull request Jan 31, 2023

client/asset: clarify eth and token versioning #2094

Merged

vctt94 force-pushed the hide-incompatible-form branch from 97a8cb2 to f77e535 Compare February 6, 2023 15:55

vctt94 and others added 2 commits February 8, 2023 11:35

client: hide order form if asset version is not supported

894f8a4

check if assets are token Add ID_VERSION_NOT_SUPPORTED message on loaderMsg when version not supported lint change text message

refactor assetsAreSupported

44bcb8f

vctt94 force-pushed the hide-incompatible-form branch from f77e535 to 44bcb8f Compare February 8, 2023 14:35

buck54321 approved these changes Feb 8, 2023

View reviewed changes

chappjc merged commit adc816d into decred:master Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: hide order form if asset version is not supported #2054

client: hide order form if asset version is not supported #2054

vctt94 commented Jan 17, 2023

martonp Jan 17, 2023

vctt94 Jan 18, 2023 •

edited

martonp Jan 17, 2023

vctt94 commented Jan 18, 2023

ukane-philemon commented Jan 18, 2023

vctt94 commented Jan 18, 2023 •

edited

ukane-philemon commented Jan 18, 2023

vctt94 commented Jan 18, 2023

chappjc commented Jan 18, 2023

vctt94 commented Jan 18, 2023 •

edited

chappjc commented Jan 18, 2023

vctt94 commented Jan 18, 2023

vctt94 commented Jan 18, 2023

chappjc Jan 19, 2023 •

edited

vctt94 commented Jan 19, 2023

martonp Jan 19, 2023

buck54321 Jan 31, 2023 •

edited

buck54321 Jan 31, 2023

buck54321 Feb 3, 2023 •

edited

buck54321 Jan 31, 2023

buck54321 Jan 31, 2023

vctt94 commented Feb 6, 2023

chappjc commented Feb 7, 2023

vctt94 commented Feb 8, 2023

client: hide order form if asset version is not supported #2054

client: hide order form if asset version is not supported #2054

Conversation

vctt94 commented Jan 17, 2023

martonp Jan 17, 2023

Choose a reason for hiding this comment

vctt94 Jan 18, 2023 • edited

Choose a reason for hiding this comment

martonp Jan 17, 2023

Choose a reason for hiding this comment

vctt94 commented Jan 18, 2023

ukane-philemon commented Jan 18, 2023

vctt94 commented Jan 18, 2023 • edited

ukane-philemon commented Jan 18, 2023

vctt94 commented Jan 18, 2023

chappjc commented Jan 18, 2023

vctt94 commented Jan 18, 2023 • edited

chappjc commented Jan 18, 2023

vctt94 commented Jan 18, 2023

vctt94 commented Jan 18, 2023

chappjc Jan 19, 2023 • edited

Choose a reason for hiding this comment

vctt94 commented Jan 19, 2023

martonp Jan 19, 2023

Choose a reason for hiding this comment

buck54321 Jan 31, 2023 • edited

Choose a reason for hiding this comment

buck54321 Jan 31, 2023

Choose a reason for hiding this comment

buck54321 Feb 3, 2023 • edited

Choose a reason for hiding this comment

buck54321 Jan 31, 2023

Choose a reason for hiding this comment

buck54321 Jan 31, 2023

Choose a reason for hiding this comment

vctt94 commented Feb 6, 2023

chappjc commented Feb 7, 2023

vctt94 commented Feb 8, 2023

vctt94 Jan 18, 2023 •

edited

vctt94 commented Jan 18, 2023 •

edited

vctt94 commented Jan 18, 2023 •

edited

chappjc Jan 19, 2023 •

edited

buck54321 Jan 31, 2023 •

edited

buck54321 Feb 3, 2023 •

edited