(VDB-955) Associate diffs with headers#24
Conversation
0750fe3 to
f8520d1
Compare
gslaughl
left a comment
There was a problem hiding this comment.
Sheesh... nice job on this! Appreciate you taking the time to clean up tests on top of everything else 😃
| FROM (SELECT DISTINCT ON (urn_id) urn_id, | ||
| block_timestamp | ||
| -- TODO: should we be using urn ink for created? | ||
| -- Can a CDP exist before collateral is locked? |
There was a problem hiding this comment.
Good point, seems worth investigating. I took a glance at the contracts and I'm not sure but it seems like urns are created via Vat.frob? And the logic around whether a CDP is safe is pretty complex, but it seems possible that an urn could be created with 0 ink, in which case we wouldn't see a diff. Might want to circle back on this in another story
| id SERIAL PRIMARY KEY, | ||
| -- TODO: remove hash? | ||
| -- Can we replace block number + hash with header_id? | ||
| -- Tricky because there might be multiple diffs contributing to a single row |
There was a problem hiding this comment.
I think it should be fine to replace block_hash with a header_id since block_hash is just being used as a mechanism to get the header's timestamp. And a header_id seems more conducive to that kind of thing. We could do the same thing with block_number, but then again that seems like something we want to expose through the api, so it might make sense to just keep it in this table.
There was a problem hiding this comment.
Yeah, going to leave this to the trigger-table-reorg-safety stories. I think it probably makes sense to remove hash and number and then fetch those things based on header_id at query time, so that we're not duplicating the data - but re-org safety and query performance are probably higher priorities that merit deeper consideration before making the change
| }) | ||
| }) | ||
|
|
||
| It("returns ilk states without timestamps if the corresponding header hasn't been synced yet", func() { |
| UNIQUE (header_id, ilk_id, dust) | ||
| ); | ||
|
|
||
| CREATE INDEX vat_ilk_dust_block_number_index |
There was a problem hiding this comment.
I wonder if for some of these the index to the header_id would be helpful 🤔
There was a problem hiding this comment.
Makes sense - will go ahead and add them, since we're joining so much on that column
| LEFT JOIN public.headers ON vat_urn_ink.header_id = headers.id | ||
| ORDER BY urn_id, block_number ASC) earliest_blocks), | ||
| updated AS (SELECT DISTINCT ON (urn_id) urn_id, api.epoch_to_datetime(block_timestamp) AS datetime | ||
| FROM ((SELECT DISTINCT ON (urn_id) urn_id, block_hash |
There was a problem hiding this comment.
Trying to think through if there could be any side effects of selecting distinct on block_hash vs block_timestamp. 🤔 Seems like it'll be okay since both block_hash and block_timestamp should both be unique.
There was a problem hiding this comment.
Good call! I think we can depend on block_timestamp being unique, but I also wonder if it might make sense to just select distinct on header_id 🤔
There was a problem hiding this comment.
Oh wait, think I misread - isn't this only selecting distinct on urn_id? And then it's just selecting the block_timestamp for each distinct urn_id so that it can use it in api.epoch_to_datetime
f8520d1 to
16bcd59
Compare
- Enables cascade delete on reorgs
- When used in JOINs
16bcd59 to
ca10c1b
Compare
Corresponding PR for sky-ecosystem/vulcanizedb#6
Diff is obnoxiously large because
header_idneeded to be added to the storage repository interface'sCreatefunction. Thought about adding theutils.StorageDiffso that we could just change that struct for similar stories in the future, but that wouldn't be able to replace themetadata(derived from the keys lookup) orvalue(derived from the decoder).With that change required, I felt it made sense to remove the
block_number+block_hashfrom the args (and parsed diff tables), since that data is already on the header (don't want to copy it in two places). Didn't want to remove it entirely from theutils.StorageDiffstruct yet since that struct is currently used for both parsing raw diffs (from the rpc/csv/etc) and passing to the keys lookup - but fixing that (using different structs for parsing raw vs keys lookup) is another change I'd be in favor or queueing up.With the
header_idadded to the tables as a not null reference to thepublic.headerstable, every component test needed a header inserted before diffs were created. The last level of changes in this PR was implementing a consistent approach for this across all of the component tests so that future cascading changes (should they occur) will at least be dealing with a more uniform suite.Appreciate everyone's time bearing with me through this - not happy the diff is so large, but excited for the feature we get in return. Hoping not to have to deal with too many rebases 😅