-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for oasisprotocol-sapphire-paratime canceled.
|
There was a problem hiding this 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.
There was a problem hiding this 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?
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 |
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. |
I literally can't test this without doing it in the browser.
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 |
When the user calls
.getSigner
on a wrapped Ethers provider, it will return an unwrapped signer.This change returns a wrapped signer.