feat: metadata api #44

justgigio · 2021-07-14T20:04:16Z

We'll keep current token meta endpoint for retro compatibility and remove it once the explorer is reaching the new endpoint.

Before Merge

Create a PR removing the old token meta endpoint and things related to them
Create new bucket and folders
Copy meta jsons to new bucket

After Merge

Delete old bucket

msbrogli · 2021-07-19T18:41:14Z

domain/metadata/token_metadata.py

+    :type nft: :py:class:`domain.metadata.token_metdata.TokenNFT`
+    """
+    id: str
+    verified: Optional[bool] = False


Why Optional? What does None mean in this case? Add the description in the docstring, please.

Just to let the json have only the fields that matter. Docstring updated.

msbrogli · 2021-07-19T18:41:25Z

domain/metadata/token_metadata.py

+    """
+    id: str
+    verified: Optional[bool] = False
+    banned: Optional[bool] = False


Why Optional? What does None mean in this case? Add the description in the docstring, please.

Just to let the json have only the fields that matter. Docstring updated.

msbrogli · 2021-07-19T18:41:35Z

domain/metadata/token_metadata.py

+    id: str
+    verified: Optional[bool] = False
+    banned: Optional[bool] = False
+    reason: Optional[str] = ''


Why Optional? What does None mean in this case? Add the description in the docstring, please.

This property only make sense for banned tokens

pedroferreira1 · 2021-07-20T15:19:35Z

.github/workflows/deploy.yml

@@ -80,7 +80,7 @@ jobs:
          REDIS_HOST: ${{ secrets.REDIS_HOST }}
          REDIS_PORT: 6379
          REDIS_DB: 0
-          TOKEN_METADATA_BUCKET: token-metadata-testnet
+          METADATA_BUCKET: metadata-testnet


You should add this in the TODO as well.

Create new buckets, migrate token info from old bucket to the new one and delete old bucket when everything is working fine.

It's still missing some

I think it covers all, but i'll be more specific

usecases/get_metadata.py

gateways/metadata_gateway.py

pedroferreira1 · 2021-07-20T16:51:44Z

domain/metadata/transaction_metadata.py

+
+
+@dataclass
+class MetaTransaction:


If we add any new property in transaction metadata we need to update the explorer service? Imagine we want to mark a tx as banned or as important (don't know why but just imagining possible situations). How do we handle this?

Yes, and i think that is the right thing to do. Otherwise we could broke things in explorer or have other unpredictable problems.

msbrogli · 2021-07-20T19:39:53Z

usecases/get_token_metadata.py



 class GetTokenMetadata:

-    def __init__(self, token_gateway: Union[TokenGateway, None] = None) -> None:
-        self.token_gateway = token_gateway or TokenGateway()
+    def __init__(self, metadata_gateway: Union[MetadataGateway, None] = None) -> None:


Use Optional[MetadataGateway].

msbrogli · 2021-07-20T19:41:00Z

usecases/get_token_metadata.py

@@ -1,17 +1,17 @@
 from typing import Union

-from gateways.token_gateway import TokenGateway
+from gateways.metadata_gateway import MetadataGateway


 class GetTokenMetadata:


Mark as deprecated and that it will be removed soon.

msbrogli · 2021-07-20T19:41:15Z

usecases/get_metadata.py

+
+class GetMetadata:
+
+    def __init__(self, metadata_gateway: Union[MetadataGateway, None] = None) -> None:


Use Optional[MetadataGateway].

msbrogli · 2021-07-20T19:42:17Z

usecases/get_metadata.py

+
+    def get(self, type: str, id: str) -> Union[dict, None]:
+        metadata_methods = {
+            'token': self.metadata_gateway.get_token_metadata,


At the end of the day, both are "BaseTransactions" in the DAG. Isn't it easier to use just one gateway?

The gateway is the same, only the method changes. Each method return a different type of meta.

usecases/get_metadata.py

msbrogli · 2021-07-20T20:05:56Z

gateways/token_gateway.py



 class TokenGateway:
    """Gateway for Token

-    :param s3_client: Client for s3 manipulation, default to domain S3Client
-    :type s3_client:
+    :param hathor_core_client: Client for make hathor-core requests, default to domain HathorCoreClient


HathorCoreClient?

Yes. It is the client to make requests for a full-node. get_token method uses it therefore will be changed soon.

msbrogli · 2021-07-20T20:06:46Z

gateways/metadata_gateway.py

+    def _metadata_bucket(self) -> str:
+        metadata_bucket = METADATA_BUCKET
+
+        if metadata_bucket is None:


Isn't it better to validate all configuration in the constructor? So we can assume everything is right when we get to this point?

I don't think so as we can have methods that don't need this constant.

All methods need this constant for now, right?

@pedroferreira1 yes, but i still feel it's not good to check in constructor. We don't have this var in test env, for instance and i don't think we must to. Of course for some tests we have to mock it, but some tests just don't care about it and i think it doesn't make sense to mock a var only for the class to not break.

I know it's just test but i think it's a good indication why maybe it's not so good to check this on constructor.

msbrogli · 2021-07-20T20:07:24Z

gateways/metadata_gateway.py

+
+        :param id: token id
+        :type id: str
+        :raises Exception: The name of the bucket used to store the jsons must be on config


I don't feel this error should belong to this method.

Sure. This error is thrown by _get_metadata method, i've removed from the docstring.

msbrogli · 2021-07-20T20:08:20Z

gateways/metadata_gateway.py

+
+        return TransactionMetadata.from_dict(transaction_metadata)
+
+    def get_token_metadata(self, id: str) -> Union[TokenMetadata, None]:


Use Optional[TokenMetadata].

msbrogli · 2021-07-20T20:10:16Z

domain/metadata/transaction_metadata.py

+
+@dataclass
+class TransactionMetadata(Metadata):
+    data: Optional[MetaTransaction] = None


Why are we using two classes? I feel it would be simper to use one class with all fields there. Well, we can group in subclasses for specific cases if there are too many fields.

We have only specific cases by now. We can use Union[Meta1, Meta2] in places that will handle metadata but i think that is not scalable.

domain/metadata/token_metadata.py

gateways/metadata_gateway.py

pedroferreira1 · 2021-08-02T16:28:00Z

gateways/metadata_gateway.py

+    def _metadata_bucket(self) -> str:
+        metadata_bucket = METADATA_BUCKET
+
+        if metadata_bucket is None:


All methods need this constant for now, right?

text/0003-metadata-api.md

domain/metadata/token_metadata.py

pedroferreira1 · 2021-08-10T16:28:26Z

text/0003-metadata-api.md

+    "nft_media": {
+      "type": "IMAGE",
+      "file": "http://curtis-white.com/mean/officer.png",
+      "loop": false


Is it required even for image files? Does it work only for videos or audios can play in loop as well?

It's not required in s3 json files but will be always present on responses.

I think the documentation must have the details for each field.

Added the datails on fields structure explanation

domain/metadata/token_metadata.py

pedroferreira1 · 2021-08-13T14:06:35Z

text/0003-metadata-api.md

+
+```
+TokenMeta {                             // if type is token
+  id: string,                           // hash id of token or transaction 


Suggestion: 'hash id of token or transaction ' -> 'token uid'

pedroferreira1 · 2021-08-13T14:06:57Z

text/0003-metadata-api.md

+  banned: boolean,                      // if token is banned
+  reason: string,                       // reason of ban
+  nft: boolean,                         // if token is a NFT
+  nft_media: {                          // media data of NFT


Suggestion: 'media data of NFT (optional)'

pedroferreira1 · 2021-08-13T14:07:16Z

text/0003-metadata-api.md

+}
+
+TransactionMeta {
+  id: string,                           // hash id of token or transaction 


Suggestion: 'hash id of token or transaction ' -> 'transaction hash'

r4mmer · 2021-10-12T15:47:21Z

Feature implemented on #67

justgigio self-assigned this Jul 14, 2021

justgigio linked an issue Jul 14, 2021 that may be closed by this pull request

feat: Tx/Token Metadata API #40

Open

justgigio requested review from luislhl, pedroferreira1, msbrogli, andreabadesso, jansegre, r4mmer and a team July 15, 2021 18:38

msbrogli requested changes Jul 19, 2021

View reviewed changes

pedroferreira1 reviewed Jul 20, 2021

View reviewed changes

msbrogli requested changes Jul 20, 2021

View reviewed changes

justgigio requested review from pedroferreira1 and msbrogli July 28, 2021 03:39

justgigio changed the base branch from dev to main July 28, 2021 03:43

justgigio changed the base branch from main to dev July 28, 2021 03:43

justgigio added 6 commits July 28, 2021 00:44

feat: create metadata dataclass

e025ecd

feat: metadata api gateway

94c109e

feat: add metadata handler

fa69de9

style: fix mypy and isort

73507fc

fix: metadata bucket env var name

a5f9763

docs: improve documentation and design markdown

ee657c0

justgigio force-pushed the feat/metadata-api branch from faf15fa to ee657c0 Compare July 28, 2021 03:44

pedroferreira1 requested changes Aug 2, 2021

View reviewed changes

feat: change nft field and add loop option

76b38df

pedroferreira1 requested changes Aug 10, 2021

View reviewed changes

justgigio requested a review from pedroferreira1 August 10, 2021 17:45

docs: fix typos and improve some explanations

ce8d7be

justgigio force-pushed the feat/metadata-api branch from f05986c to ce8d7be Compare August 10, 2021 17:49

feat: add autoplay attribute for nft media

8484d2a

justgigio added 2 commits August 12, 2021 06:47

fix: handler name on serverless

1ff0fac

docs: explain response fields on design

81b68b5

pedroferreira1 approved these changes Aug 13, 2021

View reviewed changes

docs: improve fields description

5906637

Base automatically changed from dev to main September 14, 2021 01:58

r4mmer closed this Oct 12, 2021

r4mmer deleted the feat/metadata-api branch December 1, 2021 15:00


		class GetMetadata:

		def __init__(self, metadata_gateway: Union[MetadataGateway, None] = None) -> None:


		return TransactionMetadata.from_dict(transaction_metadata)

		def get_token_metadata(self, id: str) -> Union[TokenMetadata, None]:

feat: metadata api #44

feat: metadata api #44

Conversation

justgigio commented Jul 14, 2021 • edited Loading

Before Merge

After Merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r4mmer commented Oct 12, 2021

justgigio commented Jul 14, 2021 •

edited

Loading