search

NethermindEth / cairo-vm-go

A virtual machine for Cairo written in Go
MIT License
73 stars 43 forks source link

Uint256 hints integration test #458

Closed Sh0g0-1758 closed 3 weeks ago

Sh0g0-1758 commented 1 month ago

This PR is aims to do the tasks mentioned here : #413

Current Progress :

Sh0g0-1758 commented 1 month ago

@TAdev0 have you tried it locally ? It throws an error for me when I remove the specific layout. I believe @har777 told me about it.

TAdev0 commented 1 month ago

Oh yeah I guess this is how the file is detected as using layout small, my bad

TAdev0 commented 1 month ago

indeed: 

    if strings.Contains(path, ".small") {
        layout = "small"
Sh0g0-1758 commented 1 month ago

@TAdev0 , So should I wait for another approval or can I merge right away ? I don't think the 2 approval github action is valid for branches other than main.

TAdev0 commented 1 month ago

@Sh0g0-1758 please wait for another approval so we stick to 2

Sh0g0-1758 commented 4 weeks ago

@TAdev0 , it seems like the code that you merged has a bug, the TestPoseidon is failing. 

=== RUN   TestPoseidon
    cairozero_test.go:333: 
            Error Trace:    /home/runner/work/cairo-vm-go/cairo-vm-go/integration_tests/cairozero_test.go:333
            Error:          Received unexpected error:
                            cairo-vm run builtin_tests/poseidon_test.starknet_with_keccak_compiled.json: exit status 1
                            Loading program at builtin_tests/poseidon_test.starknet_with_keccak_compiled.json
                            Running....
                            runtime error: pc 0:17 step 12: running instruction: infer res: segment 4, offset 3: range_check: check write: 2**128 < 4426822003494896[46](https://github.com/NethermindEth/cairo-vm-go/actions/runs/9553890922/job/26333768913?pr=458#step:6:47)213731521593476982257703159825582578145778919623645026501
            Test:           TestPoseidon

EDIT : nvm, The fix was pushed 6 hours ago, merged it.

TAdev0 commented 3 weeks ago

@Sh0g0-1758 can you resolve all your conflicts?

Sh0g0-1758 commented 3 weeks ago

@TAdev0 , resolved the conflicts. This good to merge ?

TAdev0 commented 3 weeks ago

@Sh0g0-1758 now you need to move all your files in _cairo_zero_hint_tests Currently they are not detected when doing make integration

Sh0g0-1758 commented 3 weeks ago

@TAdev0 , well no they are detected.

Sh0g0-1758 commented 3 weeks ago

I think we should move the tests to conform with the structure of the integration tests in a separate PR.

Sh0g0-1758 commented 3 weeks ago

If we were to move them now, they won't be detected in the CI check before merging with main.