Vdb-585 create address table #126

elizabethengelman · 2019-08-05T14:12:26Z

factors out an address table to be referenced by FK in full_sync_receipts and header_sync_receipts
created a repository for addresses, header_sync_receipts and full_sync_receipts to eliminate some duplicate code
would love some feedback on this before I make similar changes in mcd_transformers

rmulhol

LGTM!

All my comments are incredibly minor and non-blocking. Thanks for improving our test coverage while implementing this feature!

rmulhol · 2019-08-07T15:14:06Z

pkg/datastore/postgres/repositories/address_repository.go

+func (AddressRepository) GetAddressById(db *postgres.DB, id int) (string, error) {
+	var address string
+	getErr := db.Get(&address, `SELECT address FROM public.addresses WHERE id = $1`, id)
+	if getErr != nil {


Could we return address, getErr?

rmulhol · 2019-08-07T15:16:03Z

pkg/datastore/postgres/repositories/address_repository_test.go

+			Expect(createErr).NotTo(HaveOccurred())
+
+			var actualAddress dbAddress
+			getErr := db.Get(&actualAddress, `SELECT id, address FROM public.addresses LIMIT 1`)


I dig using the LIMIT 1 instead of WHERE address = $1. Doesn't need to be a part of this PR, but I wonder if we want to start using that pattern in other places where we're going db.Get in tests 🤔

rmulhol · 2019-08-07T15:17:08Z

pkg/datastore/postgres/repositories/address_repository_test.go

+			_, createErr := repo.GetOrCreateAddress(db, address)
+			Expect(createErr).NotTo(HaveOccurred())
+
+			_, getErr := repo.GetOrCreateAddress(db, address)


This is maybe superfluous, but I wonder if we'd want to potentially assign the result here and Expect(idOne).To(Equal(idTwo))

rmulhol · 2019-08-07T15:18:21Z

pkg/datastore/postgres/repositories/address_repository_test.go

+		It("creates an address record", func() {
+			addressId, createErr := repo.GetOrCreateAddressInTransaction(tx, address)
+			Expect(createErr).NotTo(HaveOccurred())
+			tx.Commit()


Maybe worth assigning commitErr := tx.Commit() and Expect(commitErr).NotTo(HaveOccurred())?

rmulhol · 2019-08-07T15:20:10Z

pkg/datastore/postgres/repositories/address_repository.go

+	stringAddressToCommonAddress := common.HexToAddress(address)
+	hexAddress := stringAddressToCommonAddress.Hex()
+
+	var addressId int


Definitely not a big deal, but I wonder if we want to pick one type between int and int64 that we're using to pass around db ids

yep, good call! quickly looking through the other repos, it looks like they're treating ids as int64s, so I'll follow suit.

rmulhol · 2019-08-07T15:22:09Z

pkg/datastore/postgres/repositories/address_repository_test.go

+		BeforeEach(func() {
+			tx, txErr = db.Beginx()
+			Expect(txErr).NotTo(HaveOccurred())
+		})


Probably doesn't matter, but I wonder if we want to have an AfterEach to close/rollback the tx. Mostly thinking about whether a tx could leak and remain open if the test fails early (and if that could conceivably have any impact)

rmulhol · 2019-08-07T15:23:44Z

pkg/datastore/postgres/repositories/watched_events_repository_test.go

@@ -79,7 +79,9 @@ var _ = Describe("Watched Events Repository", func() {
 		Expect(err).ToNot(HaveOccurred())
 		blockId, err := blocksRepository.CreateOrUpdateBlock(core.Block{})
 		Expect(err).NotTo(HaveOccurred())
-		receiptId, err := receiptRepository.CreateReceipt(blockId, core.Receipt{})
+		tx, _ := db.Beginx()


Worth assigning and asserting against an error here?

aaizuss · 2019-08-07T23:28:04Z

pkg/datastore/postgres/repositories/address_repository_test.go

+		db = test_config.NewTestDB(test_config.NewTestNode())
+		test_config.CleanTestDB(db)
+		repo = repositories.AddressRepository{}
+	})


I noticed that in tests that use the db, we tend to include an AfterEach that calls db.Close() - I don't know why we do that or if it's actually necessary though.

Yeah I think it would be better to have the test_config.CleanTestDB(db) in an AfterEach statement, I might be missing something but I think with the way it is right now any addresses that are added in the last test to be ran in this suite will be left over after the suite exits and could potentially lead to issues down the road with other tests.

I'm actually a bit unclear about when we need to do db.Close and when we don't. I thought that db.Close was something that we were doing in the mcd_transformers repo since we're building up and tearing down the db for each test run, but I could be wrong.

I created VDB-799 as a way to make sure we're handling test cleanup consistently, does that make sense to handle this as part of that story? In the mean time, I'll add test_config.CleanTestDB(db) to an AfterEach.

i-norden

LGTM! Thank you for undertaking this.

i-norden · 2019-08-12T17:23:31Z

pkg/datastore/postgres/repositories/header_sync_receipt_repository_test.go

+	"github.com/vulcanize/vulcanizedb/pkg/datastore/postgres"
+	"github.com/vulcanize/vulcanizedb/pkg/datastore/postgres/repositories"
+	"github.com/vulcanize/vulcanizedb/test_config"
+	"math/big"


Super minor (I can't find anything else to comment on 🙃 ) but could we split these and other files' imports up into the "standard lib", "external deps", and "internal deps" sections (I need to go back and do this to my seed node files too)

addressed in 07e45dd

i-norden · 2019-08-12T17:30:22Z

pkg/datastore/postgres/repositories/address_repository_test.go

+		db = test_config.NewTestDB(test_config.NewTestNode())
+		test_config.CleanTestDB(db)
+		repo = repositories.AddressRepository{}
+	})


Yeah I think it would be better to have the test_config.CleanTestDB(db) in an AfterEach statement, I might be missing something but I think with the way it is right now any addresses that are added in the last test to be ran in this suite will be left over after the suite exits and could potentially lead to issues down the road with other tests.

Will need to update this version to a release when vulcanize/vulcanizedb#126 is merged in a released

elizabethengelman force-pushed the vdb-585-create-address-table branch from 32a6b3d to 0f64198 Compare August 5, 2019 14:15

elizabethengelman requested review from i-norden, rmulhol, m0ar, aaizuss, Gslaughl and yaoandrew and removed request for i-norden August 5, 2019 22:00

rmulhol approved these changes Aug 7, 2019

View reviewed changes

aaizuss approved these changes Aug 8, 2019

View reviewed changes

i-norden approved these changes Aug 12, 2019

View reviewed changes

elizabethengelman changed the title ~~[WIP] Vdb-585 create address table~~ Vdb-585 create address table Aug 20, 2019

elizabethengelman force-pushed the vdb-585-create-address-table branch from 359ef6e to eacc4fc Compare August 20, 2019 16:02

elizabethengelman added 13 commits August 23, 2019 10:11

Add an addresses table

750754c

Move add address migration table up in the migration order

3005a1a

Add AddressRepository

b13eefb

Pass a db into GetOrCreateAddress

1373fe8

Get or create address record in a transaction

4b61c87

Add GetAddressById

2580358

Updating full_sync_receipts to have FK reference to addresses

bcd6d14

Updating header_sync_receipts to have FK reference to addresses

4e40e89

Rename ReceiptRepository -> FullSyncReceiptRepository

7e38764

Create a HeaderSyncReceiptRepository

38f6847

Update Copyrights

58c23c6

Address PR feedback

1b37863

Address more PR feedback

edc0bdf

elizabethengelman force-pushed the vdb-585-create-address-table branch from eacc4fc to edc0bdf Compare August 23, 2019 15:11

elizabethengelman merged commit b6a2a27 into staging Aug 26, 2019

elizabethengelman deleted the vdb-585-create-address-table branch August 26, 2019 20:13

elizabethengelman added a commit to vulcanize/mcd_transformers that referenced this pull request Aug 27, 2019

Update vulcanize db version

925977d

Will need to update this version to a release when vulcanize/vulcanizedb#126 is merged in a released

i-norden mentioned this pull request Dec 4, 2019

Replace addresses across all Postgres tables with fk references to a separate addresses table #105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vdb-585 create address table #126

Vdb-585 create address table #126

elizabethengelman commented Aug 5, 2019 •

edited

Loading

rmulhol left a comment

rmulhol Aug 7, 2019

elizabethengelman Aug 19, 2019

rmulhol Aug 7, 2019

rmulhol Aug 7, 2019

rmulhol Aug 7, 2019

rmulhol Aug 7, 2019

elizabethengelman Aug 19, 2019

rmulhol Aug 7, 2019

rmulhol Aug 7, 2019

elizabethengelman Aug 19, 2019

aaizuss Aug 7, 2019

i-norden Aug 12, 2019 •

edited

Loading

elizabethengelman Aug 19, 2019

i-norden left a comment

i-norden Aug 12, 2019

elizabethengelman Aug 19, 2019

i-norden Aug 12, 2019 •

edited

Loading

Vdb-585 create address table #126

Vdb-585 create address table #126

Conversation

elizabethengelman commented Aug 5, 2019 • edited Loading

rmulhol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-norden Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-norden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i-norden Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

elizabethengelman commented Aug 5, 2019 •

edited

Loading

i-norden Aug 12, 2019 •

edited

Loading

i-norden Aug 12, 2019 •

edited

Loading