Skip to content
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

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

Merged
merged 2 commits into from
May 10, 2024

Conversation

usame-algan
Copy link
Member

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) 🧑‍💻

Copy link

github-actions bot commented Apr 12, 2024

Copy link

github-actions bot commented Apr 12, 2024

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: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@@ -65,6 +66,7 @@ export const CounterfactualForm = ({
// On modal submit
const handleSubmit = async (e: SyntheticEvent) => {
e.preventDefault()
onSubmit?.('mockId')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ideal but also doesn't break anything. We can adjust the type but we are prop drilling this handler multiple times and will have to change it in all of these places and adjust related logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be Math.random() instead?

Copy link

github-actions bot commented Apr 12, 2024

📦 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! 🙌

Copy link

github-actions bot commented Apr 12, 2024

Coverage report

St.
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.
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 7901544

@usame-algan
Copy link
Member Author

This fixes the issue mentioned here cowprotocol/cowswap#4063 (comment)

@francovenica
Copy link
Contributor

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
Copy link
Contributor

francovenica commented May 8, 2024

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
Copy link
Contributor

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.

Copy link

2 similar comments
Copy link

Copy link

@usame-algan
Copy link
Member Author

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

@usame-algan usame-algan merged commit cb7515e into dev May 10, 2024
14 checks passed
@usame-algan usame-algan deleted the cf-onsubmit branch May 10, 2024 08:10
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants