Skip to content

VDB-975 Move dent#14

Merged
rmulhol merged 1 commit intostagingfrom
VDB-975-move-dent
Nov 27, 2019
Merged

VDB-975 Move dent#14
rmulhol merged 1 commit intostagingfrom
VDB-975-move-dent

Conversation

@m0ar
Copy link
Copy Markdown
Contributor

@m0ar m0ar commented Nov 20, 2019

No description provided.


import (
"database/sql"
"github.com/makerdao/vulcanizedb/libraries/shared/factories/event"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obv not merge blocking, but not sure where we landed on this... using goimports and calling it a day? 😬

const (
logDataRequired = true
numTopicsRequired = 4
logDataRequired = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

},
ForeignKeyValues: shared.ForeignKeyValues{
constants.AddressFK: DentHeaderSyncLog.Log.Address.Hex(),
// constants.AddressColumn
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity preservation, it should be there but we can't assert against it since it depends on the DB state when the tests run :)

dent.Id: dentBidId,
dent.Lot: dentLot,
dent.Bid: dentBid,
constants.HeaderFK: DentHeaderSyncLog.HeaderID,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure when to use one vs the other or if we need both but could we potentially use event.HeaderFK from vulcanizedb/libraries/shared/factories

elizabethengelman pushed a commit that referenced this pull request Nov 21, 2019
elizabethengelman pushed a commit that referenced this pull request Nov 21, 2019
VDB-429 Update Frob event transformer
)

type DentRepository struct {
type Repository struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we keeping a transformer specific repository at all? Seems like we could just initialize the shared factories/transformer with a database connection and pass that directly to event.Create. Then we could remove the Repository from the initializer entirely, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Available for whoever wants to shepherd the dual PRs :) We just decided to do it in steps a while back, moving stuff over, then removing the repos.

@rmulhol rmulhol merged commit 6079966 into staging Nov 27, 2019
@rmulhol rmulhol deleted the VDB-975-move-dent branch November 27, 2019 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants