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)
Comment |
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
![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.
ericglau
changed the title
| const ignitionModule = (c: Contract) => { | ||
| const deployArguments = getAddressArgs(c); | ||
| const contractVariableName = lowerFirstCharacter(c.name); | ||
| private lowerFirstCharacter(str: string): string { |
There was a problem hiding this comment.
nit: maybe could be moved to utils
There was a problem hiding this comment.
this is just a one-liner that is called from one place, so it seems fine to leave it
| omitFeatures: defineOmitFeatures(), | ||
| omitZipFoundry: true, | ||
| omitZipHardhat: true, // Disabled until Polkadot-specific config is added for the download package | ||
| omitZipHardhat: (opts?: GenericOptions) => { |
There was a problem hiding this comment.
Wondering if (now or later) it could be a good idea to extract download logic in their own components to keep App.svelte leaner
There was a problem hiding this comment.
Which parts are you referring to? The download logic in App.svelte seems small, as the logic is mostly in the zip-hardhat-*.ts files.
2 participants
Note for code review: