Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: reputation and limits #2575

Merged
merged 5 commits into from
Nov 17, 2023
Merged

Conversation

buck54321
Copy link
Member

@buck54321 buck54321 commented Oct 23, 2023

Adapt registration forms to bonding. Show trading limits for the tier input value. 
Add bond data to bond options registration form. Reuse registration forms for 
updating bond options on dexsettings. Show tier and reputation on dexsettings. 
Show parcel limits and reputation on markets view.

A couple of notes. Nowhere are we going to show the cost of an individual bond, 
because that value is misleading. Instead, we'll show the "bond lock", which is 
the amount that will actually be locked for the given tier.

I have removed the simnetfiatrates settings. fiat rates are all on by default. I 
have also changed coinpaprika so that it pulls all coin data, instead of just the 
ones with wallets. This enables us to e.g. show USD conversion of the bond 
lock value for assets that we don't yet have a wallet for.

image

image

image

@peterzen
Copy link
Member

Looks really good. The "2.0 parcels..." sentence sounds rather cryptic to the uninitiated though, could this info be rephrased differently?

@buck54321
Copy link
Member Author

Looks really good. The "2.0 parcels..." sentence sounds rather cryptic to the uninitiated though, could this info be rephrased differently?

For sure. Taking suggestions.

@buck54321 buck54321 force-pushed the bonding-order-limit-ui branch from 3bdc6a3 to d1658a6 Compare October 23, 2023 21:15
@martonp
Copy link
Collaborator

martonp commented Oct 26, 2023

Some notes, I haven't reviewed yet, just tested:

Markets page -

  • When not registered it shows current usage as NaN%. I think the Bond Info section should be hidden when the user does not have an account.
  • Over here the math is off.. I have 4 parcels trading limit, and each parcel is 4 lots, but it shows 1 lot is 0.5 parcels.
Screenshot 2023-10-25 at 11 49 31 PM
  • Instead of saying “x of the x unused parcels” below the order form, It should just say the max lots you are able to currently trade.
  • The parcels section and the limit bonus sections could use tooltips with a bit of explanation.
  • When I only have one of the wallets created for a market, the bond info section is showing but the wallets section (where it would allow me to create the wallet) is missing.

Register Page -

  • This page could use a short introduction to fidelity bonds at the top instead of just a disclaimer at the bottom. Currently, coming to this screen will be confusing for newcomers. It should tell the user that in order to trade they will have to lock up funds for a certain duration (and what that duration is). The more they lock, the higher their trading limit will be. And then link to a wiki page for all the details. I think this is super important. A lot of newcomers may get to this point and then quit because they don't understand what they are about to do.
  • The screen that pops up if you have no funds in your wallet still shows the individual bond cost:
Screenshot 2023-10-25 at 11 04 25 PM
  • There should be a checkbox here for auto rotation of bonds.
  • I think there should be a range selector instead of a number input. Especially if we end up having a limit on max tier.

Settings Page -

  • Should also show the trading limits like on the register page.

@buck54321
Copy link
Member Author

Over here the math is off.. I have 4 parcels trading limit, and each parcel is 4 lots, but it shows 1 lot is 0.5 parcels.

There's a factor of 2 applied.

PerTierBaseParcelLimit = 2

That factor is necessary so that an operator cannot accidentally make it impossible to block all likely taker orders, which have their weight doubled.

Instead of saying “x of the x unused parcels” below the order form, It should just say the max lots you are able to currently trade.

Good call. I do want to show some indicator of their current usage, but yeah, that sentence is crap and just showing lots and quantity is better.

When I only have one of the wallets created for a market, the bond info section is showing but the wallets section (where it would allow me to create the wallet) is missing.

Looks like that's fixed in #2577, which I need to review now.

There should be a checkbox here for auto rotation of bonds.

I don't really agree on this. I think auto-rotation should always be enabled for GUI users, or possible disabled in settings only. What I do want to do though is to not allow renewal of bonds if all of the current bonds are expired on startup, indicating the user hasn't used dexc in a while.

@buck54321
Copy link
Member Author

I think there should be a range selector instead of a number input. Especially if we end up having a limit on max tier.

I don't plan to limit trading tier, just the number of penalties that can be offset.

@buck54321
Copy link
Member Author

Settings Page -

  • Should also show the trading limits like on the register page.

For all markets? That seems like a lot.

@buck54321 buck54321 force-pushed the bonding-order-limit-ui branch from d1658a6 to ebe7a2a Compare November 1, 2023 01:14
@martonp
Copy link
Collaborator

martonp commented Nov 1, 2023

I don't really agree on this. I think auto-rotation should always be enabled for GUI users, or possible disabled in settings only. What I do want to do though is to not allow renewal of bonds if all of the current bonds are expired on startup, indicating the user hasn't used dexc in a while.

I don't think it's really possible to know that auto-rotation will be happening. Maybe put a checkbox that is checked by default? I'm thinking that if someone tries dexc and if they don't like it, they would want to get their funds back. When they open dexc 30 days later and their bonds automatically get reposted, they will be pissed.

For all markets? That seems like a lot.

I feel like you'd want to see some information to determine how many bonds you want to post.

@buck54321 buck54321 force-pushed the bonding-order-limit-ui branch from ebe7a2a to ca9f40c Compare November 8, 2023 19:40
@buck54321
Copy link
Member Author

buck54321 commented Nov 8, 2023

I don't really agree on this. I think auto-rotation should always be enabled for GUI users, or possible disabled in settings only. What I do want to do though is to not allow renewal of bonds if all of the current bonds are expired on startup, indicating the user hasn't used dexc in a while.

I don't think it's really possible to know that auto-rotation will be happening. Maybe put a checkbox that is checked by default? I'm thinking that if someone tries dexc and if they don't like it, they would want to get their funds back. When they open dexc 30 days later and their bonds automatically get reposted, they will be pissed.

I've added a ton more info in the new "What's a bond" popup, including info about auto-renewal and how to disable. I've then added a "Auto Renew" toggle on the settings page (which either just sets the tier to 0 or shows the tier form). I agree about auto-renewing when they haven't used dexc in a while. As I mentioned, we should disable auto-renew if all bonds are already expired on startup, but it will involve a fair amount of work, because we need to catch that state, prevent auto-renew in core, deliver an indicator through the API, and show new dialogs. I'd like to save that work for a separate effort, if that's OK. Will open an issue.

For all markets? That seems like a lot.

I feel like you'd want to see some information to determine how many bonds you want to post.

That's why I show the info on the tier form though. So they know how many to post. I'm not opposed to showing the limits for all markets somewhere else, but I don't like the idea of stuffing it all on the dexsettings page shown by default, and thinking of how users might actually care about the info, it's most likely that they've already seen their limits from the markets view and want to increase their limits, so the tier form is already the right place.

@buck54321
Copy link
Member Author

image

image

@martonp
Copy link
Collaborator

martonp commented Nov 9, 2023

That's why I show the info on the tier form though. So they know how many to post. I'm not opposed to showing the limits for all markets somewhere else, but I don't like the idea of stuffing it all on the dexsettings page shown by default, and thinking of how users might actually care about the info, it's most likely that they've already seen their limits from the markets view and want to increase their limits, so the tier form is already the right place.

I just feel like if I'm initially registering, I currently have no idea how much my exact trading limit will be. The range I see on the bonds page is confusing. There should be a link called Limit Details that shows exactly how much my initial limit will be on each market, and then the max it can grow to with good behavior.

Screenshot 2023-11-08 at 10 13 20 PM

I think this screens should also show the limits. If my limit is currently 80 DCR and I want to increase it to 200 DCR, I would come to this screen and have no idea what tier I should pick.

Also, I noticed on the markets page, the number should be truncated:

Screenshot 2023-11-08 at 10 10 40 PM

I think overall the whole system has many concepts and will be very confusing for newcomers. The parcel concept is not explained anywhere, and it just all feels a little intimidating the way it is currently. I'll try to think of some ideas to make it more simple .

"1 Sync the Blockchain": "1: Sync the Blockchain",
"Progress": "Progress",
"remaining": "remaining",
"2 Fund your Wallet": "2: Fund your Wallet",
"whatsabond": "Fidelity bonds are time-locked funds redeemable only by you, but in the future. This is meant to combat disruptive behavior like backing out on swaps.",
"bond_definition": "A fidelity bond is funds temporarily locked in an on-chain contract. After the contract expires, your wallet will reclaim the funds. On mainnet, funds are locked for up to 2 months.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reading this I would ask, "can I lock them for less than 2 months, and how?"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove the "up to" part?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, unless we add a setting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a setting to increase the bond lock, but not to decrease. Will change the messaging for now.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple things while lightly testing.

Is the lower bound here with no good behavior and the upper with very good behavior?
image

Unsure if related to these changes but something is weird with wallets you do not currently have created. Here I made a simnet dex with dcr, btc, and eth, and did the normal ui client creation. However when looking at the markets, it used to show that I could create the eth wallet here but now it either shows nothing or the wrong wallet. This is the btc/eth market but look at the wallets. After adding an eth wallet it looked fine, but the position of the market changed on the left which was slightly unexpected:

image

Comment on lines +137 to +139
// function roundParcels (p: number): number {
// return Math.floor(Math.round((p * 1e8)) / 1e8)
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused.

this.dexAddrForm = new forms.DEXAddressForm(page.dexAddrForm, async (xc: Exchange) => {
window.location.assign(`/dexsettings/${xc.host}`)
}, undefined, this.host)

forms.bind(page.bondDetailsForm, page.updateBondOptionsConfirm, () => this.updateBondOptions())
// forms.bind(page.bondDetailsForm, page.updateBondOptionsConfirm, () => this.updateBondOptions())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?


tmpl.tradeLimitSymbol.textContent = unitInfo.conventional.unit
tmpl.name.textContent = asset.name
// assetTmpl.confs.textContent = String(bondAsset.confs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?

@buck54321
Copy link
Member Author

Unsure if related to these changes but something is weird with wallets you do not currently have created. Here I made a simnet dex with dcr, btc, and eth, and did the normal ui client creation. However when looking at the markets, it used to show that I could create the eth wallet here but now it either shows nothing or the wrong wallet.

@JoeGruffins I'm a little confused. You claim that you made a simnet dex with dcr, btc, and eth, but then later expect that it used to show that I could create the eth wallet here. Did you create the Ethereum wallet before seeing this bug or not?

@JoeGruffins
Copy link
Member

I'm a little confused. You claim that you made a simnet dex with dcr, btc, and eth, but then later expect that it used to show that I could create the eth wallet here. Did you create the Ethereum wallet before seeing this bug or not?

Srry, by used to I mean in previous commits. This was a new client with no eth wallet yet. Let me try on master and then on this again to make sure.

@JoeGruffins
Copy link
Member

JoeGruffins commented Nov 16, 2023

I can confirm master is fine and switching to this commit the "Add a ETH wallet" pane is gone. For comparison master looks like:
image

@buck54321
Copy link
Member Author

I can confirm master is fine and switching to this commit the "Add a ETH wallet" pane is gone. For comparison master looks like: image

Oh. It's probably because this branch doesn't include #2577 yes. I need to rebase.

Adapt registration forms to bonding. Show trading limits for the
tier input value. Add bond data to bond options registration form.
Reuse registration forms for updating bond options on dexsettings.
Show tier and reputation on dexsettings. Show parcel limits and
reputation on markets view.

A couple of notes. Nowhere are we going to show the cost of an
individual bond, because that value is misleading. Instead, we'll
show the "bond lock", which is the amount that will actually be
locked for the given tier.

I have removed the simnetfiatrates settings. fiat rates are all
on by default. I have also changed coinpaprika so that it pulls
all coin data, instead of just the ones with wallets. This enables
us to e.g. show USD conversion of the bond lock value for assets
that we don't yet have a wallet for.
@buck54321 buck54321 force-pushed the bonding-order-limit-ui branch from ca9f40c to bcd91f6 Compare November 17, 2023 01:54
@buck54321 buck54321 merged commit b6c4c3e into decred:master Nov 17, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants