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

Display minimum limit if used during trade #3870

Merged
merged 5 commits into from
Jan 7, 2020

Conversation

ripcurlx
Copy link
Contributor

@ripcurlx ripcurlx commented Jan 7, 2020

In the past we didn't give the user a hint if the minimum trading fee or minimum deposit was used.
It was shown to the user during offer creation and gave especially new users the impression
that Bisq has very high trading fees and requires a crazy amount for deposit in relation to the trading amount.

Fixes #3868, #3746.

Before
Bildschirmfoto 2020-01-07 um 16 01 33
Bildschirmfoto 2020-01-07 um 17 38 36
Bildschirmfoto 2020-01-07 um 17 38 41

After
Bildschirmfoto 2020-01-07 um 16 30 28
Bildschirmfoto 2020-01-07 um 16 30 33
I've added an additional label when a minimum value (deposit or trading fee) is used.
Bildschirmfoto 2020-01-07 um 17 26 04
Bildschirmfoto 2020-01-07 um 17 27 44
Additionally if the trade amount is so low that the minimum security deposit for the buyer will be used I deactivated the option to set a specific percentage (which just wasn't used in the past in that case) and displayed the BTC value.
Bildschirmfoto 2020-01-07 um 17 26 25
If the amount is high enough so that the minimum amount doesn't need to be used the behavior is as it is right now.

In the past we allowed the user to enter a percentage of the trade amount
although it wasn't used if the minimum security deposit was higher.
@ripcurlx ripcurlx requested a review from sqrrm as a code owner January 7, 2020 16:35
@ripcurlx
Copy link
Contributor Author

ripcurlx commented Jan 7, 2020

@m52go Could you go over my wording?

I'm aware that this is quite a change if we merge it into the v1.2.5 release branch afterwards, but as with #3826 this will occur in more cases especially for new users. So IMO I think we should add this to the v1.2.5 release branch.

@sqrrm @chimp1984 What do you think?

@ripcurlx ripcurlx changed the title Display minimum limit if used during trade [WIP] Display minimum limit if used during trade Jan 7, 2020
@ripcurlx
Copy link
Contributor Author

ripcurlx commented Jan 7, 2020

Just found a small bug - will push the fix soon.

@m52go
Copy link
Contributor

m52go commented Jan 7, 2020

Ideally percentages shouldn't show for minimum values...even with clarification, 6000% security deposit will raise eyebrows.

If it's not too much work to implement, this would be best for such cases:

Your security deposit: 0.0060 BTC (required minimum)

Otherwise I recommend this (existing text is too specific, I think):

guiUtil.minValue=required minimum

So that it reads:

Your security deposit: 0.0060 BTC (6000.00% of trade amount) - required minimum

@ripcurlx
Copy link
Contributor Author

ripcurlx commented Jan 7, 2020

Ideally percentages shouldn't show for minimum values...even with clarification, 6000% security deposit will raise eyebrows.

If it's not too much work to implement, this would be best for such cases:

Your security deposit: 0.0060 BTC (required minimum)

Otherwise I recommend this (existing text is too specific, I think):

guiUtil.minValue=required minimum

So that it reads:

Your security deposit: 0.0060 BTC (6000.00% of trade amount) - required minimum

Good point - I'll adapt the code.

@sqrrm
Copy link
Member

sqrrm commented Jan 7, 2020

Concept ACK

Not sure about including in 1.2.5

@ripcurlx
Copy link
Contributor Author

ripcurlx commented Jan 7, 2020

@m52go way better now (and less code and translations changes)
Bildschirmfoto 2020-01-07 um 18 19 20
Bildschirmfoto 2020-01-07 um 18 19 26

@ripcurlx ripcurlx changed the title [WIP] Display minimum limit if used during trade Display minimum limit if used during trade Jan 7, 2020
@ripcurlx
Copy link
Contributor Author

ripcurlx commented Jan 7, 2020

Concept ACK

Not sure about including in 1.2.5

Yes, it changes some stuff in the trading process. But as only @beingindot has done some trade process release testing yet, we don't loose lots of testing confidence. Maybe if I and someone else runs the trade process again we should be on the safe side.

Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

@sqrrm sqrrm merged commit 81a14d3 into bisq-network:master Jan 7, 2020
@sqrrm
Copy link
Member

sqrrm commented Jan 7, 2020

I think you're right, let's include this in the release.

@ripcurlx ripcurlx added this to the v1.2.5 milestone Jan 8, 2020
@ripcurlx ripcurlx deleted the show-if-minimum-values-are-used branch July 16, 2021 07:13
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.

Min security deposit should be worded in info popups
3 participants