refactor(trie2): use more descriptive felt.Hash type instead of felt.Felt
#3316
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3316 +/- ##
==========================================
- Coverage 76.20% 76.16% -0.05%
==========================================
Files 346 346
Lines 32690 32738 +48
==========================================
+ Hits 24912 24935 +23
- Misses 5987 6009 +22
- Partials 1791 1794 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MaksymMalicki
force-pushed
the
maksym/refactor-triedb-address
branch
2 times, most recently
from
9210f6f to
090e07a
Compare
MaksymMalicki
force-pushed
the
maksym/refactor-triedb-hash
branch
from
ad10776 to
6bee396
Compare
MaksymMalicki
force-pushed
the
maksym/refactor-triedb-hash
branch
from
6a421db to
886e905
Compare
MaksymMalicki
temporarily deployed
to
Development
GitHub Actions
Inactive
MaksymMalicki
force-pushed
the
maksym/refactor-triedb-hash
branch
from
886e905 to
96d28a1
Compare
| stateCommitment := felt.NewFromUint64[felt.Felt](1000) | ||
| classRootHash := felt.NewFromUint64[felt.Felt](2000) | ||
| contractRootHash := felt.NewFromUint64[felt.Felt](3000) | ||
| stateCommitment := felt.NewFromUint64[felt.Hash](uint64(1000)) |
There was a problem hiding this comment.
nitpick -> This uint64(...) seems redundant?
| classNodes map[felt.Hash]map[trieutils.Path]trienode.TrieNode | ||
| contractNodes map[felt.Hash]map[trieutils.Path]trienode.TrieNode | ||
| contractStorageNodes map[felt.Hash]map[felt.Address]map[trieutils.Path]trienode.TrieNode |
There was a problem hiding this comment.
felt.Hash is the most general type, but I think it is being miss-used.
felt.Hash means that there is not a good type to define it and it makes sense to be a general type, for example the return type of hashing function.
On the other hand, here we know the semantic meaning each hash has here, so we should use the appropriate types.
As far as I understand, it should be:
classNodeskeys should be of typefelt.ClassHashcontractNodesshould be of typefelt.Addresssince it is a contract addresscontractStorageNodesshould be of typefelt.Addresstofelt.StorageAddress(orfelt.StorageSlot). <- The latter types are to be defined
| // Tracks all unique and latest nodes in the layer tree | ||
| classPaths map[trieutils.Path]trienode.TrieNode | ||
| contractPaths map[trieutils.Path]trienode.TrieNode | ||
| contractStoragePaths map[felt.Address]map[trieutils.Path]trienode.TrieNode |
There was a problem hiding this comment.
Is this Address referencing StorageAddress, if that's the case, then it is being miss-used.
Address strictly refers to a Starknet contract address – this has been discussed both in dailies and slack.
|
|
||
| stateComm := id.StateComm() | ||
| if stateComm.IsZero() { | ||
| if (*felt.Felt)(&stateComm).IsZero() { |
There was a problem hiding this comment.
let's avoid the use of this casting
|
|
||
| stateComm := id.StateComm() | ||
| if stateComm.IsZero() { | ||
| if (*felt.Felt)(&stateComm).IsZero() { |
| func (t *layerTracker) trackLayer(root, parent *felt.Felt) { | ||
| if root.Equal(parent) { | ||
| func (t *layerTracker) trackLayer(root, parent *felt.Hash) { | ||
| if (*felt.Felt)(root).Equal((*felt.Felt)(parent)) { |
There was a problem hiding this comment.
Avoid this second conversion
|
|
||
| // Child to parent layer relationship | ||
| childToParent map[felt.Felt]felt.Felt | ||
| childToParent map[felt.Hash]felt.Hash |
There was a problem hiding this comment.
Similar coment to the previous ones, what would be the right hash here?
This PR addresses #3282 (comment), and uses more descriptive
felt.Hashtype for owner fields in alltriedbandtrie2packages