Skip to content
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

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

Closed
conorsch opened this issue Sep 25, 2024 · 8 comments · Fixed by #90
Closed

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

conorsch opened this issue Sep 25, 2024 · 8 comments · Fixed by #90

Comments

@conorsch
Copy link
Contributor

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 prax-wallet/registry#91 & prax-wallet/registry#93, as well as corresponding changes in this repo to consume #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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 added a commit that referenced this issue Sep 26, 2024
Adds an easy way to run the container image locally, to aid in
debugging. Refs #73.
@conorsch
Copy link
Contributor Author

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:

const defaultChainId = "penumbra-1";
const defaultCuiolaUrl = "https://cuiloa.testnet.penumbra.zone";
export const Constants: ConstantsConfig = {
cuiloaUrl: process.env["NEXT_PUBLIC_CUILOA_URL"] ?? defaultCuiolaUrl,
chainId: process.env["NEXT_PUBLIC_CHAIN_ID"] ?? defaultChainId,
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 added a commit that referenced this issue Sep 26, 2024
Previously the `NEXT_PUBLIC_CHAIN_ID` env var was only read at build
time, and was ignored at runtime. That's not the behavior we want: we
want a default value that can be overridden at runtime to refer to any
network, so that the same built container image can be used in multiple
contexts. This change permits that, by using a variable name, rather
than a string literal, to look up the `NEXT_PUBLIC_CHAIN_ID` env var.

Also refactors the endpoint-loading logic, which isn't strictly
necessary, but I found it cleaner to only have hits for `grep -rF
process.env` in a single location in the source code.

Sprinkles in a few `justfile` additions to make testing locally a bit
more straightforward, since it was useful to me while debugging.

Refs #73.
conorsch added a commit that referenced this issue Sep 26, 2024
As a temporary measure, uses the `pnpm run dev` environment
inside the container image. This is an anti-pattern. We *should* be
using the "standalone" output of nextjs and running that directly via
`node`, but doing so does not permit runtime configuration of the
chain-id, used for loading assets, which is must-have for the built
image.

Sprinkles in a few `justfile` additions to make testing locally a bit
more straightforward, as we'll surely be exercising these config paths
again in the near future.

Refs #73.
@conorsch
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra web Oct 4, 2024
@conorsch
Copy link
Contributor Author

conorsch commented Oct 8, 2024

Resolved via #80.

@conorsch conorsch closed this as completed Oct 8, 2024
@github-project-automation github-project-automation bot moved this from 🗄️ Backlog to ✅ Done in Penumbra web Oct 8, 2024
conorsch added a commit that referenced this issue Oct 9, 2024
Now that we've resolved #73, we're unblocked from restoring rolling
deployments on merge to main, to the testnet instance [0].
This new workflow blocks on completion of the build-container logic,
then bounces the pre-existing deployment to pull the most recently built
container.

[0] https://dex-explorer.testnet.plinfra.net
conorsch added a commit that referenced this issue Oct 9, 2024
Now that we've resolved #73, we're unblocked from restoring rolling
deployments on merge to main, to the testnet instance [0].
This new workflow blocks on completion of the build-container logic,
then bounces the pre-existing deployment to pull the most recently built
container.

Refs #72.

[0] https://dex-explorer.testnet.plinfra.net
conorsch added a commit that referenced this issue Oct 9, 2024
Now that we've resolved #73, we're unblocked from restoring rolling
deployments on merge to main, to the testnet instance [0].
This new workflow blocks on completion of the build-container logic,
then bounces the pre-existing deployment to pull the most recently built
container.

Refs #72.

[0] https://dex-explorer.testnet.plinfra.net
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants