search

oasisprotocol / sapphire-paratime

The Sapphire ParaTime monorepo.
https://oasisprotocol.org/sapphire
Apache License 2.0
34 stars 24 forks source link

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

Closed CedarMist closed 4 months ago

CedarMist commented 5 months ago

Closes #268 Closes #164 Closes #225

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

netlify[bot] commented 5 months ago

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
Latest commit 11e75eaf961753fa62379318c7a3d68bc9b9c383
Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/6613d55c2c908f0008bbbe14
CedarMist commented 5 months ago

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 commented 5 months ago

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

image

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

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

CedarMist commented 5 months ago

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 commented 5 months ago

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.

CedarMist commented 4 months ago

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

CedarMist commented 4 months ago

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 commented 4 months ago

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/ethers@6.10.0/node_modules/ethers/src.ts/utils/errors.ts:694:21)
      at BrowserProvider.getRpcError (/src/sapphire-paratime/node_modules/.pnpm/ethers@6.10.0/node_modules/ethers/src.ts/providers/provider-jsonrpc.ts:1049:25)
      at BrowserProvider.getRpcError (/src/sapphire-paratime/node_modules/.pnpm/ethers@6.10.0/node_modules/ethers/src.ts/providers/provider-browser.ts:107:22)
      at BrowserProvider.getSigner (/src/sapphire-paratime/node_modules/.pnpm/ethers@6.10.0/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

CedarMist commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Fuck it