-
Notifications
You must be signed in to change notification settings - Fork 512
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 nfts_by_issuer data type #2694
Conversation
export interface NFTsByIssuerRequest | ||
extends BaseRequest, | ||
LookupByLedgerRequest { | ||
method: 'nfts_by_issuer' |
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.
Should be command
instead of method
since JS library deals with websocket client
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.
I followed https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/clio-methods/nfts_by_issuer/ , it's weird that only this page the field is "method" instead of "command". I can change it to "command" for sure.
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.
command
is used when the connection is websocket, eg. your JS script opens a websocket connection. Whereas method
field is used when the connection is http (eg. curl command)
For example, you could use this in the cmdline to query against ur local clio server,
curl 127.0.0.1:51233 -d '{"method":"nfts_by_issuer", "params": [{"issuer": "rfXeQv31yWMrhhPxMHZRzQqhw5mQrcuici"}]}'
* | ||
* @category Requests | ||
*/ | ||
export interface NFTsByIssuerRequest |
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.
This interface should be imported into the index file of methods, then client to be available for public use and matched with the response. You could follow the same steps of other methods (i.e AccountChannels
)
Could you add a test file for this new method? Cheers :) |
I'm not sure if this could be tested. I think the lib currently doesn't support integration test for Clio? |
@pdp2121 the library doesn't support test for clio, which is the reason why nft_info and nft_history don't have test cases. |
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.
Looks good other than the unnecessary change in HISTORY.md :)
Good job
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.
LGTM. Good job!
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.
looks good!
High Level Overview of Change
Title
Context of Change
Following https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/clio-methods/nfts_by_issuer/ & https://ripplelabs.atlassian.net/jira/software/projects/DEFI/boards/760?assignee=712020%3Aff4deb34-7fc1-428e-a2ee-354e1ddba309&selectedIssue=DEFI-127
Type of Change
Did you update HISTORY.md?
Test Plan