Skip to content
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

Conforming Guardian’s Sustainability Assets to an Enhanced HIP412 Standard #929

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mattsmithies
Copy link

Description:

This PR adds a new draft HIP titled "Conforming Guardian’s Sustainability Assets to an Enhanced HIP412 Standard". This new HIP was created by Sustainability/Guardian Working Group in collaboration with the community over a period of 6 months. It enables guardian standard compliant systems a method to map verified sustainability data from trustchains into a NFT (HIP412) form suitable to be consumed from any external service, such as marketplaces and wallets.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Copy link

netlify bot commented Mar 27, 2024

Deploy Preview for hedera-hips failed.

Name Link
🔨 Latest commit 4d0eaaf
🔍 Latest deploy log https://app.netlify.com/sites/hedera-hips/deploys/660d35db14fc3700081e4ae8

@fessmm
Copy link
Contributor

fessmm commented Mar 27, 2024

you should not edit the template instead create a new file

hip-0000-template.md Outdated Show resolved Hide resolved
"attributes": [
{
"trait_type": "timestamp",
"value": 1710271999.591571
Copy link
Contributor

Choose a reason for hiding this comment

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

HIP412 specifies a unix timestamp in seconds. We might need to specify that validators should consider nano seconds too for timestamp.

Copy link
Author

Choose a reason for hiding this comment

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

This should switch to string in this case.

Comment on lines 216 to 217
"trait_type": "timestamp",
"value": "1710271999.591571003"
Copy link
Contributor

Choose a reason for hiding this comment

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

The default display type in HIP412 is text. We can help explorers by adding the way we would like this represented.

Suggested change
"trait_type": "timestamp",
"value": "1710271999.591571003"
"trait_type": "timestamp",
"display_type": "datetime",
"value": "1710271999.591571003"

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, is there a way for a attribute to be non-indexable as filtering on timestamps is nonsense

Comment on lines 253 to 260
"value": [
"air",
"water",
"soil",
"biodiversity",
"equity",
"carbon"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

HIP 412 doesn't support arrays so we'll need to raise this as a new value type that can be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Existing explorers would need to make changes to their filtering features to support arrays here.

The alternative is to list them all at the root level which would allow them to act as filters without any change to existing NFT markets.

{
    "trait_type": "co-benefits-air",
    "display_type": "boolean",
    "value": true
},
{
    "trait_type": "co-benefits-water",
    "display_type": "boolean",
    "value": true
},

I do prefer the array solution originally proposed, just be aware that it can't be used as a filter out of the gate until markets update to support it.

Copy link
Author

Choose a reason for hiding this comment

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

I like this.

@mattsmithies
Copy link
Author

you should not edit the template instead create a new file

Sorry sir, will resolve.

@mattsmithies
Copy link
Author

Updated the current HIP with changes from @justynspooner, @fessmm, and @Neurone there will likely be more changes moving forward, but if anyone wants to add any additional thoughts around this, that would be great!

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