Repository navigation
fix(twap): label open orders as unfillable when Safe fallback handler is reset - #7802
limitofzero merged 21 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.
2 Skipped Deployments
|
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
|
LGTM. |
Head branch was pushed to by a user without write access
|
@fairlighteth added a focused |
|
Hey @tenderdeve, thank you for addressing all comments and issues, could you pls update the branch to resolve conflicts? |
Head branch was pushed to by a user without write access
|
@limitofzero rebased on latest |
|
Looks good |
|
@limitofzero , could you please finally merge the PR, so I will not need to retest it once again? |
|
@tenderdeve thank you, but there are some lint errors: https://github.com/cowprotocol/cowswap/actions/runs/31587080646/job/94083621865?pr=7802 |
Order exported tooltips before local ones in WarningTooltip.pure so perfectionist/sort-modules passes, and extract the child-order params mapping out of OrdersTableRowGroup to keep it under max-lines-per-function.
Head branch was pushed to by a user without write access
|
@shoom3301 thanks — the three points from your last pass are in:
Also pushed 74ac6e2 to fix Lint: reordered the exported tooltips ahead of the local ones for |
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