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.

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: load rpc additional methods by cli #756

Merged
merged 5 commits into from
May 26, 2024
Merged

Conversation

Wsteth
Copy link
Contributor

@Wsteth Wsteth commented May 18, 2024

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
Copy link
Member

xlc commented May 18, 2024

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
Copy link
Collaborator

agree with @xlc

@Wsteth
Copy link
Contributor Author

Wsteth commented May 19, 2024

@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
Copy link
Collaborator

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
Copy link
Member

xlc commented May 19, 2024

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
Copy link
Contributor Author

Wsteth commented May 20, 2024

@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
Copy link
Member

xlc commented May 20, 2024

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
Copy link
Contributor Author

Wsteth commented May 20, 2024

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
Copy link
Member

xlc commented May 20, 2024

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

@Wsteth
Copy link
Contributor Author

Wsteth commented May 20, 2024

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
Copy link
Member

xlc commented May 20, 2024

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

export const loadRpcPlugin = async (method: string) => {

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

@Wsteth
Copy link
Contributor Author

Wsteth commented May 21, 2024

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

export const loadRpcPlugin = async (method: string) => {

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
Copy link
Member

xlc commented May 22, 2024

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

@Wsteth
Copy link
Contributor Author

Wsteth commented May 23, 2024

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

DONE
pls check it.

packages/chopsticks/src/plugins/index.ts Outdated Show resolved Hide resolved
@Wsteth Wsteth requested a review from xlc May 24, 2024 02:21
@xlc
Copy link
Member

xlc commented May 24, 2024

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

@ermalkaleci
Copy link
Collaborator

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

@xlc
Copy link
Member

xlc commented May 25, 2024

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

@xlc xlc merged commit ed41f5d into AcalaNetwork:master May 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants