NethermindEth / cairo-vm-go

A virtual machine for Cairo written in Go
MIT License
82 stars 50 forks source link

Feat built in name literals #662

Closed danielcdz closed 2 months ago

danielcdz commented 2 months ago

Summary

danielcdz commented 2 months ago

I wanted to use the names in starknet.go but this causes an import cycle between this file and builtins, which can be the best approach here?

cicr99 commented 2 months ago

Hi @danielcdz . Sorry for the late reply! I see what you mean with the cyclic dependency issue. The reason why builtin module uses the starnetParser module is just because of this "enum" for builtins that defined in starknet.go file. Our suggestion for this is to move that definition to pkg/vm/builtins/builtin_runner.go, so all the logic regarding builtins is in the same place. I think it would be more organized that way and should remove the issue with the dependencies. Only think you'll have to do some adjustments in the imports for the rest of the code.

danielcdz commented 2 months ago

@cicr99 working on your suggestions!

danielcdz commented 2 months ago

I'm not 100% sure if I replaced all the strings

cicr99 commented 2 months ago

There could be other places where you could replace the string with the name variable, for example here. I just typed "bitwise" in the searcher of the editor in the root of the project and checked the results to find this case. I guess doing the same for the rest of the builtins should be enough to make sure we haven't miss other places.

danielcdz commented 2 months ago

@cicr99 all comments addresed, unit and integrations tests are passing, all possible references to builtin names changed to their variables instead of comparing the strings, I think this should be ready to go 👍