(VDB-1230) Create kiss + diss batch event transformers by gslaughl · Pull Request #150 · sky-ecosystem/vdb-mcd-transformers

gslaughl · 2020-03-31T20:47:20Z

kiss and diss are basically carbon copies, aside from the words "kiss" and "diss" being different. I wonder if it would be worth trying to reduce duplication between events with very similar signatures; as it stands, that would take some pretty serious refactoring.

gslaughl · 2020-03-31T20:50:36Z

db/migrations/20200329134301_create_median_diss_batch.sql

+    address_id INTEGER    NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE,
+    msg_sender INTEGER    NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE,
+    a_length   INTEGER    NOT NULL,
+    a          TEXT ARRAY NOT NULL,


We landed on using a Postgres array to store addresses, as hopefully this approach will be more extensible if/when we update the transformers to be able to handle arrays of more than 4 addresses.

gslaughl · 2020-03-31T20:51:48Z

db/migrations/20200329134301_create_median_diss_batch.sql

+    header_id  INTEGER    NOT NULL REFERENCES public.headers (id) ON DELETE CASCADE,
+    address_id INTEGER    NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE,
+    msg_sender INTEGER    NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE,
+    a_length   INTEGER    NOT NULL,


Thought it would be a good idea to include a_length in this table, so that we can inform clients that records with a_length > 4 will be incomplete.

Sounds good. I'll add to lift and drop as well.

Could you remind me why we can only have 4 addresses in a - is it because that's all the data field on a log allows? Also I wonder if we should document this somewhere, since it's kind of different than other transformers.

Yeah exactly, an array longer than 4 gets truncated in the log data. And definitely agree that we should make sure the client is aware of this limitation; thinking I'll include something in the vdb-postgraphile docs as well as the Google Doc.

gslaughl · 2020-03-31T20:52:51Z

go.mod

 	github.com/gogo/protobuf v1.1.1
 	github.com/howeyc/fsnotify v0.9.0 // indirect
 	github.com/jmoiron/sqlx v0.0.0-20181024163419-82935fac6c1a
+	github.com/lib/pq v1.0.0


Needed this so we can insert a Go slice into Postgres as the pq.Array type. Hopefully there aren't any issues with adding a hard dependency at this stage?

Is there a reason we are using v1.0.0 vs v.1.3.0?

Nope, GoLand did that automatically. I'd be fine with bumping the version

gslaughl · 2020-04-01T13:58:53Z

transformers/events/median_kiss/batch/transformer.go

+				event.AddressFK:           contractAddressID,
+				constants.MsgSenderColumn: msgSenderAddressID,
+				constants.ALengthColumn:   strconv.FormatUint(aLength, 10),
+				constants.AColumn:         pq.Array(aAddresses),


This line introduces a dependency on the pq library, and also necessitates a small change in vDB allowing this type of value to be in the ColumnValues.

sky-ecosystem/vulcanizedb#72

👍 if this is a problem, maybe there's a way in the sqlx library to support array input.

Good call, I forgot about that library. I'll see if that would be a better alternative

Found this old closed issue on the sqlx repo: jmoiron/sqlx#106. Looks like they hadn't implemented pq arrays, but were thinking about it, though I didn't notice it in sqlx/types.

gslaughl · 2020-04-01T14:05:38Z

transformers/integration_tests/median_diss_batch_test.go

+		header, headerErr := persistHeader(db, blockNumber, blockChain)
+		Expect(headerErr).NotTo(HaveOccurred())
+
+		// TODO: fetch event from blockchain once one exists


Thought it would be a good idea to have at least a partial integration test in this instance, since the transformer test never actually interacts with the DB, and I wanted to make sure the new Postgres array stuff worked as expected.

gslaughl · 2020-04-01T15:21:25Z

transformers/shared/transformer_utils.go

+			return nil, logNoteErr
+		}
+		addressHex := common.BytesToAddress(logDataBytes).Hex()
+		addresses = append(addresses, addressHex)


Worth noting that we're never actually adding the addresses in this list to public.addresses. We could hypothetically return an array of address FKs rather than the addresses themselves, but Postgres can't enforce referential integrity on an array of FKs, so I thought it would be safer to just use the raw addresses.

Maybe it would make sense to add a GetOrCreateAddress line in here, just so we have it in the addresses table, even if we aren't referencing it? But I figured that would be happening in the storage transformer anyway, so there's not much need to do it here. Interested to hear people's thoughts.

I wonder if it would be easier (for the storage key loader) if the addresses are already in public.address? 🤔 Using storageRepository.GetWardsAddresses as my mental model, but that may not be the right way to think about it.

Good call-- definitely worth thinking about, but so far I'm not really finding that it's any tougher when the addresses aren't in public.addresses, since we pretty much always interact with that table using the GetOrCreateAddress function.

And on the storageRepository side of things, we ultimately need a list of addresses (not address IDs) for the key loader, so it's pretty easy to just query them directly from the array in median_kiss_batch/median_diss_batch without doing any joins or anything.

yaoandrew

Thanks for working through the array args. I like where this ended up. 🚢

yaoandrew · 2020-04-01T23:25:59Z

db/migrations/20200329134301_create_median_diss_batch.sql

+    header_id  INTEGER    NOT NULL REFERENCES public.headers (id) ON DELETE CASCADE,
+    address_id INTEGER    NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE,
+    msg_sender INTEGER    NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE,
+    a_length   INTEGER    NOT NULL,


Sounds good. I'll add to lift and drop as well.

yaoandrew · 2020-04-01T23:27:53Z

db/migrations/20200331102914_create_median_kiss_batch.sql

+
+
+-- +goose Down
+DROP TABLE maker.median_kiss_batch;


Its probably redundant when we DROP the table, but in other migrations we seem to be adding a DROP for each index as well in the rollback commands.

Yeah, I've moved away from adding those statements in newer code. Like you said, they seem to be totally unnecessary.

yaoandrew · 2020-04-01T23:32:22Z

transformers/events/median_diss/batch/transformer_test.go

+
+		expectedModel.ColumnValues[event.AddressFK] = contractAddressID
+		expectedModel.ColumnValues[constants.MsgSenderColumn] = msgSenderAddressID
+		expectedModel.ColumnValues[constants.AColumn] = pq.Array([]string{address0, address1, address2, address3})


yaoandrew · 2020-04-01T23:33:53Z

transformers/events/median_kiss/batch/transformer.go

+			return nil, shared.ErrCouldNotCreateFK(msgSenderAddressErr)
+		}
+
+		aLength, aLengthErr := strconv.ParseUint(log.Log.Topics[3].Hex(), 0, 64)


yaoandrew · 2020-04-01T23:35:51Z

transformers/events/median_kiss/batch/transformer.go

+				event.AddressFK:           contractAddressID,
+				constants.MsgSenderColumn: msgSenderAddressID,
+				constants.ALengthColumn:   strconv.FormatUint(aLength, 10),
+				constants.AColumn:         pq.Array(aAddresses),


👍 if this is a problem, maybe there's a way in the sqlx library to support array input.

elizabethengelman

This looks good!

elizabethengelman · 2020-04-06T20:11:29Z

transformers/shared/transformer_utils.go

+			return nil, logNoteErr
+		}
+		addressHex := common.BytesToAddress(logDataBytes).Hex()
+		addresses = append(addresses, addressHex)


I wonder if it would be easier (for the storage key loader) if the addresses are already in public.address? 🤔 Using storageRepository.GetWardsAddresses as my mental model, but that may not be the right way to think about it.

elizabethengelman · 2020-04-06T20:21:09Z

transformers/events/median_kiss/batch/transformer.go

+				event.AddressFK:           contractAddressID,
+				constants.MsgSenderColumn: msgSenderAddressID,
+				constants.ALengthColumn:   strconv.FormatUint(aLength, 10),
+				constants.AColumn:         pq.Array(aAddresses),


Found this old closed issue on the sqlx repo: jmoiron/sqlx#106. Looks like they hadn't implemented pq arrays, but were thinking about it, though I didn't notice it in sqlx/types.

rmulhol · 2020-04-06T21:25:28Z

db/migrations/20200329134301_create_median_diss_batch.sql

@@ -0,0 +1,25 @@
+-- +goose Up
+CREATE TABLE maker.median_diss_batch


Looks like this file needs an updated timestamp - latest on staging is 20200403103019

Yup! I was just about to do that :) thanks for looking out though

rmulhol · 2020-04-06T21:27:50Z

db/migrations/20200331102914_create_median_kiss_batch.sql

@@ -0,0 +1,25 @@
+-- +goose Up
+CREATE TABLE maker.median_kiss_batch


also true of this file

gslaughl commented Apr 1, 2020

View reviewed changes

gslaughl requested review from elizabethengelman, rmulhol and yaoandrew April 1, 2020 14:14

gslaughl commented Apr 1, 2020

View reviewed changes

yaoandrew approved these changes Apr 1, 2020

View reviewed changes

gslaughl force-pushed the vdb-1228-kiss-diss-transformers branch from 0934498 to 7baa4ad Compare April 3, 2020 15:53

gslaughl changed the base branch from vdb-1228-kiss-diss-transformers to staging April 3, 2020 16:47

(VDB-1230) Create kiss + diss batch event transformers

9ddcae4

gslaughl force-pushed the vdb-1230-kiss-diss-batch branch from 2bc19d4 to 9ddcae4 Compare April 6, 2020 18:03

elizabethengelman approved these changes Apr 6, 2020

View reviewed changes

rmulhol suggested changes Apr 6, 2020

View reviewed changes

rmulhol reviewed Apr 6, 2020

View reviewed changes

Point at vDB staging, update migration timestamps

6791a41

rmulhol approved these changes Apr 6, 2020

View reviewed changes

gslaughl merged commit 2159d90 into staging Apr 6, 2020

gslaughl deleted the vdb-1230-kiss-diss-batch branch April 6, 2020 22:01

		@@ -0,0 +1,25 @@
		-- +goose Up
		CREATE TABLE maker.median_diss_batch

		@@ -0,0 +1,25 @@
		-- +goose Up
		CREATE TABLE maker.median_kiss_batch

Conversation

gslaughl commented Mar 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gslaughl Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gslaughl Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaoandrew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabethengelman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmulhol Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gslaughl Apr 1, 2020 •

edited

Loading

gslaughl Apr 1, 2020 •

edited

Loading

rmulhol Apr 6, 2020 •

edited

Loading