search

krzkaczor / cheatcalls-eip

Standardizing JSON-RPC interface for Ethereum development nodes
5 stars 1 forks source link

General feedback #23

Open sz-piotr opened 3 weeks ago

sz-piotr commented 3 weeks ago

Spec feedback, top to bottom

Type definitions

  1. type Data the comment doesn't specify that it has to be hex, and it doesn't specify that it has to be full number of bytes, so 0x123 isn't allowed, but 0x0123 is.
  2. All numbers, unless there is a really good reason should be passed as Quantity. This eliminates negative and floating point numbers as well as sticks to how Ethereum JSON-RPC behaves.
  3. I'd rename NodeInfo to CheatcallsInfo, because it doesn't contain all information about a node.
  4. bytecodeVerification seems to be unused. I'd remove it.
  5. impersonateAllStatus suggests a string. I'd rename to impersonateAllEnabled.
  6. RunMode - I'd replace "new" with "genesis"
  7. RunMode.originRpc might contain a secret, e.g. alchemy key. I am not sure if sending those over JSON-RPC is a good idea. Maybe this shouldn't be part of the returned data.
  8. InputRunMode.blockNumber should be Quantity | 'latest' and be optional. The reasoning being that otherwise there is no way to be explicit about using 'latest' - undefined is confusing.
  9. Rename MiningMode.intervalInSec to intervalSeconds.
  10. MiningOrdering values. I'd replace them with "highest-fee-first" and "oldest-first". You can also add "random".

JSON RPC Methods

  1. I think having a different format here would be nice. That way we can even skip type definitions entirely. See example:

cheat_setCode

Changes the bytecode of target account. Doesn't mine a block, doesn't modify the merkle tree roots.

interface CheatSetCodeRequest {
  account: Address
  code: Data
}

type CheatSetCodeResponse = null
  1. Passing parameters positionally is problematic for adding new functionality in the future. I would have all functions receive either no parameters or a single object parameter.
  2. cheat_setBalance rename addr to account and balanceInWei to balance.
  3. cheat_setErc20Balance. The 10^18 base unit comment is confusing. If I pass 1 do I get 10^18 or 1. I think I should get 1. Rename addr to account and balanceInBaseUnit to balance.
  4. cheat_setCode. Rename addr to account.
  5. cheat_setNonce. Rename addr to account.
  6. cheat_setStorageAt. Rename addr to account, key to slot. Change slot type to Quantity.
  7. cheat_setCoinbase. Rename addr to account.
  8. cheat_setMinGasPrice. Rename priceInWei to gasPrice. Change type to Quantity | 'default'. Passing null might be confused with setting it to 0.
  9. cheat_setNextBlockBaseFeePerGas. Rename priceInWei to baseFeePerGas. Change type to Quantity | 'default'. Passing null might be confused with setting it to 0.
  10. cheat_setBlockGasLimit. Change type to Quantity | 'default'. Setting 0 means no limit.
  11. cheat_mine rename blocks to count. Rename gapInSec to gapSeconds. It is unclear if this method advances the time or not. I think the gap parameter should be reworked.
  12. cheat_mining_mode rename to cheat_setMiningMode.
  13. cheat_increaseTime rename deltaInSec to deltaSeconds.
  14. cheat_setNextBlockTimestamp unclear how it plays with cheat_mine gap parameter. Also nextTimestamp type should be changed to Quantity | 'default'. The case is not as strong here but this will follow other methods.
  15. cheat_snapshot return Quantity.
  16. cheat_revert_snapshot rename to cheat_revertSnapshot. id type change to Quantity. Not sure why we need errors in the boolean return value if JSON-RPC already has support for errors.

Other comments

  1. Client side approach is a bad idea, because the entire point is JSON-RPC level support so that different APIs can be built on top. A proxy node is more suitable. It can wrap an rpc of hardhat or foundry and provide a cheat compatible RPC.
  2. Backwards compatibility should be about the future. Also you can highlight how each method differs from existing solutions.
  3. It is important to specify which methods change what. For example changing the account state (nonce, code, balance). Does it mine a new block? Does it recalculate the merkle roots?
krzkaczor commented 2 weeks ago

Thanks for the detailed feedback @sz-piotr!

If I miss a given point, it means I agree. ;) However, implementing everything will take some time.

ad 1. I wanted to avoid repeating what was already specified in a linked documentation of Ethereum JSON-RPC.

ad 4. This should be mentioned in the spec as I think automating verification fits this proposal nicely. (and it's often problem on dev nets)

ad 7. This problem is mentioned in security considerations section but I will make sure to emphasise it more.

ad 12. I had the same thoughts but I didn't propose it because already standarized json-rpc methods don't utilize objects.

ad 13. btw. how do you call wei equivalent for erc20s? We call them internally "base units". but yes, i'll tweak the comment

ad 26. This is something that i need more input from the node side. Some of the providers (tenderly) return something that look like uuid. Ofc this could be converted to a number but I am not sure if it makes sense.

ad 28. I know it's a bad approach but it might be fastest 0 to 1 approach that solves most issues. Internally we already use something like this.

ad 29.

Backwards compatibility should be about the future.

Can you elaborate on that? I think this section should explain how implementing this spec would change the experience of current users.

sz-piotr commented 2 weeks ago

Backwards compatibility should be about the future.

Basically I was thinking about the following points: