Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: Replace lock with wagmi #807

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from
Draft

chore: Replace lock with wagmi #807

wants to merge 31 commits into from
+800 −2,583

Conversation

samuveth
Copy link
Contributor

@samuveth samuveth commented Nov 5, 2023 •
edited

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

@samuveth samuveth changed the title chore: Replace Lock with use-wagmi chore: Replace lock with wagmi Nov 5, 2023
samuveth
samuveth commented Nov 5, 2023
View reviewed changes
src/App.vue Outdated Show resolved Hide resolved
samuveth
samuveth commented Nov 5, 2023
View reviewed changes
src/helpers/wagmiConfig.ts Outdated Show resolved Hide resolved
samuveth
samuveth commented Nov 5, 2023
View reviewed changes
package.json Show resolved Hide resolved
@samuveth samuveth marked this pull request as ready for review November 6, 2023 05:33
@bonustrack
Copy link
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.

@samuveth
Copy link
Contributor Author

samuveth commented Nov 10, 2023 •
edited

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

Fixed the modal font

@Sekhmet
Copy link
Member

Sekhmet commented Nov 10, 2023

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

@samuveth
Copy link
Contributor Author

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

@bonustrack
Copy link
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

bonustrack
bonustrack reviewed Nov 22, 2023
View reviewed changes
index.html Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
@Sekhmet Sekhmet mentioned this pull request Nov 22, 2023
Merged
@bonustrack
Copy link
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
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

@samuveth
Copy link
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.

@samuveth
Copy link
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
Copy link
Member

Sekhmet commented Nov 28, 2023

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
Member

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

@bonustrack
Copy link
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

@samuveth
Copy link
Contributor Author

samuveth commented Nov 28, 2023 •
edited

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

@samuveth
Copy link
Contributor Author

samuveth commented Nov 28, 2023 •
edited

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

@samuveth
Copy link
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
Member

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

@samuveth
Copy link
Contributor Author

samuveth commented Nov 28, 2023 •
edited

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

edit: beta not alpha

@Sekhmet
Copy link
Member

Sekhmet commented Nov 29, 2023

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

I can't deploy spaces:
image

Same error when proposing.

@samuveth
Copy link
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

@samuveth samuveth marked this pull request as draft January 16, 2024 14:14
@samuveth
Copy link
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bonustrack bonustrack Awaiting requested review from bonustrack

@Sekhmet Sekhmet Awaiting requested review from Sekhmet

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace lock with wagmi Add back agentx connector Fix wagmi reconnect
3 participants
@samuveth @bonustrack @Sekhmet