kkrt-labs / kakarot

Kakarot is a zkEVM written in Cairo, leveraging the STARK proof system.
https://kakarot.org
MIT License
964 stars 276 forks source link

tests: improve `AUTH` tests #1121

Closed enitrat closed 4 months ago

enitrat commented 4 months ago

The current auth tests can be improved https://github.com/kkrt-labs/kakarot/blob/65d1976d29f4937ef5a267abbcddeb29ee7bb937/tests/src/kakarot/instructions/test_system_operations.py#L58

KeneePatel commented 4 months ago

can I hope on this?

enitrat commented 4 months ago

sure! ping me if you need help

KeneePatel commented 4 months ago

Hey @enitrat ,

I tried researching around this topic. Correct me if I am wrong at any point. I believe the AUTH and AUTH CALL relates to the EIP 3074. So far, I got to understand the function of creating the stack and memory to a certain extent. I also familiarized myself with the fixture decorators and syscall function mocks. But I am not too sure how I will get to set the AUTH before calling the cairo_run. So, I will be pleased if you can give some directions on that.

Also, while getting the tests to run, I noticed that it's mentioned to run make unit-tests in the readme, which I believe is a small typo and should be make unit-test. Should I change that in the coming pr as well?

enitrat commented 4 months ago

Hi @KeneePatel,

how I will get to set the AUTH before calling the cairo_run

in test_system_operations.cairo you can instantiate an EVM frame with let evm = TestHelpers.init_evm_at_address(0, bytecode, 0x1234, invoker_address);

you can modify this frame to set evm.message.authorized to have a defined value

I noticed that it's mentioned to run make unit-tests in the readme, which I believe is a small typo and should be make unit-test. Should I change that in the coming pr as well?

You can change the readme

KeneePatel commented 4 months ago

Hey @enitrat,

Do you have some suggestions on what I can do so that the authority that is the invoker address will have some code?

enitrat commented 4 months ago

@KeneePatel you can add a syscall patch to the test, so that calling IAccount.get_bytecode returns something, e.g. @SyscallHandler.patch("IAccount.bytecode", lambda addr, data: [3, 0x1, 0x2, 0x3])

The signature of IAccount.bytecode is func bytecode() -> (bytecode_len: felt, bytecode: felt*).

Therefore if you mock it's return data to have a non-empty bytecode_len, that will achieve the behavior you want

enitrat commented 4 months ago

@KeneePatel you can add a syscall patch to the test, so that calling IAccount.get_bytecode returns something, e.g. @SyscallHandler.patch("IAccount.bytecode", lambda addr, data: [3, 0x1, 0x2, 0x3])

The signature of IAccount.bytecode is func bytecode() -> (bytecode_len: felt, bytecode: felt*).

Therefore if you mock it's return data to have a non-empty bytecode_len, that will achieve the behavior you want

KeneePatel commented 4 months ago

Hey @enitrat,

The test for verifying that if authority has code works just fine thanks to you.

But trying to set the authority of the message in evm: https://github.com/kkrt-labs/kakarot/blob/65d1976d29f4937ef5a267abbcddeb29ee7bb937/tests/src/kakarot/instructions/test_system_operations.cairo

by adding the lines:

    let new_option = new model.Option(is_some=1, value=invoker_address);
    evm.message.authorized = new_option;

gives an error:

E       starkware.cairo.lang.compiler.preprocessor.preprocessor_error.PreprocessorError: :41:22: The use of 'new' in reference expressions is not allowed.
E           let new_option = new model.Option(is_some=1, value=invoker_address);

Is this the right way to do?

enitrat commented 4 months ago

Since new returns a pointer, you need to store that pointer in memory using tempvar rather than just a reference with let

enitrat commented 4 months ago

Also you can't mutate the evm.message field like that unfortunately :/ you need to re-build the two entire structs.

If that's too complex, you can just push what you have for now