-
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
PoR Off-Chain #914
PoR Off-Chain #914
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! I left comments, but they are more recommendation and we can fix it in next PR. It is usually related to reuse of implementation from other files so we do not copy & paste code.
BTW I notice, you store data only in data
table. It would be better to store it also in aggregate
table which is then used for visualizations.
} | ||
|
||
async function fetchData(feed) { | ||
const rawDatum = await (await axios.get(feed.url)).data |
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.
We should make try-catch
block here.
@@ -0,0 +1,71 @@ | |||
import { PorError, PorErrorCode } from './errors' | |||
|
|||
export const DATA_FEED_REDUCER_MAPPING = { |
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.
Can't we just import the reducer, rather than copy it?
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.
would it be okay to import reducer.ts from ./src/worker/reducer.ts
? thought por can't refer from worker
folder
@@ -8,7 +8,6 @@ export class FetcherError extends Error { | |||
} | |||
|
|||
export enum FetcherErrorCode { | |||
IncompleteDataFeed, |
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.
Is this update somehow related to the 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.
This is from fetcher. I realized that we did not used it anywhere tried to clean-up.
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.
Okay! Good good!
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.
it would be appreciated if you could make a separate PR for refactoring from next time :)
@@ -0,0 +1,6 @@ | |||
export interface IData { |
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.
If possible, it would be also good if we can reuse the implementation from other files. We don't have to do it now, but we should at least make an issue for it.
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
@martinkersner @nick-bisonai I will merge the PR now, and will make a new PR for the recommend changes. |
Description
This PR is ready to review. Main changes with this PR is to make a separate directory to
POR
, where it will be running upon request.How to run PoR
POR_AGGREGATOR_HASH
yarn build
yarn start:por
Changes:
POR_AGGREGATOR_HASH
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment