-
Notifications
You must be signed in to change notification settings - Fork 495
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
Feat/rpc api #2353
base: master
Are you sure you want to change the base?
Feat/rpc api #2353
Conversation
👉 View analysis in DeepCode’s Dashboard |
'embark_getSmartContracts', | ||
this.interceptResponse.bind(this) | ||
); | ||
} |
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.
A few things we might want to consider (to improve DX):
registerRpcInterceptors()
will probably always involve registering a request and/or response interceptor, so there's a certain amount of boilerplate that's always the same- Instead of requiring a
registerRpcInterceptors()
method, we could say anRpcInterceptor
always needs aninterceptRequest(params: ProxyRequestParams<T>)
and aninterceptResponse(params: ProxyResponseParams<T, S>)
- Embark would then expect these APIs and register them as handlers for the developers
- What if an interceptor doesn't need to intercept a request or response? In such case, we would always just resolve with the same params that came in
- What if an interceptor needs multiple interceptors for request/response? We can just have a single interceptorFn take care of multiple things
@emizzle thoughts?
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.
registerRpcInterceptors() will probably always involve registering a request and/or response interceptor, so there's a certain amount of boilerplate that's always the same
Agree, except the boilerplate only exists when extending RpcInterceptor
, and this allows the flexibility of registering the interceptions from any plugin. The RpcInterceptor
class exists is more of a convenience class for registering Embark's core RPC interceptors, so if we wanted to remove boilerplate, we could have two methods on the RpcInterceptor
class as you've explained.
Instead of requiring a
registerRpcInterceptors()
method, we could say anRpcInterceptor
always needs aninterceptRequest(params: ProxyRequestParams<T>)
and aninterceptResponse(params: ProxyResponseParams<T, S>)
I agree with this completely. One note is that we would need to have the intercepted RPC method specified, so likely would be good to accept the method as a string parameter in the constructor, as it will be static for the lifetime of the RpcInterceptor
class.
} | ||
return this._nodeAccounts || []; | ||
})(); | ||
} |
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.
You'll see accounts
, nodeAccounts
and web3
accounts being implemented in many interceptors. @emizzle originally those were injected via MutationOptions
but was then decided against. What was the reasoning again? I remember @iurimatias proposed that this wasn't a good idea and you eventually agreed.
Wonder if we might want to revisit the injection...
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.
Not sure why the injection was removed, but I see we also have the resetAccount
function only in RpcModifier. I think it means tha ton config reset and account reset (like in the tests), most of these modifiers/intereceptors won't work.
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.
For the classes that extend RpcInterceptor
, web3
, accounts
, and nodeAccounts
should live in the base RpcInterceptor
class. This is for convenience when intercepting all of the core Embark RPC requests/responses. Additionally, it serves other functions such as updating the account list when node accounts are created via personal_newAccount
, and when accounts are reset during tests (as @jrainville explained above).
For other plugins that want to intercept an RPC request/response that do not extend RpcInterceptor
, they will need to define their own web3
, accounts
, and nodeAccounts
as needed. For these cases, I originally tried to get these injected via MutationOptions
, but it wasn't a good idea.
packages/stack/proxy/src/proxy.js
Outdated
@@ -54,6 +54,7 @@ export class Proxy { | |||
// Using net_version instead of eth_accounts, because eth_accounts can fail if EIP1102 is not approved first | |||
await reqMgr.send({ method: 'net_version' }); | |||
} catch (e) { | |||
console.log(e); |
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.
This is a left over and needs to be removed..
private async executeInterceptors<T, R>( | ||
registrations: Array<RpcRequestRegistration<T> | RpcResponseRegistration<T, R>>, | ||
params: ProxyRequestParams<T> | ProxyResponseParams<T, R> | ||
): Promise<ProxyRequestParams<T> | ProxyResponseParams<T, R>> { |
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.
This might look overwhelming, but since we're using executeInterceptors()
for both, requestInterceptors
and responseInterceptors
, we have to overload the method's signature here.
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.
Well done.
): Promise<ProxyRequestParams<T> | ProxyResponseParams<T, R>> { | ||
const web3 = await this.web3; | ||
const accounts = await this.accounts; | ||
const nodeAccounts = await this.nodeAccounts; |
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.
These three are a left over from MutationOptions
and can go away now, since every interceptors takes care of those themselves.
return params; | ||
} | ||
|
||
private shouldModify(filter: string | string[] | RegExp, method: string) { |
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.
Prolly better to rename to shouldIntercept()
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.
Yeah
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.
Very big work. Good job
Edit: oops, just saw that I wrote "bog" instead of "big" 🙈
|
||
private get rawTransactionManager() { | ||
return (async () => { | ||
if (!this._rawTransactionManager) { |
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.
Could you reverse the condition and early exit instead?
@@ -1,9 +1,9 @@ | |||
embark-rpc-manager | |||
embark-rpc-mutations |
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.
Shouldn't it be embark-rpc-interceptors
?
} | ||
return this._nodeAccounts || []; | ||
})(); | ||
} |
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.
Not sure why the injection was removed, but I see we also have the resetAccount
function only in RpcModifier. I think it means tha ton config reset and account reset (like in the tests), most of these modifiers/intereceptors won't work.
} | ||
// Check if we have that account in our wallet | ||
const account = accounts.find((acc) => Web3.utils.toChecksumAddress(acc.address) === Web3.utils.toChecksumAddress(params.request.params[0].from)); | ||
if (account && account.privateKey) { |
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.
Could you reverse the condition and early return please.
// - eth_signTypedData_v4 | ||
// - personal_signTypedData (parity) | ||
this.embark.events.request2("rpc:request:interceptor:register", 'signTypedData', this.ethSignTypedDataRequest.bind(this)), | ||
this.embark.events.request2("rpc:response:interceptor:register", 'signTypedData', this.ethSignTypedDataResponse.bind(this)) |
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.
This should use a Regex no?
return params; | ||
} | ||
|
||
private shouldModify(filter: string | string[] | RegExp, method: string) { |
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.
Yeah
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.
Looks good. I'll just give it a test spin later today to see if the account changing was compromised with the changes.
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.
Just tested the tests where the accounts change and I get the right accounts. So everything is good as I can see
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.
Great job getting this done and across the line. I really like your improvements -- makes everything very understandable! 👏
I've left a request changes here, because we need to move the web3
, accounts
, and nodeAccounts
properties to the RpcInterceptor
base class, along with the accounts config update event. I've left comments in the source to aid in this.
'embark_getSmartContracts', | ||
this.interceptResponse.bind(this) | ||
); | ||
} |
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.
registerRpcInterceptors() will probably always involve registering a request and/or response interceptor, so there's a certain amount of boilerplate that's always the same
Agree, except the boilerplate only exists when extending RpcInterceptor
, and this allows the flexibility of registering the interceptions from any plugin. The RpcInterceptor
class exists is more of a convenience class for registering Embark's core RPC interceptors, so if we wanted to remove boilerplate, we could have two methods on the RpcInterceptor
class as you've explained.
Instead of requiring a
registerRpcInterceptors()
method, we could say anRpcInterceptor
always needs aninterceptRequest(params: ProxyRequestParams<T>)
and aninterceptResponse(params: ProxyResponseParams<T, S>)
I agree with this completely. One note is that we would need to have the intercepted RPC method specified, so likely would be good to accept the method as a string parameter in the constructor, as it will be static for the lifetime of the RpcInterceptor
class.
} | ||
return this._nodeAccounts || []; | ||
})(); | ||
} |
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.
For the classes that extend RpcInterceptor
, web3
, accounts
, and nodeAccounts
should live in the base RpcInterceptor
class. This is for convenience when intercepting all of the core Embark RPC requests/responses. Additionally, it serves other functions such as updating the account list when node accounts are created via personal_newAccount
, and when accounts are reset during tests (as @jrainville explained above).
For other plugins that want to intercept an RPC request/response that do not extend RpcInterceptor
, they will need to define their own web3
, accounts
, and nodeAccounts
as needed. For these cases, I originally tried to get these injected via MutationOptions
, but it wasn't a good idea.
protected get web3() { | ||
return (async () => { | ||
if (!this._web3) { | ||
await this.events.request2("blockchain:started"); | ||
// get connection directly to the node | ||
const provider = await this.events.request2("blockchain:node:provider", "ethereum"); | ||
this._web3 = new Web3(provider); | ||
} | ||
return this._web3; | ||
})(); | ||
} | ||
|
||
private get nodeAccounts() { | ||
return (async () => { | ||
if (!this._nodeAccounts) { | ||
const web3 = await this.web3; | ||
this._nodeAccounts = await web3.eth.getAccounts(); | ||
} | ||
return this._nodeAccounts || []; | ||
})(); | ||
} | ||
|
||
private get accounts() { | ||
return (async () => { | ||
if (!this._accounts) { | ||
const web3 = await this.web3; | ||
const nodeAccounts = await this.nodeAccounts; | ||
this._accounts = AccountParser.parseAccountsConfig(this.embark.config.blockchainConfig.accounts, web3, dappPath(), this.logger, nodeAccounts); | ||
} | ||
return this._accounts || []; | ||
})(); | ||
} |
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.
Move these back in to the base class.
protected get web3() { | ||
return (async () => { | ||
if (!this._web3) { | ||
await this.events.request2("blockchain:started"); | ||
// get connection directly to the node | ||
const provider = await this.events.request2("blockchain:node:provider", "ethereum"); | ||
this._web3 = new Web3(provider); | ||
} | ||
return this._web3; | ||
})(); | ||
} | ||
|
||
private get nodeAccounts() { | ||
return (async () => { | ||
if (!this._nodeAccounts) { | ||
const web3 = await this.web3; | ||
this._nodeAccounts = await web3.eth.getAccounts(); | ||
} | ||
return this._nodeAccounts || []; | ||
})(); | ||
} | ||
|
||
private get accounts() { | ||
return (async () => { | ||
if (!this._accounts) { | ||
const web3 = await this.web3; | ||
const nodeAccounts = await this.nodeAccounts; | ||
this._accounts = AccountParser.parseAccountsConfig(this.embark.config.blockchainConfig.accounts, web3, dappPath(), this.logger, nodeAccounts); | ||
} | ||
return this._accounts || []; | ||
})(); | ||
} |
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.
Move these back in to the base class.
protected get web3() { | ||
return (async () => { | ||
if (!this._web3) { | ||
await this.events.request2("blockchain:started"); | ||
// get connection directly to the node | ||
const provider = await this.events.request2("blockchain:node:provider", "ethereum"); | ||
this._web3 = new Web3(provider); | ||
} | ||
return this._web3; | ||
})(); | ||
} | ||
|
||
private get accounts() { | ||
return (async () => { | ||
if (!this._accounts) { | ||
const web3 = await this.web3; | ||
const nodeAccounts = await this.nodeAccounts; | ||
this._accounts = AccountParser.parseAccountsConfig(this.embark.config.blockchainConfig.accounts, web3, dappPath(), this.logger, nodeAccounts); | ||
} | ||
return this._accounts || []; | ||
})(); | ||
} | ||
|
||
private get nodeAccounts() { | ||
return (async () => { | ||
if (!this._nodeAccounts) { | ||
const web3 = await this.web3; | ||
this._nodeAccounts = await web3.eth.getAccounts(); | ||
} | ||
return this._nodeAccounts || []; | ||
})(); | ||
} |
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.
Move these back in to the base class.
protected get web3() { | ||
return (async () => { | ||
if (!this._web3) { | ||
await this.events.request2("blockchain:started"); | ||
// get connection directly to the node | ||
const provider = await this.events.request2("blockchain:node:provider", "ethereum"); | ||
this._web3 = new Web3(provider); | ||
} | ||
return this._web3; | ||
})(); | ||
} | ||
|
||
private get nodeAccounts() { | ||
return (async () => { | ||
if (!this._nodeAccounts) { | ||
const web3 = await this.web3; | ||
this._nodeAccounts = await web3.eth.getAccounts(); | ||
} | ||
return this._nodeAccounts || []; | ||
})(); | ||
} | ||
|
||
private get accounts() { | ||
return (async () => { | ||
if (!this._accounts) { | ||
const web3 = await this.web3; | ||
const nodeAccounts = await this.nodeAccounts; | ||
this._accounts = AccountParser.parseAccountsConfig(this.embark.config.blockchainConfig.accounts, web3, dappPath(), this.logger, nodeAccounts); | ||
} | ||
return this._accounts || []; | ||
})(); | ||
} |
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.
I'm fairly certain these are not needed here. They should only be needed in the RpcInterceptor
base class.
} | ||
|
||
private setCommandHandlers() { | ||
this.events.setCommandHandler("rpc:accounts:reset", this.resetAccounts.bind(this)); |
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.
This will also not be needed here and should be moved to the RpcInterceptor
base class, along with the bound resetAccounts
method.
callback(null, params); | ||
} | ||
|
||
private async onProxyResponse<T, R>(params: ProxyResponseParams<T, R>, callback: Callback<ProxyResponseParams<T, R>>) { |
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.
Just a personal preference: when there are two types in a generic method, I prefer to prefix both with T
(for "Type"), then suffix with a descriptor, ie TRequest
and TResponse
. When I read ProxyResponseParams<TRequest, TResponse>
aloud in my head, I read "Proxy response params of type request and response". However, if it were ProxyResponseParams<T, R>
, I would read it aloud as "Proxy response params of type T and R", which isn't as immediately obvious to me. Again, just a personal preference.
callback(null, params); | ||
} | ||
|
||
private registerRequestInterceptor<T>(filter: string | RegExp, interceptor: RpcRequestInterceptor<T>, options: RegistrationOptions = { priority: 50 }, callback?: Callback<null>) { |
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.
Don't forget to document the {priority: number}
optional parameter in the registration APIs.
private async executeInterceptors<T, R>( | ||
registrations: Array<RpcRequestRegistration<T> | RpcResponseRegistration<T, R>>, | ||
params: ProxyRequestParams<T> | ProxyResponseParams<T, R> | ||
): Promise<ProxyRequestParams<T> | ProxyResponseParams<T, R>> { |
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.
Well done.
} | ||
|
||
public abstract async registerRpcInterceptors(); | ||
public abstract getFilter(); |
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.
Because with these changes Embark now takes care of registering interceptors, every interceptor needs to expose its filter. Most of the time this is static information and could as well be added as
static FILTER = 'eth_subscribe'; // for example
However, we then don't have a way to "enforce" that there's such a property on an Interceptor
class. To account for that, I now ask interceptors to implement a getFilter()
method which has to be implemented.
@@ -128,7 +124,7 @@ export default class EthSendRawTransaction { | |||
return params; | |||
} | |||
|
|||
private async ethSendRawTransactionResponse(params: ProxyResponseParams<string, any>) { | |||
async interceptResponse(params: ProxyResponseParams<string, any>) { |
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.
One thing I'd propose @emizzle , we should probably also introduce an interface next to the abstract RpcInterceptor
class. The reason for that is that RpcInterceptor
at the moment ensures certain APIs are implemented on all interceptor implementations. However, "external" plugins like this quorum one doesn't have access to RpcInterceptor
and also probably shouldn't even inherit from it because it has things like accounts,
web3, nodeAccounts
etc that may not be needed in a custom rpc interceptor plugin.
We should probably ask all interceptors to implement an IRpcInterceptor
instead and all interceptors inside rpc-interceptors
just happen to inherit from RpcInterceptor
for shared functionality.
Thoughts?
This commit refactors the `rpc-manager` to be a stack component and therefore making it available to a variaty of plugins that might need access to its APIs so they can register new RPC interceptors. RpcModifiers have been renamed to `rpc-interceptors` and extracted from the `rpc-manager` source into their own plugin. The reason for the renaming was that modifiers aren't always modifying RPC APIs, but potentially introduce brand new ones. There are a bunch of new APIs that can be used to register new RPC APIs: ``` events.request('rpc:request:interceptor:register', 'method_name', requestInterceptorFn); events.request('rpc:response:interceptor:register', 'method_name', responseInterceptorFn); function requestInterceptorFn(params: ProxyRequestParams<T>) { // mutate `params` here return params; } function responseInterceptorFn(params: ProxyResponseParams<T, S>) { // mutate `params` here return params; } ``` A few things to note here: - `method_name` is either `string`, `string[]`, or `Regex`. This make registering custom RPC APIs extremely flexible. - A `requestInterceptorFn()` gets access to `ProxyRequestParams<T>` where `T` is the type of the params being send to the RPC API. - A `responseInterceptorFn()` gets access to `ProxyResponseParams<T, S>` where `T` is the type of the original request parameters and `S` the type of the response coming from the RPC API. These APIs are used by all `rpc-interceptors` and can be used the same way by any oother plugin that aims to create new or intercept existing APIs.
This commit refactors the
rpc-manager
to be a stack component and thereforemaking it available to a variaty of plugins that might need access to its APIs
so they can register new RPC interceptors.
RpcModifiers have been renamed to
rpc-interceptors
and extracted from therpc-manager
source into their own plugin. The reason for the renaming was that modifiers aren't alwaysmodifying RPC APIs, but potentially introduce brand new ones.
There are a bunch of new APIs that can be used to register new RPC APIs:
A few things to note here:
method_name
is eitherstring
,string[]
, orRegex
. This make registering custom RPCAPIs extremely flexible.
requestInterceptorFn()
gets access toProxyRequestParams<T>
whereT
is the type of theparams being send to the RPC API.
responseInterceptorFn()
gets access toProxyResponseParams<T, S>
whereT
is the type of theoriginal request parameters and
S
the type of the response coming from the RPC API.These APIs are used by all
rpc-interceptors
and can be used the same way by any oother pluginthat aims to create new or intercept existing APIs.
Here's an example of what an RPC interceptor can look like that implements a custom
embark_getSmartContracts
RPC API:Most of this work was already done by @emizzle so all the credits go to him