fix(twap): label open orders as unfillable when Safe fallback handler is reset#7802
fix(twap): label open orders as unfillable when Safe fallback handler is reset#7802tenderdeve wants to merge 10 commits into
Conversation
… is reset When a Safe's ComposableCoW fallback handler is removed or reset, open TWAP orders can no longer be executed by the watchtower, but the orders table still showed them as pending. Thread the existing fallback-handler verification into the order status build so open orders (and their still-open parts) surface the existing Unfillable badge. Closes cowprotocol#5426
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTWAP order handling now derives fallback-handler-related unfillable states, preserves composable-order flags, propagates handler status through the orders table, and renders dedicated warnings with component tests. ChangesTWAP unfillable status
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OrdersTableRowGroup
participant useIsFallbackHandlerRequired
participant OrderRow
participant TwapOrderStatus
participant OrderEstimatedExecutionPrice
OrdersTableRowGroup->>useIsFallbackHandlerRequired: resolve fallback-handler state
useIsFallbackHandlerRequired-->>OrdersTableRow: return isFallbackHandlerBroken
OrderRow->>TwapOrderStatus: pass fallback-handler state
TwapOrderStatus->>OrderEstimatedExecutionPrice: render unfillable warning
OrderEstimatedExecutionPrice-->>OrderRow: display update-fallback-handler message
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/twap/utils/buildTwapOrdersItems.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming drifts across layers:
isFallbackHandlerRequired→isFallbackHandlerBroken.The hook is
useIsFallbackHandlerRequired, but the value is renamed toisFallbackHandlerBrokenwhen threaded through here. The semantics line up (both represent "handler is not in the correct/verified state"), but the rename could confuse future readers tracing the flag acrossTwapOrdersUpdater.tsx→buildTwapOrdersItems.ts. Consider keeping a consistent name (e.g.,isFallbackHandlerBrokenend-to-end, or documenting the rename) for easier traceability.Also applies to: 50-50, 59-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/modules/twap/utils/buildTwapOrdersItems.ts` at line 27, The flag name is drifting between layers, which makes the flow from useIsFallbackHandlerRequired through TwapOrdersUpdater and buildTwapOrdersItems harder to follow. Keep the naming consistent end-to-end by renaming the parameter and any related references to match the hook’s semantics, or explicitly document the intentional rename where the value is passed into buildTwapOrdersItems and used in the TWAP order item assembly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cowswap-frontend/src/modules/twap/utils/buildTwapOrdersItems.ts`:
- Line 27: The flag name is drifting between layers, which makes the flow from
useIsFallbackHandlerRequired through TwapOrdersUpdater and buildTwapOrdersItems
harder to follow. Keep the naming consistent end-to-end by renaming the
parameter and any related references to match the hook’s semantics, or
explicitly document the intentional rename where the value is passed into
buildTwapOrdersItems and used in the TWAP order item assembly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f444de2-eb8f-4abb-bef1-6a668bf96cec
📒 Files selected for processing (5)
apps/cowswap-frontend/src/entities/twap/services/mapTwapOrderToStoreOrder.tsapps/cowswap-frontend/src/modules/twap/types.tsapps/cowswap-frontend/src/modules/twap/updaters/TwapOrdersUpdater.tsxapps/cowswap-frontend/src/modules/twap/utils/buildTwapOrdersItems.tsapps/cowswap-frontend/src/modules/twap/utils/mapPartOrderToStoreOrder.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @tenderdeve , thank you for the attempt to fix this issue.
Before I report my feedback here, I'd like to clarify whether you verify yourself the results of AI-fixes you create? It would be awesome if you could have verified everything before sending a final outcome to our review. Thank you for understanding.
As for the PR:
- Order status and scheduled parts indeed are labeled as 'unfillable'
- however, the order itself is not displayed in the Unfillable tab in the table
- 'Fills at' still shows 'pending execution', however. should not. It would be great to show why the order is unfillable.
Addresses @elena-zh's review feedback: - Categorize composable (TWAP) orders into the Unfillable tab by respecting their derived isUnfillable flag in useOrdersTableList, instead of recomputing it purely from balance/allowance (which also overwrote the flag). Regular orders keep their existing balance-based behaviour. - Show an 'Unfillable' indicator with an explanatory tooltip in the 'Fills at' column for affected TWAP orders instead of 'pending execution'. - Rename the threaded flag to isFallbackHandlerBroken for consistency across layers.
|
@elena-zh thanks for the detailed review — and fair point on verification; I've verified this locally now (Unfillable tab + Fills-at + badge on an emulated handler-reset TWAP) and confirmed typecheck/lint pass. Pushed 4b3f47f addressing all three:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cowswap-frontend/src/modules/ordersTable/hooks/useOrdersTableList.ts`:
- Around line 91-95: The composable-order guard in useOrdersTableList is too
broad and now prevents TWAP parent orders from persisting a real isUnfillable
change into the store. Update the logic around the
recompute/setIsOrderUnfillable path so only child/computed composable orders are
skipped, while TWAP parent ParsedOrder entries still write back genuine
balance/allowance-based unfillable state. Use the existing isComposableOrder
check and the order.id / setIsOrderUnfillable flow to target the correct branch
without changing the local table recomputation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 54da3da9-7a44-4118-9b1a-86c7b6458dae
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/ordersTable/containers/OrderRow/OrderRow.styled.tsxapps/cowswap-frontend/src/modules/ordersTable/hooks/useOrdersTableList.tsapps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.tsxapps/cowswap-frontend/src/modules/twap/updaters/TwapOrdersUpdater.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cowswap-frontend/src/modules/twap/updaters/TwapOrdersUpdater.tsx
There was a problem hiding this comment.
Hey, now the changes look better, but all my comments are not addressed.
- As I mentioned above, parts should not be in 'pending execution' state. It should show the same info as for the parent order
- Parent order status should have the same design for 'unfillable' label: it should contain the warning icon saying what is wrong there (see the content in p.4)
- Parent order Fills at column should show a reason instead if 'Unfillable'. Compart it with the order below on the screenshot. It would be great to show 'Update fallback handler' there.
- Hovering a mouse on the reason should show a bit more explanation. The design should be same
- 'Update fallback handler' - title
- 'Your Safe fallback handler was changed after TWAP orders were placed. All open TWAP orders are not getting created because of that. Please, update the fallback handler in order to make the orders work again." description.
- Please, make sure that all new messages were ready for translations
…ck handler Reuse the existing balance/allowance unfillable design for open TWAP orders blocked by a reset Safe ComposableCoW fallback handler: - Fills-at column now shows an "Update fallback handler" reason (via the shared OrderEstimatedExecutionPrice label) instead of "Pending execution"/"Unfillable", for the parent order and its still-open parts. - Status badge shows the danger warning icon with a tooltip explaining the reset fallback handler (FallbackHandlerWarningTooltip), matching the other warnings. - Thread the fallback-handler reason through OrderRow so parent and parts render the same reason and tooltip. Copy matches the existing SetupFallbackHandlerWarning strings, so no new catalog entries are added. Adds unit tests for the reason label and tooltip content.
|
@elena-zh @fairlighteth updated in e44a66c — reworked the whole failed/unfillable presentation to match the existing balance/allowance design (per your comments 1-5):
Added unit tests covering the reason label and the tooltip content so this design can't silently regress. Typecheck/lint/i18n-extract all pass locally. Note: the end-to-end path (real Safe + resetting the ComposableCoW fallback handler + ~10-min watchtower refresh) I can't reproduce locally, so the manual verification is on the emulated flag path plus the added unit tests — flagging that explicitly. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/WarningTooltip/WarningTooltip.pure.tsx (1)
151-153: 📐 Maintainability & Code Quality | 🔵 TrivialAddress the self-flagged TODO for the missing return type.
The author left a
TODO/eslint-disable for the missing explicit return type onFallbackHandlerWarningTooltip. Since it returns JSX, an explicitJSX.Element(orReact.JSX.Element, depending on how this repo's React 19 types are set up) return type would let you drop the disable comment.Do you want me to open a follow-up issue to add the return type, or would you like a quick diff now?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/WarningTooltip/WarningTooltip.pure.tsx` around lines 151 - 153, The FallbackHandlerWarningTooltip component still has a self-flagged TODO and eslint-disable for a missing explicit return type; update the FallbackHandlerWarningTooltip function signature to return JSX.Element (or React.JSX.Element if that matches the repo’s React typings) and then remove the explicit-function-return-type disable comment since the annotation will satisfy the lint rule.apps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.tsx (1)
108-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fallback-handler warning copy/markup.
The "Update fallback handler" heading and the full explanatory paragraph ("Your Safe fallback handler was changed after TWAP orders were placed...") are duplicated verbatim here and in
FallbackHandlerWarningTooltip(WarningTooltip.pure.tsx, lines 149-168). Any future copy edit (or the pending i18n rework mentioned in the PR follow-ups) requires updating both places in sync. Consider extracting a sharedFallbackHandlerWarningContentcomponent/text used by both call sites.Also worth a look while touching this block:
internationalizedWarningTextis now a 4-branch nested ternary; a small lookup keyed by the flag constants would read more clearly than the current if/else chain.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.tsx` around lines 108 - 142, The fallback-handler warning heading and explanatory copy are duplicated between this `OrderEstimatedExecutionPrice` block and `FallbackHandlerWarningTooltip`, so extract shared warning content/text into a reusable component or constant and have both call sites render it. While updating this area, simplify `internationalizedWarningText` in `OrderEstimatedExecutionPrice` by replacing the nested ternary with a clearer lookup keyed by the warning constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.tsx`:
- Around line 24-27: `OrderEstimatedExecutionPrice.pure.tsx` is exporting a
non-component constant alongside `OrderEstimatedExecutionPrice`, so move
`UPDATE_FALLBACK_HANDLER_WARNING` (and related flag strings like `Insufficient
balance`/`Insufficient allowance`) into a dedicated `*.constants.ts` module in
this folder. Update `TwapOrderStatus.pure.tsx` and `OrderRow.container.tsx` to
import the constant(s) from that new file, and keep the component file exporting
only the component and its interfaces.
---
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.tsx`:
- Around line 108-142: The fallback-handler warning heading and explanatory copy
are duplicated between this `OrderEstimatedExecutionPrice` block and
`FallbackHandlerWarningTooltip`, so extract shared warning content/text into a
reusable component or constant and have both call sites render it. While
updating this area, simplify `internationalizedWarningText` in
`OrderEstimatedExecutionPrice` by replacing the nested ternary with a clearer
lookup keyed by the warning constants.
In
`@apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/WarningTooltip/WarningTooltip.pure.tsx`:
- Around line 151-153: The FallbackHandlerWarningTooltip component still has a
self-flagged TODO and eslint-disable for a missing explicit return type; update
the FallbackHandlerWarningTooltip function signature to return JSX.Element (or
React.JSX.Element if that matches the repo’s React typings) and then remove the
explicit-function-return-type disable comment since the annotation will satisfy
the lint rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 351323be-a91c-4712-a7df-241b2d1ed8cb
📒 Files selected for processing (7)
apps/cowswap-frontend/src/locales/en-US.poapps/cowswap-frontend/src/modules/ordersTable/containers/OrderRow/OrderRow.container.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.test.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/WarningTooltip/WarningTooltip.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.test.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/cowswap-frontend/src/locales/en-US.po
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
@tenderdeve , thanks, but case 2 from my previous comment #7802 (review) is still not addressed.
The parent TWAP status badge only reflected child balance/allowance warnings, so an order blocked by a reset Safe ComposableCoW fallback handler still showed a plain status with no reason. Derive the fallback-handler-unfillable state from the parent or any still-open part and render the same danger design and FallbackHandlerWarningTooltip already used on the parts and Fills-at column. Also move UPDATE_FALLBACK_HANDLER_WARNING into a dedicated constants module so the component file no longer exports a non-component value.
|
@elena-zh addressed case 2 in 6db4785 — the parent TWAP status badge now shows the same danger design + fallback-handler tooltip as the parts. The parent badge previously only reacted to child balance/allowance warnings, so a fallback-handler-blocked order rendered a plain status; it now derives the unfillable state from the parent or any still-open part. Also moved |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/ordersTable/pure/TwapStatusAndToggle/TwapStatusAndToggle.pure.tsx`:
- Around line 62-66: `isFallbackHandlerUnfillable` is duplicated between
`TwapStatusAndToggle.pure.tsx` and `OrderRow.container.tsx`; extract this
predicate into a shared ordersTable/twap utility and import it from both call
sites. Keep the shared helper named `isFallbackHandlerUnfillable` so the
existing `isFallbackHandlerBlocked` logic in `TwapStatusAndToggle` and the
row/container code can reference the same source of truth and avoid drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a270572-28b8-49cd-8387-ffcf01f5e63f
📒 Files selected for processing (6)
apps/cowswap-frontend/src/modules/ordersTable/containers/OrderRow/OrderRow.container.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.test.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/orderEstimatedExecutionPrice.constants.tsapps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/TwapStatusAndToggle/TwapStatusAndToggle.pure.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/orderEstimatedExecutionPrice.constants.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cowswap-frontend/src/modules/ordersTable/pure/OrderEstimatedExecutionPrice/OrderEstimatedExecutionPrice.pure.test.tsx
- apps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.tsx
- apps/cowswap-frontend/src/modules/ordersTable/containers/OrderRow/OrderRow.container.tsx
| const isFallbackHandlerUnfillable = (order: ParsedOrder): boolean => | ||
| order.isUnfillable === true && (order.status === OrderStatus.PENDING || order.status === OrderStatus.SCHEDULED) | ||
|
|
||
| const isFallbackHandlerBlocked = | ||
| isFallbackHandlerUnfillable(parent) || childOrders.some((child) => isFallbackHandlerUnfillable(child.order)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate fallback-handler-unfillable predicate.
isFallbackHandlerUnfillable here is byte-for-byte identical to the one defined in OrderRow.container.tsx. Extract this into a shared utility (e.g. an ordersTable/twap utils module) and import it in both places to avoid drift if the predicate logic changes.
♻️ Suggested extraction
// e.g. apps/cowswap-frontend/src/modules/ordersTable/utils/isFallbackHandlerUnfillable.ts
import { ParsedOrder } from '...'
import { OrderStatus } from '...'
export function isFallbackHandlerUnfillable(order: ParsedOrder): boolean {
return order.isUnfillable === true && (order.status === OrderStatus.PENDING || order.status === OrderStatus.SCHEDULED)
}- const isFallbackHandlerUnfillable = (order: ParsedOrder): boolean =>
- order.isUnfillable === true && (order.status === OrderStatus.PENDING || order.status === OrderStatus.SCHEDULED)
+ // imported from shared utils🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/cowswap-frontend/src/modules/ordersTable/pure/TwapStatusAndToggle/TwapStatusAndToggle.pure.tsx`
around lines 62 - 66, `isFallbackHandlerUnfillable` is duplicated between
`TwapStatusAndToggle.pure.tsx` and `OrderRow.container.tsx`; extract this
predicate into a shared ordersTable/twap utility and import it from both call
sites. Keep the shared helper named `isFallbackHandlerUnfillable` so the
existing `isFallbackHandlerBlocked` logic in `TwapStatusAndToggle` and the
row/container code can reference the same source of truth and avoid drift.
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 2m): cover the parent fallback-handler badge branch
Finding: [NON-BLOCKING] Cover the parent fallback-handler badge branch
- Location:
apps/cowswap-frontend/src/modules/ordersTable/pure/TwapStatusAndToggle/TwapStatusAndToggle.pure.tsx:62 - The latest fix adds the parent TWAP status-badge path for fallback-handler-blocked orders, but the added tests only cover the Fills-at label/tooltip path.
- Since this branch addressed the latest parent-badge review feedback, please add a focused test that a pending or scheduled parent with
isUnfillable: truerenders the fallback-handler warning on the parent status badge.
Suggested fix
- Add a
TwapStatusAndToggletest for a fallback-handler-blocked parent.
Review scope and related context
This is separate from the existing CodeRabbit thread on the duplicated isFallbackHandlerUnfillable predicate. That thread is about deduping the helper; this comment is only about preserving the newly added parent status-badge behavior with a focused test.
🤖 Prompt for AI agents
Verify this finding against current code. Fix only if still valid, keep the change minimal, and validate with the targeted test.
Context:
- apps/cowswap-frontend/src/modules/ordersTable/pure/TwapStatusAndToggle/TwapStatusAndToggle.pure.tsx:62
- The PR added fallback-handler warning rendering for the parent TWAP status badge after review feedback.
- Existing new tests cover OrderEstimatedExecutionPrice and TwapOrderStatus, but not TwapStatusAndToggle.
- Add a focused test that renders a pending or scheduled parent with fallback-handler isUnfillable and asserts the parent status badge exposes the fallback-handler warning.
Generated using the pr-review skill from the CoW Protocol skills repo.
|
Hey @tenderdeve , could you please address the last feedback and resolve conflicts? |
| executionInfo: TwapOrdersExecution | ||
| // True when an open order cannot be executed because the Safe's ComposableCoW | ||
| // fallback handler is missing/reset by the user (see issue #5426) | ||
| isUnfillable?: boolean |
There was a problem hiding this comment.
I would avoid adding isUnfillable to TwapOrderItem.
The interface contains static order data. Yes, there are executedDate and executionInfo which are being set after execution, but those fields are not reversible, once they are set, they will be here forever.
isUnfillable is a dynamic flag, it can change back and forth. Because of that I wouldn't add it here to not mix different kinds of fields.
There was a problem hiding this comment.
Agreed — dropped isUnfillable from TwapOrderItem entirely (and from the Order/part mapping it fed into). The interface is back to static order data only. The broken-fallback-handler condition is now resolved in the view layer instead.
|
|
||
| // An open (Pending) order will never be picked up by watchtower when the Safe's | ||
| // ComposableCoW fallback handler has been reset/removed, so surface it as Unfillable. | ||
| const isUnfillable = isFallbackHandlerBroken && status === TwapOrderStatus.Pending |
There was a problem hiding this comment.
FallbackHandler state is per account; I think it's a bit excessive to spread this info per order.
I would skip wiring it into the orders state and would bring it to the view part directly.
There was a problem hiding this comment.
Done — buildTwapOrdersItems no longer takes isFallbackHandlerBroken and nothing is written into the orders state. The per-account useIsFallbackHandlerRequired() is now read directly in the view (OrderRow container + OrdersTableRowGroup) and combined with each order's status via a small shared getIsFallbackHandlerUnfillable() helper, so the status badge / parts / Fills-at column derive it on render.
Per review: the ComposableCoW fallback-handler-broken state is per-account and dynamic, so stop threading it through the static TwapOrderItem interface and the persisted orders state. Instead resolve it in the view from the account-scoped useIsFallbackHandlerRequired() hook and combine with each order's status via a shared getIsFallbackHandlerUnfillable() helper. Drops isUnfillable from TwapOrderItem and the buildTwapOrdersItems/updater/ map wiring; the parent badge, parts and Fills-at column now derive the flag from the hook. Behaviour is unchanged.
|
@shoom3301 addressed both points in @elena-zh also rebased on |
…fillable-handler # Conflicts: # apps/cowswap-frontend/src/modules/ordersTable/hooks/useOrdersTableList.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/Group/OrdersTableRowGroup.pure.tsx (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMaintain component purity.
OrdersTableRowGroupis a.pure.tsxcomponent designed to receive domain data via props. Calling the global domain hookuseIsFallbackHandlerRequired()directly within this component makes it stateful and violates the architectural separation of concerns. As per the coding guidelines, component logic should reside in*.container.tsxfiles.Consider passing
isFallbackHandlerBrokenas an optional boolean prop viaOrdersTableRowGroupPropsfrom the parent container instead of calling the hook here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/Group/OrdersTableRowGroup.pure.tsx` around lines 62 - 64, Remove the useIsFallbackHandlerRequired hook call from OrdersTableRowGroup and add an optional isFallbackHandlerBroken boolean to OrdersTableRowGroupProps. Have the parent *.container.tsx component obtain the hook value and pass it through, preserving the existing status-badge behavior while keeping the .pure.tsx component prop-driven.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/Group/OrdersTableRowGroup.pure.tsx`:
- Around line 62-64: Remove the useIsFallbackHandlerRequired hook call from
OrdersTableRowGroup and add an optional isFallbackHandlerBroken boolean to
OrdersTableRowGroupProps. Have the parent *.container.tsx component obtain the
hook value and pass it through, preserving the existing status-badge behavior
while keeping the .pure.tsx component prop-driven.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd044798-ec05-49ed-a1ac-1302e2dfda20
📒 Files selected for processing (8)
apps/cowswap-frontend/src/modules/ordersTable/containers/OrderRow/OrderRow.container.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrderFillsAt/OrderFillsAt.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrderFillsAtWithDistance/OrderFillsAtWithDistance.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/Row/Group/OrdersTableRowGroup.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.test.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/TwapStatusAndToggle/TwapStatusAndToggle.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/utils/getIsFallbackHandlerUnfillable.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cowswap-frontend/src/modules/ordersTable/pure/TwapOrderStatus/TwapOrderStatus.pure.test.tsx
- apps/cowswap-frontend/src/modules/ordersTable/pure/TwapStatusAndToggle/TwapStatusAndToggle.pure.tsx
| // resolved here in the view (not persisted onto the order) and combined with the per-order status. | ||
| // It only applies to composable (TWAP) orders; surface it with the same danger design as the | ||
| // balance/allowance warnings (see issue #5426). | ||
| const isFallbackHandlerBroken = useIsFallbackHandlerRequired() |
There was a problem hiding this comment.
Uber nitpick, I wouldn't substitute concepts handler required and handler broken
| warningText === 'Insufficient balance' | ||
| const isFallbackHandlerWarning = warningText === UPDATE_FALLBACK_HANDLER_WARNING | ||
|
|
||
| const internationalizedWarningText = isFallbackHandlerWarning |
There was a problem hiding this comment.
It's actually weird.
We don't need both warningText and internationalizedWarningText.
I would convert warningText into a enum like WarningReason { Balance, Allowance, FallbackHandler }.
| // same danger design on the parent status badge so it matches the Fills-at column and the parts. | ||
| const isFallbackHandlerBlocked = | ||
| getIsFallbackHandlerUnfillable(parent.status, !!isFallbackHandlerBroken) || | ||
| childOrders.some((child) => getIsFallbackHandlerUnfillable(child.order.status, !!isFallbackHandlerBroken)) |
There was a problem hiding this comment.
I'm not sure if we need to check children statuses, it should be enough checking only the parent
4 participants
Summary
Fixes #5426.
When a Safe's ComposableCoW fallback handler is removed or reset (e.g. via the Safe TX builder), open TWAP orders can no longer be picked up by the watchtower, so they will never execute. The orders table kept showing them as
Pending, giving no signal that the order is effectively dead.The module already verifies the fallback handler (
useIsFallbackHandlerRequired) but only used it in the order-creation form — never in the orders table. This threads that signal into the status build so open orders are surfaced with the existing Unfillable badge.Changes
TwapOrdersUpdaterreadsuseIsFallbackHandlerRequired()and passes it intobuildTwapOrdersItems.buildTwapOrdersItemsmarks an itemisUnfillableonly when the handler is broken and the order is still open (Pending).isUnfillableso the table renders the badge; still-open parts of an affected order are flagged too.No new status enum — reuses the existing
order.isUnfillablebadge path, so final states (Fulfilled/Cancelled/Expired) are untouched.Testing steps
Summary by CodeRabbit
Bug Fixes
Tests