integrations/wagmi: initial re-work of wagmi support #292

CedarMist · 2024-03-22T23:58:33Z

Closes #268
Closes #164
Closes #225

Simplifies fetchCallDataPublicKey, Node 18+ support has native fetch so no need to import http or https depending on URL.
Bumps @oasisprotocol/sapphire-paratime package to 1.3.3
Adds 'background mode' for calldata public key fetcher, so it can work with synchronous code
Adds wagmi example
Adds sapphire-wagmi-v2 integration package
Adds sapphire-viem-v2 integration package
Adds hardhat-viem example
Adds 'ensure everything is encrypted' proxy

Couldn't get Synpress working with the Wagmi example so have to manually verify.

netlify · 2024-03-22T23:58:50Z

✅ Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name	Link
🔨 Latest commit	`11e75ea`
🔍 Latest deploy log	https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/6613d55c2c908f0008bbbe14

CedarMist · 2024-03-23T16:52:47Z

So, I'm still running into this problem:


PASS test/signed_calls.spec.ts (5.026 s)
PASS test/cipher.spec.ts (5.593 s)
FAIL test/compat.spec.ts (5.674 s)
  ● fetchRuntimePublicKey › non public key provider

    ReferenceError: fetch is not defined

      94 |       `Unable to fetch runtime public key for network with unknown ID: ${chainId}.`,
      95 |     );
    > 96 |   const fetchImpl = (opts?.fetch ?? globalThis?.fetch) ?? fetch;
         |                                                           ^
      97 |   if (!fetchImpl) {
      98 |     throw new Error('No fetch implementation found!');
      99 |   }

This works fine with Node v18.19.0, the CI tests say it's running Node v18.19.1:

Run actions/setup-node@v4
  with:
    node-version: 18
    always-auth: false
    check-latest: false
    token: ***
  env:
    OASIS_UNSAFE_SKIP_AVR_VERIFY: 1
    OASIS_UNSAFE_KM_POLICY_KEYS: 1
    OASIS_UNSAFE_ALLOW_DEBUG_ENCLAVES: 1
Found in cache @ /opt/hostedtoolcache/node/18.19.1/x64
Environment details
  node: v18.19.1
  npm: 10.2.4
  yarn: 1.22.22

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 fetch is defined.

Nope, found the problem in workflow ci-test.yaml

NODE_OPTIONS: "--no-experimental-fetch" # https://github.com/nock/nock/issues/2397

Removed this and it works correctly.

CedarMist · 2024-03-23T17:31:07Z

There are some remaining problems with packaging, as seen by the following imports from examples/wagmi.

Because the sapphireTransport function uses sapphire.wrap(url) it imports most of Ethers v6.

Secondly, it seems two versions of viem and @wagmi/core are being imported by examples/wagmi

@[email protected]_@[email protected][email protected][email protected][email protected]/node_modules/@wagmi/core/dist/esm
@[email protected][email protected][email protected][email protected]/node_modules/@wagmi/core/dist/esm
[email protected][email protected]/node_modules/viem/_esm
[email protected][email protected]/node_modules/viem/_esm

After changing the version dependencies in examples/wagmi/package.json has fixed this, so the peer dependencies work fine.

CedarMist · 2024-03-23T17:48:48Z

Solving the imports all of Ethers v6 problem will reduce the bundle size by up to 170kb.

The reason Ethers is imported is because sapphire.wrap('https://gateway...') wraps an Ethers v6 JsonRpcApiProvider, which is only needed because the Viem Transport requires an EIP-1193 compatible connector and there's no way to directly hook the http transport.

It's possible to intercept responses via onFetchResponse: https://github.com/wevm/viem/blob/5a015273a8868efb82640005c0bb18acfa134c1b/src/clients/transports/http.ts#L42

However, there may be another way, by wrapping the .request method returned by createTransport which invokes buildRequest, which we can then treat as an EIP-1193 provider which should avoid all of the Ethers imports.

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:

{
    "jsonrpc": "2.0",
    "id": 49,
    "error": {
        "code": -32601,
        "message": "the method eth_requestAccounts does not exist/is not available"
    }
}

Which is caused by this backtrace

	await in send (async)		
(anonymous)	@	compat.ts:267
fulfilled	@	compat.js:4
Promise.then (async)		
step	@	compat.js:6
fulfilled	@	compat.js:4
Promise.then (async)		
step	@	compat.js:6
(anonymous)	@	compat.js:7
__awaiter	@	compat.js:3
hooks.request	@	compat.ts:260
await in getSigner (async)		
(anonymous)	@	compat.ts:162
(anonymous)	@	compat.js:7
__awaiter	@	compat.js:3
(anonymous)	@	compat.ts:161
Show 30 more frames

Which means hookEIP1193Request is erroring out when calling BrowserProvider.getSigner. So it will be calling eth_requestAccounts every time a request is made which... isn't ideal.

The signer is required as in prepareRequest it will attempt to re-sign submitted transactions (eth_sendRawTransaction) to encrypt them.

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 · 2024-03-23T22:08:35Z

I've discovered a problem, which I had noted before in calldatapublickey.ts / KeyFetcher::fetch

// XXX: if provider switch chain, may return cached key for wrong chain

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.

docs/browser.md

.github/workflows/ci-test.yaml

clients/js/README.md

clients/js/src/compat.ts

docs/browser.md

docs/guide.mdx

examples/wagmi/src/App.tsx

examples/wagmi/src/main.tsx

examples/wagmi/tests/e2e/specs/tests.js

integrations/wagmi/src/index.ts

docs/browser.md

clients/js/scripts/proxy.ts

docs/guide.mdx

examples/wagmi/src/wagmi.ts

examples/wagmi/src/App.tsx

integrations/wagmi/src/index.ts

CedarMist · 2024-04-02T20:14:07Z

We need to re-do the documentation to consider both Wagmi, Ethers and Hardhat as all the tutorials seem very very specific.

lubej

LGTM overall

clients/js/scripts/proxy.ts

lubej · 2024-04-03T02:59:56Z

examples/wagmi-v2/README.md

@@ -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).


Maybe update this README here? To something like Wagmi example, or just delete the README.

integrations/wagmi-v2/src/index.ts

CedarMist · 2024-04-03T04:49:39Z

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
Hardhat supports Wagmi like this: https://www.npmjs.com/package/@nomicfoundation/hardhat-viem

Unfortunately I don't have the wagmi package split into wagmi & viem separately, so it will pull in all of the @wagmi/core dependency, thankfully that doesn't have any dependencies on React.

CedarMist · 2024-04-04T18:50:57Z

Currently it doesn't seem possible to wrap an existing client and override its transport or the transport.request property, as that's getting bound somewhere in a closure.

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 compat.ts to detect whether or not it has a signer attached. However using the sapphireTransport works as expected.

    Error: could not coalesce error (error={ "code": -32601, "payload": { "method": "eth_requestAccounts", "params": [  ] } }, payload={ "method": "eth_requestAccounts", "params": [  ] }, code=UNKNOWN_ERROR, version=6.10.0)
      at makeError (/src/sapphire-paratime/node_modules/.pnpm/[email protected]/node_modules/ethers/src.ts/utils/errors.ts:694:21)
      at BrowserProvider.getRpcError (/src/sapphire-paratime/node_modules/.pnpm/[email protected]/node_modules/ethers/src.ts/providers/provider-jsonrpc.ts:1049:25)
      at BrowserProvider.getRpcError (/src/sapphire-paratime/node_modules/.pnpm/[email protected]/node_modules/ethers/src.ts/providers/provider-browser.ts:107:22)
      at BrowserProvider.getSigner (/src/sapphire-paratime/node_modules/.pnpm/[email protected]/node_modules/ethers/src.ts/providers/provider-browser.ts:136:28)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

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 transport in wrapWalletClient

…example

lubej

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.

lubej · 2024-04-08T04:42:26Z

clients/js/src/calldatapublickey.ts

+
+    while (this.#isBackgroundRunning) {
+      await new Promise((resolve) =>
+        setTimeout(resolve, this.timeoutMilliseconds),


You should clear this setTimeout in stopBackground function.

Good point, I have refactored the timeout logic

lubej · 2024-04-08T04:44:28Z

clients/js/src/calldatapublickey.ts

+    const waitMilliseconds =
+      this.timeoutMilliseconds - this.timeoutMilliseconds / 10;
+
+    while (this.#isBackgroundRunning) {


Not a fan of this, not sure how timeoutMilliseconds is set, especially with 10% magic above. But I would do it with setInterval.

clients/js/src/compat.ts

lubej · 2024-04-08T04:49:00Z

clients/js/src/compat.ts

@@ -247,6 +252,26 @@ export function wrapEthersProvider<P extends Provider | Ethers5Provider>(
    estimateGas: hookEthersCall(provider, 'estimateGas', filled_options),
  };

+  // Provide an EIP-1193 compatible endpoint for Ethers providers
+  if (!('request' in provider) && 'send' in provider) {


Isn't there some kind of utility function for this already. isEip1193Provider which returns the correct typed provider. If not, please create one.

'request' in provider is shorter and more explicit than isEip1193Provider(provider)

I have a refactor waiting in my head that removes this messiness

lubej · 2024-04-08T04:49:26Z

clients/js/src/compat.ts

+        cipher,
+        signer as any,
+      );
+      const res = await (provider as any).send(method, params ?? []);


The above, will also solve this any.

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.

examples/hardhat-viem/contracts/Example.sol

lubej · 2024-04-08T04:51:50Z

examples/hardhat-viem/hardhat.config.ts

+      accounts: process.env.PRIVATE_KEY
+        ? [process.env.PRIVATE_KEY]
+        : [],


This code repeats x3 times.

examples/hardhat-viem/package.json

examples/wagmi-v2/src/App.tsx

lubej · 2024-04-08T05:00:22Z

integrations/viem-v2/src/index.ts

+ * 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


How about checking if window exists, and not even execute the code, that depends on Node.js runtime?

I assumed some people may use a node-process polyfill

Additional changes

CedarMist · 2024-04-08T11:50:25Z

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.

Have you tested any of the code while reviewing, or just looking at it?

lubej · 2024-04-08T14:23:11Z

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.

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 sapphire-wagmi-v2 and sapphire-viem-v2 integrations.

CedarMist · 2024-04-08T14:51:50Z

Just looking at it. What is your point? The examples could have been branched upon sapphire-wagmi-v2 and sapphire-viem-v2 integrations.

The examples are the unit tests

lubej · 2024-04-08T15:01:11Z

Just looking at it. What is your point? The examples could have been branched upon sapphire-wagmi-v2 and sapphire-viem-v2 integrations.

The examples are the unit tests

I would have to disagree, what you are describing are integration tests.

CedarMist · 2024-04-08T15:06:59Z

Fuck it

CedarMist self-assigned this Mar 22, 2024

CedarMist added p:0 Priority: very high js client labels Mar 22, 2024

CedarMist marked this pull request as ready for review March 23, 2024 17:14

CedarMist requested a review from aefhm March 23, 2024 17:14

CedarMist added 8 commits March 23, 2024 21:08

integrations/wagmi: initial re-work of wagmi support

ada5a12

updated pnpm-lock.yaml

2f3c44d

wagmi: added docs & minor cleanup

881c8f6

Try global 'fetch' function if not defined in globalThis

66e41e3

clients/js: removed --no-experimental-fetch from CI tests

3547e56

clients/js: updated docs & changelog

9b67e2d

clients/js: formatting

72ed436

examples/wagmi: added visualizer to output

195e537

CedarMist force-pushed the CedarMist/wagmi-viem branch from bbc84e9 to 195e537 Compare March 23, 2024 21:08

Add sapphire testnet to wagmi example

2155379

CedarMist requested review from matevz, lukaw3d and lubej March 28, 2024 17:17

lubej reviewed Mar 29, 2024

View reviewed changes

docs/browser.md Show resolved Hide resolved

lubej reviewed Mar 29, 2024

View reviewed changes

.github/workflows/ci-test.yaml Show resolved Hide resolved

lubej reviewed Mar 29, 2024

View reviewed changes

matevz requested changes Mar 29, 2024

View reviewed changes

clients/js/scripts/proxy.ts Outdated Show resolved Hide resolved

docs/guide.mdx Show resolved Hide resolved

docs/guide.mdx Outdated Show resolved Hide resolved

examples/wagmi/src/wagmi.ts Outdated Show resolved Hide resolved

examples/wagmi/src/App.tsx Outdated Show resolved Hide resolved

examples: formatting of wagmi example

01f81dd

lubej reviewed Mar 29, 2024

View reviewed changes

integrations/wagmi/src/index.ts Outdated Show resolved Hide resolved

lubej previously approved these changes Apr 3, 2024

View reviewed changes

CedarMist added 4 commits April 4, 2024 13:04

wagmi/viem: added hardhat-viem example and fixed TX serializer

6045522

ci: added wagmi & viem to CI + fixed lint errors

ad207a2

ci: fixes

77fe9c6

clients/js: fixed formatting

5edc5a6

CedarMist added 6 commits April 4, 2024 22:06

viem: CI test to verify encryption

30d0a52

viem: moved viem tests further up CI

4143067

wagmi: lint + CI step reorg

4ca20e2

viem: fixed proxy tests

862aa35

wagmi & viem: updated dependencies so peer types work correctly with …

9f9bfac

…example

wagmi: reorder tests + lint for CI

ee112fd

CedarMist requested review from lubej and matevz April 5, 2024 11:25

wagmi: build supports alpha release now

241d339

lubej reviewed Apr 8, 2024

View reviewed changes

lubej self-requested a review April 8, 2024 05:06

feedback from PR #292

11e75ea

CedarMist closed this Apr 8, 2024

CedarMist added a commit that referenced this pull request Jun 11, 2024

feedback from PR #292

6aef2db

aefhm pushed a commit that referenced this pull request Jul 15, 2024

feedback from PR #292

4220787

aefhm pushed a commit that referenced this pull request Jul 29, 2024

feedback from PR #292

9eefbf4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrations/wagmi: initial re-work of wagmi support #292

integrations/wagmi: initial re-work of wagmi support #292

CedarMist commented Mar 22, 2024 •

edited

Loading

netlify bot commented Mar 22, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Apr 2, 2024

lubej left a comment

lubej Apr 3, 2024

CedarMist commented Apr 3, 2024 •

edited

Loading

CedarMist commented Apr 4, 2024 •

edited

Loading

lubej left a comment •

edited

Loading

lubej Apr 8, 2024

CedarMist Apr 8, 2024

lubej Apr 8, 2024

lubej Apr 8, 2024 •

edited

Loading

CedarMist Apr 8, 2024

lubej Apr 8, 2024

CedarMist Apr 8, 2024

lubej Apr 8, 2024

lubej Apr 8, 2024

CedarMist Apr 8, 2024

CedarMist commented Apr 8, 2024

lubej commented Apr 8, 2024

CedarMist commented Apr 8, 2024

lubej commented Apr 8, 2024

CedarMist commented Apr 8, 2024

		@@ -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).

integrations/wagmi: initial re-work of wagmi support #292

integrations/wagmi: initial re-work of wagmi support #292

Conversation

CedarMist commented Mar 22, 2024 • edited Loading

netlify bot commented Mar 22, 2024 • edited Loading

✅ Deploy Preview for oasisprotocol-sapphire-paratime canceled.

CedarMist commented Mar 23, 2024 • edited Loading

CedarMist commented Mar 23, 2024 • edited Loading

CedarMist commented Mar 23, 2024 • edited Loading

CedarMist commented Mar 23, 2024 • edited Loading

CedarMist commented Apr 2, 2024

lubej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CedarMist commented Apr 3, 2024 • edited Loading

CedarMist commented Apr 4, 2024 • edited Loading

lubej left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubej Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CedarMist commented Apr 8, 2024

lubej commented Apr 8, 2024

CedarMist commented Apr 8, 2024

lubej commented Apr 8, 2024

CedarMist commented Apr 8, 2024

CedarMist commented Mar 22, 2024 •

edited

Loading

netlify bot commented Mar 22, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Mar 23, 2024 •

edited

Loading

CedarMist commented Apr 3, 2024 •

edited

Loading

CedarMist commented Apr 4, 2024 •

edited

Loading

lubej left a comment •

edited

Loading

lubej Apr 8, 2024 •

edited

Loading