Skip to content

[CD] | provider: add nonce and is web3 provider methods #42

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.

Sign up for GitHub

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

Closed
cbrzn wants to merge 5 commits into from
Closed

[CD] | provider: add nonce and is web3 provider methods #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e36e77
feat: add fetch nonce
cbrzn Mar 10, 2023
3361c2b
chore: update interface & deploy hash
cbrzn Mar 10, 2023
84c9ef0
feat: add is wallet method
cbrzn Mar 10, 2023
e0a1641
chore: update version & deploy hash
cbrzn Mar 10, 2023
6719f2d
chore: change is wallet to is web3 provider
cbrzn Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion provider/implementations/js/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@polywrap/ethereum-provider-js",
"description": "Ethereum Provider JS Plugin",
"version": "0.2.4",
"version": "0.2.5",
"license": "MIT",
"main": "build/index.js",
"files": [
Expand Down
3 changes: 3 additions & 0 deletions provider/implementations/js/polywrap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,6 @@ project:
source:
module: ./src/index.ts
schema: ./src/schema.graphql
import_abis:
- uri: wrap://ens/wraps.eth:ethereum-provider@1.1.0
abi: ../../interface/src/schema.graphql
36 changes: 34 additions & 2 deletions provider/implementations/js/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import {
IProvider_Module_Args_address as Args_address,
IProvider_Module_Args_chainId as Args_chainId,
IProvider_Module_Args_waitForTransaction as Args_waitForTransaction,
IProvider_Connection as SchemaConnection
IProvider_Connection as SchemaConnection,
IProvider_Module_Args_nonce as Args_nonce,
IProvider_Module_Args_isWeb3Provider as Args_isWeb3Provider,
} from "./wrap";
import { PluginFactory, PluginPackage } from "@polywrap/plugin-js";
import { Connection } from "./Connection";
Expand Down Expand Up @@ -46,7 +48,7 @@ export class EthereumProviderPlugin extends Module<ProviderConfig> {
* Ethers-rs defines the type of EIP 1559 tx
* as 0x02, but metamask expects it as 0x2,
* hence, the need of this workaround. Related:
* https://github.com/foundry-rs/foundry/issues/3890.
* https://github.com/MetaMask/metamask-extension/issues/18076
*
* We check if the parameters comes as array, if the error
* contains 0x2 and if the type is 0x02, then we change it
Expand All @@ -67,6 +69,27 @@ export class EthereumProviderPlugin extends Module<ProviderConfig> {
}
}

public async isWeb3Provider(
args: Args_isWeb3Provider,
_client: CoreClient
): Promise<boolean> {
const connection = await this._getConnection(args.connection);
/**
Copy link
Copy Markdown
Contributor

@krisbitney krisbitney Mar 13, 2023

Choose a reason for hiding this comment

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

For me, this name is not intuitive. I would expect isWallet to return true if the signer itself is a wallet (e.g. an instance of ethers.Wallet). It sounds like this method instead distinguishes between web3provider and jsonRpcProvider. Is that correct?

It is possible to interface with a wallet via RPC, like we do with Ganache

And with MetaMask, we would generally want to call sendTransaction, not sendRawTransaction

That is my understanding

Copy link
Copy Markdown
Contributor

@krisbitney krisbitney Mar 13, 2023

Choose a reason for hiding this comment

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

The reason for using sendTransaction is so that the wallet can handle both signing and sending securely. My understanding is that signing a transaction and then sending it with sendRawTransaction is not secure, and most wallets don't even allow it.

Have you tested this with MetaMask to make sure it works?

Copy link
Copy Markdown
Contributor Author

@cbrzn cbrzn Mar 13, 2023

Choose a reason for hiding this comment

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

It sounds like this method instead distinguishes between web3provider and jsonRpcProvider. Is that correct?

that's correct - maybe doing isWeb3Provider would be better?

It is possible to interface with a wallet via RPC, like we do with Ganache

yes, it's possible, but neither infura or alchemy accept sendTransaction because they don't manipulate signing keys. Meaning that currently, we're only able to execute transactions using a private key in testnet, but not in any other chain (we are not able to execute ethereum transactions in a script like this, for example)

And with MetaMask, we would generally want to call sendTransaction, not sendRawTransaction

metamask uses sendTransaction to make sure that the private key used to sign the transaction never leaves the user's device, which in our case it, might not always be that way. the reason is that we accept Wallet object (which has access to these private keys) as a signer (besides a Web3Provider). so effectively with metamask we are still doing sendTransaction

The reason for using sendTransaction is so that the wallet can handle both signing and sending securely. My understanding is that signing a transaction and then sending it with sendRawTransaction is not secure, and most wallets don't even allow it.

while I completely understand your point I think that we're not a wallet but rather a development tool (that probably wallets will eventually use!) and we should allow users to interact and execute transactions in any way they would like to

Have you tested this with MetaMask to make sure it works?

yes! i tested with safe wrapper demo app and worked as expected

Copy link
Copy Markdown
Contributor

@krisbitney krisbitney Mar 13, 2023

Choose a reason for hiding this comment

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

This is just my understanding of things at the moment.

I never said we should limit how users use the wrapper. I am just saying that I thought signing and sending raw transactions was supposed to be less secure. If so, they shouldn't be used in cases where a more secure option is available. I could be wrong about all of this!

Does Infura or Alchemy provide wallets? I thought they were always providers without signers

I think with MetaMask we can always do eth_sendTransaction, right? Or do we need to use eth_sendRawTransaction?

Copy link
Copy Markdown
Contributor Author

@cbrzn cbrzn Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Does Infura or Alchemy provide wallets? I thought they were always providers without signers

they do not, that's why we need to sign it locally

I think with MetaMask we can always do eth_sendTransaction, right? Or do we need to use eth_sendRawTransaction?

you're right! with metamask or any web3provider we will use eth_sendTransaction (and that's what we're doing in the other PR, if the provider is a Web3Provider (currently checked with isWallet method), we do eth_sendTransaction)

just for clarification, when I said I tested with metamask, I meant that the eth_sendTransaction is being done as expected, and with a script (giving the private key) we're doing eth_sendRawTransaction

Copy link
Copy Markdown
Contributor

@krisbitney krisbitney Mar 13, 2023

Choose a reason for hiding this comment

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

Okay, so to clarify, the Safe wrapper is using a private key to sign transactions but does not have access to an Ethereum provider, so you need to send the transactions using something like Infura?

What if we just use two methods? One would be syntax sugar for signing and sending raw transactions (which is already possible), the other for handling standard transactions. That way we are still calling eth_sendTransaction with wallets that have signers but are not of type Web3Provider.

Copy link
Copy Markdown
Contributor Author

@cbrzn cbrzn Mar 13, 2023

Choose a reason for hiding this comment

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

Okay, so to clarify, the Safe wrapper is using a private key to sign transactions but does not have access to an Ethereum provider, so you need to send the transactions using something like Infura?

in a node js script context, we need to pass the private key (and a JSON RPC, like infura or alchemy) - but in the demo app all works fresh and uses eth_sendTransaction (we use the metamask provider)

What if we just use two methods? One would be syntax sugar for signing and sending raw transactions (which is already possible), the other for handling standard transactions. That way we are still calling eth_sendTransaction with wallets that have signers but are not of type Web3Provider.

yes I like this!

Copy link
Copy Markdown
Contributor

@krisbitney krisbitney Mar 13, 2023

Choose a reason for hiding this comment

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

Awesome, okay. I apologize for making this more complicated!

* This methods gives to the plugin the capability
* to check if the provider is a wallet (i.e metamask)
* or a given private key (with `new Wallet("0x..")`)
* this allows the wrapper to check if it needs to call
* sendTransaction or sendRawTransaction RPC method
*
* It checks if the provider has the `provider` attribute
* which is something that only the `Web3Provider` has
* but not the JsonRpcProvider: Check the following link for more info:
* https://github.com/ethers-io/ethers.js/blob/v5.7.0/packages/providers/src.ts/web3-provider.ts#L122
*/
const provider = connection.getProvider();
return "provider" in provider
}

async waitForTransaction(
args: Args_waitForTransaction,
_client: CoreClient
Expand Down Expand Up @@ -119,6 +142,15 @@ export class EthereumProviderPlugin extends Module<ProviderConfig> {
return network.chainId.toString();
}

public async nonce(
args: Args_nonce,
_client: CoreClient
): Promise<number> {
const connection = await this._getConnection(args.connection);
const nonce = await connection.getSigner().getTransactionCount();
return nonce
}

private async _getConnection(connection?: SchemaConnection | null): Promise<Connection> {
return this._connections.getConnection(connection ?? this.env.connection);
}
Expand Down
2 changes: 1 addition & 1 deletion provider/interface/deployment.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"name": "ipfs_deploy",
"id": "deploy.ipfs_deploy",
"input": "wrap://fs/./build",
"result": "wrap://ipfs/QmXBQNWLpQcR95nequWFLHEcsTDP772QyVFnNuroyr2nzu"
"result": "wrap://ipfs/QmWP9Y1hoZNN6ccjdy6QiskKx3aJXYVrqmx5MCxQ72UML3"
}
]
}
Expand Down
10 changes: 10 additions & 0 deletions provider/interface/src/schema.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,14 @@ type Module {
Get the chain id of the signer's connection. Throws if signer is missing.
"""
chainId(connection: Connection): String!

"""
Get the nonce of the signer's. Throws if signer is missing.
"""
nonce(connection: Connection): Int!

"""
Check if signer is wallet-based. Throws if signer is missing.
"""
isWeb3Provider(connection: Connection): Boolean!
}