Add download Hardhat Polkadot zip option #680
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughBumps Node.js to 22 and setup-node to v5 in CI. Refactors Hardhat zip generation into a class. Adds a Polkadot-specific Hardhat zip generator, exports, and tests with snapshots. Updates UI overrides to function-based omit and optional override for Hardhat zip, with dynamic import in Polkadot UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as Solidity App.svelte
participant Ov as Overrides
participant Gen as Core Hardhat Generator
User->>UI: Click "Download Hardhat ZIP"
UI->>Ov: omitZipHardhat(opts)
alt omit = true
UI-->>User: Skip Hardhat ZIP
else omit = false
UI->>Ov: overrideZipHardhat?
alt override present
UI->>Ov: overrideZipHardhat(contract, opts)
Ov-->>UI: JSZip
else no override
UI->>Gen: zipHardhat(contract, opts)
Gen-->>UI: JSZip
end
UI-->>User: Start ZIP download
end
sequenceDiagram
autonumber
participant PolyUI as Polkadot App.svelte
participant Dyn as Dynamic Import
participant PGen as zipHardhatPolkadot
participant HGen as HardhatZipGenerator
PolyUI->>Dyn: import("@openzeppelin/wizard/zip-env-hardhat-polkadot")
Dyn-->>PolyUI: { zipHardhatPolkadot }
PolyUI->>PGen: zipHardhatPolkadot(contract, opts)
PGen->>HGen: Extend Hardhat config (add @parity/hardhat-polkadot)
HGen-->>PolyUI: JSZip (Polkadot Hardhat project)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/core/solidity/src/environments/hardhat/polkadot/package.json (1)
11-16: Consider version range strategy for dependencies.The package uses caret (^) version ranges for all dependencies. While this allows for minor/patch updates, consider whether stricter pinning is appropriate for critical dependencies like the Polkadot plugin.
For more stability, consider exact version pinning for newer/experimental dependencies:
"devDependencies": { "@openzeppelin/contracts": "^5.4.0", - "@parity/hardhat-polkadot": "^0.1.9", + "@parity/hardhat-polkadot": "0.1.9", "@nomicfoundation/hardhat-toolbox": "^6.1.0", "hardhat": "^2.16.1" }packages/core/solidity/src/zip-hardhat-polkadot.test.ts (1)
121-123: Consider extracting the duplicateasStringhelperThis
asStringfunction duplicates the one already available inpackages/core/stellar/src/utils/zip-test.ts.Consider importing the shared utility instead:
-async function asString(item: JSZipObject) { - return Buffer.from(await item.async('arraybuffer')).toString(); -} +import { asString } from '../../stellar/src/utils/zip-test';Or if cross-package imports aren't desired, extract it to a shared test utilities module within the solidity package.
packages/core/solidity/src/zip-hardhat-polkadot.ts (1)
57-62: Extra whitespace in gitignore templateThe gitignore template has inconsistent indentation with extra spaces at the beginning and end.
Clean up the formatting:
protected getGitIgnoreHardhatIgnition(): string { - return ` -# Hardhat Ignition default folder for deployments against a local Polkadot Revive Dev node -ignition/deployments/chain-420420420 - `; + return ` +# Hardhat Ignition default folder for deployments against a local Polkadot Revive Dev node +ignition/deployments/chain-420420420`; }packages/core/solidity/src/zip-hardhat.ts (1)
64-73: Consider validating constructor argument typesThe code assumes that non-address constructor arguments need manual setting, but doesn't validate the actual types. This could be enhanced to provide more specific guidance based on the actual argument types.
Consider providing more specific placeholders based on the argument type:
private declareVariables(args: FunctionArgument[]): Lines[] { const vars = []; for (let i = 0; i < args.length; i++) { if (args[i]!.type === 'address') { vars.push(`const ${args[i]!.name} = (await ethers.getSigners())[${i}].address;`); } else { + const argType = typeof args[i]!.type === 'string' ? args[i]!.type : 'contract'; + const placeholder = this.getPlaceholderForType(argType); vars.push(`// TODO: Set the following constructor argument`); - vars.push(`// const ${args[i]!.name} = ...;`); + vars.push(`// const ${args[i]!.name} = ${placeholder};`); } } return vars; } + + private getPlaceholderForType(type: string): string { + switch(type) { + case 'uint256': return '1000'; + case 'string': return '"example"'; + case 'bool': return 'true'; + default: return '...'; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/core/solidity/src/environments/hardhat/polkadot/package-lock.jsonis excluded by!**/package-lock.jsonpackages/core/solidity/src/zip-hardhat-polkadot.test.ts.snapis excluded by!**/*.snappackages/core/solidity/src/zip-hardhat.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
.github/actions/setup/action.yml(1 hunks).nvmrc(1 hunks)packages/core/solidity/src/environments/hardhat/polkadot/package.json(1 hunks)packages/core/solidity/src/zip-hardhat-polkadot.test.ts(1 hunks)packages/core/solidity/src/zip-hardhat-polkadot.test.ts.md(1 hunks)packages/core/solidity/src/zip-hardhat-polkadot.ts(1 hunks)packages/core/solidity/src/zip-hardhat.test.ts.md(7 hunks)packages/core/solidity/src/zip-hardhat.ts(4 hunks)packages/core/solidity/zip-env-hardhat-polkadot.js(1 hunks)packages/core/solidity/zip-env-hardhat-polkadot.ts(1 hunks)packages/ui/src/polkadot/App.svelte(2 hunks)packages/ui/src/solidity/App.svelte(2 hunks)packages/ui/src/solidity/overrides.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-24T20:55:43.331Z
Learnt from: CoveMB
PR: OpenZeppelin/contracts-wizard#665
File: packages/ui/src/solidity/remix.node.test.ts:0-0
Timestamp: 2025-09-24T20:55:43.331Z
Learning: In the OpenZeppelin contracts-wizard project, the UI package tests use AVA framework which provides browser API polyfills, so `atob` and other browser APIs are available in the test environment.
Applied to files:
packages/core/solidity/src/zip-hardhat-polkadot.test.ts
📚 Learning: 2025-08-15T23:23:13.097Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/signer.ts:31-38
Timestamp: 2025-08-15T23:23:13.097Z
Learning: In OpenZeppelin contracts-wizard, the `setUpgradeableAccount` function deliberately sets `c.upgradeable = false` after upgradeable setup to exclude EIP712Upgradeable and ERC7739Upgradeable variants (avoiding per-call SLOAD overhead). This architectural pattern necessitates manual `_disableInitializers()` setup in both account.ts and signer.ts since the standard upgradeable constructor logic doesn't apply when upgradeability is disabled post-setup.
Applied to files:
packages/core/solidity/src/zip-hardhat.test.ts.md
📚 Learning: 2025-08-15T22:52:34.129Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: packages/core/solidity/src/account.ts:89-0
Timestamp: 2025-08-15T22:52:34.129Z
Learning: In OpenZeppelin contracts-wizard, non-upgradeable accounts still require initializer logic (Initializable, _disableInitializers(), and initialize() function) because ERC-4337 accounts are typically deployed by factories as minimal clone proxies, which cannot use constructors effectively for initialization. This is the intended deployment pattern for ERC-4337 account abstraction, even for non-upgradeable accounts.
Applied to files:
packages/core/solidity/src/zip-hardhat.test.ts.md
🧬 Code graph analysis (3)
packages/core/solidity/src/zip-hardhat-polkadot.test.ts (2)
packages/core/solidity/src/zip-hardhat-polkadot.ts (1)
zipHardhatPolkadot(65-67)packages/core/stellar/src/utils/zip-test.ts (1)
asString(15-15)
packages/core/solidity/src/zip-hardhat-polkadot.ts (2)
packages/core/solidity/src/zip-hardhat.ts (1)
HardhatZipGenerator(91-307)packages/core/solidity/hardhat.config.js (1)
SOLIDITY_VERSION(11-11)
packages/core/solidity/src/zip-hardhat.ts (3)
packages/core/solidity/src/contract.ts (2)
Contract(3-14)FunctionArgument(65-68)packages/core/solidity/src/utils/format-lines.ts (1)
formatLinesWithSpaces(9-11)packages/core/solidity/src/print.ts (1)
printContract(23-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (solidity, default)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (14)
.nvmrc (1)
1-1: Node.js 22 upgrade aligns with PR requirements.Node.js v22 has officially moved into Active LTS status and the node.js 22 will enter long-term support (LTS) in October, making this a stable choice for production. The upgrade from Node 20 to 22 supports the Hardhat Polkadot plugin requirement mentioned in PR objectives.
.github/actions/setup/action.yml (1)
6-8: GitHub Actions setup properly updated for Node.js 22.The action correctly updates both the setup-node version to v5.0.0 and Node.js version to 22, maintaining consistency with the .nvmrc change. The commit hash ensures reproducible builds.
packages/core/solidity/src/zip-hardhat-polkadot.test.ts.md (1)
1-201: Test snapshot looks comprehensive and properly structured.The snapshot covers all essential files for a Hardhat Polkadot project including contracts, configuration, package.json, ignition modules, tests, and documentation. The content demonstrates proper integration of Polkadot-specific configuration like
polkavm: trueand Polkadot Revive-specific settings.packages/core/solidity/src/zip-hardhat.test.ts.md (2)
107-107: Comment improvement enhances clarity.The update from "Set addresses for the contract arguments below" to "Set values for the constructor arguments below" improves clarity and accuracy, as constructor arguments aren't limited to addresses.
Also applies to: 266-266, 373-373, 465-465, 546-546, 644-645, 667-667
645-645: Array format for unsafeAllow corrects Hardhat Upgrades API usage.The change from
{ unsafeAllow: 'constructor' }to{ unsafeAllow: ['constructor'] }aligns with the correct Hardhat Upgrades API format, which expects an array of strings rather than a single string.Also applies to: 667-667
packages/core/solidity/zip-env-hardhat-polkadot.ts (1)
1-1: Clean re-export structure maintains API consistency.The simple re-export pattern follows the established convention for environment-specific entry points and properly exposes the Polkadot Hardhat functionality.
packages/ui/src/polkadot/App.svelte (3)
7-7: Type imports added for enhanced type safety.Adding explicit type imports for
ContractandGenericOptionsimproves type safety for the new override functions.
12-14: Dynamic import implementation enables lazy loading.The dynamic import pattern efficiently loads the Polkadot-specific module only when needed, reducing initial bundle size while maintaining functionality.
23-29: Function-based override provides conditional Hardhat zip generation.The implementation properly uses the new override API:
omitZipHardhatfunction correctly excludes upgradeable contracts (which aren't supported for Polkadot per PR objectives)overrideZipHardhatasync function properly awaits the dynamic module and delegates tozipHardhatPolkadotpackages/core/solidity/zip-env-hardhat-polkadot.js (1)
1-1: Simple proxy module follows established patterns.The CommonJS proxy correctly routes to the built distribution file, maintaining consistency with other environment entry points in the project.
packages/ui/src/solidity/App.svelte (2)
146-146: LGTM! Clean migration to function-based overrideThe change from a boolean check to a function call
overrides.omitZipHardhat(opts)provides better flexibility for conditional logic based on the options.
204-207: Good implementation of override pattern for Hardhat zip generationThe conditional logic properly checks for the presence of
overrideZipHardhatand falls back to the defaultzipHardhatwhen not provided. This allows ecosystem-specific customization while maintaining backward compatibility.packages/ui/src/solidity/overrides.ts (1)
24-29: Well-designed extensibility for Hardhat zip generationThe conversion of
omitZipHardhatto a function and addition ofoverrideZipHardhatprovides a clean extension point for ecosystem-specific Hardhat configurations. The optional nature ofoverrideZipHardhatmaintains backward compatibility.packages/core/solidity/src/zip-hardhat.ts (1)
91-307: Excellent refactoring to class-based architectureThe refactoring from procedural code to the
HardhatZipGeneratorclass provides excellent extensibility, as demonstrated by the Polkadot subclass. The protected methods allow for selective overriding while maintaining the core logic.
| "@nomicfoundation/hardhat-toolbox": "^6.1.0", | ||
| "hardhat": "^2.16.1" | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
License should be MIT for consistency.
The package.json uses "ISC" license, but Line 117 in the test snapshot shows "MIT". This inconsistency should be resolved for consistency with the rest of the project.
Apply this diff to fix the license inconsistency:
- "license": "ISC",
+ "license": "MIT",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| "license": "MIT", | |
| } |
🤖 Prompt for AI Agents
In packages/core/solidity/src/environments/hardhat/polkadot/package.json around
line 17, the "license" field is set to "ISC" but the project snapshot and
standard use "MIT"; update the license value from "ISC" to "MIT" in the
package.json to ensure consistency across the repository.
| } | ||
| } | ||
|
|
||
| const command = `cd "${tempFolder}" && npm install && npx hardhat compile`; |
There was a problem hiding this comment.
Avoid shell injection vulnerabilities with unsanitized paths
Using string interpolation in shell commands with tempFolder from the OS temp directory could be vulnerable to command injection if the path contains special characters.
Use proper escaping or the programmatic API instead:
- const command = `cd "${tempFolder}" && npm install && npx hardhat compile`;
+ const cwd = tempFolder;
+ const exec = util.promisify(child.exec);
+
+ // Run npm install
+ await exec('npm install', { cwd });
+ // Run hardhat compile
+ const result = await exec('npx hardhat compile', { cwd });Alternatively, use child_process.spawn with proper argument arrays to avoid shell interpretation entirely.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/core/solidity/src/zip-hardhat-polkadot.test.ts around line 91, the
test builds a shell command by interpolating tempFolder into a single command
string which can be exploited via shell injection; replace this pattern with a
programmatic child_process API (e.g., child_process.spawn or execFile) and run
the commands with argument arrays and cwd set to tempFolder (or call npm and npx
as separate spawn calls) so no shell interpretation occurs and tempFolder is not
passed to a shell parser; ensure spawn is called without shell: true and handle
stdout/stderr/promises accordingly.
| protected getHardhatConfigJsonString(): string { | ||
| return `\ | ||
| { | ||
| solidity: '${SOLIDITY_VERSION}', | ||
| resolc: { | ||
| compilerSource: 'npm', | ||
| }, | ||
| networks: { | ||
| hardhat: { | ||
| polkavm: true, | ||
| nodeConfig: { | ||
| nodeBinaryPath: 'INSERT_PATH_TO_SUBSTRATE_NODE', | ||
| rpcPort: 8000, | ||
| dev: true, | ||
| }, | ||
| adapterConfig: { | ||
| adapterBinaryPath: 'INSERT_PATH_TO_ETH_RPC_ADAPTER', | ||
| dev: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }`; | ||
| } |
There was a problem hiding this comment.
Placeholder values may cause confusion for users
The hardhat config contains placeholder paths 'INSERT_PATH_TO_SUBSTRATE_NODE' and 'INSERT_PATH_TO_ETH_RPC_ADAPTER' that users need to manually replace. This could lead to confusion if users try to run commands without first replacing these values.
Consider adding validation or more prominent warnings in the generated README to ensure users understand these must be replaced before use. You could also consider making these environment variables with clear error messages when not set.
ericglau
changed the title
This reverts commit 9fb1e1f.
Remaining issues (may be out of scope of Wizard):
npx hardhat compilefails with the following error, even though compiler version is set to 0.8.27. However, runningnpx hardhat compilea second time causes compilation to succeed.Note for code review: