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

Add getNftSales() #190

Merged
merged 13 commits into from
Nov 16, 2022
Merged

Conversation

xeno097
Copy link
Contributor

@xeno097 xeno097 commented Nov 12, 2022

Changelog

  • Adds the getNftSales method in the nft namespace.
  • Adds the GetNftSalesResponse and NftSale types to describe the result of the getNftSales endpoint.
  • Adds unit and integration tests for the getNftSales method.

Extra

Filtering by the taker field doesn't seem to work correctly, when setting the param to either buyer or seller the results from the API don't change and results with the incorrect taker value are returned.
Filtering by taker=seller
image

Filtering by taker=buyer
image

Also, the docs mention that the fromBlock and toBlock params should accept hex integers as valid values but when setting them as such, the API returns an error so the type in the SDK has been implemented to allow only the types that are working correctly.
image

@thebrianchen thebrianchen changed the title Nft api get nft sales Add getNftSales() Nov 15, 2022
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 so much for taking this one on! This was definitely pretty hefty, so we truly appreciate your time and effort!

* @param options - The optional parameters to use for the request.
* @beta
*/
getNftSales(options?: GetNftSales): Promise<GetNftSalesResponse>;
Copy link
Member

Choose a reason for hiding this comment

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

Rename GetNftSales to GetNftSalesOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it!

src/api/nft-namespace.ts Show resolved Hide resolved
src/types/types.ts Outdated Show resolved Hide resolved
*
* @public
*/
export enum SortingOrder {
Copy link
Member

Choose a reason for hiding this comment

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

Can we reuse the AssetTransfersOrder and just re-export it?
ex: export { AssetTransfersOrder as SortingOrder }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I think it would be a good idea to refactor it later to have one enum representing the supported sorting orders by the API. I didn't do it in this pr to avoid a breaking change with the AssetTransfersOrder

src/types/types.ts Outdated Show resolved Hide resolved
src/types/types.ts Outdated Show resolved Hide resolved
mock.onGet().reply(400, 'fromBlock should be a non-negative integer');

await expect(
alchemy.nft.summarizeNftAttributes(contractAddress)
Copy link
Member

Choose a reason for hiding this comment

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

typo: replace with getNftSales()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that I have been swayed by the dark powers of the copy paste 🤣

return NftTakerType.BUYER;
case 'SELLER':
return NftTakerType.SELLER;
default:
Copy link
Member

Choose a reason for hiding this comment

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

Remove the UNKNOWN enum since the taker is always guaranteed to be one or the other. Can you throw an error if for some reason we can't parse the enum?

src/types/types.ts Outdated Show resolved Hide resolved
src/types/types.ts Outdated Show resolved Hide resolved
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 the speedy turnaround 👊

@thebrianchen thebrianchen merged commit 05f49ea into alchemyplatform:main Nov 16, 2022
@xeno097 xeno097 deleted the nft-api-get-nft-sales branch November 16, 2022 13:22
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