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.

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

Jump to bottom

Migrate provider configuration to RpcMethodMiddleware #5513

Closed
4 tasks
Gudahtt opened this issue Jan 13, 2023 · 2 comments
Closed
4 tasks

Migrate provider configuration to RpcMethodMiddleware #5513

Gudahtt opened this issue Jan 13, 2023 · 2 comments
Assignees
@Gudahtt
Labels
team-wallet-framework

Comments

@Gudahtt
Copy link
Member

Gudahtt commented Jan 13, 2023 •
edited by xinnanyemm
Loading

Description

Migrate the functions setup in the network controller providerConfig to the RpcMethodMiddleware.

This provider configuration will be removed in a future release of the network controller. It is used today to support certain RPC methods. It can be migrated to be alongside the rest of mobile's RPC method handlers, so that mobile is unaffected by this future change to the network controller.

Technical Details

  • Migrate the eth_sendTransaction handler from Engine.js to RpcMethodMiddleware.ts.
  • Remove the getAccounts function, instead using the getAccounts function in RpcMethodMiddleware.ts to meet this need for any RPC method currently relying on this.

Note that once both of these entries are removed from the providerConfig, it will still need to be set to an empty object. The provider config being set triggers the network controller to initialize, so it should not be removed entirely.

Acceptance Criteria

  • The provider configuration is empty.

References

This helps to unblock MetaMask/core#970

Definition of Done

  • Whenever applies, any change unit tested, reviewed(approved) and documented(JSDOC at least)
  • We have changelog entries for any related changes
  • Any changed APIs have comprehensive inline documentation
  • Any changed public APIs are well covered by unit tests
  • Other items
  • If a planning/research ticket, then the plan has been reviewed and approved by at least 1 team members
@xinnanyemm
Copy link

Hey team! Please add your planning poker estimate with Zenhub @mcmire @Gudahtt

@Gudahtt Gudahtt self-assigned this Jan 26, 2023
Gudahtt added a commit that referenced this issue Jan 26, 2023
@Gudahtt
Refactor eth_sendTransaction handler
1500463 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
This was referenced Jan 26, 2023
Merged
Merged
Gudahtt added a commit that referenced this issue Jan 26, 2023
@Gudahtt
Refactor RPC getAccounts usage
2c47bef 
The `getAccounts` function is defined and used in two different places
in the RPC pipeline: `RPCMethodMiddleware.ts` and
`web3-provider-engine`. The second one has been removed, so now all
usage is in `RPCMethodMiddleware.ts`.

`getAccounts` is passed into `web3-provider-engine` and used in the
`HookedWalletSubprovider`: https://github.com/MetaMask/web3-provider-engine/blob/cf612f898002833c36730d23972fe4c4dd483c76/subproviders/hooked-wallet.js

It is used in these methods:
* `eth_coinbase`
* `eth_accounts`
* `parity_defaultAccount`

It's also called to validate the sender, for the following methods:
* `eth_signTypedData`
* `eth_signTypedData_v3`
* `eth_signTypedData_v4`
* `encryption_public_key`
* `eth_decryptMessage`
* `personal_sign`
* `eth_sign`
* `eth_signTransaction`
* `eth_sendTransaction`

Of these methods, most of them are intercepted in
`RPCMethodMiddleware.ts`, so the requests never make it to
`web3-provider-engine`.

These three methods will make their way there: `parity_defaultAccount`,
`encryption_public_key`, and `eth_decryptMessage`. The decryption-
related messages rely on constructor parameters that mobile does not
pass in, so those always throw an error. The only method this hook is
used for in practice is `parity_defaultAccount`.

The `parity_defaultAccount` method was added to
`RPCMethodMiddleware.ts, so now that method won't make it to
`web3-provider-engine` either. Functionally it is the same as
`eth_coinbase`, so the same implementation has been used.

This relates to #5513
Gudahtt added a commit that referenced this issue Jan 26, 2023
@Gudahtt
Refactor static method middlware
ad1a794 
Some RPC methods that return static results are being handled in
`web3-provider-engine` rather than `RPCMethodMiddleware.ts`. They have
now all been moved to `RPCMethodMiddleware.ts`.

This should result in no functional changes.

This relates to #5513
This was referenced Jan 26, 2023
Merged
Merged
Gudahtt added a commit that referenced this issue Jan 27, 2023
@Gudahtt
Refactor remaining web3-provider-engine methods
e469a70 
The remaining non-static methods from `web3-provider-engine` have been
migrated to `RPCMethodMiddleware.ts` with the rest of the method
handlers. The two methods left were `personal_ecRecover` and
`parity_checkRequest`.

`personal_ecRecover` will recover the signing account from a signature.
This operation requires no private keys. Commonly dapps will do this
themselves. Persumably it was added to the dapp API at some point for
convenience. It has been preserved just to avoid making breaking
changes to the dapp API.

`parity_checkRequest` would return the result of a parity signature or
transaction. It returned `null` if none were found. The parity
signature and request methods have been throwing an error for some time
now, so there is never any result to check. As a result, this method
has been returning `null` for all requests in practice. It has been
preserved just to avoid making breaking changes to the dapp API.

This relates to #5513
Gudahtt added a commit that referenced this issue Jan 27, 2023
@Gudahtt
Refactor remaining web3-provider-engine methods
d9b319b 
The remaining non-static methods from `web3-provider-engine` have been
migrated to `RPCMethodMiddleware.ts` with the rest of the method
handlers. The two methods left were `personal_ecRecover` and
`parity_checkRequest`.

`personal_ecRecover` will recover the signing account from a signature.
This operation requires no private keys. Commonly dapps will do this
themselves. Persumably it was added to the dapp API at some point for
convenience. It has been preserved just to avoid making breaking
changes to the dapp API.

`parity_checkRequest` would return the result of a parity signature or
transaction. It returned `null` if none were found. The parity
signature and request methods have been throwing an error for some time
now, so there is never any result to check. As a result, this method
has been returning `null` for all requests in practice. It has been
preserved just to avoid making breaking changes to the dapp API.

This relates to #5513
cortisiko pushed a commit that referenced this issue Feb 3, 2023
@Gudahtt
Refactor static method middlware (#5621)
6e062b6 
Some RPC methods that return static results are being handled in
`web3-provider-engine` rather than `RPCMethodMiddleware.ts`. They have
now all been moved to `RPCMethodMiddleware.ts`.

This should result in no functional changes.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 8, 2023
@Gudahtt
Refactor eth_sendTransaction handler
241ba4e 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 16, 2023
@Gudahtt
Refactor eth_sendTransaction handler
e8c151b 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 16, 2023
@Gudahtt
Refactor RPC getAccounts usage
c823904 
The `getAccounts` function is defined and used in two different places
in the RPC pipeline: `RPCMethodMiddleware.ts` and
`web3-provider-engine`. The second one has been removed, so now all
usage is in `RPCMethodMiddleware.ts`.

`getAccounts` is passed into `web3-provider-engine` and used in the
`HookedWalletSubprovider`: https://github.com/MetaMask/web3-provider-engine/blob/cf612f898002833c36730d23972fe4c4dd483c76/subproviders/hooked-wallet.js

It is used in these methods:
* `eth_coinbase`
* `eth_accounts`
* `parity_defaultAccount`

It's also called to validate the sender, for the following methods:
* `eth_signTypedData`
* `eth_signTypedData_v3`
* `eth_signTypedData_v4`
* `encryption_public_key`
* `eth_decryptMessage`
* `personal_sign`
* `eth_sign`
* `eth_signTransaction`
* `eth_sendTransaction`

Of these methods, most of them are intercepted in
`RPCMethodMiddleware.ts`, so the requests never make it to
`web3-provider-engine`.

These three methods will make their way there: `parity_defaultAccount`,
`encryption_public_key`, and `eth_decryptMessage`. The decryption-
related messages rely on constructor parameters that mobile does not
pass in, so those always throw an error. The only method this hook is
used for in practice is `parity_defaultAccount`.

The `parity_defaultAccount` method was added to
`RPCMethodMiddleware.ts, so now that method won't make it to
`web3-provider-engine` either. Functionally it is the same as
`eth_coinbase`, so the same implementation has been used.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 16, 2023
@Gudahtt
Refactor remaining web3-provider-engine methods
75510d1 
The remaining non-static methods from `web3-provider-engine` have been
migrated to `RPCMethodMiddleware.ts` with the rest of the method
handlers. The two methods left were `personal_ecRecover` and
`parity_checkRequest`.

`personal_ecRecover` will recover the signing account from a signature.
This operation requires no private keys. Commonly dapps will do this
themselves. Persumably it was added to the dapp API at some point for
convenience. It has been preserved just to avoid making breaking
changes to the dapp API.

`parity_checkRequest` would return the result of a parity signature or
transaction. It returned `null` if none were found. The parity
signature and request methods have been throwing an error for some time
now, so there is never any result to check. As a result, this method
has been returning `null` for all requests in practice. It has been
preserved just to avoid making breaking changes to the dapp API.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 29, 2023
@Gudahtt
Refactor eth_sendTransaction handler
d7d4761 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 29, 2023
@Gudahtt
Refactor RPC getAccounts usage
c0ac2a6 
The `getAccounts` function is defined and used in two different places
in the RPC pipeline: `RPCMethodMiddleware.ts` and
`web3-provider-engine`. The second one has been removed, so now all
usage is in `RPCMethodMiddleware.ts`.

`getAccounts` is passed into `web3-provider-engine` and used in the
`HookedWalletSubprovider`: https://github.com/MetaMask/web3-provider-engine/blob/cf612f898002833c36730d23972fe4c4dd483c76/subproviders/hooked-wallet.js

It is used in these methods:
* `eth_coinbase`
* `eth_accounts`
* `parity_defaultAccount`

It's also called to validate the sender, for the following methods:
* `eth_signTypedData`
* `eth_signTypedData_v3`
* `eth_signTypedData_v4`
* `encryption_public_key`
* `eth_decryptMessage`
* `personal_sign`
* `eth_sign`
* `eth_signTransaction`
* `eth_sendTransaction`

Of these methods, most of them are intercepted in
`RPCMethodMiddleware.ts`, so the requests never make it to
`web3-provider-engine`.

These three methods will make their way there: `parity_defaultAccount`,
`encryption_public_key`, and `eth_decryptMessage`. The decryption-
related messages rely on constructor parameters that mobile does not
pass in, so those always throw an error. The only method this hook is
used for in practice is `parity_defaultAccount`.

The `parity_defaultAccount` method was added to
`RPCMethodMiddleware.ts, so now that method won't make it to
`web3-provider-engine` either. Functionally it is the same as
`eth_coinbase`, so the same implementation has been used.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 30, 2023
@Gudahtt
Refactor eth_sendTransaction handler
72568cc 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
Gudahtt added a commit that referenced this issue Mar 30, 2023
@Gudahtt
Refactor eth_sendTransaction handler
ecabaa9 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
Gudahtt added a commit that referenced this issue Apr 4, 2023
@Gudahtt
Refactor eth_sendTransaction handler
0c9d219 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
Gudahtt added a commit that referenced this issue Apr 5, 2023
@Gudahtt
Refactor eth_sendTransaction handler (#5619)
580c4d4 
The handler for `eth_sendTransaction` was previously spread between
`RPCMethodMiddleware.ts` and the static middleware of
`web3-provider-engine`. Instead it is all handled in
`RPCMethodMiddleware.ts` now.

This should have no functional impact. It is difficult to trace through
`web3-provider-engine`, but this case is one of the easier ones because
the static middleware is run first, and in this case it will always end
the request.

This relates to #5513
Gudahtt added a commit that referenced this issue Apr 5, 2023
@Gudahtt
Refactor RPC getAccounts usage
ecc33fc 
The `getAccounts` function is defined and used in two different places
in the RPC pipeline: `RPCMethodMiddleware.ts` and
`web3-provider-engine`. The second one has been removed, so now all
usage is in `RPCMethodMiddleware.ts`.

`getAccounts` is passed into `web3-provider-engine` and used in the
`HookedWalletSubprovider`: https://github.com/MetaMask/web3-provider-engine/blob/cf612f898002833c36730d23972fe4c4dd483c76/subproviders/hooked-wallet.js

It is used in these methods:
* `eth_coinbase`
* `eth_accounts`
* `parity_defaultAccount`

It's also called to validate the sender, for the following methods:
* `eth_signTypedData`
* `eth_signTypedData_v3`
* `eth_signTypedData_v4`
* `encryption_public_key`
* `eth_decryptMessage`
* `personal_sign`
* `eth_sign`
* `eth_signTransaction`
* `eth_sendTransaction`

Of these methods, most of them are intercepted in
`RPCMethodMiddleware.ts`, so the requests never make it to
`web3-provider-engine`.

These three methods will make their way there: `parity_defaultAccount`,
`encryption_public_key`, and `eth_decryptMessage`. The decryption-
related messages rely on constructor parameters that mobile does not
pass in, so those always throw an error. The only method this hook is
used for in practice is `parity_defaultAccount`.

The `parity_defaultAccount` method was added to
`RPCMethodMiddleware.ts, so now that method won't make it to
`web3-provider-engine` either. Functionally it is the same as
`eth_coinbase`, so the same implementation has been used.

This relates to #5513
Gudahtt added a commit that referenced this issue Apr 5, 2023
@Gudahtt
Refactor remaining web3-provider-engine methods
7f48461 
The remaining non-static methods from `web3-provider-engine` have been
migrated to `RPCMethodMiddleware.ts` with the rest of the method
handlers. The two methods left were `personal_ecRecover` and
`parity_checkRequest`.

`personal_ecRecover` will recover the signing account from a signature.
This operation requires no private keys. Commonly dapps will do this
themselves. Persumably it was added to the dapp API at some point for
convenience. It has been preserved just to avoid making breaking
changes to the dapp API.

`parity_checkRequest` would return the result of a parity signature or
transaction. It returned `null` if none were found. The parity
signature and request methods have been throwing an error for some time
now, so there is never any result to check. As a result, this method
has been returning `null` for all requests in practice. It has been
preserved just to avoid making breaking changes to the dapp API.

This relates to #5513
Gudahtt added a commit that referenced this issue Apr 6, 2023
@Gudahtt
Refactor RPC getAccounts usage (#5620)
1f24769 
* Refactor RPC `getAccounts` usage

The `getAccounts` function is defined and used in two different places
in the RPC pipeline: `RPCMethodMiddleware.ts` and
`web3-provider-engine`. The second one has been removed, so now all
usage is in `RPCMethodMiddleware.ts`.

`getAccounts` is passed into `web3-provider-engine` and used in the
`HookedWalletSubprovider`: https://github.com/MetaMask/web3-provider-engine/blob/cf612f898002833c36730d23972fe4c4dd483c76/subproviders/hooked-wallet.js

It is used in these methods:
* `eth_coinbase`
* `eth_accounts`
* `parity_defaultAccount`

It's also called to validate the sender, for the following methods:
* `eth_signTypedData`
* `eth_signTypedData_v3`
* `eth_signTypedData_v4`
* `encryption_public_key`
* `eth_decryptMessage`
* `personal_sign`
* `eth_sign`
* `eth_signTransaction`
* `eth_sendTransaction`

Of these methods, most of them are intercepted in
`RPCMethodMiddleware.ts`, so the requests never make it to
`web3-provider-engine`.

These three methods will make their way there: `parity_defaultAccount`,
`encryption_public_key`, and `eth_decryptMessage`. The decryption-
related messages rely on constructor parameters that mobile does not
pass in, so those always throw an error. The only method this hook is
used for in practice is `parity_defaultAccount`.

The `parity_defaultAccount` method was added to
`RPCMethodMiddleware.ts, so now that method won't make it to
`web3-provider-engine` either. Functionally it is the same as
`eth_coinbase`, so the same implementation has been used.

This relates to #5513

* Add unit tests
Gudahtt added a commit that referenced this issue Apr 6, 2023
@Gudahtt
Refactor remaining web3-provider-engine methods (#5627)
9515d43 
The remaining non-static methods from `web3-provider-engine` have been
migrated to `RPCMethodMiddleware.ts` with the rest of the method
handlers. The two methods left were `personal_ecRecover` and
`parity_checkRequest`.

`personal_ecRecover` will recover the signing account from a signature.
This operation requires no private keys. Commonly dapps will do this
themselves. Persumably it was added to the dapp API at some point for
convenience. It has been preserved just to avoid making breaking
changes to the dapp API.

`parity_checkRequest` would return the result of a parity signature or
transaction. It returned `null` if none were found. The parity
signature and request methods have been throwing an error for some time
now, so there is never any result to check. As a result, this method
has been returning `null` for all requests in practice. It has been
preserved just to avoid making breaking changes to the dapp API.

This relates to #5513
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 6, 2023

This has been completed in these PRs:

#5619
#5620
#5627

@Gudahtt Gudahtt closed this as completed Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Gudahtt Gudahtt

Labels
team-wallet-framework
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@kenhkan @Gudahtt @xinnanyemm