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

NFT APIs #514

Merged
merged 10 commits into from
Oct 11, 2023
Merged

NFT APIs #514

merged 10 commits into from
Oct 11, 2023

Conversation

pro-wh
Copy link
Collaborator

@pro-wh pro-wh commented Sep 6, 2023

this first part has a way to list NFTs by contract address

@pro-wh pro-wh added the api-layer API layer-related issues. label Sep 6, 2023
@mitjat
Copy link
Contributor

mitjat commented Sep 6, 2023

When this gets closer to stable, let's loop in the FE folks, and possibly Matevz as effectively our developer advocate.

@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch 2 times, most recently from 875ad7f to dd4a8ce Compare September 13, 2023 23:53
@pro-wh pro-wh force-pushed the pro-wh/feature/holders12 branch from a51ab0d to 4f758ee Compare September 13, 2023 23:53
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from dd4a8ce to 3f942e0 Compare September 14, 2023 23:55
@pro-wh pro-wh force-pushed the pro-wh/feature/holders12 branch 2 times, most recently from 05d394c to 702e128 Compare September 16, 2023 00:17
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 3f942e0 to 124515c Compare September 16, 2023 00:17
@pro-wh pro-wh marked this pull request as ready for review September 18, 2023 21:09
@pro-wh pro-wh force-pushed the pro-wh/feature/holders12 branch from 702e128 to e3f91ab Compare September 22, 2023 23:04
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch 2 times, most recently from 125c921 to 5f23c39 Compare September 25, 2023 23:46
@pro-wh pro-wh force-pushed the pro-wh/feature/holders12 branch from e3f91ab to 29d0b96 Compare September 25, 2023 23:46
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 5f23c39 to 899298f Compare September 26, 2023 22:45
@pro-wh pro-wh force-pushed the pro-wh/feature/holders12 branch from 29d0b96 to 6d92ff0 Compare September 26, 2023 23:01
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 899298f to 58fe181 Compare September 26, 2023 23:01
Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

👍
Just trying to figure out how the relationship between individual NFTs and ERC721 contracts should be reflected in our API.

api/spec/v1.yaml Show resolved Hide resolved
api/spec/v1.yaml Outdated Show resolved Hide resolved
@csillag
Copy link
Contributor

csillag commented Sep 27, 2023

I am getting SWAGGER OpenAPI validation errors.

🍻 Start orval v6.17.0 - A swagger client generator for typescript
⚠ SyntaxError: Swagger schema validation failed.
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema/allOf/1/properties/evm_nfts/items/required must NOT have fewer than 1 items
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema/allOf/1/properties/evm_nfts/items must have required property '$ref'
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema/allOf/1/properties/evm_nfts/items must match exactly one schema in oneOf
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema/allOf/1/properties/evm_nfts must have required property '$ref'
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema/allOf/1/properties/evm_nfts must match exactly one schema in oneOf
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema/allOf/1 must have required property '$ref'
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema/allOf/1 must match exactly one schema in oneOf
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema must have required property '$ref'
#/paths/1{runtime}1evm_nfts1{address}/get/responses/200/content/application1json/schema must match exactly one schema in oneOf
#/paths/~1{runtime}1evm_nfts1{address}/get/responses/200 must have required property '$ref'
#/paths/~1{runtime}1evm_nfts1{address}/get/responses/200 must match exactly one schema in oneOf
#/components/schemas/EvmNftList/allOf/1/properties/evm_nfts/items/required must NOT have fewer than 1 items
#/components/schemas/EvmNftList/allOf/1/properties/evm_nfts/items must have required property '$ref'
#/components/schemas/EvmNftList/allOf/1/properties/evm_nfts/items must match exactly one schema in oneOf
#/components/schemas/EvmNftList/allOf/1/properties/evm_nfts must have required property '$ref'
#/components/schemas/EvmNftList/allOf/1/properties/evm_nfts must match exactly one schema in oneOf
#/components/schemas/EvmNftList/allOf/1 must have required property '$ref'
#/components/schemas/EvmNftList/allOf/1 must match exactly one schema in oneOf
#/components/schemas/EvmNftList must have required property '$ref'
#/components/schemas/EvmNftList must match exactly one schema in oneOf
#/components/schemas/EvmNft/required must NOT have fewer than 1 items
#/components/schemas/EvmNft must have required property '$ref'
#/components/schemas/EvmNft must match exactly one schema in oneOf

These all seem to be related to the new additions; the version on the master branch passes without any issues.

@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 58fe181 to 3f13626 Compare September 27, 2023 23:25
@pro-wh pro-wh force-pushed the pro-wh/feature/holders12 branch 2 times, most recently from 16c110a to dfb27d7 Compare September 27, 2023 23:26
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 3f13626 to 47de98b Compare September 27, 2023 23:26
@pro-wh
Copy link
Collaborator Author

pro-wh commented Sep 27, 2023

yeah these look like they're related to the additions. I'll check on these tomorrow

@csillag
Copy link
Contributor

csillag commented Sep 28, 2023

yeah these look like they're related to the additions. I'll check on these tomorrow

If there will be an update, it would be very nice to sneak it the owner field, too.

@pro-wh
Copy link
Collaborator Author

pro-wh commented Sep 28, 2023

gonna add the owner field in #529

@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 0e11a4c to d71a277 Compare October 6, 2023 19:27
@pro-wh
Copy link
Collaborator Author

pro-wh commented Oct 6, 2023

"token" info moved to subobject, matching evm tokens json schema. however, note that that api was designed earlier, where more fields are required. those fields will have the Go default value when the data is not available. e.g. the zero ethereum address if we somehow don't know the preimage or token type unknown instead of null if we haven't checked the contract yet

@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from d71a277 to 359b90f Compare October 6, 2023 21:05
@pro-wh
Copy link
Collaborator Author

pro-wh commented Oct 6, 2023

moved to /{runtime}/evm_tokens/{address}/nfts

@pro-wh pro-wh requested review from mitjat and csillag October 6, 2023 21:07
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 359b90f to 501af66 Compare October 6, 2023 22:34
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 501af66 to 2bc527f Compare October 6, 2023 22:42
@pro-wh pro-wh mentioned this pull request Oct 6, 2023
Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

Thank you for the additional changes!

A reply to your comment:

"token" info moved to subobject, matching evm tokens json schema.
note that that api was designed earlier, where more fields are required.

Now is the "best" time to change that in a backwards-incompatible way -- best compared to any future time. I'm sure @csillag can accommodate optionality on the FE, and we don't have other clients for now.
OTOH if the only two potentially-defaulted fields are eth_address and token_type, I wouldn't worry. token_type has to be 721 for us to even list the NFT, presumably? And not knowing the preimage would pretty much imply a bug in nexus, because all events via which we observe NFTs are encoded with eth addresses, so we observe preimages all the time. So I'm not worried about that corner case either.

storage/client/queries/queries.go Show resolved Hide resolved
api/spec/v1.yaml Show resolved Hide resolved
api/spec/v1.yaml Show resolved Hide resolved
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch 2 times, most recently from ffa167b to 6f6547e Compare October 10, 2023 22:52
@pro-wh
Copy link
Collaborator Author

pro-wh commented Oct 10, 2023

metadata_accessed lmao

@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch 2 times, most recently from 22a1a2a to 8ef9a7e Compare October 11, 2023 21:02
@pro-wh pro-wh force-pushed the pro-wh/feature/holders13 branch from 4ddab26 to c8e416e Compare October 11, 2023 21:46
@pro-wh
Copy link
Collaborator Author

pro-wh commented Oct 11, 2023

changes worth looking at:

  1. new e2e regression test hits network resources. time bomb warning
  2. db fixes, let me know if it's right


-- Grant others read-only use.
GRANT SELECT ON ALL TABLES IN SCHEMA chain TO PUBLIC;
GRANT EXECUTE ON ALL FUNCTIONS IN SCHEMA chain TO PUBLIC;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: If 21_evm_nfts.up.sql hasn't been applied yet to prod, I'd also be fine with merging this with that migration. This way is also completely fine too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it was in 0.1.16 😬

@pro-wh pro-wh merged commit 59e9f1f into main Oct 11, 2023
@pro-wh pro-wh deleted the pro-wh/feature/holders13 branch October 11, 2023 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-layer API layer-related issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants