integrations/wagmi: initial re-work of wagmi support#292
integrations/wagmi: initial re-work of wagmi support#292
Conversation
✅ Deploy Preview for oasisprotocol-sapphire-paratime canceled.
|
|
So, I'm still running into this problem: This works fine with Node v18.19.0, the CI tests say it's running Node v18.19.1: However, this would indicate that the version of Node being used isn't the v18.19.1 that's reported, as the tests pass locally and Nope, found the problem in workflow Removed this and it works correctly. |
|
There are some remaining problems with packaging, as seen by the following imports from
Because the Secondly, it seems two versions of
After changing the version dependencies in |
|
Solving the The reason Ethers is imported is because It's possible to intercept responses via However, there may be another way, by wrapping the However, if we change `sapphireTransport to: export function sapphireTransport(): Transport {
return (params) => {
const p = wrap(params.chain!.rpcUrls.default.http[0]);
const x = custom(p)(params);
return wrapEIP1193Provider(x);
};
}We end up with an error when trying to retrieve the signer: Which is caused by this backtrace Which means The signer is required as in I think I'm going to defer this change until future, and add it into another ticket. As making this change still includes basically all of Ethers due to BrowserProvider when it's not entirely necessary as we should (ideally) be interacting directly with the EIP-1193 provider if possible? |
CedarMist
force-pushed
the
CedarMist/wagmi-viem
branch
from
bbc84e9 to
195e537
Compare
|
I've discovered a problem, which I had noted before in calldatapublickey.ts / If I switch chains, with the Wagmi example, the calldata public key returned is for the wrong chain if I switch from Localnet to Testnet. I guess we could hook the chain switch event, and clear the calldata key cache. |
|
We need to re-do the documentation to consider both Wagmi, Ethers and Hardhat as all the tutorials seem very very specific. |
| @@ -0,0 +1 @@ | |||
| This is a [Vite](https://vitejs.dev) project bootstrapped with [`create-wagmi`](https://github.com/wevm/wagmi/tree/main/packages/create-wagmi). | |||
There was a problem hiding this comment.
Maybe update this README here? To something like Wagmi example, or just delete the README.
|
One last thing I really should check is hardhat support with wagmi/viem: Wagmi supports hardhat like this: https://wagmi.sh/cli/api/plugins/hardhat Unfortunately I don't have the wagmi package split into wagmi & viem separately, so it will pull in all of the |
|
Currently it doesn't seem possible to wrap an existing client and override its So instead I considered wrapping an arbitrary transport, as such: export function wrapTransport<T extends Transport>(t:T) {
return (params:any) => {
const u = t(params);
if( Reflect.get(u, SAPPHIRE_WRAPPED_VIEM_TRANSPORT) ) {
return u;
}
const p = wrapEIP1193Provider(u);
Reflect.set(p, SAPPHIRE_WRAPPED_VIEM_TRANSPORT, true);
return p;
};
}But that errors with the following due to the user of Ethers BrowserProvider in The only thing that seems to work reliably is separating the transport and the wallet client wrapper, e.g.: walletClient = await wrapWalletClient(createWalletClient({
account: account,
chain: sapphireLocalnet,
transport: sapphireTransport()
}));As I can't seem to override |
There was a problem hiding this comment.
One note, please next time don't create huge PRs, it's a pain to review. I know it's hard, when adding new features, but you could have stacked the PRs on top of each other. Like wagmi & viem part, could have been easily 2 different PRs. Also the example projects, could have been added separately.
clients/js/src/calldatapublickey.ts
Outdated
|
|
||
| while (this.#isBackgroundRunning) { | ||
| await new Promise((resolve) => | ||
| setTimeout(resolve, this.timeoutMilliseconds), |
There was a problem hiding this comment.
You should clear this setTimeout in stopBackground function.
There was a problem hiding this comment.
Good point, I have refactored the timeout logic
clients/js/src/calldatapublickey.ts
Outdated
| const waitMilliseconds = | ||
| this.timeoutMilliseconds - this.timeoutMilliseconds / 10; | ||
|
|
||
| while (this.#isBackgroundRunning) { |
There was a problem hiding this comment.
Not a fan of this, not sure how timeoutMilliseconds is set, especially with 10% magic above. But I would do it with setInterval.
| }; | ||
|
|
||
| // Provide an EIP-1193 compatible endpoint for Ethers providers | ||
| if (!('request' in provider) && 'send' in provider) { |
There was a problem hiding this comment.
Isn't there some kind of utility function for this already. isEip1193Provider which returns the correct typed provider. If not, please create one.
There was a problem hiding this comment.
'request' in provider is shorter and more explicit than isEip1193Provider(provider)
I have a refactor waiting in my head that removes this messiness
| cipher, | ||
| signer as any, | ||
| ); | ||
| const res = await (provider as any).send(method, params ?? []); |
There was a problem hiding this comment.
The above, will also solve this any.
There was a problem hiding this comment.
It doesn't solve this any instance, the send interface is exposed by JsonRpcProvider and BrowserProvider, but this could be from ether the Ethers 5 or Ethers 6 packages.
| accounts: process.env.PRIVATE_KEY | ||
| ? [process.env.PRIVATE_KEY] | ||
| : [], |
| * This environment variable allows for the sapphire-localnet port to be | ||
| * overridden via the command-line. This is useful for debugging with a proxy. | ||
| * | ||
| * Note: this will fail gracefully in-browser |
There was a problem hiding this comment.
How about checking if window exists, and not even execute the code, that depends on Node.js runtime?
There was a problem hiding this comment.
I assumed some people may use a node-process polyfill
Have you tested any of the code while reviewing, or just looking at it? |
Just looking at it. What is your point? The examples could have been branched upon |
The examples are the unit tests |
I would have to disagree, what you are describing are integration tests. |
|
Fuck it |
3 participants
Closes #268
Closes #164
Closes #225
fetchCallDataPublicKey, Node 18+ support has nativefetchso no need to importhttporhttpsdepending on URL.@oasisprotocol/sapphire-paratimepackage to 1.3.3wagmiexamplesapphire-wagmi-v2integration packagesapphire-viem-v2integration packagehardhat-viemexampleCouldn't get Synpress working with the Wagmi example so have to manually verify.