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

Feat/tokens metadata #576

Merged
merged 44 commits into from
Sep 1, 2021
Merged

Feat/tokens metadata #576

merged 44 commits into from
Sep 1, 2021

Conversation

asimm241
Copy link
Contributor

@asimm241 asimm241 commented May 4, 2021

Description

This PR adds the support for Fungible token metadata and Non Fungible token metadata. It checks the deployed contract's compliance with sip-9 and sip-10.

It checks the standards by comparing contract abi. If it is compliance, It fetches data from the contract by making read-only call to:

For FT:

  • get-name
  • get-token-uri
  • get-symbol
  • get-decimals

For NFT:

  • get-last-token-id
  • get-token-uri

Using the token uri it fetches the metdata from the web, and store the relevent data in the database.

It exposes two APIs

  1. v1/tokens/:contractId/ft/metadata
  2. v1/tokens/:contractId/nft/metadata

Type of Change

  • New feature
  • Bug fix
  • API reference/documentation update
  • Other

Are documentation updates required?

@zone117x do we need to add documentation

Testing information

npm run test:integration:tokens
npm run test:token

Checklist

  • Code is commented where needed
  • Unit test coverage for new or modified code paths
  • npm run test passes
  • Changelog is updated

@project-bot project-bot bot added this to 💻 In development in Splat Team May 4, 2021
@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #576 (ce5b76c) into develop (e37b5af) will decrease coverage by 0.91%.
The diff coverage is 53.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #576      +/-   ##
===========================================
- Coverage    65.52%   64.60%   -0.92%     
===========================================
  Files           79       87       +8     
  Lines         8049     8759     +710     
  Branches      1257     1346      +89     
===========================================
+ Hits          5274     5659     +385     
- Misses        2770     3095     +325     
  Partials         5        5              
Impacted Files Coverage Δ
src/datastore/common.ts 78.64% <ø> (ø)
src/datastore/memory-store.ts 10.74% <0.00%> (-0.52%) ⬇️
src/index.ts 0.00% <0.00%> (ø)
src/tests-tokens/setup.ts 0.00% <0.00%> (ø)
src/tests-tokens/teardown.ts 0.00% <0.00%> (ø)
src/tests-tokens/tokens-metadata-tests.ts 0.00% <0.00%> (ø)
src/event-stream/tokens-contract-handler.ts 75.83% <75.83%> (ø)
src/helpers.ts 63.32% <77.27%> (+0.73%) ⬆️
src/api/routes/tokens/tokens.ts 91.66% <91.66%> (ø)
src/datastore/postgres-store.ts 84.63% <94.59%> (+0.49%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e37b5af...ce5b76c. Read the comment docs.

@asimm241 asimm241 requested a review from zone117x May 4, 2021 11:18
@asimm241 asimm241 changed the base branch from master to develop May 4, 2021 11:21
Copy link
Member

@zone117x zone117x left a comment

Choose a reason for hiding this comment

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

So far so good. I think next steps are ingesting token metadata from the event server.

docs/index.d.ts Outdated Show resolved Hide resolved
@agraebe
Copy link
Contributor

agraebe commented May 11, 2021

@asimm241 there is a team (Swapr) that would rely on the completion of this PR by the end of May. would this timeline work for you?

cc @aulneau @andresgalante

@aulneau
Copy link
Contributor

aulneau commented May 11, 2021

@asimm241 there is a team (Swapr) that would rely on the completion of this PR by the end of May. would this timeline work for you?

cc @aulneau @andresgalante

Expanding on this, this PR would enable our wallets to support the FT (SIP 010) and NFT (SIP 009) standards in that we'd be able to properly fetch metadata associated with a given token. This would allow us to display the ticker, name, decimals (for fungible tokens) and the asset image. We have some options on how we could do this ourselves via contract calls in the wallets, but an API based approach would be much better in the long term.

Related: leather-io/extension#1070, leather-io/extension#1028

@psq
Copy link
Contributor

psq commented May 12, 2021

I have the same questions as @aulneau, and not clear how this would get used (i.e. queried) to help wallets and explorers. for example, there is no event generated when a new sip-011 token is created (besides a contract deployment), so how would the api node know that it needs to call a bunch of functions on a contract to retrieve things like symbol, name, decimals, and metadata (which requires 2 rpc call), and the sip-010 (and sip-009) meta data schema is different (albeit "compatible", so far).

And with the experience building swapr, the metadata probably should not have a fixed schema, or at a minimum allow for storing/retrieving extra top level elements.

@asimm241
Copy link
Contributor Author

@asimm241 there is a team (Swapr) that would rely on the completion of this PR by the end of May. would this timeline work for you?

cc @aulneau @andresgalante

I think so.

@zone117x
Copy link
Member

zone117x commented May 12, 2021

there is no event generated when a new sip-011 token is created (besides a contract deployment), so how would the api node know that it needs to call a bunch of functions on a contract to retrieve things like symbol, name, decimals, and metadata (which requires 2 rpc call)

The API can check the ABI in the contract-deploy tx handler. If the ABI "contains" the expected sip-10 or sip-09 elements it can be passed to another component responsible for retrieving the initial metadata.

and the sip-010 (and sip-009) meta data schema is different (albeit "compatible", so far).

The API can/will expose differences here where necessary?

And with the experience building swapr, the metadata probably should not have a fixed schema, or at a minimum allow for storing/retrieving extra top level elements.

Can you elaborate or give some examples here?

@psq
Copy link
Contributor

psq commented May 12, 2021

The API can check the ABI in the contract-deploy tx handler. If the ABI "contains" the expected sip-11 or sip-09 elements it can be passed to another component responsible for retrieving the initial metadata.

That will not work consistently, see for example https://github.com/tokensoft/tokensoft_token_stacks/blob/main/contracts/tokensoft-token.clar#L110-L119, where the meta data url can be changed at will, and of course, the meta data file itself can be changed even more often (not that would know any token that rebranded, ever, 😉 )

I'm also considering doing something similar where I have some generic swapr token that are pre-deployed (simplifies the ui), but gets configured with the correct data, when creating a new pair for example.

Can you elaborate or give some examples here?

I'm experimenting with adding one more image format (explicitly vector) for one, and for a second, swapr will need some extra meta data to help generate the proper post conditions (find the token name, and some token may require special case, so we need something fairly fluid that would store whatever is in the metadata file (as long as it is compliant with the base schema).

Rather than storing information in the database at deployment, it may be better to consider querying the token functions on a regular basis (less than daily after the first time?), so you can provide some kind of caching, but can also guarantee it won't get stale.

@zone117x
Copy link
Member

where the meta data url can be changed at will, and of course, the meta data file itself can be changed even more often

This was discussed in the original RFC that requested this API feature. For now we are considering the values as static. If there's a strong desire to have dynamic metadata then I'd suggest an amendment to SIP-10 and SIP-09 to specify a contract print event indicating when metadata has been changed.

@psq
Copy link
Contributor

psq commented May 12, 2021

I just checked the tokens from Arkadiko, and they are also using a variable for the token uri, so this seems pretty universal amongst the major projects working on fungible tokens, at least so far, that they assume their metadata is dynamic. Seems correct to me.

@zone117x
Copy link
Member

I'd still suggest a new SIP/amendment that specifies contract events should be emitted when metadata is updated. The API could deal with TTLs / cache-invalidation for this potentially in the future, but I would not expect that to be an initial feature.

@aulneau
Copy link
Contributor

aulneau commented May 12, 2021

@zone117x question for you, is it possible scope this PR such that in addition to fetching the token_uri data, it would be able to expose the decimals and symbol that are defined in the traits? We need this for the wallets, and apps like swapr would depend on this, too.

@psq
Copy link
Contributor

psq commented May 13, 2021

currently, swapr is using call-read on each token to gather each data point, which is reasonably fast for a handful of tokens, but won't scale very well, so having one endpoint to gather all the data for a token (name, symbol, decimals, uri, metadata pointed to by uri is not none). For the LP tokens, I need to gather more data, but that's too specialized for a generic api).

src/datastore/memory-store.ts Outdated Show resolved Hide resolved
src/datastore/common.ts Outdated Show resolved Hide resolved
docs/api/tokens/fungible-token.schema.json Outdated Show resolved Hide resolved
docs/api/tokens/non-fungible-token.schema.json Outdated Show resolved Hide resolved
@andresgalante
Copy link
Member

@asimm241 I need some help to deblur the next steps on this PR:

  1. We need an answer to @aulneau's question here and
  2. You mentioned that you think this can be done before the end of the month, what the level of certainty?

@zone117x If this requires an amendment to SIP-10 and SIP-09, should we involved the authors? Is it a blocker to move on with this PR?

@zone117x
Copy link
Member

is it possible scope this PR such that in addition to fetching the token_uri data, it would be able to expose the decimals and symbol that are defined in the traits?

Yes. When SIP-09/SIP-10 contracts are deployed, the API should retrieve and store these values.

If this requires an amendment to SIP-10 and SIP-09, should we involved the authors? Is it a blocker to move on with this PR?

Supporting dynamic metadata changes is not a blocker for this PR, it's a feature request. In order for the API to support that feature, it involves SIP amendments.

@psq
Copy link
Contributor

psq commented May 14, 2021

Supporting dynamic metadata changes is not a blocker for this PR,

sure

it's a feature request.

aren't you creating more work if we know this is not going work for most tokens, as opposed to try to make something that works out of the gate?

In order for the API to support that feature, it involves SIP amendments.

can't seem be convinced this is the solution. No event will be generated if the metadata json file is updated, and that metadata file was purposefully created as something outside the contract so it could be modified easily, while the contract can not. So in order to not forever present stale data, the api node would need to query that file on a regular basis anyway (weekly or less is probably fine, it is probably not going to change that often...), and if you need to do this, you might as well get the up to date uri value before checking the file content for changes.

@zone117x
Copy link
Member

aren't you creating more work if we know this is not going work for most tokens, as opposed to try to make something that works out of the gate?

I'm not sure if that's true or meaningful with the current sample size of tokens.

can't seem be convinced this is the solution. No event will be generated if the metadata json file is updated, and that metadata file was purposefully created as something outside the contract so it could be modified easily, while the contract can not. So in order to not forever present stale data, the api node would need to query that file on a regular basis anyway (weekly or less is probably fine, it is probably not going to change that often...), and if you need to do this, you might as well get the up to date uri value before checking the file content for changes.

A contract-call can be made to signal that metadata should be updated. If you're aiming to have remote JSON file rapidly changing such that an "expiry" event via contract-call is too much too ask for, then setting an arbitrary TTL of a week or a month or whatever won't solve that problem anyway.

The clear solution here is for the API to be notified when token metadata is updated, for example via contract events. You're asking the API to implement a kind of statefulness that does not yet exist in the codebase. Different API instances can easily end up with different views/responses for the same requested resource. Additionally, attempting to cache with absolutely zero indication of a TTL will result in either scalability/wastefulness problems on one end, or long-expired data on the other end. Trying to solve that by picking an interval "in the middle" like a week doesn't solve that problem, rather it just introduces a bit of both problems.

That said, I'm not absolutely against implementing a generic cache refreshing mechanism like you're suggesting, but it is a kludge and should not be treated as the solution for dynamically updating metadata.

@psq
Copy link
Contributor

psq commented May 14, 2021

Thank you for the dialogue, @zone117x, it helps better understand where you are coming from and what your concerns are. You make valid points.

Getting a bit late for me here, so I'll sleep on it and keep on the conversation tomorrow.

@asimm241 asimm241 force-pushed the feat/tokens-metadata branch 2 times, most recently from 74144ab to 2574a84 Compare May 17, 2021 09:55
@vercel
Copy link

vercel bot commented May 27, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/blockstack/stacks-blockchain-api/FFzWCh6d97ptqwJ9saUrBUeDaX5c
✅ Preview: https://stacks-blockchain-api-git-feat-tokens-metadata-blockstack.vercel.app

@zone117x zone117x merged commit 33f11bb into develop Sep 1, 2021
@zone117x zone117x deleted the feat/tokens-metadata branch September 1, 2021 23:14
@markmhendrickson
Copy link

markmhendrickson commented Sep 7, 2021

@zone117x Does this fully support NFT metadata in addition to FT metadata?

@andresgalante
Copy link
Member

@markmhx it doesn't support NFTs

@markmhendrickson
Copy link

markmhendrickson commented Sep 7, 2021

What's with these commits then? 🤔
Screen Shot 2021-09-07 at 14 04 39

@andresgalante
Copy link
Member

🤔 good catch

@zone117x
Copy link
Member

zone117x commented Sep 7, 2021

@zone117x Does this fully support NFT metadata in addition to FT metadata?

No. NFT class metadata has some limited support, but will likely not be enabled in production deployment because it's not standardized. Individual NFT metadata (i.e. CryptoKitty token num. 1395 metadata vs CryptoKitty metadata) is not supported at all.

@blockstack-devops
Copy link

🎉 This PR is included in version 0.65.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markmhendrickson
Copy link

@zone117x mind creating an issue to capture the work that remains to flesh out NFT support? Is this next on your or @asimm241's docket, or not prioritized quite yet?

@zone117x
Copy link
Member

zone117x commented Sep 7, 2021

@markmhx see new issue #742 for completing NFT class metadata support -- however, the last I heard was that NFT class support wasn't very important, and that individual NFT token metadata support is what we want, which requires some discussion. I'm setting up a meeting with a few of the stakeholders.

In the meantime, it would be helpful if UX could start a doc or gh issue/discussion to capture initial requirements?

@markmhendrickson
Copy link

markmhendrickson commented Sep 7, 2021

Sounds good. On the UX side, we're looking to display individual names, individual images and class names for particular NFTs as seen here: leather-io/extension#1028 (comment)

I could see us dropping the need for class metadata entirely if we can move forward with just token metadata to start.

Please do indicate whether @jasperjansz needs to update these designs given limitations to what data can be made available.

@unclemantis
Copy link

unclemantis commented Dec 2, 2021

I have in .env the following:

STACKS_API_ENABLE_NFT_METADATA=1

When I connect to:

http://localhost:3999/extended/v1/tokens/nft/metadata

I get the following error:

"error": "NFT metadata processing is not enabled on this server"

What gives? Little help here. Thanks guys / gals!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to fetch all asset types in the network