-
Notifications
You must be signed in to change notification settings - Fork 167
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 Nft Api Compute Rarity #148
Add Nft Api Compute Rarity #148
Conversation
…/nft-api-compute-rarity
…lchemy-sdk-js into xeno097/nft-api-compute-rarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on -- we really appreciate it! Also thanks for catching some typos :)
Left a few nits and comments, but this is really good 🔥
src/types/types.ts
Outdated
value: string; | ||
|
||
/** The type of NFT attribute. */ | ||
trait_type: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to traitType
in order to be consistent with JS style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it 👌🏼
src/types/types.ts
Outdated
/** The type of NFT attribute. */ | ||
trait_type: string; | ||
|
||
/** The NFT's attribute frequency in the current collection. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update comment to:
A number from 0 to 1 representing the prevalence of this value for this trait type in the current collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okis, much better
const response = await alchemy.nft.computeRarity(contractAddress, tokenId); | ||
|
||
expect(response).toBeDefined(); | ||
expect(response.length).toBeGreaterThan(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the test to check that each field in the response type is present in the first element of the array.
There was a problem hiding this 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 suggestion!
test/unit/nft-api.test.ts
Outdated
'contractAddress', | ||
contractAddress | ||
); | ||
expect(mock.history.get[0].params).toHaveProperty('tokenId', tokenId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you update trait_type
to traitType
, can you also add a test to check that the method converts the field properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no problem.
…mputeRarity method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clean PR and fast turnaround times! Left one last request, but will merge this in after that 🚀
test/unit/nft-api.test.ts
Outdated
@@ -1204,6 +1209,54 @@ describe('NFT module', () => { | |||
}); | |||
}); | |||
|
|||
describe('computeRarity()', () => { | |||
const contractAddress = '0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d'; | |||
const tokenId = '7495'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more request: can you use a numerical value for tokenId
here to check that the logic to convert Bignumberish
input to a string is properly executed? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready 👌🏼
🔥 |
…g call works correctly in computeRarity tests
Changelog
computeRarity
method in thenft
namespace.NftAttributeRarity
type to represent the result of thecomputeRarity
endpoint.OwnedBaseNftsResponse
type doc.computeRarity
method.Extra
I was considering wrapping the
computeRarity
endpoint result in an object with the following structure{attributes: NftAttributeRarity[] }
and exposing that in the public API but then decided to keep the same format as the endpoint response. Also, I'll gladly accept suggestions for theNftAttributeRarity
type name if you have a better name for this use case.The other endpoints listed in #140 will be added in another pr.