Move storage utils into storage pkg#15
Conversation
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/crypto" | ||
| "github.com/makerdao/vulcanizedb/libraries/shared/factories/storage" | ||
| storage_factory "github.com/makerdao/vulcanizedb/libraries/shared/factories/storage" |
There was a problem hiding this comment.
Doesn't need to be addressed in this PR, but I'm not totally clear on how we distinguish what goes in /factories/storage vs /storage - and the fact that they're both useful for storage transformers probably makes this kind of aliasing necessary all over the place. Would be cool to reorganize stuff so that we only have one storage package likely to be imported in a given namespace
There was a problem hiding this comment.
Totally agree! I think this fits into the bigger restructuring of the project that we've mentioned - I'll add a story so we can set aside some time to do some deeper digging into how we want to structure things.
There was a problem hiding this comment.
| "github.com/ethereum/go-ethereum/rlp" | ||
| "github.com/ethereum/go-ethereum/statediff" | ||
| "github.com/makerdao/vulcanizedb/libraries/shared/storage/utils" | ||
| storage_pkg "github.com/makerdao/vulcanizedb/libraries/shared/storage" |
There was a problem hiding this comment.
On line #63 we use storage as a reference to an account's storage, so we get a naming collision. I suppose instead of using a pkg alias, I could rename the storage variable to be accountStorage or something:
for _, accountStorage := range account.Storage {
diff, formatErr := storage_pkg.FromGethStateDiff(account, stateDiff, accountStorage)
...
}
Do you have a preference? 🤔
There was a problem hiding this comment.
Yeah I generally prefer renaming vars vs imports, but it's no big deal - just drawing from https://github.com/golang/go/wiki/CodeReviewComments#imports
|
|
||
| type KeysLoader interface { | ||
| LoadMappings() (map[common.Hash]utils.StorageValueMetadata, error) | ||
| LoadMappings() (map[common.Hash]storage.StorageValueMetadata, error) |
There was a problem hiding this comment.
Can probably rename this to just ValueMetadata if it lives in the storage pkg
| metadata, ok = lookup.mappings[key] | ||
| if !ok { | ||
| return metadata, utils.ErrStorageKeyNotFound{Key: key.Hex()} | ||
| return metadata, storage.ErrStorageKeyNotFound{Key: key.Hex()} |
There was a problem hiding this comment.
Similar thing here - thinking storage.ErrKeyNotFound would be sufficient
| var ( | ||
| fakeMetadata = utils.GetStorageValueMetadata("name", map[utils.Key]string{}, utils.Uint256) | ||
| lookup storage.KeysLookup | ||
| fakeMetadata = storage.GetStorageValueMetadata("name", map[storage.Key]string{}, storage.Uint256) |
| } | ||
|
|
||
| func (transformer Transformer) Execute(diff utils.PersistedStorageDiff) error { | ||
| func (transformer Transformer) Execute(diff storage.PersistedStorageDiff) error { |
| ID: rand.Int63(), | ||
| HeaderID: fakeHeaderID, | ||
| RawStorageDiff: utils.RawStorageDiff{ | ||
| RawStorageDiff: storage.RawStorageDiff{ |
|
Cool to re-target this PR at staging now that #8 is merged? |
This was cut from #8.
In lieu of renaming
libraries/shared/storage/utils->libraries/shared/storage/diffI just moved those files out into the mainlibraries/shared/storagedir. I didn't see a huge need for another package within thestoragepackage, since all of the storage interactions in here are with storage diffs specifically. Happy to have change this if folks think that adiffdirectory would be helpful in breaking this package up some more.