AcalaNetwork / chopsticks

Create parallel reality of your Substrate network.
Apache License 2.0
138 stars 84 forks source link

feat: load rpc additional methods by cli #756

Closed Wsteth closed 5 months ago

Wsteth commented 6 months ago

This allows us to directly use the CLI to load scripts and extend our RPC methods. Others can easily extend rpc methods by using npx cli.

Here's an example:

npx @acala-network/chopsticks@latest --rpc-methods=https://example.com/test.js npx @acala-network/chopsticks@latest --rpc-methods=./test.js

The script:

return {
  async dev_testRpcMethod1(context, params, subscriptionManager) {
    console.log('dev_testRpcMethod 1')
  },
  async dev_testRpcMethod2(context, params, subscriptionManager) {
    console.log('dev_testRpcMethod 2')
  },
}
xlc commented 6 months ago

it make sense but I am worried about download and run js file from internet. that means someone can craft a malicious command that looks like setting up a chopsticks testnet but steals all the private keys

ermalkaleci commented 6 months ago

agree with @xlc

Wsteth commented 6 months ago

@xlc @ermalkaleci In that case, we can actually cancel the url loading method and only use local loading, or we can warn users that the URL method is risky.

ermalkaleci commented 6 months ago

Injecting code is never a good idea. If you need a new RPC then it will be better to write a plugin or fork it and have your custom version.

xlc commented 6 months ago

only local file is maybe fine. do you know any other npm cli tools offer similar ability to load 3rd party plugins? all I can think of is dev tools like eslint, which require user to have the plugin installed first and isn’t exactly suits here

Wsteth commented 6 months ago

@xlc npx download temporary builded publish npm scripts and run it, so if someone want to crack or do something else, they can also modify those scripts to do that.

xlc commented 6 months ago

It is different. when run npx some-package, it is the owner of the some-package you need to trust and if it contains malicious code, npm will remove such package.

I absolutely do not want people able to run npx @acala-network/chopsticks --rpc-methods=https://example.com and resulting malicious code execution.

Wsteth commented 6 months ago

It is different. when run npx some-package, it is the owner of the some-package you need to trust and if it contains malicious code, npm will remove such package.

I absolutely do not want people able to run npx @acala-network/chopsticks --rpc-methods=https://example.com and resulting malicious code execution.

I understand that situation, internet scripts may contains some malicious code and is untrustable. I mean if someone want to use injecting scirpts, he/she can also modify npx downloaded npm packages such as ~/.npm/_npx/xxxxxxxxxxx/

local injecting scripts is fine, we just want users to easily extends their rpc methods, not forks all repo and installs massive dependends.

xlc commented 6 months ago

Load from local file should be fine. Few things I would like to have:

Wsteth commented 6 months ago

Load from local file should be fine. Few things I would like to have:

  • Forbid such option from config file, which could be loaded remotely. This means user have to explicit have such option in cli
  • Prefix the cli with unsafe so people know it is unsafe
  • Make it more generic, we already have a plugin system so should just allow people to load a local plugin, which can add rpc methods as well

Ok for 1, 2. What means of 3? Can you explain more or give me some examples?

xlc commented 6 months ago

https://github.com/AcalaNetwork/chopsticks/blob/master/packages/chopsticks/src/plugins/dry-run/rpc.ts this is a plugin implements the dev_dryRUn RPC

it is loaded here https://github.com/AcalaNetwork/chopsticks/blob/dc55b39e3cbc426c725b226584a78cc50a4ba28e/packages/chopsticks/src/plugins/index.ts#L22

so we just need to modify the load plugin code to also load the user supplied file

Wsteth commented 5 months ago

https://github.com/AcalaNetwork/chopsticks/blob/master/packages/chopsticks/src/plugins/dry-run/rpc.ts this is a plugin implements the dev_dryRUn RPC

it is loaded here

https://github.com/AcalaNetwork/chopsticks/blob/dc55b39e3cbc426c725b226584a78cc50a4ba28e/packages/chopsticks/src/plugins/index.ts#L22

so we just need to modify the load plugin code to also load the user supplied file

@xlc @ermalkaleci I've done these issues, pls check it.

xlc commented 5 months ago

can you add a test and update README to document this new feature? thanks

Wsteth commented 5 months ago

can you add a test and update README to document this new feature? thanks

DONE pls check it.

xlc commented 5 months ago

can you do a yarn fix and it should be good to merge

ermalkaleci commented 5 months ago

@xlc @Wsteth why don't we spend time and make a proper plugin implementation?

xlc commented 5 months ago

It is unclear about the requirement of the proper plugin implementation and we shouldn't overengineer it for features no one is asking