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

fix: Calculate max quantity for resizing #2439

Merged
merged 7 commits into from
Apr 18, 2024
Merged

Conversation

holzeis
Copy link
Contributor

@holzeis holzeis commented Apr 17, 2024

Calculates the max quantity considering

  • open quantity: Quantity of an existing position of the opposite direction. (needed for fee calculation)
  • accumulated order matching fee: As the coordinator is not using already collected order matching fees as collateral for new trades, this has to be deducted from the max coordinator margin.

fixes #2391

@holzeis holzeis requested review from bonomat and luckysori April 17, 2024 09:26
@holzeis holzeis self-assigned this Apr 17, 2024
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

LGTM, particularly given the demo, but I don't want to approve because my review wasn't very thorough.

Comment on lines +9 to +11
-- Before this migration, the taker fee was hard-coded to 0.30%.
UPDATE positions
SET order_matching_fees = quantity * (1 / average_entry_price) * 0.0030 where average_entry_price!=0;
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 This assumes that the position hasn't gone through any resizes before the migration, which is not necessarily true after 2.2.0 was released.

I think we can live with this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, when I've written it 2.2.0 wasn't released yet. 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, any trade between Friday, 12.04 and now has no order matching fee

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's too critical as it will only move the max quantity lower. Do the user might not be able to trade everything, but he won't run into errors.

mobile/native/src/api.rs Outdated Show resolved Hide resolved
mobile/lib/features/trade/domain/trade_values.dart Outdated Show resolved Hide resolved
mobile/native/src/max_quantity.rs Show resolved Hide resolved
mobile/native/src/max_quantity.rs Outdated Show resolved Hide resolved
mobile/native/src/max_quantity.rs Outdated Show resolved Hide resolved
mobile/native/src/max_quantity.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

Actually, get the approval now and wait for @bonomat's review or another look from me tomorrow, if you think it's necessary.

@holzeis holzeis force-pushed the fix/max-quantity-with-resizing branch 2 times, most recently from 3e09247 to 2337ae6 Compare April 17, 2024 13:15
Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

I trust that the calculations is correct as I can't say that I follow the max_quantity function anymore :(

Comment on lines +9 to +11
-- Before this migration, the taker fee was hard-coded to 0.30%.
UPDATE positions
SET order_matching_fees = quantity * (1 / average_entry_price) * 0.0030 where average_entry_price!=0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, any trade between Friday, 12.04 and now has no order matching fee

// Note, this only works because it is only called from [`updatePriceAndQuantity`], which is only
// called if the max quantity lock is set.
_recalculateContracts() {
contracts = quantity + openQuantity;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be safer to do this from inside _recalculateQuantity(), or whenever quantity is being updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it wouldn't be safer, as we want to do this only if the max quantity lock is set. otherwise that would be wrong. As mentioned in the comment, this only works because its only called from the updatePriceAndQuantity which is only called if the maxQuantityLock is set.

Copy link
Contributor Author

@holzeis holzeis Apr 18, 2024

Choose a reason for hiding this comment

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

Actually after some testing, it turns out it wasn't working as expected. We achieve the original intention by simply setting the contracts to the maxQuantity after every price update.

// Note, this only works because it is only called from [`updatePriceAndQuantity`], which is only
// called if the max quantity lock is set.
_recalculateContracts() {
contracts = quantity + openQuantity;
Copy link
Contributor

Choose a reason for hiding this comment

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

openQuantity seems to be always zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is set when the trade bottom sheet tab is initialised.

mobile/lib/features/trade/trade_bottom_sheet_tab.dart Outdated Show resolved Hide resolved
mobile/native/src/max_quantity.rs Show resolved Hide resolved
mobile/native/src/max_quantity.rs Outdated Show resolved Hide resolved
mobile/native/src/max_quantity.rs Outdated Show resolved Hide resolved
@@ -166,7 +166,6 @@ class TradeBottomSheetConfirmation extends StatelessWidget {
// Fallback to 0 if we can't get the fee or the margin
Amount total =
tradeValues.margin != null ? Amount(tradeValues.margin!.sats).add(reserve) : Amount(0);
total = total.add(tradeValues.fee ?? Amount.zero());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@holzeis holzeis force-pushed the fix/max-quantity-with-resizing branch from 2337ae6 to d77d4b8 Compare April 18, 2024 12:33
@holzeis holzeis enabled auto-merge April 18, 2024 12:33
@holzeis holzeis added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit 40ef537 Apr 18, 2024
22 checks passed
@holzeis holzeis deleted the fix/max-quantity-with-resizing branch April 18, 2024 13: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.

Max quantity calculation is not correct for resizing
3 participants