(VDB-1133) Add auction reorg-safety triggers#98
Conversation
d22a144 to
31fbedd
Compare
6dbdf4a to
26538ef
Compare
4d81523 to
e7fec1d
Compare
rmulhol
left a comment
There was a problem hiding this comment.
Looking good!
Curious about StatementBegin and StatementEnd - do you think our tests sufficiently exercise those? Should we scrutinize them more closely?
| } | ||
|
|
||
| func SharedBidHistoryTriggerTests(input BidTriggerTestInput) { | ||
| func BidHistoryTriggerTests(input BidTriggerTestInput) { |
There was a problem hiding this comment.
Should this file be renamed?
| Expect(valueHistory[0].Valid).To(BeFalse()) | ||
| }) | ||
|
|
||
| It("deletes ilk state associated with diff if identical to previous state", func() { |
| ON CONFLICT (block_number, bid_id, address_id) DO UPDATE SET guy = new_diff.guy | ||
| $$ | ||
| LANGUAGE sql; | ||
| -- +goose StatementEnd |
There was a problem hiding this comment.
Did you intend to remove this? Curious because I don't see a StatementBegin removed above, but I see that there is one immediately below
There was a problem hiding this comment.
Yeah this was a StatementEnd without a matching StatementBegin; it was actually causing a weird situation where tests were failing in CI but for some reason I couldn't get the same failure locally. Took a minute to troubleshoot what was going on.
As far as I can tell, Goose requires the StatementBegins and -Ends for plpgsql queries, and not for vanilla sql. Although it doesn't complain if you include them when they aren't necessary aside from the aforementioned situation where you have a Begin w/o an End or vice-versa.
There was a problem hiding this comment.
Ah that makes sense - nice catch! Maybe worth creating a story to see if we can isolate the consequences of them missing/verify they're there?
There was a problem hiding this comment.
I might be misunderstanding you, but in my experience the migrations totally fail if you don't have those statements where they're necessary. So it shouldn't be necessary to manually go through and verify that they're there, although I can see a case for making sure they're not there where they're unnecessary, for the sake of reducing clutter.
There was a problem hiding this comment.
Ok cool - as long as things break when broken 👍
|
|
||
| -- +goose StatementBegin | ||
| CREATE OR REPLACE FUNCTION maker.update_flap_guys_until_next_diff(new_diff maker.flap_bid_guy) RETURNS VOID | ||
| CREATE OR REPLACE FUNCTION maker.update_flap_guys_until_next_diff(start_at_diff maker.flap_bid_guy, new_guy TEXT) RETURNS VOID |
There was a problem hiding this comment.
What's the rationale for breaking out the new_guy param rather than just reading it off of the diff? Wondering if that could make it trickier to change the type from text -> int referencing addresses (id)
There was a problem hiding this comment.
With this new set of triggers, we had to make these update_until_next_diff functions more flexible. The DELETE triggers use the same UPDATE logic, but update snapshots to the value of the next-most-recent diff, rather than using the value already contained within the diff being deleted. See line 222.
As a side-note, I've been deliberately avoiding including FKs like address_id in the trigger-tables where possible, in order to minimize the number of JOINs necessary at request-time, since the trigger-tables were originally intended as performance-optimizations. Definitely open to reconsidering that approach though, especially where performance isn't likely to be as big of an issue.
There was a problem hiding this comment.
👍 to avoiding foreign keys in the trigger tables - would def prefer any necessary joins be handled in the trigger. Enabling re-use of the update logic in the DELETE triggers makes sense to me 👌
e7fec1d to
6def24d
Compare
6def24d to
e412eff
Compare
Here's another PR where we're making very similar changes to every auction's triggers. Plus, each individual trigger is undergoing very similar changes, with the exception of the
createdtriggers which are a little simpler, since all the records pertaining to an individual bid have the samecreatedvalue.