fix: swap DenomIn and DenomOut in order execution functions and repla… #1039
Conversation
…ce MinAmount with MaxAmount Updated the order execution functions in pending_spot_order.go to swap the DenomIn and DenomOut parameters for consistency. Additionally, replaced the MinAmount field with MaxAmount to better reflect the intended functionality of the orders.
cosmic-vagabond
temporarily deployed
to
coverage
GitHub Actions
Inactive
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1039 +/- ##
=======================================
Coverage 44.12% 44.13%
=======================================
Files 935 935
Lines 35559 35559
=======================================
+ Hits 15691 15693 +2
+ Misses 18563 18561 -2
Partials 1305 1305 |
| MinAmount: sdk.NewCoin(order.OrderTargetDenom, sdkmath.ZeroInt()), | ||
| DenomIn: order.OrderPrice.QuoteDenom, | ||
| DenomOut: order.OrderPrice.BaseDenom, | ||
| MaxAmount: order.OrderAmount, |
There was a problem hiding this comment.
this was fine earlier, I think
There was a problem hiding this comment.
SL and sell order are pretty much the same flow, that’s why I thought it would also make sense to do the change for SL
There was a problem hiding this comment.
sell order if fine, the issue is in limit_buy only, market_buy is fine too
| @@ -291,9 +290,9 @@ func (k Keeper) ExecuteLimitBuyOrder(ctx sdk.Context, order types.SpotOrder) (*a | |||
| Sender: order.OwnerAddress, | |||
| Recipient: order.OwnerAddress, | |||
| Amount: order.OrderAmount, | |||
There was a problem hiding this comment.
amount for this should be order.OrderAmount * Rate, right ?
because that's what we need to send for swap
| MinAmount: sdk.NewCoin(order.OrderTargetDenom, sdkmath.ZeroInt()), | ||
| DenomIn: order.OrderPrice.QuoteDenom, | ||
| DenomOut: order.OrderPrice.BaseDenom, | ||
| MaxAmount: order.OrderAmount, |
There was a problem hiding this comment.
why are we sending Max amount ?
|
The amount locked when creating a limitBuy order locks atom amount which is wrong, we should lock order.Amount * buy_rate of USDC. |
|
not relevant anymore, closing |
cosmic-vagabond
deleted the
fix/tradeshield-create-order-uses-quote-as-denom-in
branch
…ce MinAmount with MaxAmount
Updated the order execution functions in pending_spot_order.go to swap the DenomIn and DenomOut parameters for consistency. Additionally, replaced the MinAmount field with MaxAmount to better reflect the intended functionality of the orders.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeDeployment Notes
Are there any specific considerations to take into account when deploying these changes? This may include new dependencies, scripts that need to be executed, or any aspects that can only be evaluated in a deployed environment.
Screenshots and Videos
Please provide any relevant before and after screenshots by uploading them here. Additionally, demo videos can be highly beneficial in demonstrating the process.