Conversation
elizabethengelman
left a comment
There was a problem hiding this comment.
😺 just a couple of questions/small things, but LGTM!
environments/mcdTransformers.toml
Outdated
| repository = "github.com/makerdao/vdb-mcd-transformers" | ||
| migrations = "db/migrations" | ||
| contracts = ["MCD_CAT", "MCD_FLAP", "MCD_FLIP_ETH_A", "MCD_FLIP_BAT_A", "MCD_FLIP_SAI", "MCD_FLOP", "MCD_JUG", | ||
| "MCD_POT", "MCD_SPOT", "MCD_VOW", "OSM_ETH", "OSM_BAT"] |
There was a problem hiding this comment.
I may not be looking at this right, but it doesn't look like Osm implementation of deny doesn't have a note modifier, so I don't think it would emit a log event, right?
There was a problem hiding this comment.
Hmm, maybe I'm looking in the wrong place? Is this the correct OSM implementation?
There was a problem hiding this comment.
Looking in etherscan, I can see that the contract at the address specified in the changelog doesn't have the note modifier, so you're totally right. Wish I could figure out how to get from a release to that version's OSM contract in GitHub though 🤔
There was a problem hiding this comment.
It looks like the way that contract versions in dss-deploy-scripts was changed as of 2.16. Instead of looking in the contracts directory for, we now can look at the .dapp.json file which includes the revision for the dss, osm and cdp_manager repos.
There was a problem hiding this comment.
For osm, here's the revision that was used for 2.17: sky-ecosystem/osm@504c474. And it looks like the note modifier was added in release 1.0.1 🎉
There was a problem hiding this comment.
Thanks, that's super helpful!
| } | ||
|
|
||
| contractAddress := log.Log.Address.String() | ||
| contractAddressID, contractAddressErr := shared.GetOrCreateAddress(contractAddress, db) |
| } | ||
|
|
||
| func (c Converter) ToModels(_ string, logs []core.HeaderSyncLog, db *postgres.DB) ([]event.InsertionModel, error) { | ||
| numTopicsRequired := shared.ThreeTopicsRequired + c.LogNoteArgumentOffset |
There was a problem hiding this comment.
The LogNoteArgumentOffset is so that we can use the same ToModels method for all deny methods right? And then just pass the offset for the Vat deny events?
There was a problem hiding this comment.
Yeah exactly. I considered implementing this field differently, like making it a boolean indicating whether the transformer is watching the Vat contract, but I felt like this way gave a more precise description of how the value is being used.
| }) | ||
| }) | ||
|
|
||
| func denyIntegrationTest(blockNumber int64, contractAddressHex, usrAddressHex string, logNoteArgumentOffset int) { |
| ) | ||
|
|
||
| Context("Cat deny events", func() { | ||
| denyIntegrationTest(int64(14764643), test_data.CatAddress(), "0x13141b8a5e4a82ebc6b636849dd6a515185d6236", defaultOffset) |
There was a problem hiding this comment.
super small, but I wonder if we could "0x13141b8a5e4a82ebc6b636849dd6a515185d6236" out into a var?
bd01e82 to
ef40476
Compare
ef40476 to
276031a
Compare
This PR attempts to use the same transformer across all contracts'
denyevents, despiteVat'sLogNoteevents being shaped slightly differently.