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

clients/js: added hook for Ethers JsonRpcApiProvider.getSigner #284

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CedarMist
Copy link
Member

When the user calls .getSigner on a wrapped Ethers provider, it will return an unwrapped signer.

This change returns a wrapped signer.

@CedarMist CedarMist added p:1 Priority: high js client labels Mar 13, 2024
@CedarMist CedarMist self-assigned this Mar 13, 2024
Copy link

netlify bot commented Mar 13, 2024

Deploy Preview for oasisprotocol-sapphire-paratime canceled.

Name Link
🔨 Latest commit 6b5f46a
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/65f16db2b26c52000892c8a0

Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Wondering if the change is the right thing to do. It seems the current behavior is already incorporated in the demos, for example https://github.com/oasisprotocol/demo-starter/blob/main/frontend/src/stores/ethereum.ts#L97-L121?

Although, I must admit there is too much boilerplate code involved in the demos IMO.

Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

Rethinking this, you are absolutely right. Can you please add a test to check whether the getSigner() call works as intended?

@CedarMist
Copy link
Member Author

Rethinking this, you are absolutely right. Can you please add a test to check whether the getSigner() call works as intended?

I don't have dappwrite or synpress setup in CI so I can't run E2E tests that require metamask.

Will add a test for .getSigner() if it works with Hardhat or with a wallet signed in Ethers, but really need to start testing with metamask otherwise we're missing a lot of coverage.

@matevz
Copy link
Member

matevz commented Apr 2, 2024

I don't have dappwrite or synpress setup in CI so I can't run E2E tests that require metamask.

Will add a test for .getSigner() if it works with Hardhat or with a wallet signed in Ethers, but really need to start testing with metamask otherwise we're missing a lot of coverage.

synpress would be ideal, but this can go into another PR. I was just thinking of testing that .getSigner() is defined and that it returns the wrapped instance.

@CedarMist
Copy link
Member Author

CedarMist commented Apr 2, 2024

I literally can't test this without doing it in the browser.

Error: could not coalesce error (error={ "code": "UNKNOWN_ERROR", "message": "could not coalesce error (error={ \"code\": -32601, \"payload\": { \"method\": \"eth_requestAccounts\", \"params\": [  ] } }, payload={ \"method\": \"eth_requestAccounts\", \"params\": [  ] }, code=UNKNOWN_ERROR, version=6.10.0)" }, payload={ "id": 2, "jsonrpc": "2.0", "method": "eth_accounts", "params": [  ] }, code=UNKNOWN_ERROR, version=6.10.0)

When called like:

const s = await ethers.provider.getSigner();

If I hook the provider in the way recommended by the hardhat docs, then I get the above error when trying to get the signer from hardhat.

// See: https://hardhat.org/hardhat-runner/docs/advanced/building-plugins#extending-the-hardhat-provider
extendProvider(async (provider, config, network) => {
  const networkConfig = config.networks[network];
  const { chainId } = networkConfig;
  const rpcUrl = 'url' in networkConfig ? networkConfig.url : '';
  if (chainId) {
    if (!sapphire.NETWORKS[chainId]) return provider;
  } else {
    if (!/sapphire/i.test(rpcUrl)) return provider;

    console.warn(
      'The Hardhat config for the network with `url`',
      rpcUrl,
      'did not specify `chainId`.',
      'The RPC URL looks like it may be Sapphire, so `@oasisprotocol/sapphire-hardhat` has been activated.',
      'You can prevent this from happening by setting a non-Sapphire `chainId`.',
    );
  }
  return sapphire.wrap(provider);
});

But, that does more reliably wrap the provider, so ... the problem is it's bypassing the internal hardhat getSigner function in via:

@aefhm aefhm added javascript Pull requests that update JavaScript code and removed js labels Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client javascript Pull requests that update JavaScript code p:1 Priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants