Skip to content

fix: deploy script contract mismatch#48

Open
dhruvi-16-me wants to merge 1 commit intoStabilityNexus:mainfrom
dhruvi-16-me:fix/deploy-script-contract
Open

fix: deploy script contract mismatch#48
dhruvi-16-me wants to merge 1 commit intoStabilityNexus:mainfrom
dhruvi-16-me:fix/deploy-script-contract

Conversation

@dhruvi-16-me
Copy link

@dhruvi-16-me dhruvi-16-me commented Feb 16, 2026

Addressed Issues:

Fixes #47

Screenshots/Recordings:

image

Additional Notes:

Changes

  • Removed conflicting auctionName variable.
  • Deployment now uses getContractFactory(CONTRACT_TO_DEPLOY).
  • Console log reflects the actual deployed contract.
  • Removed misleading comments.
  • Ensured only the contract specified by CONTRACT_TO_DEPLOY is deployed.

Checklist

  • My PR addresses a single issue, fixes a single bug or makes a single improvement.
  • My code follows the project's code style and conventions.
  • If applicable, I have made corresponding changes or additions to the documentation.
  • If applicable, I have made corresponding changes or additions to tests.
  • My changes generate no new warnings or errors.
  • I have joined the Stability Nexus's Discord server and I will share a link to this PR with the project maintainers there.
  • I have read the Contribution Guidelines.
  • Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.

⚠️ AI Notice - Important!

We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.

Summary by CodeRabbit

  • Chores
    • Refactored deployment script to support configurable contract selection with type-specific deployment parameters for ProtocolParameters, MockToken, and MockNFT contracts.
    • Enhanced deployment logging to display contract names and addresses.

@coderabbitai
Copy link

coderabbitai bot commented Feb 16, 2026

📝 Walkthrough

Walkthrough

The deployment script now uses a configurable CONTRACT_TO_DEPLOY constant instead of hardcoded contract paths, with conditional logic to handle different contract types (ProtocolParameters, MockToken, MockNFT) and their respective deployment parameters.

Changes

Cohort / File(s) Summary
Deploy Script Configuration
deploy/deploy.script.ts
Replaced hardcoded MockToken deployment with configurable CONTRACT_TO_DEPLOY constant. Added conditional logic to handle deployment of ProtocolParameters, MockToken, MockNFT, and default contracts with appropriate constructor parameters. Introduced deployerAddress extraction from signer and updated console logging to reflect actual deployed contract name.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • ceilican

Poem

🐰 A script once confused, now crystal and clear,
No more mixed-up names causing deploy-time fear!
From MockToken tangled to contracts galore,
One constant to rule them, one source of truth core!

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title concisely describes the main fix: resolving contract mismatch in the deployment script by making it deploy the intended contract.
Linked Issues check ✅ Passed All coding requirements from issue #47 are met: single source of truth (CONTRACT_TO_DEPLOY) established, conflicting variables removed, console output updated, and misleading comments removed.
Out of Scope Changes check ✅ Passed All changes focus exclusively on fixing the contract deployment mismatch; no unrelated modifications were introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.


In `@deploy/deploy.script.ts`:
- Around line 18-27: The default branch currently calls
Contract.deploy(deployerAddress) which passes an EOA where auction contracts
expect a ProtocolParameters contract address; update the deployment logic so the
protocol-parameters address is explicit and validated instead of using
deployerAddress: introduce a configurable constant (e.g.
PROTOCOL_PARAMETERS_ADDRESS) or require CONTRACT_TO_DEPLOY to be
'ProtocolParameters' before using deployerAddress, then call
Contract.deploy(PROTOCOL_PARAMETERS_ADDRESS) (or deploy a ProtocolParameters
contract when none provided) and throw or exit if PROTOCOL_PARAMETERS_ADDRESS is
missing; update any references to Contract.deploy in this file so
ProtocolParameters addresses are supplied, and log/validate the address before
deploying auction contracts.
- Around line 28-30: The console.log reads contract.target immediately after
deploy; update the deploy flow to await the contract being mined by calling
await contract.waitForDeployment() after deploy() and before accessing
contract.target so the deployment is confirmed (use the existing contract
variable returned by deploy(), and call waitForDeployment on it prior to the
console.log).
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed.


In `@deploy/deploy.script.ts`:
- Around line 28-30: The console.log reads contract.target immediately after
deploy; update the deploy flow to await the contract being mined by calling
await contract.waitForDeployment() after deploy() and before accessing
contract.target so the deployment is confirmed (use the existing contract
variable returned by deploy(), and call waitForDeployment on it prior to the
console.log).
deploy/deploy.script.ts (1)

28-30: Add await contract.waitForDeployment() before accessing contract.target.

In ethers v6, deploy() resolves before the contract is actually deployed on-chain. While the address is deterministic and available immediately, the contract code won't exist until the transaction is mined. On Hardhat's local network transactions are mined immediately, but on testnets and mainnet this becomes a real issue if the script is extended to call contract methods. Following the ethers v6 best practice, use await contract.waitForDeployment() to ensure the deployment is confirmed before proceeding.

Proposed fix
+    await contract.waitForDeployment();
     console.log(`${CONTRACT_TO_DEPLOY} deployed to address:`, contract.target);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@deploy/deploy.script.ts` around lines 28 - 30, The console.log reads
contract.target immediately after deploy; update the deploy flow to await the
contract being mined by calling await contract.waitForDeployment() after
deploy() and before accessing contract.target so the deployment is confirmed
(use the existing contract variable returned by deploy(), and call
waitForDeployment on it prior to the console.log).

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.

[Improvement] Deploy script deploys a different contract than indicated by constants and comments

1 participant