NethermindEth / cairo-vm-go

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

Create a structure for Scope value types instead of using interface #347

Closed cicr99 closed 3 months ago

cicr99 commented 6 months ago

Currently the ScopeManager contains a map[string]any to represent a scope, where the key is the name of the variables present in the scope and the value would be the value of such variable. any is being used because we cannot know the type of this value beforehand. As this is used in python code you could get a felt or a bigint, or a mapping, etc. However, the use of any is not very efficient, so the proposed solution to improve this is to create a struct ScopeValue which will hold the value of the variables. It could look like the following:

type ScopeValue struct {
    FeltValue   *fp.Element
        BigIntValue *Big.Int
}

We should add all possible types in there; it could be done on demand, as we go implementing new hints. Another idea could be to create a ScopeValueType enum for all these possible types and add a field Type ScopeValueType to the previous struct to know the type of the value without having to check which member is not nil

fishonamos commented 6 months ago

Hi @cicr99 . Can this be assigned to me?

cicr99 commented 6 months ago

Hey @fishonamos I've just assigned you to #345 as you also requested . I would like you to finish that one before assigning you to another if that's ok

danielcdz commented 6 months ago

Hello, @cicr99! I can help with this, can you assign it to me?

fmmesen commented 6 months ago

Hello @cicr99 . I'd like to contribute to this issue, could you please assign it to me?

rodrigo-pino commented 6 months ago

Hey @fmmesen it's yours!

@danielcdz sorry for replying so late, if you are still interested you're free to pick any open issue you like!

fmmesen commented 6 months ago

Thanks @rodrigo-pino ! Is there a communication channel for the project?

rodrigo-pino commented 6 months ago

Thanks @rodrigo-pino ! Is there a communication channel for the project?

Hey yes, there is a Telegram group, you will be able to find the link on the Only Dust Hack channel

fmmesen commented 6 months ago

Hello! @rodrigo-pino @cicr99 PR #369 is ready for review. Thanks!

danielcdz commented 5 months ago

@rodrigo-pino @cicr99 it's me again sorry for the delay, but if this is something I can help with, I'll be happy to work, this will be my third contribution to the project!

cicr99 commented 4 months ago

Hey @danielcdz, the solution described in this issue is no longer applicable. As we continued to implement hints, turns out that you could get a lot more of possible types for the variables you want to store in the Scope, which would mean we would have to create a field for each of them in the ScopeValue struct. The idea here is to optimize the current code, and this doesn't seem a like a big improvement, nor an elegant solution. We need to come up with a better idea, so any suggestions are welcome

danielcdz commented 4 months ago

Hello, @cicr99! I have to clarify my idea and proposal, but I think that this can be an opportunity to use the reflect package from Golang

danielcdz commented 4 months ago

@cicr99 I was looking at the code and the usages for the ScopeManager and I think using reflect would not optimize much, and we can recur to many changes on the current architecture of the ScopeManager, also, for storing multiple types the best way is to continue using map[string]any, to have an easy way to store and work with numerous types in the scope, on the other side, what I think we can change and I think it is something that we want to avoid is type casting when retrieving a variable from the scope, like here(not directly a type casting but using a specific method to retrieve the value as BigInt), and here, a good way to tackle this is using generics, and creating a method inside scope.go like this one i.e:


// GetVariableAs retrieves a variable from the current scope and asserts its type
func GetVariableAs[T any](sm *ScopeManager, name string) (T, error) {
    var zero T // Zero value of the generic type
    value, ok := sm.GetVariableValue(name)
    if !ok {
        return zero, errors.New("variable not found")
    }
    typedValue, ok := value.(T)
    if !ok {
        return zero, errors.New("variable has a different type")
    }
    return typedValue, nil
}

and example of usage will looks like this:

 // Retrieve variables with type assertion
    fpVar, err := ctx.ScopeManager.GetVariableAs[fp.Element]("fpVar")
    if err != nil {
        fmt.Println("Error:", err)
    } else {
        fmt.Println("intVar:", intValue)
    }

Doing this will be easy to implement keeping an easy way to store unknown first-hand types in the scope, and optimizing the way to retrieve the values of variables.

I will wait for your response! if this is not a good approach I can keep researching to solve this in the best way!

TAdev0 commented 4 months ago

hey @danielcdz, sorru for the very late answer.

sounds like a nice idea to me. Will make the code cleaner overall but it doesnt improve efficiency at all, it is just some kind of elegant refactoring. You need anyway to check !ok when doing typedValue, ok := value.(T) but there seem to be no other way if we keep using any

@cicr99 what do you think?

TAdev0 commented 4 months ago

just noticed this issue :

https://github.com/NethermindEth/cairo-vm-go/issues/448

which basically suggests the same

danielcdz commented 4 months ago

@TAdev0 Don't worry, and yes it would be a elegance a better way to retrieve the variables value but if we keep using any, I don't see other way to improve performance, other than adding specific types to the struct