refactor: api clients and add stx-20 balances #5315
Conversation
| @@ -19,6 +16,7 @@ function trackApiError(url: string, statusCode: number) { | |||
| * @deprecated Use `axios` directly instead. Fetch only needed for interation | |||
| * with generated stacks blockchain api library | |||
| */ | |||
| // TEMPORARY RELOCATION | |||
| export async function wrappedFetch(input: RequestInfo, init: RequestInit = {}) { | |||
There was a problem hiding this comment.
This will be removed if we refactor them all.
|
|
||
| import type { DefaultNetworkConfigurations } from '@shared/constants'; | ||
|
|
||
| // import type { AddressBalanceResponse } from '@shared/models/account.model'; |
There was a problem hiding this comment.
I removed this and am using the one imported from the api-types bc the need for this might be outdated? I'm not seeing why we would need to keep this?
| export function stx20ApiClient() { | ||
| return axios.create({ | ||
| baseURL: 'https://api.stx20.com/api/v1', | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think we want to call this once, and export, vs every time.
E.g, if we want to modify all requests for stx20.com, we'd use stx20Client.interceptors, which afaik depends on a single instance
There was a problem hiding this comment.
mm, but yeah we can't do this for custom networks
There was a problem hiding this comment.
so now I'm questioning if it is a good idea to use all the separate axios clients / vs a single client where we don't set baseURL. Thoughts?
There was a problem hiding this comment.
Thinking on the refactor here, I might close this and separate out the STX-20 balance work just to get that merged first. I'm not sure we absolutely need to refactor the api clients rn, but I'd like to think on how to set this all up a bit better.
There was a problem hiding this comment.
You are right, this should only be called once with the config nec to handle that service.
https://blog.logrocket.com/understanding-axios-create/
| @@ -0,0 +1,16 @@ | |||
| import type { AddressBalanceResponse } from '@stacks/stacks-blockchain-api-types'; | |||
There was a problem hiding this comment.
Can you check this lib is up to date?
| export function hiroApiClient(network: DefaultNetworkConfigurations) { | ||
| return axios.create({ | ||
| baseURL: defaultNetworksKeyedById[network].chain.stacks.url, | ||
| headers: { ...leatherHeaders }, | ||
| }); |
There was a problem hiding this comment.
Mmm, actually (contrary to my earlier recommendation), this wouldn't work with custom networks, should just pass the URL
There was a problem hiding this comment.
I see this now, thx.
| export interface Stx20Token extends Omit<Stx20Balance, 'balance'> { | ||
| balance: Money; | ||
| marketData: null; | ||
| } |
There was a problem hiding this comment.
Per comment in other PR, do wonder if these should be paired ahead of time at model-level?
| import { Stx20TokenAssetItemLayout } from './stx20-token-asset-item.layout'; | ||
|
|
||
| interface Stx20TokenAssetListProps { | ||
| stx20Tokens: Stx20Token[]; |
There was a problem hiding this comment.
nit, for me it's clear enough from component name this is for stx20, so just "tokens" is good as a prop name?
@kyranjamie hoping to get your feedback here on refactoring our api clients via axios and abandoning the
StacksClientandBitcoinClientpattern. I used implementing STX-20 balances, and refactored one Stacks balance query, to show how we could do this with all our current queries and setup a better pattern to follow. Open to feedback, but I don't think it would be too hard to refactor all of them here.