-
Notifications
You must be signed in to change notification settings - Fork 15
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: lp_cancel_orders_batch to accepts OrderId as string/hex #5504
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5504 +/- ##
=======================================
- Coverage 72% 72% -0%
=======================================
Files 490 495 +5
Lines 86894 88259 +1365
Branches 86894 88259 +1365
=======================================
+ Hits 62395 63169 +774
- Misses 21565 22521 +956
+ Partials 2934 2569 -365 ☔ View full report in Codecov by Sentry. |
api/bin/chainflip-lp-api/src/main.rs
Outdated
let mut final_orders: BoundedVec<CloseOrder, ConstU32<MAX_ORDERS_DELETE>> = | ||
BoundedVec::new(); | ||
for order in orders { | ||
final_orders | ||
.try_push(order.try_into()?) | ||
.expect("Impossible to fail, given the same MAX_ORDERS_DELETE"); | ||
} |
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.
can remove the mut var by collecting
let mut final_orders: BoundedVec<CloseOrder, ConstU32<MAX_ORDERS_DELETE>> = | |
BoundedVec::new(); | |
for order in orders { | |
final_orders | |
.try_push(order.try_into()?) | |
.expect("Impossible to fail, given the same MAX_ORDERS_DELETE"); | |
} | |
Ok(self | |
.api | |
.lp_api() | |
.cancel_orders_batch( | |
orders | |
.into_iter() | |
.map(TryInto::try_into) | |
.collect::<Result<Vec<_>, _>>()? | |
.try_into() | |
.expect("Impossible to fail, given the same MAX_ORDERS_DELETE"), | |
wait_for.unwrap_or_default(), | |
) | |
.await?) |
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.
Doesn't this require an extra memory allocation for the Vec<>
when collecting?
(or maybe this gets optimized away by the compiler?)
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.
Yes, I believe so. But given the performance gain is negligible, I think having without a mut var is a bit simpler and less likely to be mistakenly broken with future changes (definitely isn't a strong opinion which way is preferred here)
Pull Request
Closes: PRO-1897
Checklist
Please conduct a thorough self-review before opening the PR.
Summary
Updated the LP-API cancel_orders_batch to accepts OrderId as string/hex as well
Non-Breaking changes
If this PR includes non-breaking changes, select the
non-breaking
label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.