-
Notifications
You must be signed in to change notification settings - Fork 95
feat: add api v2 support #287
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
base: main
Are you sure you want to change the base?
Conversation
src/services/api.ts
Outdated
options?: RequestOptions | ||
): Promise<TransactionAnalyticsResponse> => { | ||
): Promise<PaginatedResponse<StatusResponse[]>> => { | ||
if (!wallet) { |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.apiUrl
type 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.apiUrl
is non versioned, and set to a string, it is returned by default.