fix: [Counterfactual] Add event bus and pending state #3242

usame-algan · 2024-02-12T20:11:46Z

What it solves

Part of #3156

How this PR fixes it

Adds an Event Bus for SafeCreationEvents
Dispatches SafeCreationEvents when deploying a counterfactual safe
Persists pending state status in undeployedSafesSlice
Shows notifications for SafeCreationEvents
Monitors undeployedSafes if they are relaying or processing
Checks for an undeployedSafe if its already deployed on-chain and cleans up local storage in case it is
Supports cancellation and speed-up safe creations
Disables the create button in the safe creation flow and adds a loading spinner while submitting

How to test it

Happy path:

Create a counterfactual safe
Deploy safe via "Activate now" or with first tx
Observe Processing notification showing
Observe the "Activate now" button is disabled and has a loading state on it
Observe a Success notification
The undeployed state should be cleaned up

Pending state is persisted:

Create a counterfactual safe
Deploye safe via "Activate now" or with first tx
Observe Processing notification showing
Reload the app
Observe the "Activate now" button is disabled and has a loading state on it
Observe a Success notification

Sad path:

Create a counterfactual safe
Deploy safe via "Activate now" or with first tx with too little gas
Observe Processing notification
Observe Timeout error after a while
Cancel transaction in wallet and execute again
This time speed up the transaction
Observe a success notification

Cleaning state:

Create a counterfactual safe
Deploy the safe via "Activate now" and immediately close the app
Wait until the transaction is successfully executed via wallet
Open the app again
Observe the undeployed state is cleaned up

Creation Loading state:

Go through the safe creation flow
Press Create
Observe that the Create button is disabled and has a loading spinner

Screenshots

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

github-actions · 2024-02-12T20:11:57Z

Branch preview

✅ Deploy successful!

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

github-actions · 2024-02-12T20:12:51Z

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}

github-actions · 2024-02-12T20:19:03Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	78.66% (-0.27% 🔻)	11524/14650
🔴	Branches	56.68% (-0.29% 🔻)	2580/4552
🟡	Functions	63.51% (-0.21% 🔻)	1833/2886
🟢	Lines	80% (-0.23% 🔻)	10396/12995

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / storeHydrator.tsx	100%	100%	100%	100%
🟢	`...` / safeCreationEvents.ts	70%	100%	66.67%	87.5%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / addressBookSlice.ts	86.49% (-2.7% 🔻)	44.44% (-11.11% 🔻)	87.5%	92.31%
🟢	`...` / persistStore.ts	95.24% (-0.92% 🔻)	66.67% (-13.33% 🔻)	100%	94.12% (-0.88% 🔻)
🔴	`...` / undeployedSafesSlice.ts	38.24% (-5.76% 🔻)	33.33% (+33.33% 🔼)	50% (-10% 🔻)	45.83% (-4.17% 🔻)
🟢	`...` / txMonitor.ts	91.86% (-1.08% 🔻)	91.3%	95.24%	92.59%
🟡	`...` / index.ts	69.49% (-0.18% 🔻)	63.64% (+1.14% 🔼)	43.75% (-3.31% 🔻)	68.63% (-0.24% 🔻)
🔴	`...` / test-utils.tsx	58.82% (-3.34% 🔻)	66.67%	37.5%	57.58% (-3.54% 🔻)
🟢	`...` / SignMessage.tsx	89.89%	82.35% (-0.98% 🔻)	66.67%	90.7%
🔴	`...` / GlobalPushNotifications.tsx	60.42% (-1.35% 🔻)	35.85% (-2.33% 🔻)	55% (-2.14% 🔻)	57.06% (-1.5% 🔻)
🟡	`...` / index.tsx	73.08%	61.54% (-1.42% 🔻)	53.33%	73.74%
🔴	`...` / utils.ts	36.36% (-11.87% 🔻)	26.83% (-10.67% 🔻)	21.43% (-8.57% 🔻)	36.11% (-11.89% 🔻)
🟢	`...` / index.tsx	81% (-1.47% 🔻)	51.16% (-1.34% 🔻)	72.73%	81.91% (-1.6% 🔻)

Test suite run success

1399 tests passing in 190 suites.

Report generated by 🧪jest coverage report action from 90a523c

github-actions · 2024-02-12T20:42:11Z

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page	Size (compressed)
`global`	`1.02 MB` (🟡 +921 B)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Three Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page	Size (compressed)	First Load
`/home`	`41.27 KB` (🟡 +3 B)	1.06 MB
`/new-safe/create`	`31.91 KB` (🟡 +48 B)	1.06 MB
`/settings/setup`	`72.18 KB` (🟡 +108 B)	1.1 MB

Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

katspaugh

We'll have to refactor all those event buses/associated notifications eventually. There are now regular txs, messages, safe creations. Lots of copypasta.

src/features/counterfactual/ActivateAccount.tsx

src/features/counterfactual/services/safeCreationEvents.ts

src/features/counterfactual/utils.ts

src/hooks/usePendingSafeStatuses.ts

src/hooks/usePendingSafeNotifications.ts

src/pages/_app.tsx

src/features/counterfactual/hooks/useIsCounterfactualSafe.ts

src/pages/_app.tsx

# Conflicts: # src/features/counterfactual/utils.ts

francovenica · 2024-02-14T01:49:08Z

The happy path and cleaning state work fine

Question:
Given the statement:
"We are not detecting cancellation and speed-up transactions. This is a general issue that we have since the ethers v6 upgrade so I suggest to ignore it when testing"

I can't test the other 2 scenarios:
The cancel one is not removing the pending state, but given the statement it would be expected

The speeding up one is working in the sense that the safe is being deployed, but the pending state also never goes away, I don't get a "deployed" notification from the safe and I have to hard refresh to have the "undeployed safes" key from the local storage cleared

So the question is. Should I ignore those 2 and test them in the RC?

github-actions · 2024-02-14T10:25:57Z

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}

usame-algan · 2024-02-14T10:26:26Z

So the question is. Should I ignore those 2 and test them in the RC?

I found a fix for this that wasn't documented in ethers v6 but now we should detect cancellations and speed-ups correctly. ~~I also fixed it for the normal safe creation flow since it was broken there as well.~~ Additionally, I increased the timeout from 1 minute to 2 minutes since I think 1 minute is too fast for this kind of case i.e. transaction is not executing so the user first checks why and then chooses speed-up or cancel this would most likely take more than one minute.

I also added one more test case to check that the Create button now has a loading state when in the safe creation flow. Noticed that it can take a second or two to find the next available nonce so its better to have a loading indication there.

francovenica · 2024-02-15T02:12:53Z

The loading status in the create button looks good
The cancelation of the tx works fine. The pending status stops and a notification saying "cancelled" is shown.

Issue:
With speed up I still have issues.
I tested 2 times with different outcomes:
First time: I waited for 5 mins and the pending status never went away. I reloaded the page and the pending status kept going for 5 seconds, stopped and I got this error

Second time: This time the pending status went away quickly, but I still got a different error:

In both cases the tx worked, so the safe was deployed just fine, but my guess is that the system is still waiting for the original tx instead of the new one that is replacing it

github-actions · 2024-02-15T09:04:35Z

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}

usame-algan · 2024-02-15T09:17:06Z

@francovenica I fixed the issue that an error was shown even if there was a speed-up.

There are still two other issues:

Sometimes there is an error shown immediately after Submitting ("Transaction not found"). I suspect this happens more on Sepolia than on other networks because there might be fewer nodes that pick up the transaction from the pool. I don't think we can do much here
Sometimes cancellation/speed-up is not picked up and its stuck in processing until it times out. I am still looking into the cause of this but I would suggest we move on and fix it separately as this looks like an ethers v6 issue to me

francovenica · 2024-02-15T12:40:02Z

I tried twice and it works really well both times. Didn't have more issues

LGTM

fix: Add event bus for counterfactual events

fe1cdda

fix: Dispatch events when executing with first tx

2787771

usame-algan force-pushed the cf-events branch from 05fb6a2 to 2787771 Compare February 12, 2024 20:38

katspaugh reviewed Feb 13, 2024

View reviewed changes

src/pages/_app.tsx Outdated Show resolved Hide resolved

refactor: Move hooks into counterfactual folder

ce91d3a

usame-algan force-pushed the cf-events branch from dc1031b to ce91d3a Compare February 13, 2024 08:35

usame-algan added 2 commits February 13, 2024 09:41

fix: Address feedback

949d517

refactor: Lazy load counterfactual hooks

ef08228

katspaugh reviewed Feb 13, 2024

View reviewed changes

src/features/counterfactual/hooks/useIsCounterfactualSafe.ts Outdated Show resolved Hide resolved

katspaugh reviewed Feb 13, 2024

View reviewed changes

src/pages/_app.tsx Outdated Show resolved Hide resolved

refactor: Rename component

a2f696f

usame-algan requested a review from katspaugh February 13, 2024 16:17

Merge remote-tracking branch 'origin/dev' into cf-events

652bdf3

# Conflicts: # src/features/counterfactual/utils.ts

katspaugh approved these changes Feb 13, 2024

View reviewed changes

fix: Detect replacement transactions, increase timeout to 2 minutes

67e7d46

fix: Disable create button while creation is pending

d88eefd

usame-algan mentioned this pull request Feb 14, 2024

Cancel and retry on safe creation timeout is misleading #1484

Open

fix: Revert change to checkSafeCreationTx

afeb7df

fix: Dont show error if there was already a success

90a523c

usame-algan merged commit f2421e2 into dev Feb 15, 2024
15 checks passed

usame-algan deleted the cf-events branch February 15, 2024 12:46

github-actions bot locked and limited conversation to collaborators Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [Counterfactual] Add event bus and pending state #3242

fix: [Counterfactual] Add event bus and pending state #3242

usame-algan commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

katspaugh left a comment •

edited

francovenica commented Feb 14, 2024

github-actions bot commented Feb 14, 2024 •

edited

usame-algan commented Feb 14, 2024 •

edited

francovenica commented Feb 15, 2024

github-actions bot commented Feb 15, 2024

usame-algan commented Feb 15, 2024

francovenica commented Feb 15, 2024

fix: [Counterfactual] Add event bus and pending state #3242

fix: [Counterfactual] Add event bus and pending state #3242

Conversation

usame-algan commented Feb 12, 2024 • edited

What it solves

How this PR fixes it

How to test it

Screenshots

Checklist

github-actions bot commented Feb 12, 2024 • edited

Branch preview

github-actions bot commented Feb 12, 2024 • edited

ESLint Summary View Full Report

github-actions bot commented Feb 12, 2024 • edited

Coverage report

Test suite run success

github-actions bot commented Feb 12, 2024 • edited

📦 Next.js Bundle Analysis for safe-wallet-web

⚠️ Global Bundle Size Increased

Three Pages Changed Size

katspaugh left a comment • edited

Choose a reason for hiding this comment

francovenica commented Feb 14, 2024

github-actions bot commented Feb 14, 2024 • edited

ESLint Summary View Full Report

usame-algan commented Feb 14, 2024 • edited

francovenica commented Feb 15, 2024

github-actions bot commented Feb 15, 2024

ESLint Summary View Full Report

usame-algan commented Feb 15, 2024

francovenica commented Feb 15, 2024

usame-algan commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

github-actions bot commented Feb 12, 2024 •

edited

katspaugh left a comment •

edited

github-actions bot commented Feb 14, 2024 •

edited

usame-algan commented Feb 14, 2024 •

edited