HUBS-124 adds more relevant ipnft metadata fields to the subgraph #172
Conversation
Signed-off-by: Stefan Adolf <stefan@molecule.to>
uses a later ipfs node for local envs Signed-off-by: Stefan Adolf <stefan@molecule.to>
uses strings for the funding amount Signed-off-by: Stefan Adolf <stefan@molecule.to>
WalkthroughThis pull request introduces comprehensive updates to the subgraph implementation for IP-NFT metadata handling. The changes span multiple files, including the GraphQL schema, mapping files, and test fixtures. The primary focus is on enhancing metadata extraction, adding new fields for project and funding details, and improving logging mechanisms. The IPFS service in the docker-compose file is also updated to a newer image version, and the deployment scripts are incrementally versioned to 1.3.1. Changes
Sequence DiagramsequenceDiagram
participant Tokenizer
participant Subgraph
participant IPFS
participant Metadata
Tokenizer->>Subgraph: Create IPT
Subgraph->>Metadata: Fetch Metadata
IPFS-->>Metadata: Retrieve JSON
Metadata->>Subgraph: Parse Metadata
Subgraph->>Subgraph: Save Enriched Metadata
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
🪧 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: 6
🧹 Nitpick comments (8)
subgraph/src/tokenizerMapping.ts (1)
27-31: Consider adding error logging for missing Ipnft.While the null check is correct, consider adding error logging when the Ipnft is not found to help with debugging.
let ipnft = Ipnft.load(event.params.ipnftId.toString()); if (ipnft) { ipnft.ipToken = event.params.tokenContract ipnft.save() + } else { + log.warning('Ipnft not found for id: {}', [event.params.ipnftId.toString()]) }subgraph/tests/metadata.test.ts (2)
10-24: Enhance test coverage with additional test cases.While the current test verifies successful metadata parsing, consider adding:
- Negative test cases for missing or malformed metadata
- Tests for all metadata fields, not just a subset
- Tests for error conditions in IPFS data fetching
25-25: Remove commented debug code.The commented
logStore()line appears to be debugging code that should be removed.subgraph/src/metadataMapping.ts (1)
73-80: Improve type conversion documentation and error handling.The comment about type conversion could be more explicit, and error handling for the conversion should be added.
- // on json metadata this can be a decimal value. I'm using a string to store as there's imo no f64 compatible decimal type on the schema scalar types - // https://thegraph.com/docs/en/subgraphs/developing/creating/ql-schema/#built-in-scalar-types + // Store decimal values as strings since The Graph's schema doesn't support floating-point types + // See: https://thegraph.com/docs/en/subgraphs/developing/creating/ql-schema/#built-in-scalar-types + try { ipnftMetadata.fundingAmount_value = _fundingAmount_value.toF64().toString() ipnftMetadata.fundingAmount_decimals = i8(_fundingAmount_decimals.toI64()) ipnftMetadata.fundingAmount_currency = _fundingAmount_currency.toString() ipnftMetadata.fundingAmount_currencyType = _fundingAmount_currencyType.toString() + } catch (e) { + log.error('Failed to convert funding amount values: {}', [e.toString()]) + }deploy/inittest.sh (1)
20-23: Consider improving the network configuration handling.The current approach using sed commands is marked as a "bad local config hack". Consider:
- Using environment variables or configuration files instead
- Adding platform-specific sed syntax for Linux compatibility
subgraph/tests/fixtures/ipnft_1.json (1)
22-24: Avoid hardcoding network-specific values in test fixtures.The contract address is hardcoded to the Sepolia network. Consider making this configurable or using a placeholder address for tests.
{ "conditionType": "evmContract", - "contractAddress": "0x152B444e60C526fe4434C721561a077269FcF61a", - "chain": "sepolia", + "contractAddress": "{{CONTRACT_ADDRESS}}", + "chain": "{{NETWORK}}", "functionName": "canRead",subgraph/schema.graphql (2)
24-26: Consider making essential metadata fields non-nullable.Fields like
organizationandtopicappear to be essential metadata. Consider making them non-nullable if they are required fields in the business logic.- initialSymbol: String - organization: String - topic: String + initialSymbol: String! + organization: String! + topic: String!
31-34: Ensure consistent naming convention for nested fields.The funding amount fields use underscores, while GraphQL typically uses camelCase. Consider updating the field names for consistency.
- fundingAmount_value: String - fundingAmount_decimals: Int8 - fundingAmount_currency: String - fundingAmount_currencyType: String + fundingAmountValue: String + fundingAmountDecimals: Int8 + fundingAmountCurrency: String + fundingAmountCurrencyType: String
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deploy/inittest.sh(1 hunks)docker-compose.yml(1 hunks)subgraph/.gitignore(1 hunks)subgraph/package.json(1 hunks)subgraph/schema.graphql(2 hunks)subgraph/src/ipnftMapping.ts(1 hunks)subgraph/src/metadataMapping.ts(2 hunks)subgraph/src/tokenizerMapping.ts(2 hunks)subgraph/tests/fixtures/ipnft_1.json(1 hunks)subgraph/tests/metadata.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- subgraph/.gitignore
- subgraph/src/ipnftMapping.ts
🔇 Additional comments (2)
subgraph/src/tokenizerMapping.ts (1)
6-6: LGTM! Import statement updated correctly.The import statement now includes the Ipnft entity alongside IPT.
subgraph/package.json (1)
11-12: Verify version label consistency across deployment environments.The version label has been updated to
1.3.1-dev.9for both Sepolia and Mainnet deployments, which is good for consistency. However, using a development version (dev.9) in the Mainnet deployment script might not be ideal for production.✅ Verification successful
Version labels are consistent, but consider production versioning strategy
The version label
1.3.1-dev.9is consistently used across all deployment configurations. While using a development version in mainnet deployment scripts is unusual, it appears to be intentional for testing purposes. Consider establishing a clear versioning strategy for production releases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other version references that need to be updated rg "1\.3\.0|1\.3\.1" --type yaml --type jsonLength of output: 546
elmariachi111
force-pushed
the
feature/hubs-124-add-more-relevant-ipnft-metadata-fields-to-the-subgraph
branch
from
05a2fbb to
c9c83f1
Compare
adds ipfs reading / conversion tests rolls back pnpm to yarn deescalates sample key releases 1.3.1 Signed-off-by: Stefan Adolf <stefan@molecule.to>
elmariachi111
force-pushed
the
feature/hubs-124-add-more-relevant-ipnft-metadata-fields-to-the-subgraph
branch
from
c9c83f1 to
d221d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
subgraph/src/metadataMapping.ts (3)
36-49: Consider adding logging for missing project details fields.For consistency with the initial_symbol handling, consider adding logging when organization or topic fields are missing. This would help with debugging.
if (_organization) { ipnftMetadata.organization = _organization.toString() + } else { + log.info("[handlemetadata] organization not found", []) } let _topic = projectDetails.get('topic') if (_topic) { ipnftMetadata.topic = _topic.toString() + } else { + log.info("[handlemetadata] topic not found", []) }
51-62: Consider handling partial research lead data.The current implementation only saves research lead data if both email and name are present. Consider:
- Adding logging when fields are missing
- Allowing partial data to be saved
if (_research_lead) { let researchLead = _research_lead.toObject() let researchLead_email = researchLead.get('email') let researchLead_name = researchLead.get('name') - if (researchLead_email && researchLead_name) { + if (researchLead_email) { ipnftMetadata.researchLead_email = researchLead_email.toString() + } else { + log.info("[handlemetadata] research_lead email not found", []) + } + if (researchLead_name) { ipnftMetadata.researchLead_name = researchLead_name.toString() + } else { + log.info("[handlemetadata] research_lead name not found", []) } }
64-82: Consider handling partial funding amount data.The current implementation only saves funding amount data if all fields are present. Consider:
- Adding logging for missing fields
- Allowing partial data to be saved when available
if (_funding_amount) { let funding_amount = _funding_amount.toObject() let _fundingAmount_value = funding_amount.get('value') let _fundingAmount_decimals = funding_amount.get('decimals') let _fundingAmount_currency = funding_amount.get('currency') let _fundingAmount_currencyType = funding_amount.get('currency_type') - if (_fundingAmount_value && _fundingAmount_decimals && _fundingAmount_currency && _fundingAmount_currencyType) { + if (_fundingAmount_value) { ipnftMetadata.fundingAmount_value = _fundingAmount_value.toF64().toString() + } else { + log.info("[handlemetadata] funding_amount value not found", []) + } + if (_fundingAmount_decimals) { ipnftMetadata.fundingAmount_decimals = i8(_fundingAmount_decimals.toI64()) + } else { + log.info("[handlemetadata] funding_amount decimals not found", []) + } + if (_fundingAmount_currency) { ipnftMetadata.fundingAmount_currency = _fundingAmount_currency.toString() + } else { + log.info("[handlemetadata] funding_amount currency not found", []) + } + if (_fundingAmount_currencyType) { ipnftMetadata.fundingAmount_currencyType = _fundingAmount_currencyType.toString() + } else { + log.info("[handlemetadata] funding_amount currency_type not found", []) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
subgraph/.gitignore(1 hunks)subgraph/package.json(1 hunks)subgraph/src/metadataMapping.ts(2 hunks)subgraph/tests/fixtures/ipnft_1.json(1 hunks)subgraph/tests/metadata.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- subgraph/.gitignore
- subgraph/package.json
- subgraph/tests/metadata.test.ts
- subgraph/tests/fixtures/ipnft_1.json
🔇 Additional comments (4)
subgraph/src/metadataMapping.ts (4)
19-21: Remove duplicate save() call.The entity is saved twice: once after basic metadata and again at the end of the function. Remove the first save() call as it's redundant.
ipnftMetadata.description = description.toString() - ipnftMetadata.save() } else {
22-23: LGTM! Good addition of logging.The logging statement will help with debugging when essential metadata fields are missing.
25-34: LGTM! Well-structured properties handling.Good practices observed:
- Proper null checks
- Explicit default value
- Helpful logging for debugging
74-75: LGTM! Helpful type conversion documentation.Good practice: The comment clearly explains the rationale behind using string type for decimal values and includes a reference to the documentation.
| @@ -9,6 +9,9 @@ type Ipnft @entity { | |||
| ipts: [IPT!] @derivedFrom(field: "ipnft") | |||
| metadata: IpnftMetadata | |||
| updatedAtTimestamp: BigInt | |||
|
|
|||
| # as of 1.3 the Tokenizer only will create 1 ipt instance, in contrast to the ipts field above. | |||
| ipToken: Bytes | |||
There was a problem hiding this comment.
what is the reason to add this field when we have the ipts field? for the Tokenizer 1.3 we would have an array with only one element
There was a problem hiding this comment.
exactly. That's what the comment says. This is here as we since 1.3 only need 1 IPT instance but never updated the subgraph accordingly. Mainly as we need to also upgrade the frontend. If we'd agree that the only IPT is just one field, we can easily adopt the frontend and switch off the ipts array. The more pragmatic reason for this field being a plain address is that Tom can simply use it for ordering IPNFTs.
elmariachi111
deleted the
feature/hubs-124-add-more-relevant-ipnft-metadata-fields-to-the-subgraph
branch
|
Hello I’m currently testing a large-scale smart contract deployment on the Sepolia testnet and need a higher amount of Sepolia ETH than most faucets provide. My project involves deploying + smart contracts, each managing multiple wallets, so I’m looking for [X amount] of Sepolia ETH to continue testing. I’ve already used available faucets, but they have daily limits that slow down my progress. If anyone has extra Sepolia ETH or knows a source for larger distributions, I’d really appreciate the help. 1000 or 500 sepolia is all I need 0x96070538C13533Cb838B2fb1799A3e5f621155eD I’m happy to return any unused ETH after testing or contribute back to the community in other ways. Thanks in advance! Best wishes |
extends the Subgraph with more IPNFT metadata. Please look at the matchstick test and notice the elegance how the graph allows you to test this case https://github.com/moleculeprotocol/IPNFT/pull/172/files#diff-e695976b80e70791b2ad0d0c0facbf1cbad3db6164e6f4c6f1b78e39a48012fc. Please be careful when just adopting this on the frontend: if you just trust this approach, the latency between minting & data appears on the subgraph will need to be handled. It's pretty nice to just query and display historic dat
Summary by CodeRabbit
Release Notes v1.3.1
New Features
Bug Fixes
Chores
Tests