-
Notifications
You must be signed in to change notification settings - Fork 4
Add ForkWatchJob #4
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few comments. In addition the example app.settings needs to be updated with the new property and the documentation needs update for the new job with short summary what it does.
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
|
||
class VirtualCheckpoint | ||
{ | ||
public int Height; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use property, not field for DTOs. And convention follows to end with DTO in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
} | ||
} | ||
|
||
class VirtualCheckpoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move to separate .cs file
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
public string Hash; | ||
} | ||
|
||
class WatchJobExecutor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move to separate .cs file , seems like Service, And convention follows to end with Service in the name.
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
|
||
public void Execute() | ||
{ | ||
rpcConfig = JobCommon.GetRpcConfig(config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Config can be injected into the method and gathered inside of the job.
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
if (shouldSend) | ||
{ | ||
shouldBacktrace = true; | ||
var url = ""; // TODO: upload to pastbin and get url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Active TODO
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
|
||
if (tips == null || tips.Length == 0) | ||
{ | ||
logger.LogWarning("ForkWatch: Got bad tip"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add additional information, bad tip
is not descriptive enough to determine a potential problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
return; | ||
} | ||
|
||
// check virtual checkpoint rolled-back |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments should explain why something is done, not how or what. Comment convention uses upper first letter.
CheckerApi/Jobs/ForkWatchJob.cs
Outdated
var message = $"ForkWatch: Virtual checkpoint {lastCheckpoint.Hash} at height {lastCheckpoint.Height} replaced by {hash}"; | ||
notificationManager.TriggerHook(message); | ||
}); | ||
// TODO: if foundReorg, move to "PREPARE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Active TODO
CheckerApi/Utils/JobCommon.cs
Outdated
using Microsoft.Extensions.Configuration; | ||
using System.Net; | ||
|
||
public class JobCommon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move as method inside of Job. Each job inherits that class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This PR adds a job to watch chain fork