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.

Sign up for GitHub

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

Jump to bottom

Use different security deposit for fiat-crypto and crypto-crypto trades #2742

Merged
merged 2 commits into from
Apr 20, 2019
Merged

Use different security deposit for fiat-crypto and crypto-crypto trades #2742

merged 2 commits into from
Apr 20, 2019

Conversation

ripcurlx
Copy link
Contributor

Sets back security deposit for crypto-crypto trades to values before the DAO and only keeps the new ones for fiat-crypto trades.

@ripcurlx ripcurlx requested a review from ManfredKarrer as a code owner April 18, 2019 14:52
@ripcurlx ripcurlx changed the title Use different security deposit for fiat-crypto and crypto-crypto trades [WIP] Use different security deposit for fiat-crypto and crypto-crypto trades Apr 18, 2019
@ripcurlx
Copy link
Contributor Author

I'll change the condition to differentiate from currency code to payment account so we are more flexible in the future if we ever decide to have different deposit limits for different payment accounts.

@ripcurlx ripcurlx force-pushed the set-lower-security-deposit-for-altcoins branch from ba5804b to 0a2135e Compare April 18, 2019 15:55
@ripcurlx ripcurlx changed the title [WIP] Use different security deposit for fiat-crypto and crypto-crypto trades Use different security deposit for fiat-crypto and crypto-crypto trades Apr 18, 2019
sqrrm
sqrrm reviewed Apr 18, 2019
View reviewed changes
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.

I think this is mostly correct as far as I know, I'm not that familiar with the trade protocol though so I can't tell if there is some case where the deposit should be updated.

Also, see inline comments.

core/src/main/java/bisq/core/btc/wallet/Restrictions.java Outdated Show resolved Hide resolved
core/src/main/java/bisq/core/btc/wallet/Restrictions.java Outdated Show resolved Hide resolved
desktop/src/main/java/bisq/desktop/main/offer/MutableOfferDataModel.java Outdated Show resolved Hide resolved
@ripcurlx ripcurlx force-pushed the set-lower-security-deposit-for-altcoins branch from 0a2135e to 55d09a3 Compare April 18, 2019 17:38
@ripcurlx
Copy link
Contributor Author

I think this is mostly correct as far as I know, I'm not that familiar with the trade protocol though so I can't tell if there is some case where the deposit should be updated.

Also, see inline comments.

Thanks for the review. I force pushed all changes.

sqrrm
sqrrm approved these changes Apr 18, 2019
View reviewed changes
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

As a reviewer I prefer when the previous commits are kept and just add a new one rather than a force push. Easier to follow which changes were made after comments were made that way.

core/src/main/java/bisq/core/user/PreferencesPayload.java
@@ -47,6 +47,8 @@

import javax.annotation.Nullable;

import static bisq.core.btc.wallet.Restrictions.getDefaultBuyerSecurityDepositAsPercent;
Copy link
Member

Choose a reason for hiding this comment

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

More static imports, in case you meant to revert them all, otherwise it's ok, just feels a bit excessive for these kinds of use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I left it by intent to reduce the line length for L125 & L127

desktop/src/main/java/bisq/desktop/util/validation/SecurityDepositValidator.java Outdated
import bisq.core.util.BSFormatter;

import javax.inject.Inject;

import static bisq.core.btc.wallet.Restrictions.getMaxBuyerSecurityDepositAsPercent;
import static bisq.core.btc.wallet.Restrictions.getMinBuyerSecurityDepositAsPercent;
Copy link
Member

Choose a reason for hiding this comment

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

More static imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it doesn't help to make the code more readable. Commit below.

@ripcurlx
Copy link
Contributor Author

utACK

As a reviewer I prefer when the previous commits are kept and just add a new one rather than a force push. Easier to follow which changes were made after comments were made that way.

True - I wanted to reduce the amount of commits, but in a PR that gets reviewed it makes it harder to follow.

sqrrm
sqrrm approved these changes Apr 19, 2019
View reviewed changes
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

ManfredKarrer
ManfredKarrer approved these changes Apr 20, 2019
View reviewed changes
Copy link
Contributor

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

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

utACK

@ManfredKarrer ManfredKarrer merged commit 262fa85 into bisq-network:master Apr 20, 2019
@ripcurlx ripcurlx mentioned this pull request May 3, 2019
Closed
@sqrrm sqrrm mentioned this pull request May 7, 2019
Closed
@ripcurlx ripcurlx deleted the set-lower-security-deposit-for-altcoins branch September 26, 2019 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sqrrm sqrrm sqrrm approved these changes

@ManfredKarrer ManfredKarrer ManfredKarrer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ripcurlx @ManfredKarrer @sqrrm