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

chore: cleanup contract references in auth-server and nft-quest #37

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

itsacoyote
Copy link
Contributor

@itsacoyote itsacoyote commented Dec 10, 2024

Description

Refactors reference of contract ABIs and addresses in auth-server and nft-quest.

  • Remove part of deploy script for nft-quest-contracts that wrote a .env.local. Removed to prevent confusion over what was being used in runtime during dev.
  • Reorganized folders for abi in auth-server and nft-quest. Cleaner to reference ABIs as well as centralize the location for contract addresses by chain.
  • Removed reference to contract addresses in nuxt.config.ts, instead use the abi folders.
  • Standardize defaultChainId as runtimeConfig var to use for setting the chain id in auth-server and nft-quest. This should allow overriding with env variables work on things like dev. example: NUXT_PUBLIC_DEFAULT_CHAIN_ID=300 pnpm nx dev nft-quest

Copy link

github-actions bot commented Dec 10, 2024

Visit the preview URL for this PR (updated for commit b7bb3f6):

https://zksync-auth-server-staging--pr37-refactor-chain-data-001eutsg.web.app

(expires Tue, 17 Dec 2024 18:51:28 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 509a9c9ea42583076f531c53cf2979c544d5d0b7

@itsacoyote itsacoyote force-pushed the refactor-chain-data branch 2 times, most recently from 75145e7 to caa900e Compare December 10, 2024 18:32
Copy link
Member

@JackHamer09 JackHamer09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really in favor of these changes. Looks like overcomplication to me and against usual standard practices.
Except just storing defaultChainId in runtime config and using env to set it, that's good (it just doesn't use the env now as far as I can see).

@@ -0,0 +1,6 @@
import type { AddressByChain } from "./types";

export const addressByChain: AddressByChain = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to store addresses in the same way as we do in auth-server. Less files, everything is in one place and less repetitive code

@@ -2,20 +2,25 @@ import { waitForTransactionReceipt, writeContract } from "@wagmi/core";
import type { Address } from "viem";
import { getGeneralPaymasterInput } from "viem/zksync";

import { Nft, Paymaster } from "~/abi";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think abi folder should just store contract abi's, not other information.

nft: "0x4D533d3B20b50b57268f189F93bFaf8B39c36AB6",
paymaster: "0x60eef092977DF2738480a6986e2aCD10236b1FA7",
},
defaultChainId: zksyncSepoliaTestnet.id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just do same as in auth server, use default chain env by default or testnet if the env is missing. I thought that was the idea of this PR, but env isn't used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runtimeconfig vars will automatically get replaced if it follows the naming scheme: "NUXT_PUBLIC_{public runtime config prop}". Writing it in is redundant and ignored if the env var is defined.

@@ -0,0 +1,351 @@
import type { AddressByChain } from "../types";

export const addressByChain: AddressByChain = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why split all addresses to different files? Now if you need to change an address or just find them you'll have to go through all these files..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants