Prepare the collector #297
Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes Phase 3 of the multi-chain expansion project by migrating CollectorNFT components from local contract configuration to the shared @fretchen/chain-utils package. The migration follows the established pattern from Phase 2 (GenImNFT migration) and enables the CollectorNFT to support multi-chain deployment in the future.
Changes:
- Added
CollectorNFTv1ABIto@fretchen/chain-utilswith minimal ABI containing only the functions needed by the website - Migrated
SimpleCollectButtoncomponent to useuseAutoNetwork()hook with deferred chain switching - Removed
collectorNFTContractConfigfrom legacy configuration, keeping only LLMv1 (out of scope for multi-chain) - Updated documentation to reflect Phase 3 completion
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
shared/chain-utils/src/abi/CollectorNFTv1.ts |
New minimal ABI for CollectorNFTv1 with getMintStats and mintCollectorNFT functions |
shared/chain-utils/src/abi/index.ts |
Added export for CollectorNFTv1ABI |
website/components/SimpleCollectButton.tsx |
Migrated to use useAutoNetwork() hook and chain-utils, adds network switching before transaction |
website/test/SimpleCollectButton.test.tsx |
Updated mocks for useAutoNetwork and chain-utils, removed obsolete network check test |
website/utils/getChain.ts |
Removed collectorNFTContractConfig, preserved getChain() for LLMv1, updated comments |
website/components/MyNFTList.tsx |
Removed unnecessary SupportedChainId type cast (unrelated cleanup) |
website/MULTICHAIN_EXPANSION_PROPOSAL.md |
Updated to mark Phase 3 as complete with implementation details |
| outputs: [ | ||
| { name: "mintCount", type: "uint256" }, | ||
| { name: "currentPrice", type: "uint256" }, | ||
| { name: "lastMinter", type: "address" }, |
There was a problem hiding this comment.
The documented ABI output at line 553 shows lastMinter as the third output parameter, but the actual contract (CollectorNFTv1.sol line 181) and the implemented ABI (CollectorNFTv1.ts line 18) correctly use nextPrice. This documentation should be updated to match the actual implementation.
| { name: "lastMinter", type: "address" }, | |
| { name: "nextPrice", type: "uint256" }, |
| setIsLoading(true); | ||
|
|
||
| // Ensure correct network before transaction | ||
| const switched = await switchIfNeeded(); | ||
| if (!switched) { | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
|
|
||
| const [, currentPrice] = mintStats as [bigint, bigint, bigint]; | ||
|
|
||
| writeContract({ | ||
| ...collectorNFTContractConfig, | ||
| address: getCollectorNFTAddress(network), | ||
| abi: CollectorNFTv1ABI, | ||
| functionName: "mintCollectorNFT", | ||
| args: [genImTokenId], // CollectorNFTv1 doesn't need URI parameter | ||
| value: currentPrice, | ||
| }); | ||
|
|
||
| setIsLoading(false); |
There was a problem hiding this comment.
Setting isLoading(false) immediately after calling writeContract() creates a potential timing issue. The writeContract() function is non-blocking, so isLoading becomes false before isPending becomes true, which could briefly enable the button between states.
For consistency with other components in the codebase (see NFTCard.tsx lines 264-295), consider either:
- Remove the custom
isLoadingstate entirely and rely solely on wagmi'sisPendingandisConfirmingstates, OR - Keep
isLoadingtrue and let it be cleared by the success/error handlers, similar to ImageGenerator.tsx line 293
The current pattern is inconsistent with the rest of the codebase.
No description provided.