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.

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

feat(examples): upgrade wagmi to v2 and implement viem custom client within example #2307

Conversation

tash-2s
Copy link
Contributor

@tash-2s tash-2s commented Feb 23, 2024

This PR is created with the exact same commits as #2294, but now the source branch is within this latticexyz/mud repository.


This PR targets #2306, which implements external wallet support using Wagmi v1 in the example project. In this PR, I've updated the code to use Wagmi v2 and the viem custom client. This code doesn't use the mud_getContract wrapper, allowing the viem client to be used more straightforwardly. As a result, the structure of the React components has been improved as well.

```
 WARN  Issues with peer dependencies found
packages/client-react-external-wallet
└─┬ wagmi 2.5.7
  └─┬ @wagmi/connectors 4.1.14
    └─┬ @metamask/sdk 0.14.3
      ├─┬ @metamask/sdk-communication-layer 0.14.3
      │ └─┬ socket.io-client 4.7.4
      │   └─┬ engine.io-client 6.5.3
      │     └─┬ ws 8.11.0
      │       └── ✕ unmet peer utf-8-validate@^5.0.2: found 6.0.3 in @metamask/sdk-communication-layer
      ├─┬ @metamask/sdk-install-modal-web 0.14.1
      │ └─┬ react-i18next 13.5.0
      │   └── ✕ unmet peer i18next@">= 23.2.3": found 22.5.1 in @metamask/sdk-install-modal-web
      └─┬ react-i18next 13.5.0
        └── ✕ unmet peer i18next@">= 23.2.3": found 22.5.1 in @metamask/sdk
```
Copy link

changeset-bot bot commented Feb 23, 2024

⚠️ No Changeset found

Latest commit: 583ae77

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@tash-2s tash-2s changed the title WIP: viem custom client within external wallet example feat(examples): upgrade wagmi to v2 and implement viem custom client within example Feb 25, 2024
@@ -14,12 +14,14 @@
"@latticexyz/common": "link:../../../../packages/common",
"@latticexyz/dev-tools": "link:../../../../packages/dev-tools",
"@latticexyz/store-sync": "link:../../../../packages/store-sync",
"@tanstack/react-query": "5.22.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wagmi v2 needs this @tanstack/react-query installed alongside.

"contracts": "workspace:*",
"p-retry": "^5.1.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To adapt the transaction sending extension in this example, p-retry, which is utilized in the common library, has been installed here. If the direction of this implementation proves to be suitable, I plan to move the custom client implementation to the common library later. Then, I can remove p-retry from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are to adapt Wagmi v2 API changes.

Comment on lines -15 to +27
setup().then(({ mud, wagmiConfig }) => {
setup().then(({ network, wagmiConfig }) => {
root.render(
<WagmiConfig config={wagmiConfig}>
<ExternalWallet />
<MUDReadProvider value={mud}>
<MUDWriteProvider>
<WagmiProvider config={wagmiConfig}>
<QueryClientProvider client={queryClient}>
<ExternalWallet />
<MUDNetworkProvider value={network}>
<App />
{import.meta.env.DEV && <DevTools />}
</MUDWriteProvider>
</MUDReadProvider>
</WagmiConfig>
</MUDNetworkProvider>
</QueryClientProvider>
</WagmiProvider>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The write context has been removed since we don't have states to hold for that.

Comment on lines +16 to +17
// `walletClient = connectorWalletClient.extend(burnerActions);` is unnecessary for an external wallet
walletClient = connectorWalletClient.extend(setupObserverActions(network.onWrite));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the code for creating the custom client.
I've realized that nonce management and retry functionality are unnecessary (though they work) for external wallets. This is because MetaMask ignores the nonce value, and the retry behavior might not be desirable for external wallets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're still going to ~always have a session wallet as the primary sender of txs (to have ~invisible tx signing) so may make sense to just keep this here.

Comment on lines +26 to +31
const burnerActions = (client: WalletClient): Pick<WalletActions<Chain, Account>, "sendTransaction"> => {
// TODO: Use the `debug` library once this function has been moved to the `common` library.
const debug = console.log;

return {
sendTransaction: async (args) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functions similarly to mud_writeContract and mud_sendTransaction through the viem client interactions, with the exception of transaction simulation. This approach is consistent with the standard viem action behavior, which requires users to initiate the simulation separately when needed.

@tash-2s tash-2s marked this pull request as ready for review February 25, 2024 22:01
@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 25, 2024

This PR is ready for review! 👐 @holic Could you review this when you have time?

Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a bunch going on here that we can just upstream into mud directly as improvements that we know we need to make, which will help to simplify the diff of this PR

Comment on lines +16 to +17
// `walletClient = connectorWalletClient.extend(burnerActions);` is unnecessary for an external wallet
walletClient = connectorWalletClient.extend(setupObserverActions(network.onWrite));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're still going to ~always have a session wallet as the primary sender of txs (to have ~invisible tx signing) so may make sense to just keep this here.


const nonce = nonceManager.nextNonce();
debug("sending tx with nonce", nonce, "to", args.to);
return sendTransaction(client, { ...args, nonce } as typeof args);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should prob use getAction('sendTransaction') here instead of importing from viem, in case the client has overridden this method already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

session wallet

Yes, we should use all custom actions (nonce, retry, write$) for session wallets.

In the upcoming burner wallet PR #2309, all these actions are applied to the burner wallet. When using both external and burner wallets, I believe the external wallet doesn't need to be extended. This is because, we don't require nonce management for MetaMask, and dev-tools, which takes a single wallet client, should be used with the burner wallet.


getAction

Right, using getAction is better since it accounts for more extensions beforehand. I'll switch to getAction.

return result;
},
});
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we pull these actions out into a separate, independent PR that adds them to the common package and import them here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do! 💪

tables,
useStore,
publicClient,
latestBlock$,
storedBlockLogs$,
waitForTransaction,
onWrite,
write$: write$.asObservable().pipe(share()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this data flow feels a little weird now, curious if you have ideas for improving this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it weird? 🤔 I think the app requires a single write$ for the dev-tool, which can be used for any wallet clients. Hence, it made sense to me to include this in the setup.

export const supportedChains: MUDChain[] = [
{
...mudFoundry,
// This is not an actual explorer URL; however, Wagmi requires a URL here. Wagmi uses the `blockExplorers` config of this chain to determine the `blockExplorerUrls` value for `wallet_addEthereumChain`. The `blockExplorerUrls` must be either null or an array containing at least one URL; however, Wagmi exclusively employs the array format. Thus, we need to include at least one URL here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we pull this out and fix it in the common package instead of here?

Copy link
Contributor Author

@tash-2s tash-2s Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do in a separate PR!

account: client.account,
});
const tx = await client.writeContract(request);
network.waitForTransaction(tx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be neat if this was a custom client action too, which means we could simplify the args to potentially just the public client + wallet client

there's a bit of a chicken-and-egg problem though, where functions like syncToZustand require the public client and return waitForTransaction, which we'd need to extend the public client

maybe we just pass in the plain public client to set up the sync then extend it before returning it to the rest of the app?

const publicClient = createPublicClient(...);
const syncResult = syncToZustand({ publicClient, ... }) 

return {
  publicClient: publicClient.extend(syncActions(syncResult)),
  ...
}

thoughts?

Copy link
Contributor Author

@tash-2s tash-2s Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having waitForTransaction as a viem client action makes sense. I'll look into this.

@holic
Copy link
Member

holic commented Aug 15, 2024

closing as we're taking a different approach for onboarding

@holic holic closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants