penumbra-zone / dex-explorer

Web app for visualization the state of the Penumbra DEX
Apache License 2.0
10 stars 1 forks source link

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

Closed conorsch closed 1 month ago

conorsch commented 1 month ago

There's a new testnet chain penumbra-testnet-phobos-2, running here: https://testnet.plinfra.net There's also a dex-explorer instance running against that chain https://dex-explorer.testnet.plinfra.net (#72). Despite recent updates to the prax registry in https://github.com/prax-wallet/registry/pull/91 & https://github.com/prax-wallet/registry/pull/93, as well as corresponding changes in this repo to consume https://github.com/penumbra-zone/dex-explorer/issues/57, the test_usd asset still isn't loading correctly:

dex-explorer-undefined-asset

The asset on the lefthand side is test_usd, or at least that's how it was specified on when swapped via pcli. But the app fails to understand the asset, which indicates that we likely have the wrong string set in the prax registry still: https://github.com/prax-wallet/registry/blob/7cfdd0000c4f4c458e147b85ba6c7f3f87066c11/input/chains/penumbra-testnet-phobos-2.json#L144 Let's debug that and fix it.

conorsch commented 1 month ago

With an assist from @hdevalence, I've confirmed that the test_usd asset string appears to be correct. One can cross-check it by using the AssetMatadataById RPC: https://buf.build/studio/penumbra-zone/penumbra/penumbra.core.component.shielded_pool.v1.QueryService/AssetMetadataById?target=https%3A%2F%2Ftestnet.plinfra.net&selectedProtocol=grpc-web So why isn't it displaying correctly in the application? Are other assets displaying correctly? At least UM is. Let's test with more.

conorsch commented 1 month ago

Ah-ha, it looks like a problem with the container build story. Testing locally via pnpm dev with the same database backend, I see the assets rendering correctly:

dex-explorer-dev-env-working

So something with the npm run build is emitting bad or incomplete artifacts.

conorsch commented 1 month ago

After much debugging, and poring over the NextJS docs, it's now clear to me that the application does not actually accept runtime configuration via env var for chain id. The code looks up NEXT_PUBLIC_CHAIN_ID here: https://github.com/penumbra-zone/dex-explorer/blob/eff9854eaab688b9424c1e72ba614314aa3f98c9/src/constants/configConstants.ts#L6-L11 allegedly with fallback to penumbra-1, but if you set NEXT_PUBLIC_CHAIN_ID=penumbra-testnet-phobos-2 at runtime, the app will still query for penumbra-1. That's because the NEXT_PUBLIC_* env vars are baked in at build time based on the value of the environment variable, and cannot be updated at runtime. Notably the nextjs docs do describe this behavior:

After being built, your app will no longer respond to changes to these environment variables. For instance, if you use a Heroku pipeline to promote slugs built in one environment to another environment, or if you build and deploy a single Docker image to multiple environments, all NEXT_PUBLIC_ variables will be frozen with the value evaluated at build time, so these values need to be set appropriately when the project is built. If you need access to runtime environment values, you'll have to setup your own API to provide them to the client (either on demand or during initialization).

We currently build a container image, ghcr.io/penumbra-zone/dex-explorer, to make it easy for anyone to run a dex-explorer. There are currently multiple deployments, tracking different chains, based on this image. We must update the application config such that the same built artifact, i.e. the container, can be used to run against multiple chains.

So, how to do that? From my limited understanding of the nextjs docs, it appears we should be using getServerSideProps to pass server-side info to the client code cleanly. There's also a possibility that we can prevent the inlining-at-build-time behavior by tweaking the env var lookups to use a var rather than a string (ctrl+f for "will not be inlined"). Will try that next, otherwise fall back to running pnpm run dev as container entrypoint, hacky as that is.

conorsch commented 1 month ago

There's also a possibility that we can prevent the inlining-at-build-time behavior by tweaking the env var lookups to use a var rather than a string (ctrl+f for "will not be inlined").

No, that approach didn't work: when I tested, the server-side code reports the runtime value, but the client-side code reports the build-time value. That's not good enough for our use case.

otherwise fall back to running pnpm run dev as container entrypoint, hacky as that is

That's the approach I went with in #76, as a temporary solution to have a working staging environment again. @JasonMHasperhoven, I'd very much appreciate you taking a look at the config logic, and resolving as you see fit, so that we can use pnpm run build standalone output again.

JasonMHasperhoven commented 1 month ago

There are some optimization that we would miss if we don't pre-build nextjs. I think the move is to have multiple images that point to different environments, so then we run the test image in our staging env and the prod image in the prod env.

conorsch commented 1 month ago

I think the move is to have multiple images that point to different environments, so then we run the test image in our staging env and the prod image in the prod env.

I'd prefer to keep the built artifact, i.e. the container, environment-agnostic, using runtime environment variables to configure the same app code to interact with different networks. We already have the ability to inject e.g. a postgres database URL at runtime, and the same should be true of asset registry info. Discussed this out of band with @JasonMHasperhoven, who agreed that GetServerSideProps looks like it would solve our problem. If that turns out to be the case, we can close out #76 as an unused experiment.

cronokirby commented 1 month ago

Another idea: what if running the server involved running the next build in the current environment, and then serving that? This way you could have one container, and then dynamically configure the build process, but then also not require changes to the code.

conorsch commented 1 month ago

Resolved via #80.