One weird thing is that this table has the columns urn_identifier and ilk_identifier, even though those columns will only ever be not-null for flip bid events. I initially was hoping to have a separate table just for flip bid events in order to circumvent this issue, but the problem is that we might not know whether an event is from a flip, flap or flop contract when the event is inserted.

Since the structure of this table is likely to be kind of confusing, I put it in the maker schema, and made a new, hopefully more performant version of the api.urn_bid_events query that makes use of this table, rather than exposing the table directly. Although, there shouldn't really be any issue with exposing the table as long as the client is aware of what's going on here. Otherwise, we should be able to make similar performance-enhancements to other queries like all_flip_bid_events by making use of this table.

Haven't fully though this through yet, but I wonder if we'd be missing some postgraphile filtering capabilities by wrapping this table in a query. Though, I totally understand your rational for not exposing the trigger table since there will null values sometimes - way to lookout for the end user! 😄

The more I think about it, I think you're solution is 💯 and we can deal with the potential need for postgraphile filtering in the future.

Yeah I def agree that it's a bummer to negate the filtering flexibility we'd get by exposing the table directly. Which is why I think it would be ideal if we could just expose it, and somehow inform the end-user about the data's weirdness.

Although, on some level I think it's kind of self-explanatory, since nobody would really include the ilk_identifier or urn_identifier fields in a graphql query where they only wanted flop or flap events. And if they did accidentally include those columns in the query, I don't think it would be that confusing when they came back null, assuming a base level of knowledge about the different auction types. 🤷‍♂

I think we're planning to expose all three schemas in the API, if that's relevant? But agreed that having null for ilk_identifier and urn_identifier doesn't seem that confusing if you're querying against a contract address for flap/flop 🤷‍♂

Would probably be more inclined to go that route that to deliberately omit this table

One thing that's got me thinking, I wonder if we'll need to communicate anything about the need to use checksum/non-checksum addresses when querying by urn identifier 🤔

The column is case insensitive iirc, but idk if that means you'll get results if you pass in a string with different casing than whatever's in that row?

Good to know we'll be exposing all 3 schemas! In that case I guess we'd have to explicitly omit the query if we want to hide it from users, although I'm inclined to just leave it as is.

And that's a really good point about case-sensitivity, it hadn't really occurred to me. Our GetOrCreateAddress function always inserts checksummed addresses, so we should probably make sure people are aware of that. Maybe it would also be worth adding case-insensitivity to the urn_bid_events query; that shouldn't take much effort.

More broadly, maybe we should consider making our trigger-tables conform to a simpler standard (e.g. all lower-case addresses), to make it easier on the user. In fact, I wonder if postgraphile has a "case-insensitive" plugin?

Yeah, I think conforming to a single standard would be a definite win. I think I'd probably lean toward checksum version rather than all lower case because you get the former when you copy an address on Etherscan, but this may also be a good place to pose the question to stakeholders about which they prefer if we generally need to choose one

The LIMITs in this query are probably superfluous since there should only be one ilk per address_id in the flip_ilk storage table, and only 1 usr per (bid_id, address_id) in flip_bid_usr, but since those tables don't have UNIQUE constraints I'm just handling that edge case with a LIMIT.

Looking at this again, I think this chunk is pretty silly 😅 would it make sense instead to include the log_id in this table, and cascade-delete when an event_log is deleted? Then we could omit this trigger altogether

Adding the log_id to this table is a good idea!

🤯so cool!

I may be misreading the documentation, but should USING be used in conjunction with a JOIN? Or does USING have an implied join?

Either way, I haven't used USING before, nice find! 😎

Yeah, it's basically an implied JOIN. You have to specify how to 'join' the tables in the WHERE clause , i.e. what's happening on lines 52 and 53.

Adding the log_id to this table is a good idea!

Haven't fully though this through yet, but I wonder if we'd be missing some postgraphile filtering capabilities by wrapping this table in a query. Though, I totally understand your rational for not exposing the trigger table since there will null values sometimes - way to lookout for the end user! 😄

The more I think about it, I think you're solution is 💯 and we can deal with the potential need for postgraphile filtering in the future.

I wonder if it would be helpful to include an assertion that the bid_event.ilk is null until the flipIlk is inserted?

Not sure how we'd phrase it, but I wonder if we could specify in this Describe block, and the next one ("when diffs are inserted after events") that these tests are specific to the flip contract?

👍 nice indexes