VDB-1002: use VDB InsertionModel for vow file#33
Conversation
050b692 to
53014f3
Compare
|
|
||
| package constants | ||
|
|
||
| const ( |
transformers/test_data/vow_file.go
Outdated
| SchemaName: constants.MakerSchema, | ||
| TableName: constants.VowFileLabel, | ||
| OrderedColumns: []event.ColumnName{ | ||
| constants.HeaderFK, constants.WhatColumn, constants.DataColumn, constants.LogFK, |
There was a problem hiding this comment.
Should use the constants from event in core for non domain specific things :)
const LogFK ColumnName = "log_id"
const AddressFK ColumnName = "address_id"
const HeaderFK ColumnName = "header_id"They still exist in both repos to allow for step-by-step migration of the transformers. No biggie either, we'll notice breakage when the vdb-mcd-transformers code is nuked anyway 🎉
| constants.HeaderFK: VowFileHeaderSyncLog.HeaderID, | ||
| constants.LogFK: VowFileHeaderSyncLog.ID, | ||
| ColumnValues: event.ColumnValues{ | ||
| constants.WhatColumn: "wait", |
There was a problem hiding this comment.
I'm actually more inclined to declaring these locally in all transformers, to make them as self-sufficient as possible. What's your take? 🤔
There was a problem hiding this comment.
🤔 My thought was to reduce duplication, since in this case we'd end up defining a WhatColumn constant in several converters. I understand the instinct to make the transformers self-sufficient, but I'm not immediately seeing a downside to using the constants package in these, since we're also including the shared package as well. That said, I don't feel super strongly about it either way, so I'm up for changing it back if folks think that's best.
There was a problem hiding this comment.
I'm fine with either approach, but I think I would lean toward having shared constants for terminology that's reused across contracts in the dss repo. My thinking is that even if we break down the code in this repo further along any number of abstractions, we'd probably keep event transformers targeting dss together - so we might as well take advantage of its naming scheme.
53014f3 to
cd782c2
Compare
| var models []event.InsertionModel | ||
| for _, log := range logs { | ||
| err := shared.VerifyLog(log.Log, numTopicsRequired, logDataRequired) | ||
| err := shared.VerifyLog(log.Log, shared.FourTopicsRequired, shared.LogDataRequired) |
There was a problem hiding this comment.
Doesn't need to happen in this PR, but I'm wondering whether VerifyLog and the constants for fields required should potentially be moved over to vulcanizedb
| model := shared.InsertionModel{ | ||
| SchemaName: "maker", | ||
| SchemaName: constants.MakerSchema, | ||
| TableName: "vow_flog", |
There was a problem hiding this comment.
Also doesn't need to happen in this PR, but could be cool to reuse the labels from the constants for all of the table names. Perhaps even renaming Label to Table
cd782c2 to
95d1dc1
Compare
No description provided.