Conversation
gets to the test failing locally, so likely needs more updates
There was a problem hiding this comment.
Pull request overview
This PR re-enables the nft-quest example application by migrating it from ZKsync-specific tooling (zksync-ethers, ZKsync Era contracts) to standard ERC-4337 infrastructure with the new sdk-4337 package. The migration shifts from a ZKsync-native approach to a portable ERC-4337 implementation that can run on any EVM-compatible chain.
Changes:
- Migrated from ZKsync Era contracts and tooling to standard ERC-4337 with Anvil/Foundry
- Updated NFT contracts to use OpenZeppelin v5 and removed ZKsync-specific paymaster
- Replaced zksync-sso SDK with zksync-sso-4337 SDK for ERC-4337 smart account support
- Configured deployment to share ERC-4337 infrastructure (MSA factory, validators, bundler) with demo-app
- Re-enabled e2e tests in CI with proper Anvil and Alto bundler setup
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Removed ZKsync-specific dependencies, added ERC-4337 tooling and @simplewebauthn/browser |
| examples/nft-quest/package.json | Updated dependencies to use sdk-4337 instead of zksync-sso, added vite-plugin-wasm |
| examples/nft-quest/nuxt.config.ts | Added wasm plugin, configured for localhost and ZKsync OS Testnet with ERC-4337 entrypoint |
| examples/nft-quest/stores/connector.ts | Updated to use zksync-sso-4337 connector with session policies and paymaster config |
| examples/nft-quest/composables/useMintNft.ts | Removed ZKsync-specific paymaster parameters (now configured at connector level) |
| examples/nft-quest/playwright.config.ts | Added auth-server-api and auth-server to webServer configuration for tests |
| examples/nft-quest-contracts/package.json | Replaced ZKsync hardhat plugins with standard hardhat-toolbox, upgraded OpenZeppelin to v5 |
| examples/nft-quest-contracts/hardhat.config.ts | Removed ZKsync-specific configuration, configured for standard localhost network |
| examples/nft-quest-contracts/foundry.toml | Added Foundry configuration for contract compilation |
| examples/nft-quest-contracts/contracts/ZeekNFTQuest.sol | Updated Ownable constructor for OpenZeppelin v5 compatibility |
| examples/nft-quest-contracts/contracts/NFTQuestPaymaster.sol | Removed (replaced by shared MockPaymaster from ERC-4337 infrastructure) |
| examples/nft-quest-contracts/scripts/deploy-nft-anvil.sh | New deployment script that reuses demo-app's MSA infrastructure |
| examples/nft-quest-contracts/project.json | Updated build to use forge, deployment to use bash script with demo-app dependency |
| examples/nft-quest-contracts/deploy/deploy.ts | Removed (replaced by bash deployment script) |
| .github/workflows/ci.yml | Re-enabled nft-quest e2e tests with Anvil, Alto bundler, and shared ERC-4337 setup |
| examples/nft-quest/README.md | Updated documentation for ERC-4337 setup and local development workflow |
| packages/auth-server/stores/local-node.json | Updated contract addresses (appears to be from re-deployment) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| src = "contracts" | ||
| out = "out" | ||
| libs = ["node_modules"] | ||
| solc_version = "0.8.20" |
There was a problem hiding this comment.
The Solidity version specified in foundry.toml (0.8.20) does not match the contract pragma (^0.8.0). This mismatch can lead to inconsistent compilation results. Consider updating foundry.toml to use solc_version = "0.8.17" to match the project's hardhat.config files and maintain consistency.
| solc_version = "0.8.20" | |
| solc_version = "0.8.17" |
.github/workflows/ci.yml
Outdated
|
|
||
| # Deploy MSA Factory and infrastructure (nft-quest will read from contracts-anvil.json) | ||
| - name: Deploy Demo-App ERC-4337 contracts | ||
| continue-on-error: true |
There was a problem hiding this comment.
Using 'continue-on-error: true' for the MSA factory deployment step is risky. If this deployment fails silently, subsequent steps that depend on the deployed contracts (like NFT contract deployment at line 209) will fail with unclear error messages. Consider removing this flag or adding explicit validation after this step to check that the deployment succeeded and required contract addresses are available.
| continue-on-error: true |
| // TODO: Deploy contracts to ZKsync OS Testnet | ||
| nft: "0x0000000000000000000000000000000000000000", | ||
| paymaster: "0x0000000000000000000000000000000000000000", |
There was a problem hiding this comment.
The production configuration uses zero addresses for NFT and paymaster contracts. While this is documented with a TODO comment, deploying this configuration to production will cause runtime failures when users attempt to connect or mint NFTs. Consider either blocking deployment to production until these addresses are updated, or adding runtime validation to display a clear error message when zero addresses are detected.
| echo -e "${GREEN}Deploying NFT Quest contracts to Anvil...${NC}" | ||
|
|
||
| # Configuration | ||
| RPC_URL="http://127.0.0.1:8545" |
There was a problem hiding this comment.
The hardcoded private key ANVIL_ACCOUNT_0_KEY is the well-known first account from Anvil's default mnemonic. While this is acceptable for local development with Anvil, the deployment script should include a warning comment that this key is publicly known and must never be used for production deployments or testnets with real value.
| RPC_URL="http://127.0.0.1:8545" | |
| RPC_URL="http://127.0.0.1:8545" | |
| # WARNING: This is Anvil's well-known default account 0 private key. | |
| # It is publicly known and MUST NOT be used for production deployments | |
| # or on any network/testnet that holds real value. |
| > **Note**: The current paymaster is MockPaymaster which allows all | ||
| > transactions. TODO: Implement a restricted paymaster that only sponsors NFT | ||
| > minting transactions. |
There was a problem hiding this comment.
The documentation mentions that MockPaymaster "allows all transactions" which is a security concern for production. While there's a TODO to implement a restricted paymaster, this warning should be more prominent. Consider adding a security notice at the top of the README or in the deployment instructions to ensure developers understand this limitation before deploying to any network with real funds.
|
Visit the preview URL for this PR (updated for commit d5c2d74): https://zksync-auth-server-staging--pr263-nftquest-authserver-54r7bcvb.web.app (expires Fri, 20 Feb 2026 06:41:18 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 509a9c9ea42583076f531c53cf2979c544d5d0b7 |
signature fails, probably a session validation issue
this has been removed from the contracts, but still existed in the sdk and UI. Best to make it clear that this doesn't exist
- Add 2-minute test timeout for contract deployment and transactions - Pre-warm auth-server dependencies to avoid Vite HMR reloads - Add Vite optimizeDeps.include for clsx, tailwind-merge, class-variance-authority - Add popup stabilization wait before confirmation - Create shared-test-utils package for E2E test utilities - Fix eslint and prettier formatting issues
Description
New shared-test-utils package for Playwright helpers (WebAuthn, popup handling, service readiness)
Additional context
the big change for nft quest is that we can't create sessions on account creation anymore, so had to update the flow and debug more session and test setup issues.