HUBS-90 adds metadata templates to the subgraph #166
Conversation
elmariachi111
force-pushed
the
feature/hubs-90-read-ipnft-ipfs-metadata-during-subgraph-indexing
branch
from
c2968cb to
71be927
Compare
Signed-off-by: stadolf <[email protected]>
elmariachi111
force-pushed
the
feature/hubs-90-read-ipnft-ipfs-metadata-during-subgraph-indexing
branch
from
71be927 to
9130b99
Compare
WalkthroughThe changes in this pull request involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Subgraph
participant IPNFT
participant IPFS
Client->>Subgraph: Request IPNFT Metadata
Subgraph->>IPNFT: Fetch IPNFT Data
IPNFT-->>Subgraph: Return IPNFT Data
Subgraph->>IPFS: Retrieve Metadata
IPFS-->>Subgraph: Return Metadata
Subgraph-->>Client: Send IPNFT Metadata
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
subgraph/src/metadataMapping.ts (1)
1-22: Summary: Good start, but needs some refinements for robustness.Overall, the
handleMetadatafunction provides a good foundation for processing IPNFT metadata. However, there are several areas where it could be improved:
- Error handling for JSON parsing
- Clarification of the metadata identifier
- Handling of partial metadata information
- Conditional saving of metadata objects
These improvements will make the function more robust and flexible in handling various metadata scenarios.
Would you like assistance in implementing these suggestions or clarifying any of the proposed changes?
subgraph/schema.graphql (3)
10-12: LGTM! Consider makingupdatedAtTimestampnon-nullable.The addition of
metadataandupdatedAtTimestampfields to theIpnfttype enhances the data model by allowing for structured metadata storage and tracking of update times. This is a good improvement that maintains backwards compatibility.Consider making
updatedAtTimestampnon-nullable (i.e.,updatedAtTimestamp: BigInt!) if it's guaranteed to always have a value. This would ensure data integrity and simplify queries that rely on this field.
14-19: LGTM! Consider adding optional fields for extended metadata.The introduction of the
IpnftMetadatatype with essential NFT metadata fields is a good addition to the schema. It provides a structured way to store and query IPNFT metadata.Consider the following suggestions to enhance the metadata structure:
Add optional fields for extended metadata that might be specific to your use case. For example:
attributes: [Attribute!]Where
Attributecould be a new type withtrait_typeandvaluefields.Consider adding a
createdAt: BigInt!field to track when the metadata was first created.If versioning of metadata is important, consider adding a
version: Int!field.These suggestions can make the metadata structure more flexible and informative for future use cases.
Line range hint
1-19: Summary: Good additions for metadata handling.The changes to the GraphQL schema enhance the IPNFT data model by introducing structured metadata storage and update tracking. These additions are backwards compatible and provide more detailed information about IPNFTs.
Consider the following architectural implications:
Ensure that the subgraph indexing code is updated to populate the new
metadataandupdatedAtTimestampfields for theIpnfttype.Update any API or client code that queries the subgraph to take advantage of the new metadata fields.
If you plan to migrate existing IPNFTs to include metadata, consider creating a migration strategy to populate the new fields for existing entries.
Review and update any documentation or API specifications to reflect these schema changes.
These changes lay a good foundation for more detailed IPNFT information and should improve the overall functionality of the system.
subgraph/subgraph.yaml (1)
231-243: LGTM with minor formatting suggestions.The addition of the IpnftMetadata data source aligns well with the PR objective of adding metadata templates to the subgraph. However, there are a few minor formatting inconsistencies:
- The indentation of this new data source should be aligned with other data sources in the file.
- The "network" field is typically placed right after the "kind" field in other data sources.
Consider applying these formatting changes for consistency:
- - name: IpnftMetadata - kind: file/ipfs - mapping: - apiVersion: 0.0.7 - language: wasm/assemblyscript - file: ./src/metadataMapping.ts - handler: handleMetadata - entities: - - IpnftMetadata - abis: - - name: IPNFT - file: ./abis/IPNFT.json - network: foundry + - name: IpnftMetadata + kind: file/ipfs + network: foundry + mapping: + apiVersion: 0.0.7 + language: wasm/assemblyscript + file: ./src/metadataMapping.ts + handler: handleMetadata + entities: + - IpnftMetadata + abis: + - name: IPNFT + file: ./abis/IPNFT.jsonsubgraph/src/ipnftMapping.ts (3)
104-107: Check for empty string innewUriThe check
if (!newUri)might not catch cases wherenewUriis an empty string. In TypeScript, an empty string is a falsy value, but it might be safer to explicitly check for an empty string to ensure proper handling.Modify the condition to:
if (!newUri || newUri === '') { log.debug("No new URI found for token, likely just minted {}", [event.params._tokenId.toString()]) return }
15-15: Remove unused importTransferEventThe
TransferEventimport is added but not used in the code. Unused imports can clutter the code and may lead to confusion.Consider removing the unused import:
-import { - Transfer as TransferEvent -} from '../generated/IPNFT/IPNFT'
104-107: Ensure consistency in logging levelsIn
handleMetadataUpdated, the code useslog.debugto report missingnewUri. Depending on the importance of this event, you might consider usinglog.warningorlog.errorto make it more noticeable in the logs.Evaluate the logging level to ensure that important issues are appropriately flagged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- subgraph/package.json (1 hunks)
- subgraph/schema.graphql (1 hunks)
- subgraph/src/ipnftMapping.ts (3 hunks)
- subgraph/src/metadataMapping.ts (1 hunks)
- subgraph/subgraph.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- subgraph/package.json
🧰 Additional context used
🔇 Additional comments (2)
subgraph/src/metadataMapping.ts (1)
1-4: LGTM: Imports and function signature are appropriate.The imports and function signature are well-defined and appropriate for handling metadata in a subgraph context. The use of
Bytesas the input type is suitable for processing raw metadata content.subgraph/subgraph.yaml (1)
231-243: Please clarify the implications of using "file/ipfs" kind.The new IpnftMetadata data source uses
kind: file/ipfs, which differs from theethereum/contractkind used by other data sources. Additionally, it doesn't include asourcesection, which is present in other data sources.
- Could you explain the implications of using
file/ipfskind?- Is the absence of a
sourcesection intentional and expected for this kind of data source?- How does this data source interact with the IPFS system to fetch and process metadata?
Understanding these aspects will help ensure that the implementation aligns with the intended functionality for handling IPNFT metadata.
To help verify the usage of
file/ipfskind in The Graph's subgraph system, let's search for any documentation or examples:
only adds metadata relation if metadata is good Signed-off-by: stadolf <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
subgraph/src/ipnftMapping.ts (1)
78-91: Add unit tests forupdateIpnftMetadataThe new
updateIpnftMetadatafunction is crucial for handling IPNFT metadata updates. To ensure its correctness and robustness, consider adding unit tests that cover various scenarios, such as:
- Valid IPFS URIs
- Invalid URIs that do not start with
ipfs://- Empty or null URI values
- Edge cases with unusual but valid URIs
Would you like assistance in creating unit tests for this function or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- subgraph/src/ipnftMapping.ts (3 hunks)
- subgraph/src/metadataMapping.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- subgraph/src/metadataMapping.ts
🧰 Additional context used
🪛 Biome
subgraph/src/ipnftMapping.ts
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
subgraph/src/ipnftMapping.ts (3)
Line range hint
33-68: LGTM: handleReadAccess function implemented correctlyThe function correctly handles the ReadAccessGrantedEvent, creating a unique identifier for read access and saving it in the CanRead entity. The error handling and IPNFT existence check are properly implemented.
For improved code clarity, consider extracting the unique identifier generation logic into a separate helper function. This would make the main function more concise and easier to read.
🧰 Tools
🪛 Biome
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
78-88: LGTM with suggestion: updateIpnftMetadata functionThe function correctly handles the metadata update process for IPNFTs. However, there's a minor issue that could be improved:
When an invalid URI format is detected, the function logs an error but continues execution. Consider adding a return statement after the error log to prevent updating the IPNFT with potentially invalid data.
Suggested improvement:
function updateIpnftMetadata(ipnft: Ipnft, uri: string, timestamp: BigInt): void { let ipfsLocation = uri.replace('ipfs://', ''); if (!ipfsLocation || ipfsLocation == uri) { log.error("Invalid URI format for tokenId {}: {}", [ipnft.id, uri]); + return; } ipnft.tokenURI = uri; ipnft.metadata = ipfsLocation; ipnft.updatedAtTimestamp = timestamp; IpnftMetadataTemplate.create(ipfsLocation); }This change ensures that the IPNFT is not updated with invalid metadata.
112-116: LGTM with suggestion: handleMetadataUpdated function improvementsThe function has been updated to include null and empty string checks for newUri, which improves error handling. The use of the new updateIpnftMetadata helper function aligns with the refactoring done in other parts of the code.
However, there's a minor inconsistency in the error logging:
- When newUri is null or an empty string, it logs a debug message saying "no new uri found for token, likely just minted". This message might be misleading since this function handles metadata updates, not minting.
Consider updating the log message to better reflect the metadata update context:
- log.debug("no new uri found for token, likely just minted {}", [event.params._tokenId.toString()]) + log.debug("No new URI found for token {} during metadata update", [event.params._tokenId.toString()])This change makes the log message more accurate in the context of metadata updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- subgraph/src/ipnftMapping.ts (3 hunks)
🧰 Additional context used
🪛 Biome
subgraph/src/ipnftMapping.ts
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (5)
subgraph/src/ipnftMapping.ts (5)
3-3: LGTM: Import statements updated correctlyThe new import statements align with the added functionality in the file. The inclusion of BigInt, various event types, and IpnftMetadataTemplate suggests enhanced handling of IPNFT-related operations.
Also applies to: 11-11, 14-16, 18-19
🧰 Tools
🪛 Biome
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
Line range hint
70-75: LGTM: handleReservation function implemented correctlyThe function properly handles the ReservedEvent by creating a new Reservation entity with the provided information. The implementation is concise and correct.
96-99: LGTM: handleMint function updated correctlyThe handleMint function has been updated to use the new updateIpnftMetadata helper function, which simplifies the metadata update process during minting. The reservation removal logic remains intact, ensuring consistency with the previous implementation.
3-3: Static analysis hint: Ignore BigInt shadowing warningThe static analysis tool flagged the import of BigInt as potentially shadowing the global BigInt property. However, in the context of AssemblyScript and The Graph, importing BigInt from @graphprotocol/graph-ts is correct and necessary.
This warning can be safely ignored as it's likely a false positive due to the tool not being aware of the specific environment of The Graph's AssemblyScript.
🧰 Tools
🪛 Biome
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
Line range hint
1-118: Overall assessment: Significant improvements to IPNFT handlingThis update introduces several enhancements to the IPNFT handling:
- New event handlers for read access and reservations.
- A helper function for updating IPNFT metadata, improving code reusability.
- Improved error handling and logging in existing functions.
These changes align well with the PR objectives of adding metadata templates to the subgraph. The code is generally well-structured and follows good practices. The minor suggestions provided in the review comments will further improve code clarity and robustness.
Great job on enhancing the IPNFT functionality!
Signed-off-by: stadolf <[email protected]>
elmariachi111
force-pushed
the
feature/hubs-90-read-ipnft-ipfs-metadata-during-subgraph-indexing
branch
from
c952a3e to
2b386c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
subgraph/src/ipnftMapping.ts (2)
Line range hint
33-71: LGTM: handleReadAccess function implemented correctlyThe function correctly processes read access events and creates a unique identifier for each access grant. However, consider adding error logging when the IPNFT doesn't exist or when encoding fails.
Consider adding error logging:
if (!ipnft) { + log.error("IPNFT not found for tokenId: {}", [event.params.tokenId.toString()]); return; } // ... (existing code) if (!encoded) { + log.error("Failed to encode tuple for tokenId: {}", [event.params.tokenId.toString()]); return; }🧰 Tools
🪛 Biome
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
113-117: LGTM: handleMetadataUpdated function improvedThe function now includes better error handling for the newUri and uses the updateIpnftMetadata helper, reducing code duplication. These changes align with previous review suggestions.
Consider adding more detailed error logging:
if (!newUri || newUri == "") { - log.debug("no new uri found for token, likely just minted {}", [event.params._tokenId.toString()]) + log.warning("No new URI found for token {}, metadata update skipped", [event.params._tokenId.toString()]) return }This change provides more context about why the function is returning early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- subgraph/src/ipnftMapping.ts (3 hunks)
🧰 Additional context used
🪛 Biome
subgraph/src/ipnftMapping.ts
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (5)
subgraph/src/ipnftMapping.ts (5)
3-3: LGTM: Import statements updated correctlyThe new imports align with the added functionality, and renaming IPNFT to IPNFTContract improves code clarity.
Also applies to: 11-11, 14-16, 18-19
🧰 Tools
🪛 Biome
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
Line range hint
73-77: LGTM: handleReservation function implemented correctlyThe function properly creates and saves a new Reservation entity with the relevant event parameters.
78-89: LGTM: updateIpnftMetadata function implemented correctlyThe function effectively updates the IPNFT metadata, validating the URI format and updating all necessary fields. This implementation aligns with the previous refactoring suggestion, improving code maintainability.
97-100: LGTM: handleMint function updated correctlyThe function now uses the updateIpnftMetadata helper, reducing code duplication. The removal of the Reservation after minting is a logical step in the process.
3-3: Ignore static analysis warning about BigInt importThe static analysis tool flagged the import of BigInt as potentially shadowing the global BigInt property. However, in the context of AssemblyScript and The Graph, importing BigInt from @graphprotocol/graph-ts is correct and necessary. This warning can be safely ignored as it's likely a false positive due to the tool's lack of context about The Graph's development environment.
🧰 Tools
🪛 Biome
[error] 3-3: Do not shadow the global "BigInt" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
elmariachi111
changed the title
elmariachi111
deleted the
feature/hubs-90-read-ipnft-ipfs-metadata-during-subgraph-indexing
branch
this doesn't exactly work as expected but also doesn't really break sth :)
https://linear.app/molecule-to/issue/HUBS-90/read-ipnft-ipfs-metadata-during-subgraph-indexing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation