kkrt-labs / kakarot-rpc

Kakarot ZK EVM Ethereum RPC adapter
MIT License
136 stars 97 forks source link

feat: create integration tests on eth-rpc crate that use either Katana, Madara or TestSequencer to make soundness tests automatic #253

Closed Eikix closed 1 year ago

Eikix commented 1 year ago

Feature Request

Describe the Feature Request

Currently, checking that we're moving towards our common goal -> working rpc for ethcc requires lots of manipulations.

It'd be easier to use a eth sdk plugged onto eth-rpc

Describe Preferred Solution

Investigate during one day a clean architecture to create integration tests for eth-rpc that can:

Eikix commented 1 year ago

After a day of investigation, comment down here your findings and propose an architecture for integration tests.

jobez commented 1 year ago

shared findings below with monsier Kariy of dojo and they gracefully provided me this branch to experiment w/ https://github.com/dojoengine/dojo/tree/custom-steps

tried deploying kakaswap on katana manually and am running into the steps issue: ``` ValidateTransactionError(VirtualMachineExecutionErrorWithTrace { trace: "Error in the called contract (0x06aff555213713b554a6120368c2f974d67d794beb0dcbacd4dc80841e8ea16d):\nError at pc=0:7:\nGot an exception while executing a hint.\nCairo traceback (most recent call last):\nUnknown location (pc=0:163)\nUnknown location (pc=0:149)\n\nError in the called contract (0x06aff555213713b554a6120368c2f974d67d794beb0dcbacd4dc80841e8ea16d):\nError at pc=0:155:\nCould not reach the end of the program. RunResources has no remaining steps.\nCairo traceback (most recent call last):\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3227)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3227)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3196)\nUnknown location (pc=0:190)\n", source: CairoRunError(VmException(VmException { pc: 7, inst_location: None, inner_exc: Hint((0, CustomHint("Error in the called contract (0x06aff555213713b554a6120368c2f974d67d794beb0dcbacd4dc80841e8ea16d):\nError at pc=0:155:\nCould not reach the end of the program. RunResources has no remaining steps.\nCairo traceback (most recent call last):\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3227)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3227)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3244)\nUnknown location (pc=0:3196)\nUnknown location (pc=0:190)\n"))), error_attr_value: None, traceback: Some("Cairo traceback (most recent call last):\nUnknown location (pc=0:163)\nUnknown location (pc=0:149)\n") })) }) ``` searching `cairo-rs` for the error string: https://github.com/search?q=repo%3Alambdaclass%2Fcairo-vm%20%20RunResources%20has%20no%20remaining%20steps&type=code Reminding myself how many steps our python vm was setup to have: https://github.com/jobez/kakarot/blob/main/tests/fixtures/starknet.py#L37 And trying to find where in Katana this is being hardcoded: for call, it is explicit https://github.com/dojoengine/dojo/blob/main/crates/katana/core/src/starknet/mod.rs#L264 it is explicitly passed as an argument to `EntryPointExecutionContext` (blockifier type) which in turn hands it down to a `RunResources` (cairo-rs type) instance: https://github.com/starkware-libs/blockifier/blob/f143c25ff3ede6b3e61876ad2cb5659aa8375dce/crates/blockifier/src/execution/entry_point.rs#L94 for executing transactions the touch point of katana to blockifier begins here: https://github.com/dojoengine/dojo/blob/main/crates/katana/core/src/starknet/mod.rs#L431 there is dispatching, but i think the touchpoint/flow from blockifier to cairo-rs when katana is executing transactions is happening here: https://github.com/dojoengine/dojo/blob/main/crates/katana/core/src/starknet/mod.rs#L431 s need to revisit later because blockifier has some type dispatching diffing with how madara sets the step limit: https://github.com/keep-starknet-strange/madara/blob/main/crates/primitives/starknet/src/transaction/mod.rs#L556

was able to deploy with a couple of changes, will share below for those interested and create a notion doc

1. ensure you have dojo set to `custom-steps` branch 2. start with ``` cargo run -- --validate-max-steps 16777216 --invoke-max-steps 16777216 --allow-zero-max-fee --gas-price 0 ``` 3. deploy kakarot (if you need more detailed instructions here, ping me) 4. deploy kakaswap (same as above)
jobez commented 1 year ago

Ok, here's my proposal:

I can start using this katana branch: https://github.com/dojoengine/dojo/compare/main...custom-steps.

We will pass an Environment object with the necessary steps we need (2**18, per python -> 16777216) to the TestSequencer https://github.com/dojoengine/dojo/blob/4e87bcb4437961537596debfcc49df134ef8ad5a/crates/katana/core/src/starknet/config.rs#L56, like so: https://github.com/dojoengine/dojo/blob/4e87bcb4437961537596debfcc49df134ef8ad5a/crates/katana/rpc/tests/starknet.rs#L23

I did an exploration today and have found it capable of deploying and interacting with kakaswap.

First PR would probably be more calls on the opcodes contract and erc20 https://github.com/kkrt-labs/kakarot/blob/main/tests/integration/solidity_contracts/Solmate/test_erc20.py

Sensible?

Eikix commented 1 year ago

Very sensible! I like it a lot.

I see you've been assigned to https://github.com/kkrt-labs/kakarot/issues/617, do you want to pause this issue and work on the integration tests?

In any case, this is a great idea, really. I guess the flow of a first PR could be:

Then in a subsequent PR -> same flow, but try to deploy a smart contract and call it?

jobez commented 1 year ago

yeah, the exploration here is to try to also use forge scripting to do the deploying/interacting from within a TestSequencer test. this configuration gives us

that issue that you mentioned is blocked a bit until i get more info about the error state that the issue is reporting. lmk if you feel differently and see another way forward with it 👂

ClementWalter commented 1 year ago

I think that rn we want to focus only on tests using forge and drop ethers-rs because it's higher level (a regular devstack will probably have either forge or hardhat, but not ethers-rs)

Once we're ok with the foundry stack (probably not tomorrow indeed), then we'll add the hardhat one.

I see that we already defined a foundry deps in crates/core/Cargo.toml, so I guess that we could invoke the forge script programmatically as well, in a test using the TestSequencer as you suggest.

ClementWalter commented 1 year ago

TL;DR for me is:

These tests will be run to make sure that we are compatible with the tooling, not to make sure that the EVM behaves correctly: the tests in Kakarot EVM insure that the EVM is sound, the tests in Kakarot RPC insure that we manage to communicate with the Kakarot EVM with the standard EVM tooling

jobez commented 1 year ago

tldr:

(possible) next steps:

Open for feedback 👂

# attempting to run the forge script from the cli as a shell command I was hitting this blocker: ``` 2023-07-10T20:43:41.939552Z DEBUG Connection{peer=Client}: h2::proto::connection: Connection::poll; connection error error=GoAway(b"", NO_ERROR, Library) 2023-07-10T20:43:41.939607Z DEBUG Connection{peer=Client}: rustls::common_state: Sending warning alert CloseNotify ## Setting up (1) EVMs. Error: Failure on receiving a receipt for 0x04162693c2f517c7caa67b864f99525a7a4eccc96b86b34fc9c89e011ac08504: (code: -32001, message: JSON-RPC error: code=53, message="Unsupported transaction version", data: None) Failure on receiving a receipt for 0x00492b8cd91153f4f09facd19373b9218bc977034afa4cbb41b74ee0a129c94a: (code: -32001, message: JSON-RPC error: code=53, message="Unsupported transaction version", data: None) Add `--resume` to your command to try and continue broadcasting the transactions. [crates/eth-rpc/tests/rpc.rs:1090] child.wait() = Ok( ExitStatus( unix_wait_status( 256, ), ), ) ``` The way that forge is checking for receipts is, too, async: https://github.com/foundry-rs/foundry/blob/master/cli/src/cmd/forge/script/receipts.rs#L132 so I found myself debugging a pretty complex async test that was starting a shell command starting another complicated async system. I also tried running it with the `--slow` flag, which makes the transaction process sequential, but was still getting issues. Elias was kind enough to also suggest a simpler structure to my draft: https://github.com/kkrt-labs/kakarot-rpc/commit/4aac8dd35b6aa7aceaec980096dca86c262d56fb It is worth exploring if it is more straight forward if we call directly from rust, see: https://github.com/foundry-rs/foundry/blob/afdbbc05cc479468b15a6f42b577b62e0fd4895e/forge/tests/it/fork.rs#L55 # Actually using forge to interact with a deployed contract The next question is whether you can both deploy and interact with a contract in a forge script. When I tried adding `counter.inc()` to the forge, I am getting this error: ``` [1114577] → new PlainOpcodesScript@0x5b73C5498c1E3b4dbA84de0F1833c4a029d90519 └─ ← 5346 bytes of code [9079256848778920862] PlainOpcodesScript::run() ├─ [0] VM::envUint(EVM_PRIVATE_KEY) [staticcall] │ └─ ← ├─ [0] VM::startBroadcast() │ └─ ← () ├─ [0] → new @0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0 │ └─ ← 116320 bytes of code └─ ← "EvmError: Revert" ``` I was able to interact with a deployed contract using cast: `cast call 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512 "count()" `, so its not as simple as the forge deploy not working. Need to explore more.
Eikix commented 1 year ago

Thank you for submitting a detailed recollection of your findings! It really helps. My intuition is that even if we have hive tests (end-to-end tests) and ethereum/tests, It has some value to have eth-rpc tests that utilize test-sequencer, in order to have more granularity in tests,

Maybe this can wait a little for when we have more bandwidth

Eikix commented 1 year ago

What is the status of this issue? I believe @ftupas already addressed this? Can close?