Conversation
src/services/api.ts
Outdated
| options?: RequestOptions | ||
| ): Promise<TransactionAnalyticsResponse> => { | ||
| ): Promise<PaginatedResponse<StatusResponse[]>> => { | ||
| if (!wallet) { |
There was a problem hiding this comment.
@tomiiide I know that's not part of the PR, but is this check actually correct? Wallet is optional. Recent transactions list call (visible on the main explorer page) pulls the recent transactions, while wallet specific ones are visible after we search by wallet.
ie. If we don't provide the wallet, it's still fine, and even necessary for the explorer homepage.
There was a problem hiding this comment.
Hmm, yeah, this would break when we are just fetching all recent transactions.
@chybisov it makes sense to remove this check then no?
There was a problem hiding this comment.
We can remove that check if that's okay for the API. Please check within the widget as well, maybe we should move it there to avoid making requests when wallet is not connected.
There was a problem hiding this comment.
@chybisov Yeah, we do a check in the query function on the widget.
https://github.com/lifinance/widget/blob/main/packages/widget/src/hooks/useTransactionHistory.ts#L20
This PR:
Why was it implemented this way
config.apiUrltype is updated tostring | { v1: string, v2 : string}, this way is it easy to extend. For example if we have a v3 API url, we just add it to the config.getApiUrl(version)getter method is added to the config for safe and dynamic fetching of theapiUrl. It returns the url for different API versions. For backward compatibility, when theconfig.apiUrlis non versioned, and set to a string, it is returned by default.