Conversation
8dbc222 to
a3ac8d6
Compare
| Transformer: vow.Transformer{}, | ||
| Describe("1.1.0 Cat Contract", func() { | ||
| var catFileConfig = event.TransformerConfig{ | ||
| ContractAddresses: []string{test_data.Cat110Address()}, |
There was a problem hiding this comment.
I think this is a good example of how this may be confusing/unclear to future developers - like for testing cat_file_box we have to use the 1.1.0 version. But for the other events, we could use either contract, and I'm not sure that this is really clear anywhere in the code. I don't have a good idea on how to clarify this some, but just wanted to point it out.
There was a problem hiding this comment.
Also, just thinking that maybe it would be good to include tests for the other events, but for 1.1.0. 🤔
There was a problem hiding this comment.
Totally. I'm confused 🤪 just thinking about this. So for something like cat_file_vow that exists in both versions and is unchanged, we won't duplicate those tests for 1.1.0? And since cat_file_box was introduced in 1.1.0 we'll only have tests for it there? We should probably see how this plays out as we make changes.
There was a problem hiding this comment.
Yeah, we'd end up duplicating tests for 1.1.0... which isn't ideal either. I'm good with holding off with making that change for now, and seeing how the rest of these changes pan out before making a decision.
| "github.com/makerdao/vdb-mcd-transformers/transformers/test_data" | ||
| "github.com/makerdao/vulcanizedb/pkg/core" | ||
|
|
||
| //"github.com/makerdao/vdb-mcd-transformers/transformers/events/cat_file/box" |
There was a problem hiding this comment.
Do we need these commented out imports?
There was a problem hiding this comment.
nice catch! 🙈
| Expect(CatFileVowSignature()).To(Equal("0xd4e8be8300000000000000000000000000000000000000000000000000000000")) | ||
| }) | ||
|
|
||
| It("generates cat file box signature", func() { |
There was a problem hiding this comment.
Should this be in a different place alphabetically? This is probably super OCD and obviously non-blocking 😅
yaoandrew
left a comment
There was a problem hiding this comment.
LGTM all non-blocking comments 
🚀
| models, toModelsErr := transformer.ToModels(constants.Cat110ABI(), []core.EventLog{test_data.CatFileBoxEventLog}, db) | ||
| Expect(toModelsErr).NotTo(HaveOccurred()) | ||
|
|
||
| contractAddressID, msgSenderErr := shared.GetOrCreateAddress(test_data.Cat110Address(), db) |
There was a problem hiding this comment.
💯 I like the way this reads much better. I was putting something like this in my tests - shared.GetOrCreateAddress(test_data.CatFileBoxEventLog.Log.Address.Hex(), db)
a3ac8d6 to
7e5d335
Compare
7e5d335 to
a6c46c5
Compare
| func (e exporter) Export() ([]event.TransformerInitializer, []storage.TransformerInitializer, []interface1.ContractTransformerInitializer) { | ||
| return []event.TransformerInitializer{}, | ||
| return []event.TransformerInitializer{ | ||
| cat_file_box.EventTransformerInitializer, |
There was a problem hiding this comment.
I think this needs to be removed from this file if we don't want to kick off a backfill
| "github.com/makerdao/vulcanizedb/pkg/core" | ||
|
|
||
| . "github.com/onsi/ginkgo" | ||
| . "github.com/onsi/gomega" |
There was a problem hiding this comment.
super nbd but I don't think these newlines are necessary in the imports
e9b1223 to
3192b30
Compare
No description provided.