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): add external wallet support example #2268

Closed

Conversation

tash-2s
Copy link
Contributor

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

This React example project uses external wallets instead of burner wallets. It currently employs Wagmi v1, but the same component structure should be usable with Wagmi v2 or other libraries.

The example works as expected, except when multiple wallets are installed, which will be resolved in v2. Users must transfer some balance to external wallet accounts for transaction fees beforehand.

out.mp4

Copy link

changeset-bot bot commented Feb 15, 2024

⚠️ No Changeset found

Latest commit: 09586fd

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 external wallet: react template + wagmi@1 feat(examples): add external wallet support example Feb 16, 2024
@tash-2s tash-2s force-pushed the react-external-wallet-example branch from ad34bd5 to 07d7eaf Compare February 16, 2024 03:38
@tash-2s tash-2s marked this pull request as ready for review February 16, 2024 03:38
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC why is this example app set up differently from the others? (setup.tsx, calling syncStore in App etc.)

I would expect /mud to be mostly unchanged, but maybe the setup has to be different for external wallets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment!

While introducing the top-level WagmiConfig, the distinction between the synchronous network setup and the asynchronous store setup became apparent, which resulted in the diff. The benefit of this PR structure is it doesn't wait for the sync fn for non-relevant code.

Perhaps I should retain the setup().then(async (result) => { root.render( ... code?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it needs to follow the other examples exactly (though I defer to @holic for that), but it would be interesting to explain the set up maybe in the PR description 🙌

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 16, 2024

In the existing templates, there is a certainty that wallets (burner wallets) are present and do not change throughout the app, at the setup stage. As a result, all underlying components have access to the fixed wallets and transaction interfaces. However, with an external wallet setup, the wallet is not fixed (or empty), and it can change. Also, we don't want to block the UI as much as possible for users who do not have wallets. This contributes to the difference in structure.

Another solution might be preparing a burner wallet at first anyway and then later replacing it with an injected wallet.

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.

Another solution might be preparing a burner wallet at first anyway and then later replacing it with an injected wallet.

I don't love this, especially on real chains where the burner wallet would be empty and not able to pay gas anyway.

I think we're better off changing the overall structure to get folks thinking more about only doing writes when a wallet is connected.

I have a bunch of comments related to this but realizing this is quite a big lift. Maybe we should sketch out some designs for our ideal React component tree and render flow?


useEffect(() => {
syncStore(networkConfig, publicClient).then(setStore);
}, []);
Copy link
Member

Choose a reason for hiding this comment

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

Should this useEffect have networkConfig and publicClient as dependencies so it can be reevaluated if the e.g. wallet changes?

Is there a way we can do this without the useEffect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I should take into account the config and client for this.

});

return { worldContract, write$ };
};
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could separate the public/wallet clients some more, so that the public client doesn't depend on the wallet (since it's just used to read from RPC).

It would involve some rethinking this worldContract approach, maybe separating it into two: readWorldContract and writeWorldContract or something. I don't love this though.

The main reason we even use this contract instance abstraction is so that we can overwrite some internal behavior, like adding better tx queuing and nonce handling.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can achieve the same goals with a custom viem client that overrides these methods and pass that custom client through to wagmi, etc.

Would need to make sure that calling various viem actions or wagmi hooks would lean on the custom client behavior rather than other viem internals

)}
</>
);
};
Copy link
Member

Choose a reason for hiding this comment

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

The way these various parts are separated is a little hard for me to grok. I wonder if there are other approaches that would make this easier to understand.

One pattern we've used in some apps is a loading screen that is shown while we're syncing data from the chain or indexer. We could use that here while waiting for a public client.

And like mentioned above, we could separate the things that depend on the wallet client so it doesn't block showing the UI, etc.

useEffect(() => {
if (import.meta.env.DEV) {
import("@latticexyz/dev-tools").then(({ mount }) =>
mount({
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably have to rethink how Dev Tools mounts if the wallet client is mutable now.

You can see this in your demo video, where the Dev Tools address no longer matches the connected address:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't notice that issue, but you're right. I'll look into it.

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 16, 2024

@holic Thank you so much for your comment! I'll create a design sketch to reevaluate the structure. I'll also reconsider how we manage the transaction interface.

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 16, 2024

EDIT: This comment is now outdated and can be ignored.


As you both mentioned, the React structure can be much simpler. Should be something like this:

networkConfig = getNetworkConfig()
wagmiConfig = getWagmiConfig()
syncStorePromise = syncStore(networkConfig, wagmiConfig) // not awaited

<WagmiConfigProvider config={wagmiConfig}> // and rainbowkit
  <MUDProvider value={{networkConfig, syncStorePromise}}>
    <App> and its children can:
      useAccount() // from WagmiConfigProvider
      useNetworkConfig() // from MUDProvider
      useStore() // (Store | null) from MUDProvider
      ...

I'll look into the viem's "getContract" / sending transaction matter. (custom client)


EDIT: The above store doesn't support the network related changes (e.g., chain, world). Should be something like this:

networkConfig = getNetworkConfig()
wagmiConfig = getWagmiConfig()

<WagmiConfigProvider config={wagmiConfig}> // and rainbowkit
  <NetworkConfigProvider config={networkConfig}>
    <Store>
      wagmiConfig = useWagmiConfig()
      networkConfig = useNetworkConfig()
      [store, setStore] = useState(null)
      useEffect(() => {
        syncStore(networkConfig, wagmiConfig).then(setStore)
        return () => setStore(null)
      }, [networkConfig.something, wagmiConfig.something])

      <StoreProvider store={store}>
        <App> and its children can:
          useAccount() // from WagmiConfigProvider
          useNetworkConfig() // from NetworkConfigProvider
          useStore() // (Store | null) from StoreProvider 
          ...

> React Hook "useEffect" is called conditionally. React Hooks must be called in the exact same order in every component render
@tash-2s tash-2s mentioned this pull request Feb 19, 2024
@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 19, 2024

I've updated the React structure as follows:

<WagmiConfig config={wagmiConfig}> // manages external wallet
  <ExternalWallet /> // displays the external wallet UI
  <MUDReadProvider value={mud}> // manages read and immutable mud states (chain, world, publicClient, syncToZustand's result, etc.)
    <MUDWriteProvider> // manages write mud state (transaction interface, etc.)
      <App /> // and its children:
        mudRead = useMUDRead() // can be used similarly to the former `useMUD().network`
        mudWrite = useMUDWrite() // can be used similarly to the former `useMUD().systemCalls`

Also, the following issues have been addressed:

  • The getContract method of MUD is now correctly used. The returned worldContract is managed for each wallet account. (When an active account changes, the worldContract is regenerated using the new account.)
  • dev-tools now accurately displays an active wallet account. It re-mounts when the account changes.

Other considerations:

  • I've reintroduced the top-level setup().then(... code. That change was not directly relevant to this PR.
  • This setup presumes that the chain and world are fixed and immutable, similar to other templates. Therefore, the publicClient and syncToZustand are also fixed on setup.

I've not yet explored viem v2/custom client and am still using getContract. But, the current structure should be applicable to the v2 setup as well since App and its children use the abstracted hooks instead of directly using with wagmi hooks.

I believe I've addressed all comments except for those regarding the v2/custom client. Could you please take a look? @holic

@holic
Copy link
Member

holic commented Feb 21, 2024

Sketching out my thoughts:

If we move to a viem custom client, we can remove the need for getContract wrapper and threading through the worldContract. I think this means we wouldn't need a "MUD write context" anymore, and system calls can just be pure functions that take in the viem wallet client as an argument.

// extend viem client with our own tx queue with nonce handling (like `getContract` does today)
const client = createWalletClient({ ... }).extend(transactionQueue());

await callSomeSystem(client, ...);

Ultimately trying to move away from "bound" functions and objects into a more functional approach where we pass in the args, which means we can lean on more libs out of the box (i.e. viem/wagmi actions) instead of wrappers and threading data through in complex data flows.

Then we could keep the "MUD read context" focused more on what it's actually doing: setting up the sync stack to hydrate the client with data you can query from.

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 21, 2024

@holic I agree with your points!

Yes, by having the custom wallet client, we can remove the getContract wrappers.

const mudWalletActions = (client) => ({sendTransaction: (...) => {...}})
const customWalletClient = walletClient.extend(mudWalletActions)

Since Wagmi hooks don't retain the customWalletClient, we call the extend function after obtaining the client from Wagmi whenever necessary. We don't need to store it somewhere, thus making the MUD write context unnecessary.

Inside the callSomeSystem(customWalletClient, ...) function, we would use viem_writeContract or viem_getContract to write to the world.

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 23, 2024

I've merged main and now have viem v2 (#2284) in this branch.

@tash-2s
Copy link
Contributor Author

tash-2s commented Feb 23, 2024

Closing this in favor of #2306

@tash-2s tash-2s closed this Feb 23, 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.

3 participants