Add ContractAddressID to cat storage tables#269
Conversation
2365a39 to
9940fc8
Compare
rmulhol
left a comment
There was a problem hiding this comment.
LGTM! A few questions, nothing blocking
| // HACK because the cat queries now take an address_id. If the other storage queries start taking an | ||
| // address_id, remove this hack | ||
| var err error | ||
| if intInsertSql == cat.InsertCatIlkChopQuery || intInsertSql == cat.InsertCatIlkLumpQuery { |
There was a problem hiding this comment.
I guess we never deal with cat_ilk_flip in here?
There was a problem hiding this comment.
Oddly no, but I guess since it's test data.
| ilkID int64 | ||
| ilkErr error | ||
| contractAddressID int64 | ||
| contractAddressErr error |
There was a problem hiding this comment.
nbd but I feel like ilkErr and contractAddressErr could be declared within the BeforeEach so that they're not in scope for subsequent It statements - seems like we only care about them in the expectations on lines 165 + 167
| err := db.Get(&wardsResult, `SELECT diff_id, header_id, address_id, usr AS key, wards.wards AS value FROM maker.wards`) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(wardsResult.AddressID).To(Equal(strconv.FormatInt(flapAddressID, 10))) | ||
| Expect(wardsResult.AddressID).To(Equal(flapAddressID)) |
|
|
||
| func (repository *StorageRepository) insertLive(diffID, headerID int64, live string) error { | ||
| _, err := repository.db.Exec(insertCatLiveQuery, diffID, headerID, live) | ||
| addressID, addressErr := shared.GetOrCreateAddress(repository.ContractAddress, repository.db) |
There was a problem hiding this comment.
Given the number of times that these inserts can happen, I think it might be cool to have a singleton-ish helper function that memoizes the addressID the first time it's called - so that we don't need to go to the DB for it every time
There was a problem hiding this comment.
fwiw we already do what I'm suggesting here: https://github.com/makerdao/vdb-mcd-transformers/blob/staging/transformers/storage/median/repository.go#L292
There was a problem hiding this comment.
That makes sense to me - originally I had this passing around the ID but I didn't care for that.
transformers/component_tests/cat/configured_transformer_test.go
Outdated
Show resolved
Hide resolved
This also updates its component test. Updating the repository test is going to cause all the cat storage types to be changed, so that will happen on the next commit.
I don't know who made the storage_repository_behaviors, but in the words of Liam Neeson, I will look for them, I will find them, and I will....
This caused a huge, and unfortunate, series of changes. - Adding to cat_ilk_flip forced adding to cat_ilk_chop and cat_ilk_lump - The data_generator needed a hack because CatIlkChop and CatIlkLump now require an addressID - InsertFieldWithIlkAndAddress was added to shared/utlities, because not every InsertFieldWithIlk needs an address - I changed MappingResWithAddress to take an int64. Since this is just in test code, it should be fine, but it caused a large number of cascading changes I didn't anticipate.
Unit tests in the repository.
These were missing, because the original changes were driven by the compiler.
- Cache the contractAddressID instead of querying it all the time. - Pass the address id to the utility function instead of the address so we don't need to look it up yet again. - Cleanup the configured transformer test a little, moving variables that aren't used repeatedly into the BeforeEach.
Where appropriate - this means you won't forget the addressID.
faf2a4c to
86485d0
Compare
A few things that make this larger than might be expected.