Conversation
rmulhol
left a comment
There was a problem hiding this comment.
Looking really good! Planning to take a closer look later, but dropping some initial thoughts
42eae41 to
54145b6
Compare
rmulhol
left a comment
There was a problem hiding this comment.
Looks good to me - a couple minor comments, would be curious to hear your thoughts, nothing merge blocking
| all_urns.block_height, ink, coalesce(art, 0), created, updated | ||
| FROM api.urn_snapshot | ||
| WHERE block_height <= all_urns.block_height | ||
| ORDER BY urn_identifier, ilk_identifier, updated DESC) AS latest_urns |
There was a problem hiding this comment.
Can we just ORDER BY updated here (omitting urn_identifier, ilk_identifier)? Also, do we need AS latest_urns?
There was a problem hiding this comment.
Unfortunately we can't omit urn_identifier and ilk_identifier from ORDER BY because they are in the DISTINCT above. I'm not sure about latest_urns, I have to double-check.
| AS $_$ | ||
|
|
||
| SELECT * | ||
| FROM api.all_urns(get_urn.block_height) |
There was a problem hiding this comment.
Seems kinda heavyweight to query all_urns for one urn - might it be better to:
SELECT * FROM api.urn_snapshot WHERE ilk_identifier = x AND urn_identifier = y AND block_height <= n ORDER BY block_height DESC LIMIT 1
There was a problem hiding this comment.
I was trying to be consistent with the other ones that use all_urns, but you're probably right.
| AS $$ | ||
| SELECT * | ||
| FROM api.get_urn( | ||
| (SELECT identifier FROM maker.ilks WHERE ilks.id = flip.ilk_id), |
There was a problem hiding this comment.
Hm, this is way out of scope for this PR, but it seems like api.get_flip should be returning a maker.flip and we should be deleting api.flip_bid_snapshot - basically a story to do exactly what you're doing here for a different place where we've got a type used in queries + a trigger table
There was a problem hiding this comment.
Mostly just caught my eye because we have an ilk_id and urn_id on that type, instead of the identifiers - which seems odd. If we're going to have a custom type for queries, I would have thought we would have done the join to get the identifiers in the query and not exposed internal DB FKs
There was a problem hiding this comment.
I'm not quite sure I followed that last bit - is there an action item here besides a story about flip_bid_snapshot?
There was a problem hiding this comment.
No, sorry - kinda just thinking out loud. Will create stories for this stuff
e245ab2 to
591a585
Compare
This replaces the complicated all_urns with a much simpler SQL call that goes right at the urn_snapshots, now that we have them.
In addition remove the all_urn_states query test because that query goes away, the get_urn_query_test because that query also goes away, and the computed columns tests because those functions go away as well. Wee! Finally convert any place that uses urn_state and use urn_snapshot directly in the return types.
This is so you can tell what field is actually failing.
To make sure we return the urns in a predictable order I need to make the query for distinct urn_snapshots a subquery, because the order by updated only applies within the distinct grouping, to ensure we get the latest urn. I also added a test for this query.
Assuming all_urns only returns unique individual urns (as it should) using this in the other queries should mean you'll always only get one urn.
591a585 to
2dbb3d7
Compare
I did this as three migrations. The first to replace the all_urns function with a much simpler one. The second to remove all references to urn_state, and the third to rename obsolete_urn_state to obsolete_urn_snapshot. The code changes (non SQL edition) are almost all around tests.