Skip to content

fix: request type in getNftTransfers and missing await in send #49

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.

Sign up for GitHub

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

Open
mdqst wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: request type in getNftTransfers and missing await in send #49

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export class AnkrProvider {
* @param params A GetNftTransfersRequest object.
* @returns Promise<GetNftTransfersReply>
*/
async getNftTransfers(params: GetTransfersRequest): Promise<GetNftTransfersReply> {
async getNftTransfers(params: GetNftTransfersRequest): Promise<GetNftTransfersReply> {
Copy link
Contributor

@memekas memekas Apr 10, 2025

Choose a reason for hiding this comment

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

GetTransfersRequest is used for the getTokenTransfers and for the getNftTransfers. Therefore, the interface has a name without specifics. Let's change the @params description

Copy link
Collaborator

@jovfer jovfer Apr 10, 2025

Choose a reason for hiding this comment

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

@memekas wdyt on GetTransfersRequest as main type + 2 type aliases like GetTokenTransfersRequest and GetNftTransfersRequest ?
Like

export interface GetTokenTransfersRequest extends GetTransfersRequest {}
export interface GetNftTransfersRequest extends GetTransfersRequest {}

return this.send<GetNftTransfersReply>("ankr_getNftTransfers", params)
}

Expand Down Expand Up @@ -253,7 +253,7 @@ export class AnkrProvider {
private async send<TReply>(method: string, params: any): Promise<TReply> {
const request = {method, params, id: (this._nextId++), jsonrpc: "2.0"};
const response = await axios.post<JsonRPCPayload>(this.url, JSON.stringify(request), this.requestConfig);
return AnkrProvider.getResult(response.data) as TReply
return await AnkrProvider.getResult(response.data) as TReply;
Copy link
Collaborator

@jovfer jovfer Mar 31, 2025

Choose a reason for hiding this comment

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

Hi @mdqst, thank you for the contribution! Could you please elaborate a bit on this line? To the best of my understanding, getResult is a synchronous one, so no need in the await.

PS: I'm not a js/ts expert, so I can be wrong here.

Copy link
Author

@mdqst mdqst Mar 31, 2025

Choose a reason for hiding this comment

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

Hi @mdqst, thank you for the contribution! Could you please elaborate a bit on this line? To the best of my understanding, getResult is a synchronous one, so no need in the await.

PS: I'm not a js/ts expert, so I can be wrong here.

Thanks for feedback, jovfer! You’re right to double-check this. The reason for using await here is to ensure consistency in handling Promise-based results. While getResult is currently synchronous, if there's any possibility that it might be refactored to return a Promise in the future (e.g., for async processing, network calls, or other side effects), using await proactively prevents potential issues.

Copy link
Collaborator

@jovfer jovfer Apr 10, 2025

Choose a reason for hiding this comment

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

Hey @mdqst, thanks for the explanation.

using await proactively prevents potential issues.

Sorry, I'd disagree here.

  • it's synchronous now, so the await on it would be just misleading, and it decreases readability
  • it's for local parsing of already received JSON, so the possibility of refactoring it to something async is close to zero.

So I'd request to remove it from the PR. Please also see the comment from @memekas regarding types.

}

private static getResult(payload: JsonRPCPayload): any {
Expand All @@ -265,4 +265,4 @@ export class AnkrProvider {
}
return payload.result;
}
}
}