[Counterfactual] Safe creation #3180

usame-algan · 2024-01-31T09:50:13Z

What it solves

Resolves #3179

How this PR fixes it

Adjust safe creation flow to create counterfactual safes for 1/1 setups
Adds fallbacks for CGW calls so the UI is unblocked
Fetches native balance
Adds feature flag
Uses an incremental saltNonce for all safe deployments

How to test it

Connect your wallet and create a 1/1 safe
Observe no cost on the review screen
Press Create
The dashboard should be immediately visible
There should be no errors
Send ETH to the safe address
Observe it being visible on the assets page
Create another 1/1 safe
Observe the same safe address

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-01-31T09:50:25Z

Branch preview

✅ Deploy successful!

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

github-actions · 2024-01-31T09:53:20Z

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-01-31T09:55:51Z

📦 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` (🟡 +944 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!

Four Pages Changed Size

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

Page	Size (compressed)	First Load
`/balances/nfts`	`20.47 KB` (🟡 +33 B)	1.04 MB
`/home`	`40.81 KB` (🟡 +29 B)	1.06 MB
`/new-safe/create`	`29.93 KB` (🟡 +275 B)	1.05 MB
`/new-safe/load`	`19.14 KB` (🟡 +34 B)	1.04 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.

github-actions · 2024-01-31T09:57:57Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	78.77% (-0.07% 🔻)	11328/14381
🔴	Branches	56.46% (-0.15% 🔻)	2508/4442
🟡	Functions	63.3% (+0.12% 🔼)	1806/2853
🟢	Lines	80.09% (-0.09% 🔻)	10212/12751

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / undeployedSafesSlice.ts	44%	0%	60%	50%
🟡	`...` / utils.ts	73.08%	60%	60%	72.73%
🟢	`...` / utils.ts	100%	100%	100%	100%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / safeCoreSDK.ts	92.16% (-1.72% 🔻)	83.33% (-7.58% 🔻)	100%	93.33% (-2.02% 🔻)
🟢	`...` / useLoadBalances.ts	93.18% (-1.56% 🔻)	66.67% (-8.33% 🔻)	100%	95% (-2.06% 🔻)
🟢	`...` / index.tsx	96.15%	50% (-12.5% 🔻)	83.33%	96%
🔴	`...` / index.tsx	47.87% (-2.13% 🔻)	11.11% (-2.22% 🔻)	10% (+0.91% 🔼)	49.44% (-3.26% 🔻)

Test suite run success

1390 tests passing in 188 suites.

Report generated by 🧪jest coverage report action from ee9496f

katspaugh · 2024-02-01T10:21:46Z

src/components/new-safe/create/steps/ReviewStep/index.tsx

@@ -151,6 +166,41 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
      saltNonce: saltNonce.toString(),
    }

+    if (isCounterfactual) {
+      const counterfactualNonce = await getCounterfactualNonce(provider, { ...props, saltNonce: '0' })


If we're switching to an incremental nonce, I would do it for all deployments, not just counterfactual. For the benefitsts that Micah outlined and for consistency.

It would require us to check on-chain if a contract is already deployed to that address for every safe that is created but I think it would simplify our code so I will look into refactoring this part of safe creation a bit more.

katspaugh · 2024-02-01T10:22:35Z

src/components/new-safe/create/steps/ReviewStep/index.tsx

@@ -133,6 +145,9 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
      ? formatVisualAmount(getTotalFee(maxFeePerGas, maxPriorityFeePerGas, gasLimit), chain?.nativeCurrency.decimals)
      : '> 0.001'

+  // Only 1 out of 1 safe setups are supported for now
+  const isCounterfactual = data.threshold === 1 && data.owners.length === 1 && isCounterfactualEnabled


Please try to extract all the logic into a service and only do UI rendering in components.

github-actions · 2024-02-02T11:50:46Z

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}

src/components/new-safe/create/logic/utils.test.ts

schmanu · 2024-02-06T16:34:03Z

src/components/new-safe/create/steps/ReviewStep/index.tsx

@@ -117,9 +124,9 @@ const ReviewStep = ({ data, onSubmit, onBack, setStep }: StepRenderProps<NewSafe
    return {
      owners: data.owners.map((owner) => owner.address),
      threshold: data.threshold,
-      saltNonce,
+      saltNonce: Date.now(), // Doesn't matter for the gas estimation


Are you sure about that? I thought the gas cost depend also on how many zeros are in the call data?

I double checked:

A byte of calldata costs either 4 gas (if it is zero) or 16 gas (if it is any other value).

Thanks for checking! In that case I will move the saltNonce calculation into a hook so we can reuse it here.

I think with Date.now() we would get slightly too high gas estimations as the current Date likely contains more zeros then the number incremented by one each time.

So I guess we could also leave it for now. And change the comment slightly

I think with Date.now() we would get slightly too high gas estimations as the current Date likely contains more zeros then the number incremented by one each time.

I didn't see this message in time. Just checked and its around a 60 gas difference which I think we can ignore. I reverted the change back and updated the comment instead.

schmanu · 2024-02-06T16:36:38Z

src/components/new-safe/create/logic/utils.ts

+
+export const getAvailableSaltNonce = async (provider: BrowserProvider, props: DeploySafeProps): Promise<string> => {
+  const safeAddress = await computeNewSafeAddress(provider, props)
+  const isContractDeployed = await isSmartContract(provider, safeAddress)


Could we not run into Rate Limit issues here? If someone deployed e.g. 10-20 Safes already?

I haven't had any issues so far with around 10 safes created but we should handle possible error responses from the RPC call. In case this becomes an issue with users we could still optimize this e.g. by not incrementally checking saltNonces but using something like binary search.

If the RPC call throws we can't compute a safeAddress and thus can't create a safe so I would display an error to the user.

Hmm but in that case the user would not be able to create new Safes for that owner right?
If every Safe creation attempts hits the Rate limit, the user will see the error on every attempt. We should experiment if this ever happens by i.e. creating 30 Safes for an owner.

I don't think this will realistically happen. What is the use-case for a wallet to create the exact same safe setup 30 times? I agree though that we should try to limit test it cc. @francovenica

We could also add a fallback to the readonly provider in case the wallet provider fails to increase the chances of success.

src/components/new-safe/create/steps/ReviewStep/index.tsx

schmanu · 2024-02-06T16:41:30Z

src/features/counterfactual/store/undeployedSafeSlice.ts

+const initialState: UndeployedSafeState = {}
+
+export const undeployedSafeSlice = createSlice({
+  name: 'undeployedSafe',


Suggested change

name: 'undeployedSafe',

name: 'undeployedSafes',

Our other slices are named in plural

github-actions · 2024-02-06T17:54:41Z

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}

…e tests

francovenica · 2024-02-07T21:38:39Z

I've checked that the safe is created properly
Transferring tokens to the safe are shown in the assets tab (not in the sidebar, but that's expected for now)
Re-creating the safe gives you the same address. Even clearing the storage creates the same address, which is expected

LGTM

usame-algan requested a review from schmanu January 31, 2024 09:50

katspaugh reviewed Feb 1, 2024

View reviewed changes

usame-algan force-pushed the cf-safe-creation branch from f165f48 to a30a045 Compare February 2, 2024 12:39

safe-global deleted a comment from MissLov3ly Feb 2, 2024

usame-algan force-pushed the cf-safe-creation branch from 9572728 to e4d6fe7 Compare February 2, 2024 14:17

usame-algan requested a review from katspaugh February 2, 2024 14:20

usame-algan force-pushed the cf-safe-creation branch from ca1af16 to 87fed24 Compare February 5, 2024 14:33

schmanu reviewed Feb 6, 2024

View reviewed changes

usame-algan requested a review from schmanu February 6, 2024 18:14

usame-algan added 10 commits February 6, 2024 21:04

feat: Create counterfactual 1/1 safes

4be39b1

fix: Add feature flag

db2b7f0

fix: Lint issues

f9d008d

fix: Use incremental saltNonce for all safe creations

93b1229

fix: Replace useCounterfactualBalance hook with get function and writ…

d84138c

…e tests

refactor: Move creation logic out of Review component

e26baf1

fix: useLoadBalance check for undefined value

fdf49f3

fix: Extract saltNonce, safeAddress calculation into a hook

300f4fd

refactor: Rename redux slice

f49d3c7

fix: Show error message in case saltNonce can't be retrieved

e040a48

usame-algan force-pushed the cf-safe-creation branch from b5e32f6 to e040a48 Compare February 6, 2024 20:04

usame-algan mentioned this pull request Feb 7, 2024

[Counterfactual] Add deploy safe flow #3199

Merged

3 tasks

fix: Disable create button if deploy props are loading

7b3b385

usame-algan force-pushed the cf-safe-creation branch from 8481c7a to 7b3b385 Compare February 7, 2024 08:32

usame-algan force-pushed the cf-safe-creation branch from dd67f1d to 095c8b6 Compare February 7, 2024 09:22

fix: Revert hook change and update comment

ee9496f

usame-algan force-pushed the cf-safe-creation branch from 095c8b6 to ee9496f Compare February 7, 2024 09:23

schmanu approved these changes Feb 7, 2024

View reviewed changes

usame-algan mentioned this pull request Feb 7, 2024

[Counterfactual] Add first steps widget #3218

Merged

3 tasks

usame-algan merged commit 74ed3ce into dev Feb 8, 2024
15 checks passed

usame-algan deleted the cf-safe-creation branch February 8, 2024 09:56

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Counterfactual] Safe creation #3180

[Counterfactual] Safe creation #3180

usame-algan commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

katspaugh Feb 1, 2024

usame-algan Feb 2, 2024

katspaugh Feb 1, 2024

github-actions bot commented Feb 2, 2024 •

edited

Loading

schmanu Feb 6, 2024

schmanu Feb 6, 2024

usame-algan Feb 6, 2024

schmanu Feb 6, 2024

usame-algan Feb 6, 2024

usame-algan Feb 7, 2024

schmanu Feb 6, 2024

usame-algan Feb 6, 2024

usame-algan Feb 6, 2024 •

edited

Loading

schmanu Feb 7, 2024

usame-algan Feb 7, 2024 •

edited

Loading

usame-algan Feb 7, 2024

schmanu Feb 6, 2024

usame-algan Feb 6, 2024

github-actions bot commented Feb 6, 2024 •

edited

Loading

francovenica commented Feb 7, 2024

[Counterfactual] Safe creation #3180

[Counterfactual] Safe creation #3180

Conversation

usame-algan commented Jan 31, 2024 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

Checklist

github-actions bot commented Jan 31, 2024 • edited Loading

Branch preview

github-actions bot commented Jan 31, 2024

ESLint Summary View Full Report

github-actions bot commented Jan 31, 2024 • edited Loading

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

⚠️ Global Bundle Size Increased

Four Pages Changed Size

github-actions bot commented Jan 31, 2024 • edited Loading

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 2, 2024 • edited Loading

ESLint Summary View Full Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usame-algan Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usame-algan Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2024 • edited Loading

ESLint Summary View Full Report

francovenica commented Feb 7, 2024

usame-algan commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

usame-algan Feb 6, 2024 •

edited

Loading

usame-algan Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024 •

edited

Loading