Skip to content

Conversation

@nikgraf
Copy link
Collaborator

@nikgraf nikgraf commented Feb 24, 2025

  • Add branded type Id with refined that validates the ID. see https://effect.website/docs/code-style/branded-types/#refined
  • Add dynamic validation for all Graph functions
  • Changed the following system IDs (since they were invalid e.g. length of 21):
    • QUOTES_THAT_SUPPORT_CLAIMS_PROPERTY from quotesThatSupportClaims to WoWTti3qGM1nvz28K9zu1f
    • ACADEMIC_FIELD_TYPE from ExCjm3rzYVfpMRwDchdrE to KwKGhkY5AmzmHHggg7abvi

@nikgraf nikgraf requested a review from baiirun February 24, 2025 20:49
@nikgraf nikgraf self-assigned this Feb 24, 2025
@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2025

🦋 Changeset detected

Latest commit: 3cc92b0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphprotocol/grc-20 Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@baiirun baiirun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Main thing is figuring out how to migrate any existing data to the changed ids that you found.

@baiirun
Copy link
Contributor

baiirun commented Feb 24, 2025

Should CITY_TYPE also be updated? Are there other ids that are 22 characters instead of 21?

@nikgraf
Copy link
Collaborator Author

nikgraf commented Feb 24, 2025

I think not, but will check again and will update the list of updated IDs

@nikgraf nikgraf requested a review from baiirun February 26, 2025 08:57
"@graphprotocol/grc-20": minor
---

Update `QUOTES_THAT_SUPPORT_CLAIMS_PROPERTY` and `ACADEMIC_FIELD_TYPE` values to be valid IDs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to figure out the migration path for these since there's already data in the mainnet KG using these ids.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's KG?

One thing we could do is simply create new entities and change the entries manually. In this case in old version we still have old versions pointing to invalid IDs, but since the data is already on chain we can't do a simply SQL migration script or can we?


if (cover) {
// add ID of cover "Image" to property "Cover"
assertValid(cover);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be useful to pass a specific error message to assertValid to denote what the issue is. Would help consumers know which of their ids is invalid and why more easily.

For example this one might be something like Invalid id for "cover". Here's why ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 will do this in a follow up PR

}
}

export function assertValid(id: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be useful to pass a specific error message to the Error so consumers get more context as to why the error happened.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 will do this in a follow up PR

@nikgraf nikgraf merged commit 4b4cc33 into main Mar 3, 2025
3 checks passed
@nikgraf nikgraf deleted the ng/id-validation branch March 3, 2025 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants