Add functionalities for main function inputs #681
Conversation
…arguments and initial gas to the memory
* Add parsing logic for input user args * Add flags for available gas, input user args, writing args to memory * Fix unit tests for user arguments parsing * Lint the PR * Add user args to hint context * Refactor the code * Fix unconditional append of ExternalWriteArgsToMemory, bug fixes in integration tests * Add fixes of the call size calculation and include ExternalWriteArgsToMemory hint when gas present * Add layouts for integration tests * Add error handling * Fixes in entry code generation * Address changes mentioned in a discussion * Add comment regarding writing to memory in a hint for the future reference in the integration tests with args * Changes in calculations of the initial PC offset, CALL opcode offset incremented by mainFuncOffset, writing user args to the AP in the hint * Turn back VM config to private field * Add error handling on assign of `userArgs` to the initial scope * Lint project * Bump go version from 1.20 -> 1.21 (#678) * Bump go version from 1.20 -> 1.21 * Update golangci-lint * Simplify the Makefile * Correction in the makefile
…ration tests pipeline
MaksymMalicki
changed the title
| return fmt.Errorf("get gas: %v", err) | ||
| } | ||
| gasVal := mem.MemoryValueFromUint(gas) | ||
| err = vm.Memory.Write(1, vm.Context.Ap, &gasVal) |
There was a problem hiding this comment.
just confirming here, gas value is written to the execution segment, right?
integration_tests/cairo_vm_test.go
Outdated
| {"./cairo_1_programs/", false}, | ||
| {"./cairo_1_programs/dict_non_squashed", false}, | ||
| {"./cairo_1_programs/with_input", false}, |
There was a problem hiding this comment.
Just a comment here, for testing purposes this makes sense atm, but in only
{"./cairo_1_programs/", false},
will be needed. That should traverse the entire folder, including the subfolders.
There was a problem hiding this comment.
Plus these should probably stay commented for the current integration tests to pass
There was a problem hiding this comment.
so turns out we aren't doing the recursive in-depth lookup of the directories, so we need to provide the nested folders as roots. When the nested file is a dir, then we skip it in the loop. I commented them out
pkg/hintrunner/hintrunner.go
Outdated
| @@ -15,14 +15,15 @@ type HintRunner struct { | |||
| hints map[uint64][]h.Hinter | |||
| } | |||
|
|
|||
| func NewHintRunner(hints map[uint64][]h.Hinter, userArgs []starknet.CairoFuncArgs) HintRunner { | |||
| func NewHintRunner(hints map[uint64][]h.Hinter, userArgs []starknet.CairoFuncArgs, writeApOffset uint64, availableGas uint64) HintRunner { | |||
There was a problem hiding this comment.
Just to think about it. Perhaps it would be better to jus pass a hintContext as a second parameter in this function, instead of passing all those variables. I see in the tests this is not being taken into account, so perhaps in that case you can pass nil and that means to build a default one
There was a problem hiding this comment.
yes, now that I thought about it, I agree
| context := *h.InitializeDefaultContext() | ||
| if userArgs != nil { | ||
| err := context.ScopeManager.AssignVariable("userArgs", userArgs) | ||
| // Error handling: this condition should never be true, since the context was initialized above |
There was a problem hiding this comment.
I would like to keep this comment, because we usually shouldn't have to use panic instead of handling the error. The fact is that in this case, that line should never be reached, so the comment would be flagging that.
There was a problem hiding this comment.
Brought the comment back
pkg/hintrunner/hintrunner_test.go
Outdated
| @@ -20,7 +20,7 @@ func TestExistingHint(t *testing.T) { | |||
|
|
|||
| hr := NewHintRunner(map[uint64][]hinter.Hinter{ | |||
| 10: {&allocHint}, | |||
| }, nil) | |||
| }, nil, 0, 0) | |||
There was a problem hiding this comment.
This is what I meant in the comment above about the NewHintRunner method
There was a problem hiding this comment.
Yes, after adding the functionality for creating a new hinturnner context, those were removed
pkg/vm/builtins/builtin_runner.go
Outdated
| @@ -25,6 +25,7 @@ const ( | |||
| AddModeType | |||
| MulModType | |||
| GasBuiltinType | |||
| SystemType | |||
There was a problem hiding this comment.
Nope, this was not removed 😅
No description provided.