-
Notifications
You must be signed in to change notification settings - Fork 89
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
[2] feat(twap): generate all part orders #2759
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
) | ||
const orders = Object.values(get(twapPartOrdersAtom)) | ||
|
||
return orders.flat().filter((order) => order.safeAddress === accountLowerCase && order.chainId === chainId) |
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.
is order.safeAddress
always lowercase?
const twapOrders = Object.values(twapOrdersList) | ||
|
||
const ordersParts$ = twapOrders.map((twapOrder) => { | ||
return generateTwapOrderParts(twapOrder, account.toLowerCase(), chainId) |
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.
shouldn't you lowercaase inside the mehtod instead?
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.
In terms of performance, it's better to call toLowerCase()
once. I would like to move the result of account.toLowerCase()
in const outside of map()
. Wdyt?
.map((_, index) => createPartOrderFromParent(twapOrder, index)) | ||
.filter(isTruthy) | ||
|
||
const ids = await Promise.all(parts.map((part) => computeOrderUid(chainId, safeAddress, part as Order))) |
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.
do we really need to cast the part into an order?
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.
Unfortunately yes. Because computeOrderUid
is based on @cowprotocol/contracts
but, in the rest of CowSwap we mostly use types from @cowprotocol/cow-sdk
.
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.
Unfortunately yes. Because computeOrderUid
is based on @cowprotocol/contracts
but, in the rest of CowSwap we mostly use types from @cowprotocol/cow-sdk
.
@@ -9,11 +9,13 @@ const AUTH_THRESHOLD = ms`1m` | |||
export function getTwapOrderStatus( | |||
order: TWAPOrderStruct, | |||
isExecuted: boolean, | |||
executionDate: Date, | |||
executionDateStr: string | null, |
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.
why not passing the Date or null directly?
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.
Good point, thanks!
Fixed
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.
Great changes!
… fix/twap-handler-banner
Summary
Fixes #2640
Implemented generating of all TWAP order parts using the algorithm from #2640
To Test
It should be tested in the last PR