-
Notifications
You must be signed in to change notification settings - Fork 54
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
refactor(network): retrieves api versions and urls based on network store #362
Conversation
e478d99
to
69b3d96
Compare
69b3d96
to
1115948
Compare
BASE_API_MAINNET_URL= | ||
BASE_API_TESTNET_URL= | ||
BASE_API_SANDBOX_URL= |
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.
in docker compose env server and browser hosts are different
*/ | ||
export const initiateNetworkData = async () => { | ||
networks = await Promise.all( | ||
export const initiateNetworkVersions = async () => { |
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.
ideally this shouldn't happen. We could have a dedicated components/components blocking app rendering during init. I understand that it happens in SettingsProvider, however it's opaque and one should really dig in to understand this.
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.
Yea we could, but the version isn't really mandatory for the app the to render. I think we only show it in the NetworkSelect
component.
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.
Well it's used in many api urls
@@ -11,4 +13,6 @@ export type Network = { | |||
rpcEndpoint?: string; | |||
version: string | null; | |||
enabled: boolean; | |||
apiVersion: ApiVersion; | |||
marketApiVersion: ApiVersion; |
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.
moved these here as it's really a network config, and we have selected network definition logic based on configs already.
static sandboxVersion() { | ||
return `${browserEnvConfig.NEXT_PUBLIC_API_BASE_URL}/v1/version/sandbox`; | ||
|
||
static get baseApiUrl() { |
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.
fixes a bug introduced by me earlier - api url should vary based on the network for the above endpoints
*/ | ||
export const initiateNetworkData = async () => { | ||
networks = await Promise.all( | ||
export const initiateNetworkVersions = async () => { |
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.
Yea we could, but the version isn't really mandatory for the app the to render. I think we only show it in the NetworkSelect
component.
Network service was bothering me a bit and I went for refactoring. The root of it is attempt to depend on a single source of truth which is a network store that already holds a selected network with all its configurations.