[Counterfactual] Show notification when submitting counterfactual tx #3226

usame-algan · 2024-02-08T16:57:01Z

What it solves

Resolves #

How this PR fixes it

Closes the deploy safe tx-flow when submitting the transaction
Shows a notification when submitting the deploy safe transaction
Shows an info screen on the first-tx + deploy flow breaking down the cost of the transaction
Fixes an issue where the counterfactual state was cleared even if the transaction failed
Estimates the actual gas costs of the signed transaction and uses that instead of what useDeployGasLimit returns

How to test it

Create a counterfactual safe
Choose the Activate now option from settings
Submit the transaction
Observe the modal closes
Observe there is a notification
Deploy the safe with the first transaction
Observe there is an info screen visible
Cancel the transaction in your wallet
Observe an error notification shown
The counterfactual state should not be cleared in that case

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

…own costs

github-actions · 2024-02-08T16:57:14Z

Branch preview

✅ Deploy successful!

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

github-actions · 2024-02-08T16:58:18Z

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-08T17:00:49Z

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

Two 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.17 KB` (🟡 +14 B)	1.06 MB
`/settings/setup`	`72.04 KB` (🟡 +102 B)	1.09 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-02-08T17:04:23Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	78.87% (+0.02% 🔼)	11479/14555
🔴	Branches	56.69% (+0.14% 🔼)	2556/4509
🟡	Functions	63.58% (+0.03% 🔼)	1824/2869
🟢	Lines	80.18% (+0.03% 🔼)	10355/12915

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / utils.ts	46.34% (-0.23% 🔻)	21.05% (-9.72% 🔻)	30%	47.3% (+1.03% 🔼)

Test suite run success

1398 tests passing in 189 suites.

Report generated by 🧪jest coverage report action from d83ce80

katspaugh · 2024-02-09T12:30:03Z

src/features/counterfactual/CounterfactualForm.tsx

+            )}{' '}
+            {chain?.nativeCurrency.symbol}
+          </strong>
+          . This is an estimation and the final cost can be slightly higher.


Suggested change

. This is an estimation and the final cost can be slightly higher.

. This is an estimation and the final cost can be higher.

katspaugh · 2024-02-09T12:30:28Z

src/features/counterfactual/CounterfactualForm.tsx

        <Divider className={commonCss.nestedDivider} sx={{ pt: 3 }} />
-


Waren gut, diese.

This also happened another time but I can't figure out how they are deleted. Afaik we don't have any lint or prettier rules to remove these...

katspaugh · 2024-02-09T12:31:22Z

src/features/counterfactual/utils.ts

+      variant: 'info',
+      groupKey: 'cf-activate-account',
+      message: 'Transaction submitted',
+      detailedMessage: 'Your Safe Account will be deployed on-chain after the transaction has been executed.',


Suggested change

detailedMessage: 'Your Safe Account will be deployed on-chain after the transaction has been executed.',

detailedMessage: 'Your Safe Account will be deployed onchain after the transaction is executed.',

src/features/counterfactual/CounterfactualForm.tsx

schmanu · 2024-02-09T15:28:50Z

src/features/counterfactual/utils.ts

@@ -65,8 +67,11 @@ export const dispatchTxExecutionAndDeploySafe = async (

    const deploymentTx = await sdkUnchecked.wrapSafeTransactionIntoDeploymentBatch(signedTx, txOptions)

+    // We need to estimate the actual gasLimit after the user has signed since it is more accurate than what useDeployGasLimit returns
+    const gas = await signer.estimateGas({ data: deploymentTx.data, value: deploymentTx.value, to: deploymentTx.to })


Should we compare this to the estimation and give a prompt / warning if the difference is more than e.g. 50k gas?

I like the idea but this would only happen after the user has pressed Sign in their wallet. I am not sure how helpful it would be to see a warning somewhere in our UI at that point so I would leave it out for now and hope there are no substantial differences between those first transactions. Worst case we have the info text to warn the user about higher cost on execution.

src/features/counterfactual/hooks/useDeployGasLimit.ts

github-actions · 2024-02-09T16:39:36Z

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}

francovenica · 2024-02-10T03:15:20Z

Issue/Question:
1 - In this PR I cannot create a tx. I tried send funds and adding owners, but they always fail. I tried increasing the gas fee, but is to no avail.
Are regular tx working? I always get the same error. I want to verify the kind of notifications I see when I execute a send funds tx but currently I can't.

usame-algan · 2024-02-10T09:14:53Z

In this PR I cannot create a tx. I tried send funds and adding owners, but they always fail. I tried increasing the gas fee, but is to no avail.

There was a small issue with our last commit but I just pushed a small fix. It should work again now.

francovenica · 2024-02-12T01:06:50Z

I can execute tx like adding/swapping owners.

Some feedback (we can discuss what is possible to change or not)

When you execute the "Activate safe" tx, you get this notification, which is fine, but you have to click in the details to see the text and also in the list of notifications you cannot see the messege, since there is no "details" link to click, you only see a "transaction submitted" which it can be anything.
(This one is fine, but you got to click on details, a minor feedback)

This is the same tx, but saying just "tx submitted" doesn't describe that this was the tx that deployed the safe

Once the tx that deploys the safe is finally processed and fully executed, I'd expecte a green card notification saying "success" or something. Right now no notification shows up once the safe is finally deployed
When I execute a tx like swap owner. I only get only the notificaton of "check in blockexplorer" which is the one deploying the safe, but there is no message implying this for the user. I'd expect here for the notification I showed earlier to show up (The "your safe account will be deployed...." notification)

usame-algan · 2024-02-12T08:38:08Z

When you execute the "Activate safe" tx, you get this notification, which is fine, but you have to click in the details to see the text and also in the list of notifications you cannot see the messege

I adjusted the wording to be more specific

Once the tx that deploys the safe is finally processed and fully executed, I'd expecte a green card notification saying "success" or something. Right now no notification shows up once the safe is finally deployed

This didn't work because we usually show the green "Success" notification when we detect the transaction in the history which we can't for these transactions because they don't have a txId. I changed it to show the success event instead of the processed event when the transaction is successfully executed and when we clear the counterfactual state.

liliya-soroka · 2024-02-12T10:46:22Z

@usame-algan , I don't see success notification after the safe was activated :
activate safe using owner wallet

usame-algan · 2024-02-12T12:09:16Z

I don't see success notification after the safe was activated :

Thanks, I added a success notification there as well.

liliya-soroka · 2024-02-12T13:14:40Z

Verified

feat: Show notification when submitting counterfactual tx and break d…

8dabae4

…own costs

usame-algan requested a review from katspaugh February 8, 2024 16:57

usame-algan changed the title ~~[Counterfactual] Show notification when submitting counterfactual tx and break d…~~ [Counterfactual] Show notification when submitting counterfactual tx Feb 8, 2024

usame-algan requested a review from schmanu February 9, 2024 10:19

usame-algan added 3 commits February 9, 2024 11:42

fix: Use actual gas estimation for execution

7547899

fix: Adjust wording

d0d3687

Merge remote-tracking branch 'origin/dev' into cf-deployment-info

469ced3

usame-algan force-pushed the cf-deployment-info branch from 63dfdf2 to 469ced3 Compare February 9, 2024 11:32

katspaugh reviewed Feb 9, 2024

View reviewed changes

katspaugh approved these changes Feb 9, 2024

View reviewed changes

fix: Adjust wording

9842d05

schmanu reviewed Feb 9, 2024

View reviewed changes

fix: Adjust wording and write tests for useDeployGasLimit

edff9a7

fix: spread deploymentTx so that signing works again

7a458ff

fix: Dispatch success event, adjust notification wording

00c87d2

usame-algan force-pushed the cf-deployment-info branch from 6dd0239 to 00c87d2 Compare February 12, 2024 09:07

fix: Show success notification when deploying safe separately

9629b22

Merge remote-tracking branch 'origin/dev' into cf-deployment-info

d83ce80

usame-algan merged commit 32747ba into dev Feb 12, 2024
14 of 15 checks passed

usame-algan deleted the cf-deployment-info branch February 12, 2024 13:15

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Counterfactual] Show notification when submitting counterfactual tx #3226

[Counterfactual] Show notification when submitting counterfactual tx #3226

usame-algan commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

katspaugh Feb 9, 2024

katspaugh Feb 9, 2024 •

edited

Loading

usame-algan Feb 9, 2024

katspaugh Feb 9, 2024

schmanu Feb 9, 2024

usame-algan Feb 9, 2024

github-actions bot commented Feb 9, 2024 •

edited

Loading

francovenica commented Feb 10, 2024

usame-algan commented Feb 10, 2024

francovenica commented Feb 12, 2024

usame-algan commented Feb 12, 2024 •

edited

Loading

liliya-soroka commented Feb 12, 2024 •

edited

Loading

usame-algan commented Feb 12, 2024

liliya-soroka commented Feb 12, 2024

	. This is an estimation and the final cost can be slightly higher.
	. This is an estimation and the final cost can be higher.

		<Divider className={commonCss.nestedDivider} sx={{ pt: 3 }} />

	detailedMessage: 'Your Safe Account will be deployed on-chain after the transaction has been executed.',
	detailedMessage: 'Your Safe Account will be deployed onchain after the transaction is executed.',

[Counterfactual] Show notification when submitting counterfactual tx #3226

[Counterfactual] Show notification when submitting counterfactual tx #3226

Conversation

usame-algan commented Feb 8, 2024 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

Checklist

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

Branch preview

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

ESLint Summary View Full Report

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

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

⚠️ Global Bundle Size Increased

Two Pages Changed Size

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

Coverage report

Test suite run success

katspaugh Feb 9, 2024

Choose a reason for hiding this comment

katspaugh Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

usame-algan Feb 9, 2024

Choose a reason for hiding this comment

katspaugh Feb 9, 2024

Choose a reason for hiding this comment

schmanu Feb 9, 2024

Choose a reason for hiding this comment

usame-algan Feb 9, 2024

Choose a reason for hiding this comment

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

ESLint Summary View Full Report

francovenica commented Feb 10, 2024

usame-algan commented Feb 10, 2024

francovenica commented Feb 12, 2024

usame-algan commented Feb 12, 2024 • edited Loading

liliya-soroka commented Feb 12, 2024 • edited Loading

usame-algan commented Feb 12, 2024

liliya-soroka commented Feb 12, 2024

usame-algan commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

github-actions bot commented Feb 8, 2024 •

edited

Loading

katspaugh Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited

Loading

usame-algan commented Feb 12, 2024 •

edited

Loading

liliya-soroka commented Feb 12, 2024 •

edited

Loading