Conversation
cmd/assignIlkLumpToZero.go
Outdated
| Use: "assignIlkLumpToZero", | ||
| Short: "Create a diff with a zero ilk lump value.", | ||
| Long: `Inserts an artificial storage_diff with the storage_key for cat_ilk_lump, and a | ||
| storage_value of zero for all ilks for the given block height. This storage_diff will then be picked up in the execute |
There was a problem hiding this comment.
Noting for posterity that we're looking at changing this to enable setting lump to zero at different block heights for different ilks
c45678a to
8436472
Compare
| height. The storage_diffs will then be picked up in the execute process to be transformed into maker.cat_ilk_lump records. | ||
| The block-number (b) and ilks (i) arguments are required. | ||
|
|
||
| Note: the ilk argument(s) passed in need to be the hex representation of the ilk. |
There was a problem hiding this comment.
I wanted to call this out - we'll need to pass the ilks in as their hex representation, i.e. 0x4241542d41000000000000000000000000000000000000000000000000000000, 0x4554482d41000000000000000000000000000000000000000000000000000000.
Though not ideal, I thought it was easier than converting the ilkIdentifiers (i.e. ETH-A, BAT-A) into these hex representations. Another idea I had was to look up the ilks from the db, based on their identifiers. Not sure which is easiest/makes the most sense but up for some ideas!
There was a problem hiding this comment.
Totally on board with your reasoning here, especially since this is a write operation that only we will run (and it only needs to be ran a few times)
8436472 to
07e3cbf
Compare
rmulhol
left a comment
There was a problem hiding this comment.
LGTM!
Don't think any of my comments are blocking, though would be curious to make sure we're certain that an empty hash is correctly transformed to 0 on the cat_ilk_lump table
| height. The storage_diffs will then be picked up in the execute process to be transformed into maker.cat_ilk_lump records. | ||
| The block-number (b) and ilks (i) arguments are required. | ||
|
|
||
| Note: the ilk argument(s) passed in need to be the hex representation of the ilk. |
There was a problem hiding this comment.
Totally on board with your reasoning here, especially since this is a write operation that only we will run (and it only needs to be ran a few times)
| rootCmd.AddCommand(assignIlkLumpToZeroCmd) | ||
| assignIlkLumpToZeroCmd.Flags().IntVarP(&blockNumber, "block-number", "b", -1, "the block associated with artificial ilk lump diffs") | ||
| assignIlkLumpToZeroCmd.MarkFlagRequired("block-number") | ||
| assignIlkLumpToZeroCmd.Flags().StringSliceVarP(&ilks, "ilks", "i", []string{}, "the ilk to set lump to zero") |
There was a problem hiding this comment.
This is interesting - so for a string slice we don't need to pass --ilks ilk1,ilk2,ilk3 but can do --ilk ilk1 --ilk ilk2 --ilk ilk3?
There was a problem hiding this comment.
Nice catch - I need to change the example to to --ilks ilk1 --ilks ilk2. But yes, as far as I can tell, cobra command handles slice by requiring that you pass each element in the slice as a separate flag. Kind of like how we were handling watched addresses in the old geth patch version.
cmd/assignIlkLumpToZero.go
Outdated
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/makerdao/vdb-mcd-transformers/transformers/storage/cat/v1_0_0" | ||
| "github.com/makerdao/vdb-mcd-transformers/transformers/test_data" | ||
| storage2 "github.com/makerdao/vulcanizedb/libraries/shared/storage" |
There was a problem hiding this comment.
super nbd but wondering if we might be able to remove the alias on this import?
cmd/assignIlkLumpToZero.go
Outdated
| } | ||
|
|
||
| func (generator ZeroValueDiffGenerator) CreateZeroValueIlkLumpDiff(blockNumber int, ilks []string) error { | ||
| fmt.Println("ilks", ilks) |
There was a problem hiding this comment.
maybe worth removing and/or using logging tools with a more descriptive message?
There was a problem hiding this comment.
Yikes, nice catch! 🤦♀️ On line 90 there's a log line with the ilks that have been passed in that is a better message.
cmd/assignIlkLumpToZero.go
Outdated
| return generator.CreateZeroValueIlkLumpDiff(blockNumber, ilks) | ||
| } | ||
|
|
||
| type ZeroValueDiffGenerator struct { |
There was a problem hiding this comment.
Maybe worth pulling out this struct into a separate namespace outside of the cmd directory?
There was a problem hiding this comment.
Yeah, I was thinking that too, but I'm not sure where would be best to put it. Thinking that some of the checkMigrations code could be pulled into it's own file. Any ideas?
There was a problem hiding this comment.
not merge blocking so I'd be fine to defer, but I think my default rn would be to create a new top level directory similar to backfill/ or data_generator/
| BlockHash: fakes.FakeHash, | ||
| BlockHeight: blockNumber, | ||
| StorageKey: common.HexToHash(batALumpKey), | ||
| StorageValue: common.Hash{}, |
There was a problem hiding this comment.
would potentially like to see a test that an empty hash propagates to a zero value on the cat_ilk_lump table, though not sure the best way to do that without running execute. Maybe just for documentation's sake it would make sense to include a test that HexToInt(common.Hash{}.Hex()) == 0?
07e3cbf to
5a9663e
Compare
No description provided.