-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Increase trade limit. Make trade limit a DAO parameter #2413
Merged
ripcurlx
merged 5 commits into
bisq-network:master
from
ManfredKarrer:increase-trade-limits
Feb 15, 2019
Merged
Increase trade limit. Make trade limit a DAO parameter #2413
ripcurlx
merged 5 commits into
bisq-network:master
from
ManfredKarrer:increase-trade-limits
Feb 15, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Set max trade limit to 2 BTC (for altcoins) - Add MAX_TRADE_LIMIT to Param - Round first month trade limit to ensure we stick with precision 4 for the btc amount - Use risk factors to derive trade limits for different payment method risk categories
Starting with an existing client throws following error in the background:
|
ripcurlx
suggested changes
Feb 13, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK - the initial loading of existing payment methods without the trade limits property causes an error.
- Assign paymentMethods in static final field - Add static fields for default trade limits - Remove deprecated payment methods - Remove onAllServicesInitialized and use static initializer instead - Re-purpose maxTradeLimit for indicating risk factor - Calculate real trade limit in the getMaxTradeLimitAsCoin method - Rename getActivePaymentMethods to getPaymentMethods
- We had an automate remove accounts for those payment methods for long time, so we can assume that no traders have any of those accounts still in their persisted user objects and it is safe to completely remove them. Only part where we cannot remove it is the PB definitions (actually I think we could remove those as well, but not 100% sure and it seems to be more safe to mark those as deprecated and leave the entries).
ripcurlx
reviewed
Feb 14, 2019
- Make all PaymentMethod constructors private - Use PaymentMethod.getPaymentMethodById in Offer for getting the PaymentMethod. This change a bit the context as now we always create the PaymentMethod from the actual code base in contrast to the data which have been used when creating the offer. As our fields as final and must not change in software updates it should have no issues but we have to keep that in mind to not alter the default values. - Added a runtimeException in case the maxTradeLimit does not match one of our default values. - Use PaymentMethod.getDummyPaymentMethod(GUIUtil.SHOW_ALL_FLAG) instead of new PaymentMethod(GUIUtil.SHOW_ALL_FLAG))
ripcurlx
approved these changes
Feb 15, 2019
ACK - works now for me 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
the btc amount
risk categories
Implements bisq-network/proposals#71