Skip to content

fix(network-subgraphs): [ETH-876] limit the length of Stream entity id field #992

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jtakalai
Copy link
Contributor

@jtakalai jtakalai commented May 20, 2025

Changes

If streamId is too long, cut and use hash (for uniqueness). Currently no streams have such long IDs, so for existing clients, this change is transparent.

minor cleanup: let -> const

Why

Overlong ID could break the subgraph indexing

Future work

This provides a simple upgrade path for SDK: instead of querying Stream.id, query Stream.streamId (which as an added bonus is of type String and not ID so can be queried with "contains")

Copy link

linear bot commented May 20, 2025

@teogeb
Copy link
Contributor

teogeb commented May 21, 2025

My suggestion is that we limit the length of the stream ID in StreamRegistry#createStream. If I understood correctly, there is no need for overlong stream ids in practice. The approach in this PR adds significant amount of complexity and confusion as "Stream#id" field in The Graph is no longer same as Stream's ID.

@jtakalai
Copy link
Contributor Author

jtakalai commented May 21, 2025

My suggestion is that we limit the length of the stream ID in StreamRegistry#createStream.

It's an option that requires a change to the StreamRegistry contract. The current PR works as a subgraph update. Of course it can still be changed later with minimal hassle.

The complexity added here isn't very significant, and confusion can be remedied with documentation.

not part of this PR
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants