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 api search contract metadata #163

Merged

Conversation

xeno097
Copy link
Contributor

@xeno097 xeno097 commented Oct 28, 2022

Changelog

  • Adds the searchContractMetadata method in the nft namespace.
  • Refactors unit tests for the getNftContractMetadata method.
  • Moves the verifyNftContractMetadata util function to the test/test-util.ts file to reuse it for the searchContractMetadata unit tests.
  • Adds unit and integration tests for searchContractMetadata method.

@thebrianchen thebrianchen self-requested a review October 28, 2022 15:19
Copy link
Member

@thebrianchen thebrianchen left a comment

Choose a reason for hiding this comment

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

thanks for submitting this! just a small change and we should be good to go!

export function verifyNftContractMetadata(
actualNftContract: NftContract,
expectedNftContract: NftContract,
address: string,
Copy link
Member

Choose a reason for hiding this comment

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

why do we need all the additional variables in the params here?

i'm thinking that we would just do
expect(actualNftContract.address).toEqual(expectedNftContract.address);

Copy link
Contributor Author

@xeno097 xeno097 Nov 2, 2022

Choose a reason for hiding this comment

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

Playing with the API I saw that every element in the searchContractMetadata endpoint response has the same structure as the response from the getNftContractMetadata endpoint which was unit tested using the following function

image
image

So to keep the testing method consistent I decided to reuse the verifyNftContractMetadata function for the searchContractMetadata tests too. I can change it to test only the contract address if you want.

Copy link
Member

Choose a reason for hiding this comment

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

ah I see what you mean, thanks for clarifying!

please leave this as is, i'll refactor this later!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, nice! Thank you!

Copy link
Member

@thebrianchen thebrianchen left a comment

Choose a reason for hiding this comment

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

thanks again!

@thebrianchen thebrianchen merged commit 670d503 into alchemyplatform:main Nov 2, 2022
@xeno097 xeno097 deleted the nft-api-search-contract-metadata branch November 2, 2022 23:33
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.

2 participants