Skip to content
This repository was archived by the owner on Aug 22, 2025. It is now read-only.

chore: Replace lock with wagmi#807

Draft
samwhosung wants to merge 31 commits into
masterfrom
samuv/replace-lock
Draft

chore: Replace lock with wagmi#807
samwhosung wants to merge 31 commits into
masterfrom
samuv/replace-lock

Conversation

@samwhosung

@samwhosung samwhosung commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

Summary

Closes: snapshot-labs/sx-monorepo#125
Closes: #805
Closes: snapshot-labs/sx-monorepo#124

How to test

  1. Try connecting with all connectors, including safe on safe.global
  2. Try various actions like voting

To-Do

@samwhosung samwhosung changed the title chore: Replace Lock with use-wagmi chore: Replace lock with wagmi Nov 5, 2023
Comment thread src/App.vue Outdated
Comment thread src/helpers/wagmiConfig.ts Outdated
Comment thread src/networks/types.ts
Comment thread package.json
Comment thread src/components/VotingPowerIndicator.vue Outdated
@samwhosung samwhosung marked this pull request as ready for review November 6, 2023 05:33
@bonustrack

Copy link
Copy Markdown
Member

In the modals it seem like Calibre font is not working can you reproduce that? Also we will need Starknet connector if we want merge this.

@samwhosung

samwhosung commented Nov 10, 2023

Copy link
Copy Markdown
Contributor Author

@bonustrack you said in DM "I would try integrate first without Argent"?

Fixed the modal font

@Sekhmet

Sekhmet commented Nov 10, 2023

Copy link
Copy Markdown
Member

@samuveth we won't be able to merge this without Starknet support because it will cause 50% of the app to be non-functional.

Comment thread src/components/Modal/Account.vue
Comment thread vite.config.ts
@samwhosung

Copy link
Copy Markdown
Contributor Author

@Sekhmet added back the ArgentX/Starknet connector. Please have a look

@bonustrack

Copy link
Copy Markdown
Member

@samuveth When I open the deployment here: https://bafybeicwfiiggbhy66b3ztq6ed3xkassephuuzcdo3zzmi4jsbhty5yru4.on.fleek.co/#/ it directly try to log me, I see connect wallet button loading. Can you reproduce?

Also:

  • If someone cancel login we shouldn't show the error message "User rejected wallet connection..."
  • I can't see my ENS name fabien.eth when I log in, was this removed?
  • Ideally Starknet connector should be lazy loaded

@samwhosung samwhosung requested a review from Sekhmet November 23, 2023 13:44
@bonustrack

Copy link
Copy Markdown
Member

Publish a proposal, cast a vote, edit settings doesn't work for me it return this error:

index-c9534916.js:1 TypeError: Cannot read properties of null (reading 'provider')

You can use this space to test, it use vanilla (ticket) strategy sep:0x012b261effbf548f2b9a495d50b81a8a7c1dd941. I haven't tested with Starknet.

Support for EIP-6963 seam to work well, I've tried using both MetaMask and Rabbi chrome extension and it works. It would be great if we can support wallet logo or show a default logo instead of always MetaMask, we have a list of all wallet and image here: https://github.com/snapshot-labs/lock/blob/master/src/utils.ts Zerion should be added in the list.

image

@bonustrack

Copy link
Copy Markdown
Member

I see also there is some increase in bundle size, biggest dependency is Safe SDK, is possible to lazy load it?
https://bafybeigiabs43nayawrgpogf4dohcvgp3ujc53siaote5tfofdwtxkepua.on.fleek.co/stats.html 5.86mb
https://testnet.snapshotx.xyz/stats.html 4.64mb

@samwhosung

Copy link
Copy Markdown
Contributor Author

I see also there is some increase in bundle size, biggest dependency is Safe SDK, is possible to lazy load it? https://bafybeigiabs43nayawrgpogf4dohcvgp3ujc53siaote5tfofdwtxkepua.on.fleek.co/stats.html 5.86mb https://testnet.snapshotx.xyz/stats.html 4.64mb

Weird I see 8.43mb and 8.92mb.

Don't see any way to lazy load it, also the SDK is used to detect the safe wallet which is needed to hide/show the connector and auto-connect when used as a safe app.

@samwhosung

Copy link
Copy Markdown
Contributor Author

Made a fix and tested with Metamask, WC, Coinbase. Also tried with ArgentX and got weird network error that isn't related I think, @Sekhmet can you help test argent?

@Sekhmet

Sekhmet commented Nov 28, 2023

Copy link
Copy Markdown
Member

Made a fix and tested with Metamask, WC, Coinbase. Also tried with ArgentX and got weird network error that isn't related I think, @Sekhmet can you help test argent?

Can you rebase on latest master as on this branch we updated contracts and fixed Starknet related issue with sequencer being deprecated? This alone might fix it.

@bonustrack

Copy link
Copy Markdown
Member

Can we also remove loading on connect wallet when we open the app?

@bonustrack

Copy link
Copy Markdown
Member

I see also there is a lot of JS file being loaded by default compared to current version of SX:

With your PR:
image

Without:
image

Do you know what are these? Maybe something is wrong with lazy loading

@samwhosung

samwhosung commented Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

Can we also remove loading on connect wallet when we open the app?

It's connected to isConnecting. I suggest we do like on hey.xyz, we remove the loading from "Connect wallet" button completely and do loading in the modal when connecting and close modal after it's connected. We also won't need the loading for reconnect anymore because wagmi.cache will help instantly reconnect without delay

Like this:
image

@samwhosung

samwhosung commented Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

Do you know what are these? Maybe something is wrong with lazy loading

This is because wagmi calls isAuthorized on each connector which loads the provider. Currently there is no way to lazy load this part, see discussions here https://github.com/search?q=repo%3Awevm%2Fwagmi+lazy&type=discussions

@samwhosung

Copy link
Copy Markdown
Contributor Author

We can reduce the bundle size by replacing etherjs with viem, which is smaller and is used by wagmi already.

@bonustrack

Copy link
Copy Markdown
Member

Hum this is problematic if providers aren't lazy loaded.

@samwhosung

samwhosung commented Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

Seems to be possible in v2, which isn't out of alpha yet I think.

edit: beta not alpha

@Sekhmet

Sekhmet commented Nov 29, 2023

Copy link
Copy Markdown
Member

I can sometimes connect with ArgentX, but on refresh it throws:
image

I can't deploy spaces:
image

Same error when proposing.

@samwhosung

Copy link
Copy Markdown
Contributor Author

I'm currently talking to wagmi if we can fix lazy loading connectors with v1 or v2. Putting this PR on standby for now

@samwhosung samwhosung marked this pull request as draft January 16, 2024 14:14
@samwhosung

Copy link
Copy Markdown
Contributor Author

Seems like use-wagmi updated to wagmi-v2 three weeks ago unicape/use-wagmi@73daf26

I'll still be busy with boost but could pick this up afterwards

@bonustrack bonustrack added bug Something isn't working dependencies Pull requests that update a dependency file and removed bug Something isn't working dependencies Pull requests that update a dependency file labels Aug 14, 2024
@bonustrack bonustrack added bug Something isn't working and removed bug Something isn't working labels Oct 5, 2024
@bonustrack bonustrack added bug Something isn't working dependencies Pull requests that update a dependency file labels Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace lock with wagmi Add back agentx connector Fix wagmi reconnect

3 participants