Fix: Monitor/fix reported checkpoint BTC height query bugs #299
Conversation
There was a problem hiding this comment.
Thanks! Have to say hooks are quite cryptic to deal with due to the use of concrete keepers 😢 Still remember @vitsalis and I had a long debugging session for the same sort of bugs.
app/app.go
Outdated
| @@ -507,10 +507,11 @@ func NewBabylonApp( | |||
| app.GetSubspace(checkpointingtypes.ModuleName), | |||
| privSigner.ClientCtx, | |||
| ) | |||
| app.CheckpointingKeeper = *checkpointingKeeper.SetHooks( | |||
| checkpointingtypes.NewMultiCheckpointingHooks(app.EpochingKeeper.Hooks(), app.ZoneConciergeKeeper.Hooks()), | |||
| checkpointingKeeper.SetHooks( | |||
There was a problem hiding this comment.
Not fully sure why do we remove app.CheckpointingKeeper = * here. Here * does a dereference, thus copies a new instance of the checkpointing keeper. That is, with app.CheckpointingKeeper = *, app.CheckpointingKeeper is a different object than checkpointingKeeper. Although reusing checkpointingKeeper should work as well, copying a new object seems more clear. your call
app/app.go
Outdated
| ) | ||
| app.ZoneConciergeKeeper.SetCheckpointingKeeper(app.CheckpointingKeeper) | ||
| app.CheckpointingKeeper = checkpointingKeeper | ||
| app.ZoneConciergeKeeper.SetCheckpointingKeeper(checkpointingKeeper) |
There was a problem hiding this comment.
| app.ZoneConciergeKeeper.SetCheckpointingKeeper(checkpointingKeeper) | |
| app.ZoneConciergeKeeper.SetCheckpointingKeeper(app.CheckpointingKeeper) |
just to ensure that future PRs do not affect this piece of code due to the deference bugs 😓
This PR fixed the bugs found during integration tests with the vigilante monitor. The
ReportedCheckpointBtcHeightdid not work because of the incorrect setting of hooks. This PR also added fuzz tests for the monitor queries.