Conversation
new transformed unrecognized noncanonical unwatched
and block number is outside the reorg window
| @@ -198,7 +194,24 @@ func (watcher StorageWatcher) handleDiffWithInvalidHeaderHash(diff types.Persist | |||
| return fmt.Errorf(msg, diff.ID, maxBlockErr) | |||
| } | |||
| if diff.BlockHeight < int(maxBlock)-ReorgWindow { | |||
There was a problem hiding this comment.
Wanted another set of eyes on this. When handling diffs with invalid header hash, it looks like if the diff's blockHeight is still within the ReorgWindow, then we're not changing the status on that diff. I think this makes sense, because it's possible that the given diff is actually canonical, but the reorg is currently in progress. So, I think by not setting the status, this diff will be pulled in again for processing in the future. Is that how others are understanding this as well?
| logrus.Infof("error transforming diff: %s", transformErr.Error()) | ||
| } | ||
| if handleErr := watcher.handleTransformError(transformErr, diff); handleErr != nil { | ||
| return handleErr |
There was a problem hiding this comment.
Should this get the fmt.Errorf treatment?
| var status string | ||
| getStatusErr := db.Get(&status, `SELECT status FROM public.storage_diff WHERE id = $1`, fakePersistedDiff.ID) | ||
| Expect(getStatusErr).NotTo(HaveOccurred()) | ||
| Expect(status).To(Equal("unwatched")) |
There was a problem hiding this comment.
nbd but I wonder if we should wrap these strings (e.g. "new", "unrecognized", "noncanonical", "unwatched", "transformed") in vars so that we can change them in one place if we change the enum in the DB
| 'transformed', | ||
| 'unrecognized', | ||
| 'noncanonical', | ||
| 'unwatched' |
There was a problem hiding this comment.
no real action item here but definitely want to think about how we can potentially accommodate multiple plugins/execute processes - where a diff that's "unwatched" for one plugin might still be relevant to another. Given that we're getting better about configuring which contracts we're tracking, I almost wonder if we can eventually discard this status entirely 🤔
There was a problem hiding this comment.
🤔 Yeah, this is a good call. Do you think it's worth removing this status now? We could leave those diffs with a status as 'new' for now? I think those diffs won't get constantly pulled if we've set DiffBlocksFromHeadOfChain.
companion PR: sky-ecosystem/vdb-mcd-transformers#273