fix: request type in getNftTransfers and missing await in send#49
fix: request type in getNftTransfers and missing await in send#49mdqst wants to merge 1 commit intoAnkr-network:mainfrom
getNftTransfers and missing await in send#49Conversation
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hey @mdqst, thanks for the explanation.
using await proactively prevents potential issues.
Sorry, I'd disagree here.
- it's synchronous now, so the
awaiton 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.
| * @returns Promise<GetNftTransfersReply> | ||
| */ | ||
| async getNftTransfers(params: GetTransfersRequest): Promise<GetNftTransfersReply> { | ||
| async getNftTransfers(params: GetNftTransfersRequest): Promise<GetNftTransfersReply> { |
There was a problem hiding this comment.
GetTransfersRequest is used for the getTokenTransfers and for the getNftTransfers. Therefore, the interface has a name without specifics. Let's change the @params description
There was a problem hiding this comment.
@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 {}
| 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; |
There was a problem hiding this comment.
Hey @mdqst, thanks for the explanation.
using await proactively prevents potential issues.
Sorry, I'd disagree here.
- it's synchronous now, so the
awaiton 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.
3 participants
getNftTransfersnow correctly expectsGetNftTransfersRequestinstead ofGetTransfersRequest.awaitinsend<TReply>to ensure proper error handling.