-
Notifications
You must be signed in to change notification settings - Fork 32
add EIP status history feature #100
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
Conversation
| return 'Declined for Inclusion'; | ||
| case 'Included': | ||
| return 'Included'; | ||
| case 'Withdrawn': |
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.
added Withdrawn because i noticed it in several EIP json files, and felt Declined would be inaccurate:
6873.json - withdrawn
7667.json - withdrawn
7999.json - withdrawn
| import { EIP } from '../types/eip'; | ||
| import eipsDataRaw from './eips.json'; | ||
|
|
||
| export const eipsData = eipsDataRaw as EIP[]; |
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.
the reason for the type assertion is because typescript won't infer the union types inside the new statusHistory field.
i believe it's safer to keep the union and do a type assertion than use a string and leave it to inference. we can always strengthen safety in the future with a compile time check (eg. in compile-eips.mjs).
wolovim
left a comment
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.
thanks for the PR! as discussed in dms, i think this is the right abstraction and starting point to be able to show eip history.
there's enough going on now that i think its worth validating data types when contributors edit eips. just tacked on ajv schema validation that runs during the build step - it actually caught one case where isHeadliner was found outside of the forkRelationships.
the other thing that comes to mind is how we might support references beyond just status updates, e.g., when an eip was presented, debated, mentioned. multiple ways forward that can be sorted out later (e.g., a new discussions key or adding new types and entries to the status history).
Description
This PR introduces a new field to the EIP data type:
statusHistory. This enables Forkcast to encode the minimum necessary information for users to understand how the inclusion status of an EIP progresses over forks and ACD calls.This PR only adds status history for the most recent ACDE 226 call. In a future PR, we can add more status history and UI components to take advantage of the new data.
How I reviewed this PR