safe-wallet-web icon indicating copy to clipboard operation
safe-wallet-web copied to clipboard

fix: [Counterfactual] Call onSubmit handler when submitting a transaction

Open usame-algan opened this issue 10 months ago β€’ 5 comments

What it solves

We didn't call the onSubmit handler in CounterfactualForm so safe apps didn't receive a safeTxHash when submitting transactions.

How this PR fixes it

  • Call onSubmit when submitting in CounterfactualForm

How to test it

  1. Create a counterfactual safe
  2. Send some WETH to it
  3. Open the CowSwap safe app
  4. Create a TWAP order
  5. Submit the transaction in the tx-flow
  6. Observe that CowSwap shows a success message and is not stuck

Checklist

  • [ ] I've tested the branch on mobile πŸ“±
  • [ ] I've documented how it affects the analytics (if at all) πŸ“Š
  • [ ] I've written a unit/e2e test for it (if applicable) πŸ§‘β€πŸ’»

usame-algan avatar Apr 12 '24 13:04 usame-algan

Branch preview

βœ… Deploy successful!

https://cf_onsubmit--walletweb.review-wallet-web.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

github-actions[bot] avatar Apr 12 '24 13:04 github-actions[bot]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: :white_check_mark: success
  • Annotations: 0 total

Report generated by eslint-plus-action

github-actions[bot] avatar Apr 12 '24 13:04 github-actions[bot]

πŸ“¦ Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. πŸ€–

This PR introduced no changes to the JavaScript bundle! πŸ™Œ

github-actions[bot] avatar Apr 12 '24 13:04 github-actions[bot]

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟑 Statements
79.08% (-0.01% πŸ”»)
11194/14155
πŸ”΄ Branches 58.52% 2649/4527
🟑 Functions 66.03% 1802/2729
🟒 Lines
80.36% (-0.01% πŸ”»)
10087/12552
Show files with reduced coverage πŸ”»
St.:grey_question:
File Statements Branches Functions Lines
🟑
... / CounterfactualForm.tsx
69.12% (-1.03% πŸ”»)
42.86% 66.67%
69.23% (-1.08% πŸ”»)

Test suite run success

1419 tests passing in 198 suites.

Report generated by πŸ§ͺjest coverage report action from 7901544507fb49686a75e578e3e1f0d3c3dea577

github-actions[bot] avatar Apr 12 '24 13:04 github-actions[bot]

This fixes the issue mentioned here https://github.com/cowprotocol/cowswap/pull/4063#issuecomment-2039588336

usame-algan avatar Apr 24 '24 08:04 usame-algan

Tried to swap WETH for USDC and I got a timeout on the tx and now the app is stuck here: image

The cowSwap app still shows a tx is pending in the UI, but the address never got any order in cowswap at all: https://explorer.cow.fi/sepolia/address/0xd134A0150f82C11f2B4368Ee0fA8133Fa450b5a6 image

Also the link to the tx in the sepolia block explorer is emtpy as well: https://sepolia.etherscan.io/tx/0x22f82e47fd2f4564b5af08d81f4f98dd3481af78c3203996643c7bea3cf9a877

francovenica avatar May 08 '24 14:05 francovenica

A sideeffect of this is that the safe seems to have been deployed or at least "the safe thinks so".

If I try to do a send funds I get the "Safe already deployed" even tho no tx are present in the queue or history at all, and the sidebar still shows the "not deployed" icon image

francovenica avatar May 08 '24 14:05 francovenica

Given Usame's advice I tried again with Rabby and this time it worked. I was able to do the token approval for swapping + safe activation just fine and then tried the swap (the swap end up failing for low amount being swapped, so it was expected)

The safe used: https://cf_onsubmit--walletweb.review-wallet-web.5afe.dev/transactions/history?safe=sep:0xfeAacD8DDFb0E9B26136971C244e782b2EB05675

So, the ticket is fine, but given that most people use MM they might find the issue I reported first.

francovenica avatar May 08 '24 15:05 francovenica

I will merge this so the CoW team is unblocked but we should follow up on this once Metamask works again.

usame-algan avatar May 10 '24 08:05 usame-algan