(VDB-1064) Query historical_ilk_states table instead of using get_ilk#92
(VDB-1064) Query historical_ilk_states table instead of using get_ilk#92
Conversation
e184dea to
fdb5f67
Compare
| -- +goose StatementBegin | ||
|
|
||
| -- Function returning the history of a given ilk as of the given block height | ||
| CREATE FUNCTION api.all_ilk_states(ilk_identifier TEXT, block_height BIGINT DEFAULT api.max_block(), |
There was a problem hiding this comment.
This function should be totally obsolete now, since its functionality can replicated by filtering the allHistoricalIlkStates function that postgraphile exposes
fdb5f67 to
bbce647
Compare
rmulhol
left a comment
There was a problem hiding this comment.
Impressively small diff 😍
| -- Extend frob_event with ilk_state | ||
| CREATE FUNCTION api.frob_event_ilk(event api.frob_event) | ||
| RETURNS api.ilk_state AS | ||
| RETURNS api.historical_ilk_state AS |
There was a problem hiding this comment.
Probably missing something but I can't see where in the migrations the api.historical_ilk_state is defined 🤔
I see it in the schema but wanting to double check that the relevant migration (with the name change) was checked in
There was a problem hiding this comment.
Yeah, historical_ilk_state is the name of the trigger table.
It got moved around in the migration order, which is the only reason it's appearing in the schema diff
| CreateCatRecords(db, header, valuesMap, catMetadatas, catRepo) | ||
| CreateJugRecords(db, header, valuesMap, jugMetadatas, jugRepo) | ||
| CreateSpotRecords(db, header, valuesMap, spotMetadatas, spotRepo) | ||
| if len(vatMetadatas) >= 1 { |
There was a problem hiding this comment.
What's the rationale behind this predicate? Are there places where we want to create metadata for non-vat contracts but not for the vat?
There was a problem hiding this comment.
Yeah, we're doing that a few times in the tests for get_ilk. E.g. https://github.com/makerdao/vdb-mcd-transformers/blob/vdb-1064-replace-ilk-query/transformers/component_tests/queries/get_ilk_query_test.go#L233
So I guess we could get rid of this conditional when we delete the get_ilk function, after the ilk_state type is deleted.
There was a problem hiding this comment.
Awesome. My initial thought is to wonder whether those tests correctly simulate realistic conditions, but I suppose it probably makes sense that we'd want to be able to verify that the records are correct even if some important data is missing.
This PR replaces the
api.get_ilkfunction with queries to the more performantapi.historical_ilk_statetable. We're still usingget_ilkin some of our tests (specifically theilk_statecomputed columns tests), but once we refactor theall_ilksquery to usehistorical_ilk_states, we should be able to delete theilk_statetype altogether, along with its computed columns.