fix: use orderCreationHash to derive explorer link in ETH-flow transactions#7210
fix: use orderCreationHash to derive explorer link in ETH-flow transactions#7210esouliot wants to merge 10 commits intocowprotocol:developfrom
Conversation
…n SnackbarsWidget
|
@esouliot is attempting to deploy a commit to the cow Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThree frontend files were updated to support ETH-flow order creation hashes and adjust notification timing: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
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 @esouliot , thank you for the contribution!
But the fix should be more complex and should follow the same behavior as on the progress bar modal:
- once ETH-flow order is being executed onchain, it should show the link to etherscan (block explorer)
- once TX is mined, and CoW order is created on Explorer, the link should navigate to an order on CoW Explorer
Could you please fix it?
Thanks
- Match event to "Order Created" step in EthFlowStepper - Add new ON_PENDING_ORDER member to CowWidgetEvents and OrderStatusEvents enums - Add new ORDER_PENDING member to ToastMessageType enum - Create new ON_PENDING_ORDER handler in OrderNotificationsUpdater. - Create new emitPendingOrderEvent function - Use emitPendingOrderEvent when an ETH Flow transaction has an OrderStatus of PENDING
|
Hi @elena-zh, thanks for your feedback. Per your request, I've added a new event that fires off when an ETH Flow transaction changes to OrderStatus.PENDING inside PendingOrdersUpdater. This triggers a Snackbar modal to appear with a CoW Explorer link and should correspond to the "Order Created" stage of the EthFlowStepper widget on-page. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx (1)
65-65: Minor: Use self-closing tag.The
OrderNotificationelement has no children, so a self-closing tag would be more concise.♻️ Suggested fix
- <OrderNotification - title={t`Order Created`} - chainId={chainId} - orderUid={order.uid} - messageType={ToastMessageType.ORDER_PENDING} - ></OrderNotification> + <OrderNotification + title={t`Order Created`} + chainId={chainId} + orderUid={order.uid} + messageType={ToastMessageType.ORDER_PENDING} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx` at line 65, The JSX for the OrderNotification component is currently using an explicit open and close tag with no children; change it to a self-closing tag to be more concise and idiomatic (replace <OrderNotification>...</OrderNotification> with <OrderNotification /> wherever the component is rendered in handlers.tsx, specifically the OrderNotification element).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx`:
- Around line 54-68: The ON_PENDING_ORDER handler needs access to an optional
transaction hash to render Etherscan links; update the payload type so the
handler can forward it to OrderNotification → OrderNotificationContent →
TransactionContentWithLink: extend BaseOrderPayload to include transactionHash?:
string (or create a specific PendingOrderPayload used by the ON_PENDING_ORDER
event) and then change the handler signature (payload: BaseOrderPayload) to
accept the new optional field and pass transactionHash={payload.transactionHash}
into OrderNotification; follow the pattern used by OnCancelledOrderPayload to
keep compatibility.
---
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx`:
- Line 65: The JSX for the OrderNotification component is currently using an
explicit open and close tag with no children; change it to a self-closing tag to
be more concise and idiomatic (replace
<OrderNotification>...</OrderNotification> with <OrderNotification /> wherever
the component is rendered in handlers.tsx, specifically the OrderNotification
element).
🪄 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: f93a9dd4-28e7-4779-9636-9af591840910
📒 Files selected for processing (7)
apps/cowswap-frontend/src/common/updaters/orders/PendingOrdersUpdater.tsapps/cowswap-frontend/src/modules/orders/events/events.tsapps/cowswap-frontend/src/modules/orders/index.tsapps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsxapps/cowswap-frontend/src/modules/orders/utils/emitPendingOrderEvent.tslibs/events/src/types/events.tslibs/events/src/types/toastMessages.ts
| [OrderStatusEvents.ON_PENDING_ORDER]: { | ||
| icon: 'success', | ||
| handler: (payload: BaseOrderPayload) => { | ||
| const { chainId, order } = payload | ||
|
|
||
| return ( | ||
| <OrderNotification | ||
| title={t`Order Created`} | ||
| chainId={chainId} | ||
| orderUid={order.uid} | ||
| messageType={ToastMessageType.ORDER_PENDING} | ||
| ></OrderNotification> | ||
| ) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how TransactionContentWithLink derives links and what props OrderNotification passes to it
echo "=== OrderNotification component ==="
rg -n -A 20 'export function OrderNotification' apps/cowswap-frontend/src/modules/orders/containers/OrderNotification/
echo ""
echo "=== TransactionContentWithLink usage ==="
rg -n -C 5 'TransactionContentWithLink' apps/cowswap-frontend/src/modules/orders/
echo ""
echo "=== BaseOrderNotificationProps definition ==="
ast-grep --pattern 'interface BaseOrderNotificationProps {
$$$
}'
echo ""
echo "=== Check if EnrichedOrder has ethflowData with creation hash ==="
rg -n 'ethflowData' libs/Repository: cowprotocol/cowswap
Length of output: 9146
🏁 Script executed:
#!/bin/bash
# Get the full BaseOrderNotificationProps interface
echo "=== BaseOrderNotificationProps full definition ==="
rg -n -A 15 'interface BaseOrderNotificationProps' apps/cowswap-frontend/src/modules/orders/
echo ""
echo "=== OrderNotification complete component ==="
rg -n -A 40 'export function OrderNotification' apps/cowswap-frontend/src/modules/orders/containers/OrderNotification/
echo ""
echo "=== OrderNotificationContent props ==="
rg -n -B 5 -A 15 'export interface OrderNotificationContentProps' apps/cowswap-frontend/src/modules/orders/pure/OrderNotificationContent/Repository: cowprotocol/cowswap
Length of output: 9387
🏁 Script executed:
#!/bin/bash
# Check ON_CANCELLED_ORDER handler to see how it passes transactionHash
echo "=== ON_CANCELLED_ORDER handler ==="
rg -n -B 2 -A 15 'ON_CANCELLED_ORDER' apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx
echo ""
echo "=== EnrichedOrder type definition ==="
rg -n -B 2 -A 20 'type EnrichedOrder' apps/cowswap-frontend/src/ --type ts --type tsx
echo ""
echo "=== BaseOrderPayload definition ==="
rg -n -B 2 -A 10 'type BaseOrderPayload' apps/cowswap-frontend/src/ --type ts --type tsxRepository: cowprotocol/cowswap
Length of output: 763
🏁 Script executed:
#!/bin/bash
# Search for payload type definitions in the handlers file and nearby
echo "=== Payload type imports in handlers.tsx ==="
rg -n -B 5 'BaseOrderPayload\|OnCancelledOrderPayload' apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx | head -40
echo ""
echo "=== Search for BaseOrderPayload definition ==="
rg -n 'type BaseOrderPayload\|interface BaseOrderPayload' apps/cowswap-frontend/src/
echo ""
echo "=== Search for OnCancelledOrderPayload definition ==="
rg -n 'type OnCancelledOrderPayload\|interface OnCancelledOrderPayload' apps/cowswap-frontend/src/
echo ""
echo "=== Check handlers.tsx imports and file context ==="
head -30 apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsxRepository: cowprotocol/cowswap
Length of output: 1410
🏁 Script executed:
#!/bin/bash
# Check if `@cowprotocol/events` is in the repo or external
echo "=== Search for `@cowprotocol/events` location ==="
find . -type d -name events -o -type f -path "*/@cowprotocol/events*" | head -20
echo ""
echo "=== Check package.json for `@cowprotocol/events` ==="
rg -n '@cowprotocol/events' package.json pnpm-lock.yaml | head -10
echo ""
echo "=== Search for BaseOrderPayload definition in repo ==="
rg -n 'export.*BaseOrderPayload\|export type BaseOrderPayload' apps/ libs/
echo ""
echo "=== Search for OnCancelledOrderPayload definition in repo ==="
rg -n 'export.*OnCancelledOrderPayload\|export type OnCancelledOrderPayload' apps/ libs/
echo ""
echo "=== Check if there's an `@cowprotocol/events` local package ==="
ls -la libs/*/package.json | grep eventsRepository: cowprotocol/cowswap
Length of output: 1305
🏁 Script executed:
#!/bin/bash
# Check the local events package
echo "=== libs/events structure ==="
ls -la libs/events/
echo ""
echo "=== Search for BaseOrderPayload in libs/events ==="
rg -n 'BaseOrderPayload' libs/events/
echo ""
echo "=== Search for OnCancelledOrderPayload in libs/events ==="
rg -n 'OnCancelledOrderPayload' libs/events/
echo ""
echo "=== Check libs/events/src for type definitions ==="
find libs/events/src -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: cowprotocol/cowswap
Length of output: 2212
🏁 Script executed:
#!/bin/bash
# Read the full type definitions
echo "=== BaseOrderPayload and related types ==="
cat -n libs/events/src/types/orders.tsRepository: cowprotocol/cowswap
Length of output: 1472
🏁 Script executed:
#!/bin/bash
# Check if there's a handler for OnPostedOrderPayload or related
echo "=== Check for OnPostedOrderPayload handler ==="
rg -n 'OnPostedOrderPayload\|ON_POSTED_ORDER\|ON_PENDING_ORDER' apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx
echo ""
echo "=== Check events.ts for all event mappings ==="
cat -n libs/events/src/types/events.ts
echo ""
echo "=== Check PostedOrderNotification component ==="
rg -n -A 10 'export function PostedOrderNotification\|export const PostedOrderNotification' apps/cowswap-frontend/src/modules/orders/containers/Repository: cowprotocol/cowswap
Length of output: 2190
🏁 Script executed:
#!/bin/bash
# Check what the full handlers.tsx looks like
echo "=== Full handlers.tsx file ==="
cat -n apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsxRepository: cowprotocol/cowswap
Length of output: 5467
Extend BaseOrderPayload to include optional transactionHash field for ETH-flow Etherscan links.
The ON_PENDING_ORDER handler receives BaseOrderPayload, which currently contains only chainId and order: EnrichedOrder, but lacks a transactionHash field. Without this, the handler cannot pass transactionHash to OrderNotification → OrderNotificationContent → TransactionContentWithLink to generate an Etherscan link, even though OnCancelledOrderPayload (line 31 in libs/events/src/types/orders.ts) demonstrates that the pattern supports optional transaction hashes.
If the intent is to display Etherscan links for ETH-flow pending orders (similar to the cancelled order handler on line 98), extend BaseOrderPayload in libs/events/src/types/orders.ts to include transactionHash?: string, or create a separate payload type for ON_PENDING_ORDER that includes this field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/handlers.tsx`
around lines 54 - 68, The ON_PENDING_ORDER handler needs access to an optional
transaction hash to render Etherscan links; update the payload type so the
handler can forward it to OrderNotification → OrderNotificationContent →
TransactionContentWithLink: extend BaseOrderPayload to include transactionHash?:
string (or create a specific PendingOrderPayload used by the ON_PENDING_ORDER
event) and then change the handler signature (payload: BaseOrderPayload) to
accept the new optional field and pass transactionHash={payload.transactionHash}
into OrderNotification; follow the pattern used by OnCancelledOrderPayload to
keep compatibility.
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @esouliot , thank you so much for another fix.
I've noticed that you added another pop-up with 'order created' context.
There is no need in this pop-up. The initial request was to change a link on the 'order submitted pop-up' from etherescan to cow explorer once the order creation TX is executed onchain.
You can check how it was working in https://swap-dev-git-fix-twap-slippage-cowswap-dev.vercel.app/.
Thanks.
* reverts 2d206e0
…orders * update tx.hash in TransactionContentWithLink * add extended duration to snackbar for ETH-flow orders in OrdersNotificationUpdater
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/index.tsx (1)
25-28: Use immutabledurationcomputation (avoidletreassignment).Lines 25–28 should use a
constwith a ternary expression instead of mutable reassignment. Additionally, apply explicit boolean coercion (!!) topayload.isEthFlow.Proposed refactor
- let duration: number | undefined - if (eventTyped === OrderStatusEvents.ON_POSTED_ORDER && 'isEthFlow' in payload && payload.isEthFlow) { - duration = 20_000 - } + const duration = + eventTyped === OrderStatusEvents.ON_POSTED_ORDER && 'isEthFlow' in payload && !!payload.isEthFlow + ? 20_000 + : undefinedPer guidelines: "Never reassign via
let; favourconstand ternaries/early returns" and "Use!!valuefor explicit boolean conversions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/index.tsx` around lines 25 - 28, Replace the mutable duration declaration with an immutable const computed via a ternary: set duration to 20_000 when eventTyped === OrderStatusEvents.ON_POSTED_ORDER and !!payload.isEthFlow is true, otherwise undefined; update the declaration where duration is currently declared (near the check using eventTyped, OrderStatusEvents.ON_POSTED_ORDER and payload.isEthFlow) to use this const expression instead of a let and reassignment.apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsx (1)
10-11: Use the orders module public API instead of importing internals directly.These imports couple this container to low-level files in another module. Please import via that module’s barrel API to keep boundaries stable.
As per coding guidelines: "imports from another module must go through that module's
index.tsbarrel".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsx` around lines 10 - 11, Replace direct imports of the internals with the orders module's public barrel export: stop importing OrderStatus and useOrder from their internal paths and instead import both OrderStatus and useOrder from the orders module's index (the module barrel). Update the import statement that currently references OrderStatus and useOrder so it reads a single import from the orders module barrel (so the container uses the public API rather than internal files).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsx`:
- Around line 45-49: The current selection for the displayed hash in
TransactionContentWithLink uses orderUid when orderCreationHash is missing for
ETH-flow orders; change the fallback so that when isEthFlowCreating is true (or
when isEthFlow && transactionHash && !status), prefer transactionHash before
falling back to orderUid: update the conditional that sets hash (references:
isEthFlowCreating, orderCreationHash, isEthFlow, transactionHash, status,
orderUid) so transactionHash is used as the intermediate fallback when
orderCreationHash is not yet populated, and only use orderUid as the final
fallback.
---
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsx`:
- Around line 10-11: Replace direct imports of the internals with the orders
module's public barrel export: stop importing OrderStatus and useOrder from
their internal paths and instead import both OrderStatus and useOrder from the
orders module's index (the module barrel). Update the import statement that
currently references OrderStatus and useOrder so it reads a single import from
the orders module barrel (so the container uses the public API rather than
internal files).
In
`@apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/index.tsx`:
- Around line 25-28: Replace the mutable duration declaration with an immutable
const computed via a ternary: set duration to 20_000 when eventTyped ===
OrderStatusEvents.ON_POSTED_ORDER and !!payload.isEthFlow is true, otherwise
undefined; update the declaration where duration is currently declared (near the
check using eventTyped, OrderStatusEvents.ON_POSTED_ORDER and payload.isEthFlow)
to use this const expression instead of a let and reassignment.
🪄 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: f6169c39-4be2-460f-95d4-b5247cdf7ca8
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsxapps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/index.tsx
* rewrite tx.hash definition to match old pattern while maintaining ethFlow cases * make duration definition immutable
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsx (1)
45-45: Consider simplifying the nested ternary for readability.The current expression is functionally correct but the nested ternary can be harder to parse at a glance. Since the first condition
isOrder && !isEthFlowleads toorderUidand the fallback also usesorderUid, this simplifies to:♻️ Suggested simplification
const tx = { - hash: (isOrder && !isEthFlow ? orderUid : isEthFlowCreating ? transactionHash : orderUid) || '', + hash: isEthFlowCreating ? transactionHash : orderUid || '', hashType: (isSafeOrder || isSafeTx) && !isOrder ? HashType.GNOSIS_SAFE_TX : HashType.ETHEREUM_TX,This expresses the same logic more directly: use
transactionHashfor ETH-flow creating orders, otherwise useorderUid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsx` at line 45, The nested ternary used for the hash prop is hard to read: simplify the expression in the component where hash is set (referencing isOrder, isEthFlow, isEthFlowCreating, orderUid, transactionHash) so it picks transactionHash when isEthFlowCreating is true, otherwise uses orderUid; replace the current complex ternary with a clearer conditional that returns transactionHash when isEthFlowCreating, else orderUid (falling back to '' if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsx`:
- Line 45: The nested ternary used for the hash prop is hard to read: simplify
the expression in the component where hash is set (referencing isOrder,
isEthFlow, isEthFlowCreating, orderUid, transactionHash) so it picks
transactionHash when isEthFlowCreating is true, otherwise uses orderUid; replace
the current complex ternary with a clearer conditional that returns
transactionHash when isEthFlowCreating, else orderUid (falling back to '' if
needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9369cb43-3e3b-497e-bf4a-4b814858a2fa
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/orders/containers/TransactionContentWithLink/index.tsxapps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cowswap-frontend/src/modules/orders/updaters/OrdersNotificationsUpdater/index.tsx
|
@elena-zh for me it works well now ggreettttt.mov |
|
|
||
| if (!content) return | ||
|
|
||
| const duration = |
There was a problem hiding this comment.
I guess this is a temporary change and should be reverted before merging
There was a problem hiding this comment.
Hi @shoom3301, I purposely set the duration of the snackbar to be 20s for ETH flow transactions since the default 8s timeout would have the modal disappear before the link could respond to the state change of the transaction. The 20s timeout allows for the ETH transaction to be verified (~12s) and then adds on another 8s for the viewer to see the changeover. As for the choice of a hard-coded duration, it was the least invasive way to get the snackbar to reflect the state change as requested in prior feedback.
Hey @shoom3301 , thank you. I see some commits from @esouliot after my feedback. so it might have been fixed. However, I was not notified to retest it. @esouliot , could you please confirm whether you addressed the issue and I can retest it? |
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Anyways, I've retested it, the changes LGTM now.
@elena-zh |
3 participants
Summary
orderCreationHashfromethFlowtoPostedOrderNotificationas thetransactinHashprop. This allows TransactionContentWithLink to derive an Etherscan link for unfilled ETH-flow orders instead of defaulting to the CoW Explorer.Fixes Issue #6925
To Test
Summary by CodeRabbit