VDB-950 persist untransformed diffs by elizabethengelman · Pull Request #52 · sky-ecosystem/vdb-mcd-transformers

elizabethengelman · 2019-12-10T17:41:17Z

Implements the changes from (VDB-950) Write raw diffs before transforming vulcanizedb#8 in this repo.
will need to update the VDB version once (VDB-950) Write raw diffs before transforming vulcanizedb#8 is merged in and a new release is cut. For now it is referencing a specific commit sha.

still need to pass in a value instead of 0 for the component tests

…itoryBehaviors since this method handles both varible and mapping results

rmulhol

Thanks for taking the lead on this! LGTM - don't think any of my comments are blocking, but curious to hear what you think about them 👍:pray:

rmulhol · 2019-12-10T22:34:32Z

data_generator/data_generator.go

 	"flag"
 	"fmt"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/makerdao/vulcanizedb/pkg/datastore/postgres/repositories"


Could be cool to remove the whitespace in the imports in re-run goimports -w, but nbd

rmulhol · 2019-12-10T22:34:47Z

data_generator/data_generator.go

 		)) RETURNING id`
 	insertVatFrobSql = `INSERT INTO maker.vat_frob (header_id, urn_id, v, w, dink, dart, log_id)
-		VALUES($1, $2::NUMERIC, $3, $4, $5::NUMERIC, $6::NUMERIC, $7)
+		VALUES($1::NUMERIC, $2::NUMERIC, $3, $4, $5::NUMERIC, $6::NUMERIC, $7::NUMERIC)


rmulhol · 2019-12-10T22:37:55Z

db/migrations/20181114200131_create_vat_storage_tables.sql

 CREATE TABLE maker.vat_ilk_spot
 (
    id        SERIAL PRIMARY KEY,
+    diff_id   INTEGER NOT NULL REFERENCES storage_diff (id) ON DELETE CASCADE,


Thinking we may end up wanting indexes on every diff_id, but can probably queue that up for another story after confirming

👍 totally makes sense. I'll probably do this in another PR.

rmulhol · 2019-12-10T22:39:23Z

db/migrations/20181114200131_create_vat_storage_tables.sql

 CREATE TABLE maker.vat_ilk_rate
 (
    id        SERIAL PRIMARY KEY,
+    diff_id   INTEGER NOT NULL REFERENCES storage_diff (id) ON DELETE CASCADE,


Another not-necessary-in-this-PR thing - wondering if the storage_diff (id) should be a bigint. Seems like we could end up with more rows than the max integer

👍 Nice catch! Updated in 1ad6498.

I'm also wondering if this sort of change is something we'd want to consider for header_id and address_id. Seems like the number of headers and addresses will grow way slower than diffs, but it would probably also be easier to handle the type change now vs. later.

Yeah good call, I don't see a reason not to use it for those other fields. Thinking we may need to update sky-ecosystem/vulcanizedb#8 so that the actual primary key on the table is a bigserial, and would be up for making a similar change to other tables whenever

new story: https://makerdao.atlassian.net/browse/VDB-1082

rmulhol · 2019-12-10T22:43:20Z

go.mod


 replace github.com/ethereum/go-ethereum => github.com/vulcanize/go-ethereum v0.0.0-20190731183759-8e20673bd101
+
+replace github.com/makerdao/vulcanizedb => github.com/makerdao/vulcanizedb v0.0.11-0.20191210170334-0491389f8183


worth potentially just changing the version in the require? probably not a big deal if we'll be updating after we cut a new version shortly

rmulhol · 2019-12-10T23:14:00Z

transformers/storage/cat/repository_test.go

-		fakeHeaderID int64
-		fakeAddress  = "0x12345"
-		fakeUint256  = "12345"
+		db                   *postgres.DB


very minor and not an issue with this PR but maybe it's worth defining the db var here as test_config.NewTestDB(test_config.NewTestNode()) here so that it's not a new connection for every test

rmulhol · 2019-12-10T23:16:22Z

transformers/storage/cat/repository_test.go

+			inputs := shared_behaviors.StorageBehaviorInputs{
+				ValueFieldName:   cat.Live,
+				Value:            fakeUint256,
+				StorageTableName: "maker.cat_live",


Definitely not a part of this PR but writing it down so I don't forget, seems like we could restructure this struct to accept a Schema and TableName and then use the constants from transformers/shared/constants/table.go

👍 definitely like that idea! I'll plan to do this in another PR.

rmulhol · 2019-12-10T23:25:13Z

transformers/storage/test_helpers/datatypes.go

 type VariableRes struct {
 	BlockMetadata
-	Value string
+	DiffID int64 `db:"diff_id"`


Super minor but maybe worth having a DiffMetadata struct that includes BlockMetadata and a DiffID and then inheriting that instead of both of those in subsequent structs?

nice idea, do you mean something like in fdfe915?

rmulhol · 2019-12-10T23:25:20Z

transformers/storage/test_helpers/datatypes.go

+	Value  string
 }

 type AuctionVariableRes struct {


Is this still used?

nope! nice catch!

rmulhol · 2019-12-10T23:27:14Z

transformers/storage/test_helpers/test_helpers.go

+		BlockHeight:   int(header.BlockNumber),
+		StorageKey:    key,
+		StorageValue:  value,
+	}


Can we make use of the helper in vulcanizedb here?

gslaughl

LGTM!

gslaughl · 2019-12-10T22:31:12Z

transformers/storage/cat/repository_test.go

+			vatMetadata := utils.GetStorageValueMetadata(cat.Vat, nil, utils.Address)
+			inputs := shared_behaviors.StorageVariableBehaviorInputs{
+				ValueFieldName:   cat.Vat,
+				Value:            fakeAddress,
+				StorageTableName: "maker.cat_vat",
+				Repository:       &repo,
+				Metadata:         vatMetadata,
+			}
+
+			shared_behaviors.SharedStorageRepositoryVariableBehaviors(&inputs)


elizabethengelman · 2019-12-11T21:49:24Z

go.mod

+	github.com/sirupsen/logrus v1.2.0
+	github.com/spf13/viper v1.3.2
+	golang.org/x/crypto v0.0.0-20191119213627-4f8c1d86b1ba
+	gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405


the rest of these changes are the result of running gomoderator.py against vdb staging

rmulhol · 2019-12-12T16:45:33Z

db/migrations/00024_create_storage_diffs_table.sql

+-- +goose Up
+CREATE TABLE public.storage_diff
+(
+    id             SERIAL PRIMARY KEY,


Should copy over the new migration from vulcanizedb that makes this a bigserial

nice catch! will do.

rmulhol · 2019-12-12T17:00:29Z

transformers/component_tests/queries/test_helpers/test_helpers.go

+			panic(fmt.Sprintf("valuesMap value type not recognized %v", v))
+		}
+
+		persistedDiff := test_helpers.CreateDiffRecord(db, header, common.Hash{}, key, valueForDiffRecord)


elizabethengelman changed the title ~~Vdb 950 persist untransformed diffs~~ [WIP] VDB-950 persist untransformed diffs Dec 10, 2019

elizabethengelman added 20 commits December 10, 2019 15:03

Update storage repos to include diffID in Create

d6890f2

Use SharedStorageRepositoryVariableBehaviors for cat storage repo

343979f

Update query test_helpers to take in a diffId

6f74da6

still need to pass in a value instead of 0 for the component tests

Bring updated migration files over from vulcanizedb

ade6dba

Add diff_id to cat storage tables

521afea

Add diff_id to cdp_manager storage tables

a568843

Add diff_id to flap storage tables

de38ac4

Add diff_id to flip storage tables

344520d

Add diff_id to flop storage tables

560b7c0

Add diff_id to jug storage tables

f2051f4

Add diff_id to spot storage tables

3c1bd1b

Add diff_id to vat storage tables

86cbd00

Add diff_id to vow storage tables

34d4cbb

Use diff id in component query test setup

f2d4b12

fixup! Add diff_id to spot storage tables

8d5676d

Update storage repo test

7d21f86

Update storage component tests

eda27aa

Update query tests to account for diff_id in storage tables

2cd04cd

Update data generator to account for diff_id

1ad378e

Rename SharedStorageVariableRepositoryBehaviors -> SharedStorageRepos…

fe188c4

…itoryBehaviors since this method handles both varible and mapping results

elizabethengelman force-pushed the vdb-950-persist-untransformed-diffs branch from b5f838a to c7b9f01 Compare December 10, 2019 21:48

elizabethengelman requested review from gslaughl, m0ar, rmulhol and yaoandrew December 10, 2019 22:09

elizabethengelman changed the title ~~[WIP] VDB-950 persist untransformed diffs~~ VDB-950 persist untransformed diffs Dec 10, 2019

rmulhol approved these changes Dec 10, 2019

View reviewed changes

gslaughl approved these changes Dec 11, 2019

View reviewed changes

elizabethengelman mentioned this pull request Dec 11, 2019

(VDB-1045) Isolate execute container #45

Merged

elizabethengelman force-pushed the vdb-950-persist-untransformed-diffs branch from 5754d65 to fdfe915 Compare December 11, 2019 21:22

elizabethengelman added 6 commits December 11, 2019 15:40

Update VDB version

3c7b3dc

Fixes after rebasing with staging

dfa88e2

Apply goimports changes

60d825f

Update diff_id to BIGINT in storage tables

1586abe

Address PR feedback

2de1c48

Apply gofmt changes

ff1e4e8

elizabethengelman force-pushed the vdb-950-persist-untransformed-diffs branch from 157cf3e to ff1e4e8 Compare December 11, 2019 21:40

elizabethengelman commented Dec 11, 2019

View reviewed changes

Create a new diff record for each new storage record in test_helpers

f9a8617

rmulhol reviewed Dec 12, 2019

View reviewed changes

elizabethengelman added 2 commits December 12, 2019 11:06

Bring vdb storage diff migration change over to mcd_transformers repo

7407f1c

Check in schema change to make diff_id foreign keys bigints

4557caa

elizabethengelman merged commit 4414784 into staging Dec 12, 2019

elizabethengelman deleted the vdb-950-persist-untransformed-diffs branch December 12, 2019 17:43


		replace github.com/ethereum/go-ethereum => github.com/vulcanize/go-ethereum v0.0.0-20190731183759-8e20673bd101

		replace github.com/makerdao/vulcanizedb => github.com/makerdao/vulcanizedb v0.0.11-0.20191210170334-0491389f8183

Conversation

elizabethengelman commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmulhol 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!

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elizabethengelman Dec 11, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elizabethengelman commented Dec 10, 2019 •

edited

Loading

elizabethengelman Dec 11, 2019 •

edited

Loading