-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix PoR #931
Fix PoR #931
Conversation
…into i-913/feat/script-for-PoR
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.
lgtm!
throw new OraklError(OraklErrorCode.FailedInsertData) | ||
} | ||
} | ||
|
||
export async function insertAggregateData({ |
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 have a question.
Where is this function referenced?
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.
@nick-bisonai Inside fetchWithAggregator
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.
@nick-bisonai Here is the reference function.
https://github.com/Bisonai/orakl/blob/master/fetcher/src/job/job.api.ts#L69
The function is called to store the aggregated data through orakl-api
.
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.
LGTM! I left couple of minor comments. Please let me know if I did not misunderstood something :)
I have fixed the issue which you mentioned. Now the PR is ready to merge, please let me know if you have a further comments. |
core/src/por/reporter.ts
Outdated
const heartbeat = aggregator.heartbeat | ||
|
||
if (updatedAt + heartbeat < timestamp) { | ||
if (heartbeat >= POR_LATENCY_BUFFER && updatedAt + heartbeat - POR_LATENCY_BUFFER < now) { |
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.
@bayram98 I think this does not help us to catch the error at the beginning. It will actually hide the issue until the time when we expect the heartbeat to occur.
heartbeat >= POR_LATENCY_BUFFER
might evaluate to false
and if there is no deviation change, then the function shouldReport
returns false and behaves as if nothing wrong happened.
If heartbeat < POR_LATENCY_BUFFER
is true, then the settings is wrongly defined, and we should throw an error.
if (heartbeat < POR_LATENCY_BUFFER) {
throw Error('Heartbeat cannot be smaller then latency buffer.')
}
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.
@martinkersner I added your suggestion to check if heartbeat and POR_LATENCY_BUFFER are set correctly or not. Please check it out.
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.
Beautiful! Let's merge!
Description
This PR is ready to review. Mainly tried to solve the issue from previous Pr(#914).
Changes:
insertAggregateData
function to insert aggregatedDataPOR
service type-60
sec time buffer. This would fix the issue of submitting for 3min instead of 2min.Next:
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment