Dynamic client side envs 2 #80

JasonMHasperhoven · 2024-10-02T13:59:05Z

As per @conorsch 's request, this adds support to set envs dynamically post-build.

~~Routes need to pass envs from getServerSideProps as props~~
~~_app will take this and provide it as a context (in v2 we could use mobx or whatever state manager we decide on)~~
~~Components can consume useEnvContext to access the envs~~

Client now makes a request to /api/env to get the clientSideEnv
Refactored app to use fetchers (similar to minifront's design) -- which consume useRegistry & useEnv

JasonMHasperhoven · 2024-10-02T14:02:46Z

src/pages/_app.tsx

-      <Analytics />
-      <SpeedInsights />
-      <Component {...pageProps} />
+      <EnvContext.Provider value={pageProps.envs}>


Meant as a placeholder until we have finalized a state manager.

We we anticipate this env context vars to be critical to a number of things in client side code, we should use react-query to fetch and sync its state with a mobx EnvContextStore.

conorsch · 2024-10-02T16:29:05Z

Deploying from this tag, without updating the env vars, I see:

⨯ Error: Registry not found for 
     at Ct.get (/app/node_modules/.pnpm/@[email protected]/node_modules/@penumbra-labs/registry/dist/index.js:2:113241) 
     at h (/app/.next/server/chunks/215.js:8:6274) 
     at d (/app/.next/server/chunks/215.js:8:6293) 
     at c (/app/.next/server/chunks/215.js:8:6607)
     at b (/app/.next/server/pages/trade/[[...params]].js:1:2857)

Which indicates that we no longer have sane default values. Can we add a default value for PENUMBRA_CHAIN_ID="penumbra-1"? Or at least a more informative error message that indicates a required environment variable should have been set and wasn't. Remember that RC is running a deployment, as well, and changes like the lack of defaults will cause that instance to fail when it's upgraded, unless we preserve defaults.

After setting the env vars properly, according to the new env vars, I confirmed that the assets were loading correctly for the testnet deployment, which is magnificent.

conorsch

Works with the new env vars set! Asking for a bit more error-handling around the new vars: let's provide defaults, or at least log which required env vars weren't set if they're missing. See example error message in comments above.

grod220

There is a lot in this pull request that opens a number of questions on how we want to handle data fetching within this app:

client vs server requests
async vs sync requests
global caching & performance
react-query syncing w/ state manager
using a state manager versus react-context

Think it is worthwhile for others on the web team to take a look and we discuss in today's meeting

grod220 · 2024-10-03T10:04:13Z

src/components/pairSelector.tsx

+  const tokenAssetsList = useTokenAssets();
  const [tokenAssets, setTokenAssets] = useState<Record<string, Token>>({});
  const [selectedAssets, setSelectedAssets] = useState<Token[]>([]);

  useEffect(() => {
-    const tokenAssets = fetchAllTokenAssets();
-    setTokenAssets(Object.fromEntries(tokenAssets.map(asset => [asset.symbol, asset])));
-  }, []);
+    setTokenAssets(Object.fromEntries(tokenAssetsList.map(asset => [asset.symbol, asset])));
+  }, [tokenAssetsList]);


This should be in react-query

Correct, but let's keep that to another PR (also this is v1 code).

grod220 · 2024-10-03T10:07:43Z

src/pages/block/[block_height].tsx

+export async function getServerSideProps() {
+  const envs = getClientSideEnvs();
+
+  return {
+    props: {
+      envs,
+    },
+  };
+}


Two thoughts:

Can we put this at a kind of parent layout component so we don't have to do this for every page we create (and need to remember or else risk errors)?

What if we created an backend API route for returning this and we put it in react-query (client side) instead?

Unfortunately with this implementation it seems to be not possible

Perhaps that's a better approach

grod220 · 2024-10-03T10:09:35Z

src/utils/token/tokenFetch.tsx

+export function useTokenAssets() {
+  const envs = useEnvContext();
+  return fetchAllTokenAssets(envs['PENUMBRA_CHAIN_ID'] ?? '');
+}


As this is, I'm pretty sure that fetchAllTokenAssets() does not cache. If env context was in react-query, we could compose within react-query to handle caching globally for this work. Otherwise, every component's use of useTokenAssets() will be a new instance and re-running on every render.

grod220 · 2024-10-03T10:10:47Z

src/pages/_app.tsx

-      <Analytics />
-      <SpeedInsights />
-      <Component {...pageProps} />
+      <EnvContext.Provider value={pageProps.envs}>


We we anticipate this env context vars to be critical to a number of things in client side code, we should use react-query to fetch and sync its state with a mobx EnvContextStore.

JasonMHasperhoven · 2024-10-04T17:44:13Z

Changing the implementation to use a request to fetch the client-side env. WIP.

JasonMHasperhoven · 2024-10-07T11:33:50Z

It's ready for review again. I indeed like this implementation better @grod220 & added defaults for the client side @conorsch.

grod220 · 2024-10-07T12:39:26Z

src/fetchers/env.ts

+  return useQuery({
+    queryKey: ['clientEnv'],
+    queryFn: async (): Promise<ClientEnv> => {
+      return fetch('/api/env').then(resp => resp.json() as unknown as ClientEnv);


suggestion: In async functions, I find sticking to async await to be more readable than promise chaining:

queryFn: async (): Promise<ClientEnv> => { const res = await fetch('/api/env'); return (await res.json()) as ClientEnv; },

grod220 · 2024-10-07T12:51:59Z

src/fetchers/registry.ts

+    queryKey: ['penumbraRegistry', chainId],
+    queryFn: async (): Promise<Registry | null> => {
+      if (!chainId) {
+        return null;


issue: in general, let's try to avoid null when we can. This can go into bike shedding territory, but I'm in the camp that it is a mistake in the language.

There is another issue though, the returned loading and error states from useRegistry are not quite true. We should combine them together:

export const useRegistry = () => { const { data: env, isLoading: isEnvLoading, error: envError } = useEnv(); const { data: registry, isLoading: isRegistryLoading, error: registryError, } = useQuery({ queryKey: ['penumbraRegistry', env], queryFn: async () => { const chainId = env?.PENUMBRA_CHAIN_ID; if (!chainId) { throw new Error('chain id not available to query registry'); } return chainRegistryClient.remote.get(chainId); }, staleTime: Infinity, enabled: Boolean(env), }); return { registry, isLoading: isEnvLoading || isRegistryLoading, error: envError || registryError, }; };

grod220 · 2024-10-07T12:54:12Z

src/fetchers/tokenAssets.ts

+import { uint8ArrayToBase64, base64ToUint8Array } from '@/old/utils/math/base64';
+import { Token } from '@/old/utils/types/token';
+
+export const useTokenAssets = (): Token[] => {


issue: with useRegistry() returning data/loading/error, this hook should also return the same so that can properly be handled by consumers if desired.

grod220 · 2024-10-07T12:55:35Z

src/fetchers/tokenAssets.ts

+    }) as Token[];
+};
+
+export const useTokenAsset = (tokenId: Uint8Array | string): null | Token => {


issue: we should return some data field + loading + error states

grod220 · 2024-10-07T12:57:17Z

src/fetchers/tokenAssets.ts

+        inner: asset.penumbraAssetId?.inner && uint8ArrayToBase64(asset.penumbraAssetId.inner),
+        imagePath: imagePathFromAssetImages(asset.images),
+      };
+    }) as Token[];


issue: it is tempting to use our own types, but we should do our best to use types from BufBuild that are more universally relied upon in the penumbra ecosystem. For instance, we shouldn't have components that accept a Token, but probably ones that accept a ValueView type

grod220 · 2024-10-07T12:59:19Z

src/utils/env/getClientSideEnv.ts

+export function getClientSideEnv() {
+  const whitelist: string[] = ['PENUMBRA_CHAIN_ID', 'PENUMBRA_CUILOA_URL'];
+
+  const env = whitelist.reduce(
+    (env, key) => ({
+      ...env,
+      [key]: process.env[key] ?? '',
+    }),
+    defaults,
+  ) as ClientEnv;
+
+  return env;
+}


suggestion: Think this would work the same

export function getClientSideEnv(): ClientEnv { const whitelist: string[] = ['PENUMBRA_CHAIN_ID', 'PENUMBRA_CUILOA_URL']; return whitelist.reduce( (env, key) => ({ ...env, [key]: process.env[key] ?? '', }), defaults, ); }

grod220

Talked offline:

For hooks consumed by deprecated code, label as deprecated. Given they don't consume loading/error states, can keep simple as needed.
For hooks consumed by new code, should follow data/loading/error propagation pattern
No nulls (if possible)

conorsch

Beautiful! Thanks for your hard work on this, it's markedly more straightforward to deploy and configure now.

It's a nice touch to redirect the trading view to penumbra:gm on the testnet; however, that route should probably be configurable, too, maybe with the previous setting of penumbra:usdc retained as default behavior. Otherwise, deployments against the penumbra-1 chain will show nonsense data, because there's no gm asset. But we can circle back to that change later, I know there' a lot of work in-flight right now!

JasonMHasperhoven added 2 commits October 2, 2024 17:58

Implement dynamic client side envs

03179ea

Remove configConstants

fb644e1

JasonMHasperhoven commented Oct 2, 2024

View reviewed changes

JasonMHasperhoven changed the title ~~Implement dynamic client side envs~~ Dynamic client side envs 2 Oct 2, 2024

Fix lint issue

6e8bdf6

JasonMHasperhoven mentioned this pull request Oct 2, 2024

Dynamic client side envs #79

Closed

JasonMHasperhoven requested review from conorsch and a team October 2, 2024 14:07

JasonMHasperhoven marked this pull request as ready for review October 2, 2024 15:01

docs: update readme with new env vars

5c197bb

conorsch requested changes Oct 2, 2024

View reviewed changes

grod220 reviewed Oct 3, 2024

View reviewed changes

This was referenced Oct 3, 2024

Remove the use of a raw indexer #78

Closed

build: use dev runtime for container image #76

Closed

Add useEnv fetcher

a13e384

JasonMHasperhoven marked this pull request as draft October 4, 2024 17:43

JasonMHasperhoven changed the title ~~Dynamic client side envs 2~~ [WIP] Dynamic client side envs 2 Oct 4, 2024

JasonMHasperhoven added 2 commits October 7, 2024 15:08

Refactor app to use fetchers

4a5cd3c

Remove ClientEnv type file

99101c3

JasonMHasperhoven changed the title ~~[WIP] Dynamic client side envs 2~~ Dynamic client side envs 2 Oct 7, 2024

JasonMHasperhoven requested a review from a team October 7, 2024 11:17

JasonMHasperhoven added 4 commits October 7, 2024 15:23

Merge branch 'main' into dynamic-client-side-envs2

6948ae3

Fix merge conflict + view pool bug

11428fa

Add defaults for client env

a3eb862

Fix type import

726c0d0

JasonMHasperhoven marked this pull request as ready for review October 7, 2024 11:30

grod220 reviewed Oct 7, 2024

View reviewed changes

grod220 approved these changes Oct 7, 2024

View reviewed changes

JasonMHasperhoven added 4 commits October 7, 2024 20:16

Refactor fetchers to suffix Deprecated

95871aa

Fix duplicate lpAssetView.tsx file

68a0a8e

Fix lint issues

c9de74a

Fix lp asset view

b014301

conorsch self-requested a review October 8, 2024 15:58

conorsch approved these changes Oct 8, 2024

View reviewed changes

conorsch merged commit df94a11 into main Oct 8, 2024
3 checks passed

conorsch deleted the dynamic-client-side-envs2 branch October 8, 2024 16:05

This was referenced Oct 8, 2024

Asset registry not correct for testnet-phobos-2 #73

Closed

Make initial route redirect configurable #92

Closed

conorsch mentioned this pull request Nov 1, 2024

bug: registry assets fail to load in prod build #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic client side envs 2 #80

Dynamic client side envs 2 #80

JasonMHasperhoven commented Oct 2, 2024 •

edited

Loading

JasonMHasperhoven Oct 2, 2024

grod220 Oct 3, 2024

conorsch commented Oct 2, 2024

conorsch left a comment

grod220 left a comment

grod220 Oct 3, 2024

JasonMHasperhoven Oct 3, 2024

grod220 Oct 3, 2024

JasonMHasperhoven Oct 3, 2024

grod220 Oct 3, 2024

grod220 Oct 3, 2024

JasonMHasperhoven commented Oct 4, 2024 •

edited

Loading

JasonMHasperhoven commented Oct 7, 2024

grod220 Oct 7, 2024

grod220 Oct 7, 2024 •

edited

Loading

grod220 Oct 7, 2024

grod220 Oct 7, 2024

grod220 Oct 7, 2024

grod220 Oct 7, 2024

grod220 left a comment

conorsch left a comment

Dynamic client side envs 2 #80

Dynamic client side envs 2 #80

Conversation

JasonMHasperhoven commented Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Oct 2, 2024

conorsch left a comment

Choose a reason for hiding this comment

grod220 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonMHasperhoven commented Oct 4, 2024 • edited Loading

JasonMHasperhoven commented Oct 7, 2024

Choose a reason for hiding this comment

grod220 Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 left a comment

Choose a reason for hiding this comment

conorsch left a comment

Choose a reason for hiding this comment

JasonMHasperhoven commented Oct 2, 2024 •

edited

Loading

JasonMHasperhoven commented Oct 4, 2024 •

edited

Loading

grod220 Oct 7, 2024 •

edited

Loading