-
Notifications
You must be signed in to change notification settings - Fork 7
Support external verification status tracking in job verification page #14
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
base: staging
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for verify-sourcify-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
app/routes/jobs.$jobId.tsx
Outdated
| <p className="text-sm text-gray-500">Statuses refresh every {EXTERNAL_VERIFICATION_STATUS_REFRESH_RATE_SECONDS} seconds.</p> | ||
| </div> | ||
| <div className="space-y-4"> | ||
| {Object.entries(jobData.externalVerifications) |
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 we order the verifiers? I would do it by adoption, which should be in this order: Etherscan, Blockscout, Routescan.
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 would actually put Etherscan last because it's not part of the verifier alliance :)
app/routes/jobs.$jobId.tsx
Outdated
| </div> | ||
| )} | ||
|
|
||
| {/* External Verifier Statuses */} |
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 it's easy, can we maybe refactor the new part of the page into a component? The JobDetails component grew quite big imo
| const externalVerifierStatusesRef = useRef<ExternalVerifierStatusMap>({}); | ||
| const { serverUrl } = useServerConfig(); | ||
| const hasExternalVerificationData = hasAllRequiredExternalVerifications(jobData?.externalVerifications); | ||
| const isJobFullyCompleted = Boolean(jobData?.isJobCompleted) && hasExternalVerificationData; |
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 there a scenario where we wouldn't store data for all external verifiers? I think requiring the data to be present isn't perfect.
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 found this also makes the page refetching data for old jobs which don't have external verifications for an indefinite amount of time.
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 there a scenario where we wouldn't store data for all external verifiers? I think requiring the data to be present isn't perfect.
I think we don't have to support the case in which we change settings in Sourcify (e.g. we disable a specific verifier). UI should just mirror whatever configuration is currently in use. If we change the config of server, then we manually update also UI. The alternative would be to returned the list of enabled storage services so the UI can adapt.
I found this also makes the page refetching data for old jobs which don't have external verifications for an indefinite amount of time.
Right it makes sense. I think we should limit with a maximum amount of re-fetch
See argotorg/sourcify#2425