Conversation
gslaughl
left a comment
There was a problem hiding this comment.
A couple nitpicks + questions, but LGTM! 🚢
| google.golang.org/grpc v1.23.1/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyacEbxg= | ||
| gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw= | ||
| gopkg.in/bsm/ratelimit.v1 v1.0.0-20160220154919-db14e161995a/go.mod h1:KF9sEfUPAXdG8Oev9e99iLGnl2uJMjc5B+4y3O7x610= | ||
| gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= |
There was a problem hiding this comment.
Do we want to check this in?
There was a problem hiding this comment.
I think so, but would be happy to remove if others disagree. As I understand it, the purpose of the go.sum is to cache the checksums of all deps - so it seems like we should include basically anything that's added other than sensitive/private files.
| log_id BIGINT NOT NULL REFERENCES header_sync_logs (id) ON DELETE CASCADE, | ||
| msg_sender INTEGER NOT NULL REFERENCES addresses (id) ON DELETE CASCADE, |
There was a problem hiding this comment.
Postgraphile will probably ask for indexes on log_id and msg_sender
transformers/test_data/pot_drip.go
Outdated
| "github.com/makerdao/vdb-mcd-transformers/transformers/shared/constants" | ||
| "github.com/makerdao/vulcanizedb/libraries/shared/factories/event" | ||
| "github.com/makerdao/vulcanizedb/pkg/core" | ||
| "math/rand" |
There was a problem hiding this comment.
nbd but goimports could help out here
| msgSender := common.BytesToAddress(log.Log.Topics[1].Bytes()).Hex() | ||
| addressId, addressErr := shared.GetOrCreateAddress(msgSender, db) | ||
| if addressErr != nil { | ||
| _ = shared.ErrCouldNotCreateFK(addressErr) |
There was a problem hiding this comment.
We probably want to return the error here
| Address: common.HexToAddress(PotAddress()), | ||
| Topics: []common.Hash{ | ||
| common.HexToHash(constants.PotDripSignature()), | ||
| common.HexToHash("0x00000000000000000000000087e76b0a50efc20259cafe0530f75ae0e816aaf2"), |
There was a problem hiding this comment.
I like this approach of not including more topics than necessary in our test data. Now that I think about it, would it make sense to omit the Data field on line 19 for converters that don't need log data?
There was a problem hiding this comment.
tbh I removed them because I saw some error when they were included. But I'm not able to reproduce that and thinking maybe I copy/pasted the empty topics wrong. I'm inclined to re-add them and keep everything to accurately simulate what we'd expect over the RPC
cdeb462 to
aad2835
Compare
aad2835 to
b4268a2
Compare
No description provided.