Feat: private key signer #3784

katspaugh · 2024-05-31T10:03:50Z

What it solves

An experimental web3-onboard module to allow connecting directly with a private key as a Safe signer.

The idea is that you can have a (non-primary) signer key stored in your password manager/OS keychain and log in with it like with a password.

The private key is then stored in the session storage and it reconnects if the page is reloaded.

adc2f">

github-actions · 2024-05-31T10:04:04Z

Branch preview

✅ Deploy successful!

Website:
https://pk--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook:
https://pk--walletweb.review.5afe.dev/storybook/

github-actions · 2024-05-31T10:04:58Z

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-05-31T10:07:33Z

📦 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`	`1003.74 KB` (🟡 +52.28 KB)

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
`/`	`24.86 KB` (🟡 +1 B)	1 MB
`/licenses`	`4.97 KB` (🟢 -29 B)	1008.71 KB

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-05-31T10:09:38Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	79.18% (-0.26% 🔻)	11555/14593
🔴	Branches	58.3% (-0.11% 🔻)	2791/4787
🟡	Functions	66.18% (-0.54% 🔻)	1851/2797
🟢	Lines	80.5% (-0.25% 🔻)	10416/12939

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / index.ts	16.39%	0%	0%	17.86%
🟢	`...` / pk-popup-store.ts	80%	50%	0%	88.89%
🟡	`...` / session.ts	70%	50%	50%	71.43%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟡	`...` / wallets.ts	65% (-2.57% 🔻)	25% (-3.57% 🔻)	77.78%	68.75% (-1.25% 🔻)

Test suite run success

1444 tests passing in 199 suites.

Report generated by 🧪jest coverage report action from a7f0855

francovenica · 2024-06-03T01:10:07Z

It looks really promising! and it will going to be really useful for automation to be able to switch owners, so test on 2/x safes can be performed

My initial feedback.

Clear the storage when you disconnect. One for safty reasons (hopefully no laptop is stolen ever, but just in case) and second because now when you disconnect and connect again, it takes the same Pk you already introduced, so there is actually no way to connect with a different owner beyond clearing the LS
Switching networks is not possible. (maybe not possible since there is not a wallet to ask for a network changing, but still something to take in account)

github-actions · 2024-06-13T09:32:14Z

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}

katspaugh · 2024-06-13T14:39:35Z

@francovenica fixed both, thank you!

schmanu

If I understand correctly this is meant to mainly be a tool for our e2e tests to use multiple signers.

I think we should make sure that this won't accidentally turned on in prod.

schmanu · 2024-06-14T07:48:26Z

src/hooks/wallets/wallets.ts

@@ -45,6 +46,7 @@ const WALLET_MODULES: { [key in WALLET_KEYS]: (chain: ChainInfo) => WalletInit }
  [WALLET_KEYS.LEDGER]: () => ledgerModule() as WalletInit,
  [WALLET_KEYS.TREZOR]: () => trezorModule({ appUrl: TREZOR_APP_URL, email: TREZOR_EMAIL }) as WalletInit,
  [WALLET_KEYS.KEYSTONE]: () => keystoneModule() as WalletInit,
+  [WALLET_KEYS.PK]: (chain) => pkModule(chain.chainId, chain.rpcUri) as WalletInit,


I think we ensure that this wallet can only be enabled when IS_PRODUCTION is false as it is a tool for our e2e tests.

To enable it, you'd have to go to the production config service, create a PK wallet, and enable it on a chain. Don't think this can be done accidentally.

Edit: actually you have to create a wallet for it to be disabled if not explicitly enabled. The config service is a little bit backwards...

schmanu · 2024-06-14T07:49:24Z

src/services/private-key-module/index.ts

+                const signedMessage = await wallet.signTypedData(params[1].domain, params[1].data, params[1].value)
+                return signedMessage


Nit:

Suggested change

const signedMessage = await wallet.signTypedData(params[1].domain, params[1].data, params[1].value)

return signedMessage

return wallet.signTypedData(params[1].domain, params[1].data, params[1].value)

schmanu

In that case we can merge it, but need to document somewhere not to enable this wallet for new chains / in prod.

katspaugh · 2024-06-14T09:08:18Z

@schmanu I ended up adding a prod check as you suggested because this will allow us to use this wallet on dev even if the CGW toggle is set to prod.

Feat: private key signer

d041305

katspaugh requested a review from schmanu May 31, 2024 10:03

Merge branch 'dev' of github.com:safe-global/safe-wallet-web into pk

b2056a2

katspaugh added 3 commits June 13, 2024 13:20

Replace hdwallet with ethers wallet

e946bc2

Clear storage on disconnect

05b1d4c

Allow switching chain

d1924f9

katspaugh force-pushed the pk branch from 390f554 to d1924f9 Compare June 13, 2024 14:38

katspaugh marked this pull request as ready for review June 13, 2024 14:39

Merge branch 'dev' of github.com:safe-global/safe-wallet-web into pk

b9adb91

schmanu reviewed Jun 14, 2024

View reviewed changes

schmanu approved these changes Jun 14, 2024

View reviewed changes

Replace HDWallet with ethers

dba1235

katspaugh force-pushed the pk branch from e960025 to dba1235 Compare June 14, 2024 09:01

Use PK wallet only in dev/staging

a7f0855

katspaugh merged commit 010d629 into dev Jun 14, 2024
14 checks passed

katspaugh deleted the pk branch June 14, 2024 09:23

github-actions bot locked and limited conversation to collaborators Jun 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: private key signer #3784

Feat: private key signer #3784

katspaugh commented May 31, 2024

github-actions bot commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024

github-actions bot commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

francovenica commented Jun 3, 2024

github-actions bot commented Jun 13, 2024 •

edited

Loading

katspaugh commented Jun 13, 2024

schmanu left a comment

schmanu Jun 14, 2024

katspaugh Jun 14, 2024 •

edited

Loading

schmanu Jun 14, 2024

schmanu left a comment

katspaugh commented Jun 14, 2024 •

edited

Loading

		const signedMessage = await wallet.signTypedData(params[1].domain, params[1].data, params[1].value)
		return signedMessage

	const signedMessage = await wallet.signTypedData(params[1].domain, params[1].data, params[1].value)
	return signedMessage
	return wallet.signTypedData(params[1].domain, params[1].data, params[1].value)

Feat: private key signer #3784

Feat: private key signer #3784

Conversation

katspaugh commented May 31, 2024

What it solves

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

Branch preview

github-actions bot commented May 31, 2024

ESLint Summary View Full Report

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

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

⚠️ Global Bundle Size Increased

Two Pages Changed Size

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

Coverage report

Test suite run success

francovenica commented Jun 3, 2024

github-actions bot commented Jun 13, 2024 • edited Loading

ESLint Summary View Full Report

katspaugh commented Jun 13, 2024

schmanu left a comment

Choose a reason for hiding this comment

schmanu Jun 14, 2024

Choose a reason for hiding this comment

katspaugh Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

schmanu Jun 14, 2024

Choose a reason for hiding this comment

schmanu left a comment

Choose a reason for hiding this comment

katspaugh commented Jun 14, 2024 • edited Loading

github-actions bot commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading

katspaugh Jun 14, 2024 •

edited

Loading

katspaugh commented Jun 14, 2024 •

edited

Loading