-
Notifications
You must be signed in to change notification settings - Fork 185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement logic to build genesis state for the sequencer #2371
base: main
Are you sure you want to change the base?
Conversation
for k, v := range aux.Contracts { | ||
key, err := new(felt.Felt).SetString(k) | ||
if err != nil { | ||
return err | ||
} | ||
g.Contracts[*key] = v | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to do dereferencing? Just do:
for k, v := range aux.Contracts {
var key felt.Felt
key, err = key.SetString(k)
if err != nil {
return err
}
g.Contracts[key] = v
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, in your approach key is allocated to the stack and not the heap, whereas in the original case key is allocated to the heap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the motivation for this change so that key is allocated to the stack rather than the heap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested change doesn't work, since SetString
only operates on a pointer
func (z *Felt) SetString(number string) (*Felt, error) {
and initialising a nil pointer will result in a panic when calling SetString
(ie using var key *felt.Felt
)
wip - fix test fix stuff
fb11e07
to
459db5d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2371 +/- ##
==========================================
- Coverage 74.70% 74.51% -0.19%
==========================================
Files 110 111 +1
Lines 12021 12313 +292
==========================================
+ Hits 8980 9175 +195
- Misses 2349 2419 +70
- Partials 692 719 +27 ☔ View full report in Codecov by Sentry. |
@@ -39,3 +40,52 @@ func AdaptOrderedEvents(events []vm.OrderedEvent) []*core.Event { | |||
}) | |||
return utils.Map(events, AdaptOrderedEvent) | |||
} | |||
|
|||
func AdaptStateDiff(trace *vm.TransactionTrace) *core.StateDiff { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I can see we only use trace.StateDiff
in this function, why do we pass trace
as a parameter?
newNonce := new(felt.Felt).SetUint64(1) | ||
p.stateDiff.Nonces[*contractAddress] = newNonce.Add(currentNonce, newNonce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have felt.Zero, can we have felt.One?
newNonce := new(felt.Felt).SetUint64(1) | |
p.stateDiff.Nonces[*contractAddress] = newNonce.Add(currentNonce, newNonce) | |
p.stateDiff.Nonces[*contractAddress] = currentNonce.Add(currentNonce, &felt.One) |
} | ||
|
||
func (p *PendingStateWriter) SetCompiledClassHash(classHash, compiledClassHash *felt.Felt) error { | ||
// assumption: SetContractClass was called for classHash and succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make these assumptions a part of the function’s doc comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have finished reviewing everything except vm logic. Everything is good, just small changes and questions. I'll continue reviewing vm logic.
return nil | ||
} | ||
stateDiff := trace.StateDiff | ||
newStorageDiffs := make(map[felt.Felt]map[felt.Felt]*felt.Felt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could reduce memory allocation this way
newStorageDiffs := make(map[felt.Felt]map[felt.Felt]*felt.Felt) | |
newStorageDiffs := make(map[felt.Felt]map[felt.Felt]*felt.Felt, len(stateDiff.StorageDiffs)) |
mergeStorageDiffs := func(oldMap, newMap map[felt.Felt]map[felt.Felt]*felt.Felt) { | ||
for addr, newAddrStorage := range newMap { | ||
if oldAddrStorage, exists := oldMap[addr]; exists { | ||
maps.Copy(oldAddrStorage, newAddrStorage) | ||
} else { | ||
oldMap[addr] = newAddrStorage | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just inline this function?
type auxConfig struct { | ||
Classes []string `json:"classes"` // []path-to-class.json | ||
Contracts map[string]GenesisContractData `json:"contracts"` // address -> {classHash, constructorArgs} | ||
FunctionCalls []FunctionCall `json:"function_calls"` // list of functionCalls to Call() | ||
BootstrapAccounts []Account `json:"bootstrap_accounts"` // accounts to prefund with strk token | ||
Txns []*rpc.Transaction `json:"transactions"` // declare NOT supported | ||
} | ||
var aux auxConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I can see, we use auxConfig
once.
type auxConfig struct { | |
Classes []string `json:"classes"` // []path-to-class.json | |
Contracts map[string]GenesisContractData `json:"contracts"` // address -> {classHash, constructorArgs} | |
FunctionCalls []FunctionCall `json:"function_calls"` // list of functionCalls to Call() | |
BootstrapAccounts []Account `json:"bootstrap_accounts"` // accounts to prefund with strk token | |
Txns []*rpc.Transaction `json:"transactions"` // declare NOT supported | |
} | |
var aux auxConfig | |
var aux struct { | |
Classes []string `json:"classes"` // []path-to-class.json | |
Contracts map[string]GenesisContractData `json:"contracts"` // address -> {classHash, constructorArgs} | |
FunctionCalls []FunctionCall `json:"function_calls"` // list of functionCalls to Call() | |
BootstrapAccounts []Account `json:"bootstrap_accounts"` // accounts to prefund with strk token | |
Txns []*rpc.Transaction `json:"transactions"` // declare NOT supported | |
} |
validate := validator.Validator() | ||
return validate.Struct(g) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we inline validate
?
validate := validator.Validator() | |
return validate.Struct(g) | |
return validator.Validator().Struct(g) |
for classHash, class := range newClasses { | ||
// Sets pending.newClasses, DeclaredV0Classes, (not DeclaredV1Classes) | ||
if err = genesisState.SetContractClass(&classHash, class); err != nil { | ||
return nil, nil, fmt.Errorf("declare v0 class: %v", err) | ||
} | ||
|
||
if cairo1Class, isCairo1 := class.(*core.Cairo1Class); isCairo1 { | ||
if err = genesisState.SetCompiledClassHash(&classHash, cairo1Class.Compiled.Hash()); err != nil { | ||
return nil, nil, fmt.Errorf("set compiled class hash: %v", err) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use descriptive variable names instead of this comment?
// Sets pending.newClasses, DeclaredV0Classes, (not DeclaredV1Classes)
I don't quite understand why only DeclaredV0Classes
in newClasses
.
} | ||
|
||
default: | ||
panic("transaction type not supported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic("transaction type not supported") | |
return nil, nil, fmt.Errorf("unsupported transaction type: %v", txn.Type) |
fmt.Println("==") | ||
fmt.Println(genesisSD) | ||
fmt.Println(traceSD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Println("==") | |
fmt.Println(genesisSD) | |
fmt.Println(traceSD) |
} else if compiledClass, cErr := compiler.Compile(response.V1); cErr != nil { | ||
return nil, cErr | ||
} else if coreClass, err = sn2core.AdaptCairo1Class(response.V1, compiledClass); err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code is easier to understand
} else if compiledClass, cErr := compiler.Compile(response.V1); cErr != nil { | |
return nil, cErr | |
} else if coreClass, err = sn2core.AdaptCairo1Class(response.V1, compiledClass); err != nil { | |
return nil, err | |
} | |
} else { | |
var compiledClass *starknet.CompiledClass | |
if compiledClass, err = compiler.Compile(response.V1); err != nil { | |
return nil, err | |
} | |
if coreClass, err = sn2core.AdaptCairo1Class(response.V1, compiledClass); err != nil { | |
return nil, err | |
} | |
} |
// StateDiffAndClasses returns the pending state's internal data. The returned objects will continue to be | ||
// read and modified by the pending state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, could we make these fields public?
This PR implements the
genesis
pkg that builds astate diff
from declaring classes, deploying contracts and invoking functions. The sequencer can then commit thisstate diff
to state, so that upon startup, a set of account contracts have already been deployed and funded (it is not possible to achieve this from an empty state by submitting transactions, since Starknet requires accounts to be funded to perform any actions on the state - originally this wasn't a requirement in Starknet). This is particularly useful as it allows users to start the sequencer with any number of accounts pre-deployed, and pre-funded, etc, and opens up the possibility of using Juno as a devnetIn essence: