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

fix(rpc): reduce Ethereum rpc API and improve websocket API #1106

Merged
merged 8 commits into from
Sep 20, 2023

Conversation

lumtis
Copy link
Member

@lumtis lumtis commented Sep 11, 2023

Description

  • Keep only the namespaces available on regular Ethereum rpcs. Disable eth_sendTransaction, eth_sign and eth_signTypedData
  • Set message read limit to 32 MB

To test:

  • Start smoke tests
  • Check APIs

Before:

curl -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"rpc_modules","id":1}' http://localhost:9545
{"jsonrpc":"2.0","id":1,"result":{"debug":"1.0","eth":"1.0","miner":"1.0","net":"1.0","personal":"1.0","rpc":"1.0","txpool":"1.0","web3":"1.0"}}

After

curl -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"rpc_modules","id":1}' http://localhost:9545
{"jsonrpc":"2.0","id":1,"result":{"eth":"1.0","net":"1.0","rpc":"1.0","web3":"1.0"}}

curl -X POST --data '{"jsonrpc":"2.0","method":"personal_sign","params":["0x48656c6c6f", "0x604BB0f27A24374f1a784C458aFB9210CaAA2EDA", "password"],"id":1}' -H "Content-Type: application/json" http://localhost:9545/
{"jsonrpc":"2.0","id":1,"error":{"code":-32601,"message":"the method personal_sign does not exist/is not available"}}

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_sendTransaction","params":[{}],"id":1}' -H "Content-Type: application/json" http://localhost:9545

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_sign","params":["0x604BB0f27A24374f1a784C458aFB9210CaAA2EDA", "0x48656c6c6f"],"id":1}' -H "Content-Type: application/json" http://localhost:9545

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_signTypedData","params":["0x604BB0f27A24374f1a784C458aFB9210CaAA2EDA", {}],"id":1}' -H "Content-Type: application/json" http://localhost:9545

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

@lumtis lumtis self-assigned this Sep 11, 2023
@lumtis lumtis changed the title fix(rpc): reduce Ethereum rpc API fix(rpc): reduce Ethereum rpc API and fix improve websocket API Sep 11, 2023
@lumtis lumtis marked this pull request as ready for review September 11, 2023 09:41
@lumtis lumtis requested a review from CharlieMc0 September 11, 2023 09:43
@lumtis lumtis changed the title fix(rpc): reduce Ethereum rpc API and fix improve websocket API fix(rpc): reduce Ethereum rpc API and improve websocket API Sep 11, 2023
rpc/websockets.go Outdated Show resolved Hide resolved
@lumtis lumtis requested a review from ws4charlie September 11, 2023 20:49
@lumtis
Copy link
Member Author

lumtis commented Sep 12, 2023

Could be great to also have a review from @zeta-chain/devops on this, thanks🙏

Copy link
Collaborator

@brewmaster012 brewmaster012 left a comment

Choose a reason for hiding this comment

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

@lumtis LGTM overall but need one clarification

why do we disable eth_sendTransaction RPC?
As I understand, this is required for Ethereum wallet such as metamask to send tx to our EVM.

@lumtis
Copy link
Member Author

lumtis commented Sep 13, 2023

why do we disable eth_sendTransaction RPC?
As I understand, this is required for Ethereum wallet such as metamask to send tx to our EVM.

@brewmaster012 We use eth_sendRawTransaction with Metamask.
eth_sendTransaction allows sending unsigned transactions and getting them signed with unlocked accounts on the Node, hence why considered potentially unsafe.

@GMaiolo
Copy link

GMaiolo commented Sep 15, 2023

From a quick glance, we're using ethers.Signer.sendTransaction() (in Hub and Labs for swaps, and the Faucet for transfers) and the useSignTypedData hook from wagmi in our full-stack apps.

@lumtis are those two getting disabled in this PR or am I looking in the wrong place and it's a different thing?

@lumtis
Copy link
Member Author

lumtis commented Sep 17, 2023

From a quick glance, we're using ethers.Signer.sendTransaction() (in Hub and Labs for swaps, and the Faucet for transfers) and the useSignTypedData hook from wagmi in our full-stack apps.

@lumtis are those two getting disabled in this PR or am I looking in the wrong place and it's a different thing?

Yeah, the Ethereum RPC naming is a bit confusing.

You have eth_sendTransaction to send tx to be signed on the node, and eth_sendRawTransaction to send tx signed on the client
https://ethereum.org/en/developers/tutorials/sending-transactions-using-web3-and-alchemy/#difference-between-send-and-send-raw

Unless we have a sytem we're we manage private keys on the node that users can unlock, we should be only using eth_sendRawTransaction

@lumtis lumtis merged commit de85c7f into develop Sep 20, 2023
18 of 19 checks passed
@lumtis lumtis deleted the fix/disable-rpc-personal branch September 20, 2023 14:59
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.

6 participants