Cairo: decimals customization #654
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 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 📝 WalkthroughWalkthroughAdds configurable ERC20 decimals across core and alpha Cairo builders and blueprints, stores DECIMALS in a new ERC20ImmutableConfig trait, updates premint handling (getInitialSupply), propagates decimals through tests and APIs, and adds a Decimals input to the UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI (ERC20Controls)
participant Builder as buildERC20
participant AddBase as addBase
participant Comp as ERC20Component
participant Cfg as ERC20ImmutableConfig
participant Artifact as Contract Artifact
User->>UI: enter name, symbol, decimals
UI->>Builder: ERC20Options { name, symbol, decimals }
Builder->>Builder: toUint(decimals, 'u8')
Builder->>AddBase: name, symbol, decimals(u8), premint?
AddBase->>Comp: add component (name, symbol)
AddBase->>Cfg: define ImmutableConfig { DECIMALS = u8 }
AddBase->>Artifact: finalize module with component + config + premint minting
Builder-->>UI: generated contract artifact
sequenceDiagram
autonumber
participant Test as Test Suite
participant Builder as buildERC20
participant AddBase as addBase
participant Cfg as ERC20ImmutableConfig
Test->>Builder: opts.decimals = '6'
Builder->>Builder: toUint('6','u8') => 6
Builder->>AddBase: decimals = 6
AddBase->>Cfg: DECIMALS = 6
Test->>Builder: opts.decimals = '256'
Builder->>Builder: toUint('256','u8') => error
Builder-->>Test: OptionsError ("Value is greater than u8 max value")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Comment |
Reviewer's GuideThis PR introduces customizable decimals for ERC20 tokens by extending the builder API, templates, tests, UI, and generation scripts to accept and propagate a decimals parameter (u8) via the ERC20ImmutableConfig trait instead of the previous DefaultConfig. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/core/cairo/src/erc20.ts:75` </location>
<code_context>
const allOpts = withDefaults(opts);
- addBase(c, toByteArray(allOpts.name), toByteArray(allOpts.symbol));
+ const decimals = toUint(opts.decimals, 'decimals', 'u8');
+ addBase(c, toByteArray(allOpts.name), toByteArray(allOpts.symbol), decimals);
+
</code_context>
<issue_to_address>
Potential inconsistency: using opts.decimals instead of allOpts.decimals.
Using opts.decimals instead of allOpts.decimals may cause issues if decimals is not provided, since other options use allOpts. For consistency and to avoid unexpected behavior, use allOpts.decimals.
</issue_to_address>
### Comment 2
<location> `packages/core/cairo_alpha/src/erc20.ts:75` </location>
<code_context>
const allOpts = withDefaults(opts);
- addBase(c, toByteArray(allOpts.name), toByteArray(allOpts.symbol));
+ const decimals = toUint(opts.decimals, 'decimals', 'u8');
+ addBase(c, toByteArray(allOpts.name), toByteArray(allOpts.symbol), decimals);
+
</code_context>
<issue_to_address>
Potential inconsistency: using opts.decimals instead of allOpts.decimals.
Using opts.decimals instead of allOpts.decimals may cause issues if decimals is not provided. For consistency and to ensure defaults are applied, use allOpts.decimals.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/cairo/src/erc20.test.ts (1)
10-20: Add missing defaultdecimals: '18'in ERC20Options defaults
In bothpackages/core/cairo/src/erc20.tsandpackages/core/cairo_alpha/src/erc20.ts, includedecimals: '18'in thewithDefaults(opts)defaults object to prevent drift between tests, UI, and generator.packages/core/cairo_alpha/src/erc20.test.ts (1)
197-221: Add a regression test to ensure premint respects custom decimals.
Current builder hardcodes 18 for premint; add a test that would fail if that bug reappears, and assert on the printed mint amount.test('erc20 getInitialSupply', async t => { t.is(getInitialSupply('1000', 18), '1000000000000000000000'); t.is(getInitialSupply('1000.1', 18), '1000100000000000000000'); t.is(getInitialSupply('.1', 18), '100000000000000000'); t.is(getInitialSupply('.01', 2), '1'); let error = t.throws(() => getInitialSupply('.01', 1)); t.not(error, undefined); t.is((error as OptionsError).messages.premint, 'Too many decimals'); error = t.throws(() => getInitialSupply('1.1.1', 18)); t.not(error, undefined); t.is((error as OptionsError).messages.premint, 'Not a valid number'); }); + +test('erc20 premint uses custom decimals', t => { + const c = buildERC20({ + name: 'MyToken', + symbol: 'MTK', + decimals: '6', + premint: '1.5', + }); + const printed = printContract(c); + t.true(printed.includes('self.erc20.mint(recipient, 1500000)')); +});
🧹 Nitpick comments (14)
packages/ui/src/cairo/ERC20Controls.svelte (1)
40-46: Prefer numeric input hints for better UX (keep value as string).
Add inputmode and a placeholder to guide valid u8 input without changing bound type.- <input bind:value={opts.decimals} use:error={errors?.decimals} /> + <input + inputmode="numeric" + placeholder="18" + bind:value={opts.decimals} + use:error={errors?.decimals} + />packages/core/cairo/src/erc20.ts (1)
52-64: Minor: ensure decimals is defaulted in withDefaults for consistency.
Not strictly needed (decimals is required), but it future-proofs the function and makes the “Required” return contract truthful.function withDefaults(opts: ERC20Options): Required<ERC20Options> { return { ...opts, ...withCommonContractDefaults(opts), + decimals: opts.decimals ?? defaults.decimals, burnable: opts.burnable ?? defaults.burnable, pausable: opts.pausable ?? defaults.pausable, premint: opts.premint || defaults.premint, mintable: opts.mintable ?? defaults.mintable, votes: opts.votes ?? defaults.votes, appName: opts.appName ?? defaults.appName, appVersion: opts.appVersion ?? defaults.appVersion, }; }packages/core/cairo/src/erc20.test.ts (3)
74-77: Add a premint + custom decimals snapshot to exercise supply scalingConsider adding a case that premints with non-18 decimals to assert the minted base units reflect DECIMALS.
Apply this diff near the other premint/custom tests:
+testERC20('erc20 preminted, custom decimals', { + decimals: '6', + premint: '1', +});
78-87: Edge tests for decimals boundariesNice upper-bound test. Please add 0 and 255 boundary checks, plus a non-numeric rejection, to lock behavior.
Add:
+test('erc20 votes, decimals at u8 bounds', t => { + t.notThrows(() => + buildERC20({ name: 'MyToken', symbol: 'MTK', decimals: '0' }), + ); + t.notThrows(() => + buildERC20({ name: 'MyToken', symbol: 'MTK', decimals: '255' }), + ); +}); + +test('erc20 votes, decimals not a number', t => { + const error = t.throws(() => + buildERC20({ name: 'MyToken', symbol: 'MTK', decimals: '18.0' }), + ); + t.is((error as OptionsError).messages.decimals, 'Not a valid number'); +});
197-220: Consider rounding/precision edge cases for getInitialSupplyCoverage looks good; optionally add a case with custom decimals (e.g., getInitialSupply('1', 6) -> '1000000') to tie decimals to supply math.
packages/core/cairo_alpha/src/generate/erc20.ts (1)
12-12: Broaden blueprint coverage for decimalsConsider including a common alternative like '6' to exercise generation paths beyond the default.
Apply:
- decimals: ['18'], + decimals: ['18', '6'],packages/ui/src/cairo_alpha/ERC20Controls.svelte (1)
40-46: Harden the Decimals input UX and constraintsUse a numeric input with bounds to reduce invalid entries and hint defaults.
Apply:
- <label class="labeled-input"> + <label class="labeled-input"> <span class="flex justify-between pr-2"> Decimals - <HelpTooltip>The number of decimals the token uses.</HelpTooltip> + <HelpTooltip link="https://docs.openzeppelin.com/contracts-cairo/tokens#decimals"> + The number of decimals the token uses (u8: 0–255). Common values: 18, 6. + </HelpTooltip> </span> - <input bind:value={opts.decimals} use:error={errors?.decimals} /> + <input + type="number" + min="0" + max="255" + step="1" + inputmode="numeric" + placeholder="18" + bind:value={opts.decimals} + use:error={errors?.decimals} + /> </label>packages/core/cairo_alpha/src/erc20.test.ts (4)
74-77: Rename test: title says “votes” but votes aren’t enabled.
This is misleading; either enable votes (and provide appName/appVersion) or rename the test.-testERC20('erc20 votes, custom decimals', { +testERC20('erc20 custom decimals', { decimals: '6', });
78-87: Drop unnecessary async and tighten the assertion type.
No awaits here. Also cast the thrown error once for clarity.-test('erc20 votes, decimals too high', async t => { - const error = t.throws(() => +test('erc20 decimals too high', t => { + const error = t.throws(() => buildERC20({ name: 'MyToken', symbol: 'MTK', decimals: '256', }), ); - t.is((error as OptionsError).messages.decimals, 'Value is greater than u8 max value'); + const e = error as OptionsError; + t.is(e.messages.decimals, 'Value is greater than u8 max value'); });
110-120: Drop unnecessary async.
Function is synchronous.-test('erc20 votes, no name', async t => { +test('erc20 votes, no name', t => {
122-134: Drop unnecessary async.
Function is synchronous.-test('erc20 votes, empty version', async t => { +test('erc20 votes, empty version', t => {packages/core/cairo_alpha/src/erc20.ts (3)
212-214: Optional: tighten premintPattern to reject bare '.' inputs.
Current regex allows '.' which normalizes to 0 silently. Consider requiring digits on at least one side.-export const premintPattern = /^(\d*\.?\d*)$/; +export const premintPattern = /^(?:\d+|\d*\.\d+)$/;
75-77: Nit: prefer parsing from allOpts for consistency.
Functionally identical here, but using allOpts mirrors the rest of the builder.- const decimalsU8 = toUint(opts.decimals, 'decimals', 'u8'); + const decimalsU8 = toUint(allOpts.decimals, 'decimals', 'u8');
137-149: Validation messages look good; consider trimming whitespace before empty checks.
If UI ever allows whitespace-only strings, trim before testing emptiness.- if (!allOpts.appName) { + if (!allOpts.appName?.trim()) { @@ - if (!allOpts.appVersion) { + if (!allOpts.appVersion?.trim()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
packages/core/cairo/src/erc20.test.ts.snapis excluded by!**/*.snappackages/core/cairo_alpha/src/erc20.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/core/cairo/src/erc20.test.ts(6 hunks)packages/core/cairo/src/erc20.test.ts.md(32 hunks)packages/core/cairo/src/erc20.ts(5 hunks)packages/core/cairo/src/generate/erc20.ts(1 hunks)packages/core/cairo_alpha/src/erc20.test.ts(6 hunks)packages/core/cairo_alpha/src/erc20.test.ts.md(32 hunks)packages/core/cairo_alpha/src/erc20.ts(5 hunks)packages/core/cairo_alpha/src/generate/erc20.ts(1 hunks)packages/ui/src/cairo/ERC20Controls.svelte(1 hunks)packages/ui/src/cairo_alpha/ERC20Controls.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T15:18:09.410Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#638
File: packages/core/cairo_alpha/src/scripts/update-scarb-project.ts:62-79
Timestamp: 2025-08-19T15:18:09.410Z
Learning: In the cairo_alpha package (packages/core/cairo_alpha), the OpenZeppelin dependency in test_project/Scarb.toml is expected to be in { git, tag } form rather than a simple registry-style string, so the update script only needs to update the tag value, not convert between formats.
Applied to files:
packages/core/cairo_alpha/src/erc20.test.ts.md
🧬 Code graph analysis (2)
packages/core/cairo_alpha/src/erc20.test.ts (2)
packages/core/cairo_alpha/src/erc20.ts (1)
buildERC20(70-102)packages/core/cairo_alpha/src/error.ts (1)
OptionsError(3-7)
packages/core/cairo_alpha/src/erc20.ts (2)
packages/core/cairo_alpha/src/utils/convert-strings.ts (2)
toUint(87-102)toByteArray(35-41)packages/core/cairo_alpha/src/contract.ts (3)
ContractBuilder(101-308)components(124-126)BaseImplementedTrait(57-68)
⏰ 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). (3)
- GitHub Check: validate-cairo-alpha
- GitHub Check: validate-cairo
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (20)
packages/core/cairo/src/generate/erc20.ts (1)
12-12: LGTM: blueprint now includes decimals default.
Matches the new ERC20Options and keeps the generator consistent.packages/core/cairo/src/erc20.ts (4)
1-1: Import looks correct.
BaseImplementedTraitis used below for the immutable config trait.
22-22: LGTM: defaults include decimals '18'.
Aligns with blueprint/UI defaults.
42-42: LGTM: ERC20Options adds decimals:string.
Matches conversion via toUint('u8').
190-206: Confirm ImmutableConfig wiring matches upstream ERC20 expectations.
Trait name and super-variable align with ‘ERC20Component::ImmutableConfig’ and ‘DECIMALS: u8’? If upstream changed identifiers, this would silently break.packages/core/cairo/src/erc20.test.ts (3)
15-16: Good defaulting of decimals in testsSetting decimals: '18' before spreading opts ensures snapshots include decimals and still allow overrides. Order is correct.
Also applies to: 33-34
110-116: Explicit decimals in error-path testsIncluding decimals in these builds keeps snapshots stable across default changes.
Also applies to: 122-128
180-186: API equivalence covers custom decimalsGood coverage ensuring public and internal APIs print identically with decimals: '6'.
packages/core/cairo/src/erc20.test.ts.md (2)
45-47: Snapshots correctly reflect ImmutableConfig::DECIMALSAll variants now declare ERC20ImmutableConfig with DECIMALS; this matches the new builder behavior.
Also applies to: 108-110, 183-185, 288-290, 419-421, 531-533, 629-631, 704-706, 779-781, 854-856, 958-960, 1073-1075, 1192-1194, 1293-1295, 1413-1415, 1568-1570, 1757-1759
16-16: Import consistency for ERC20 hooksImports include ERC20HooksEmptyImpl only where no custom ERC20HooksImpl is provided; where hooks are implemented, ERC20Component alone is imported. Looks consistent with intent.
Also applies to: 61-61, 136-136, 221-221, 337-337, 464-464, 580-580, 657-657, 731-733, 807-807, 897-897, 997-997, 1116-1116, 1234-1234, 1326-1326, 1472-1472, 1645-1645
packages/core/cairo_alpha/src/erc20.test.ts.md (2)
45-47: Alpha snapshots: ImmutableConfig::DECIMALS presentAlpha variants mirror core with DECIMALS config; placement and values are correct, including the custom 6-decimals case.
Also applies to: 108-110, 183-185, 288-290, 419-421, 531-533, 629-631, 704-706, 779-781, 854-856, 958-960, 1074-1076, 1192-1196, 1296-1298, 1417-1419, 1573-1575, 1763-1765
16-16: Alpha imports: correct usage of ERC20HooksEmptyImpl and IUpgradeable pathImport patterns align with whether hooks are implemented, and the alpha IUpgradeable path (interfaces::upgrades) is consistent across snapshots.
Also applies to: 62-62, 137-137, 222-223, 338-338, 465-465, 581-581, 658-658, 734-734, 808-808, 898-898, 998-998, 1119-1119, 1237-1237, 1329-1329, 1478-1478, 1650-1650
packages/core/cairo_alpha/src/generate/erc20.ts (1)
25-27: Generator signature and yield remain soundYielding Required from generateAlternatives with the new decimals field is correct.
packages/ui/src/cairo_alpha/ERC20Controls.svelte (1)
13-23: Defaults includedecimalsand are correctly spread into UI opts
The coreerc20.defaultsdefinesdecimals: '18', and the Svelte component spreads...erc20.defaultsinto itsopts, so the decimals control will initialize predictably.packages/core/cairo_alpha/src/erc20.test.ts (3)
15-16: Good: defaulting tests to 18 decimals keeps internal/external API parity stable.
33-34: Good: API-equivalence path explicitly includes decimals, avoiding accidental default drift.
180-186: Good: API-equivalence covers custom decimals.packages/core/cairo_alpha/src/erc20.ts (3)
22-22: Default decimals declared in public defaults is correct and explicit.
189-205: Nice: immutable DECIMALS via ERC20ImmutableConfig trait is clean and future-proof.
Good separation from constructor params; avoids storage writes.
42-42: Requiredecimalsin allbuildERC20calls
This is a breaking API change:ERC20Options.decimalsis now mandatory. Update everybuildERC20({ … })invocation—including test suites inpackages/core/solidity,stylus,cairo, andcairo_alpha—to pass adecimalsfield. Verify production code and UI wiring supply this value.
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/cairo_alpha/src/erc20.ts (1)
52-64: Missing decimals fallback in withDefaults can throw at runtime.If a caller omits
decimals,toUint(allOpts.decimals, …)will receiveundefinedand crash. Add a fallback here so callers that rely on defaults don’t break.Apply this diff:
function withDefaults(opts: ERC20Options): Required<ERC20Options> { return { ...opts, ...withCommonContractDefaults(opts), + decimals: opts.decimals ?? defaults.decimals, burnable: opts.burnable ?? defaults.burnable, pausable: opts.pausable ?? defaults.pausable, premint: opts.premint || defaults.premint, mintable: opts.mintable ?? defaults.mintable, votes: opts.votes ?? defaults.votes, appName: opts.appName ?? defaults.appName, appVersion: opts.appVersion ?? defaults.appVersion, }; }packages/core/cairo/src/erc20.ts (1)
213-229: Tightenpremintvalidation and avoid minting zero.Current regex accepts "." (no digits). That leads to generating a constructor mint of 0. Tighten the pattern and skip codegen when normalized supply is zero.
-export const premintPattern = /^(\d*\.?\d*)$/; +export const premintPattern = /^(?:\d+(?:\.\d+)?|\.\d+)$/;-function addPremint(c: ContractBuilder, amount: string, decimals: bigint) { +function addPremint(c: ContractBuilder, amount: string, decimals: bigint) { if (amount !== undefined && amount !== '0') { if (!premintPattern.test(amount)) { throw new OptionsError({ premint: 'Not a valid number', }); } - const premintAbsolute = toUint(getInitialSupply(amount, Number(decimals)), 'premint', 'u256'); + const normalized = getInitialSupply(amount, Number(decimals)); + if (normalized === '0') { + return; + } + const premintAbsolute = toUint(normalized, 'premint', 'u256'); c.addUseClause('starknet', 'ContractAddress'); c.addConstructorArgument({ name: 'recipient', type: 'ContractAddress' }); c.addConstructorCode(`self.erc20.mint(recipient, ${premintAbsolute})`); } }
♻️ Duplicate comments (1)
packages/core/cairo/src/erc20.ts (1)
75-77: Resolved prior concern: usingallOpts.decimalsand threading parsed u8.This addresses the earlier review about bypassing defaults and hardcoding 18.
🧹 Nitpick comments (8)
packages/common/CHANGELOG.md (1)
4-6: Wording nit: “schema” instead of “scheme”Prefer “schema” to align with MCP schemas and internal naming.
- - Cairo: Add decimals to ERC20 scheme ([#654](https://github.com/OpenZeppelin/contracts-wizard/pull/654)) + - Cairo: Add decimals to ERC20 schema ([#654](https://github.com/OpenZeppelin/contracts-wizard/pull/654))packages/mcp/CHANGELOG.md (1)
3-4: Tighten phrasing and unify ERC20 spellingRemove “value”, use “schema”, and prefer “ERC20” (no hyphen) for consistency with other packages.
- - Cairo: Support decimals value added to ERC-20 scheme ([#654](https://github.com/OpenZeppelin/contracts-wizard/pull/654)) + - Cairo: Add decimals to ERC20 schema ([#654](https://github.com/OpenZeppelin/contracts-wizard/pull/654))packages/common/src/ai/descriptions/cairo.ts (1)
39-40: Clarify constraints/default for decimalsOptional: Mention u8 range and default to help users/tools validate inputs.
- decimals: 'The number of decimals to use for the contract.', + decimals: 'The number of decimals to use for the contract (0-255). Default is 18.',packages/ui/src/cairo/ERC20Controls.svelte (1)
40-46: Harden Decimals input: restrict to numeric/u8 and improve UX.Constrain the field client-side to avoid accidental non-numeric input and reflect the backend’s u8 bounds.
Apply this diff:
- <input bind:value={opts.decimals} use:error={errors?.decimals} /> + <input + bind:value={opts.decimals} + use:error={errors?.decimals} + inputmode="numeric" + pattern="[0-9]{1,3}" + placeholder="18" + />packages/core/cairo_alpha/src/erc20.ts (2)
212-224: Tighten premint regex to reject “.” (dot-only) inputs.
/^(\d*\.?\d*)$/accepts a bare dot which later normalizes to zero. Prefer requiring at least one digit while still allowing “.5”.Apply this diff:
-export const premintPattern = /^(\d*\.?\d*)$/; +export const premintPattern = /^(?:\d+(?:\.\d*)?|\.\d+)$/;
238-269: Consider deduping getInitialSupply with core package.Implementation mirrors the core Cairo version; extract to a shared util to avoid divergence.
packages/mcp/src/cairo/tools/erc20.test.ts (1)
42-63: Add coverage for non-18 and boundary decimals.Recommend adding cases for decimals '6', '0', and '255', plus a negative/overflow rejection to mirror core tests.
I can add MCP tests for:
- decimals: '6', premint: '1.23'
- decimals: '0', premint: '100'
- decimals: '255' (no premint)
- invalid: decimals: '256' and '-1'
packages/core/cairo/src/erc20.ts (1)
52-64: Consistency: includedecimalsinwithDefaults.While
decimalsis non-optional in the interface, adding it here aligns with the pattern used for other fields and hardens against runtime callers.Example change (outside the current diff hunk):
function withDefaults(opts: ERC20Options): Required<ERC20Options> { return { ...opts, ...withCommonContractDefaults(opts), decimals: opts.decimals ?? defaults.decimals, burnable: opts.burnable ?? defaults.burnable, pausable: opts.pausable ?? defaults.pausable, premint: opts.premint || defaults.premint, mintable: opts.mintable ?? defaults.mintable, votes: opts.votes ?? defaults.votes, appName: opts.appName ?? defaults.appName, appVersion: opts.appVersion ?? defaults.appVersion, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
.changeset/swift-eels-repair.md(1 hunks)packages/common/CHANGELOG.md(1 hunks)packages/common/package.json(1 hunks)packages/common/src/ai/descriptions/cairo.ts(1 hunks)packages/core/cairo/CHANGELOG.md(1 hunks)packages/core/cairo/package.json(1 hunks)packages/core/cairo/src/erc20.ts(6 hunks)packages/core/cairo_alpha/CHANGELOG.md(1 hunks)packages/core/cairo_alpha/src/erc20.ts(6 hunks)packages/mcp/CHANGELOG.md(1 hunks)packages/mcp/package.json(1 hunks)packages/mcp/src/cairo/schemas.ts(1 hunks)packages/mcp/src/cairo/tools/erc20.test.ts(2 hunks)packages/mcp/src/cairo/tools/erc20.ts(2 hunks)packages/ui/src/cairo/ERC20Controls.svelte(1 hunks)packages/ui/src/cairo_alpha/ERC20Controls.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/mcp/package.json
- packages/core/cairo_alpha/CHANGELOG.md
- .changeset/swift-eels-repair.md
- packages/core/cairo/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/cairo_alpha/ERC20Controls.svelte
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-19T15:31:24.984Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#0
File: :0-0
Timestamp: 2025-08-19T15:31:24.984Z
Learning: Changes within packages/core/cairo_alpha should have a corresponding changelog entry in packages/core/cairo_alpha/CHANGELOG.md under the Unreleased section to track these changes. When cairo_alpha is eventually promoted to cairo (stable), these entries will be moved into a changeset for cairo (stable).
Applied to files:
packages/core/cairo/CHANGELOG.mdpackages/common/CHANGELOG.mdpackages/mcp/CHANGELOG.md
📚 Learning: 2025-08-15T22:49:25.653Z
Learnt from: ernestognw
PR: OpenZeppelin/contracts-wizard#609
File: .changeset/sour-hats-grow.md:2-6
Timestamp: 2025-08-15T22:49:25.653Z
Learning: In OpenZeppelin contracts-wizard, breaking changes that have concrete migration paths (like dependency migrations from Community Contracts to OpenZeppelin Contracts) can be handled as minor version bumps instead of major bumps, per maintainer ernestognw's versioning policy.
Applied to files:
packages/core/cairo/CHANGELOG.mdpackages/common/package.jsonpackages/common/CHANGELOG.md
📚 Learning: 2025-08-19T15:21:06.991Z
Learnt from: ericglau
PR: OpenZeppelin/contracts-wizard#638
File: packages/core/cairo_alpha/src/account.test.ts.md:18-18
Timestamp: 2025-08-19T15:21:06.991Z
Learning: In the OpenZeppelin contracts-wizard repository, the cairo_alpha package (packages/core/cairo_alpha) and the stable cairo package (packages/core/cairo) are separate implementations that do not need to have the same code or matching dependency versions. The cairo_alpha package targets dependencies that are either newer than or the same as the stable cairo package, allowing it to test and support newer Cairo/Scarb/OpenZeppelin versions while the stable version maintains compatibility with stable releases.
Applied to files:
packages/common/CHANGELOG.mdpackages/mcp/CHANGELOG.md
🧬 Code graph analysis (3)
packages/mcp/src/cairo/schemas.ts (1)
packages/common/src/ai/descriptions/cairo.ts (1)
cairoERC20Descriptions(37-42)
packages/core/cairo_alpha/src/erc20.ts (4)
packages/core/cairo_alpha/src/utils/convert-strings.ts (2)
toUint(87-102)toByteArray(35-41)packages/core/cairo_alpha/src/contract.ts (3)
ContractBuilder(101-308)components(124-126)BaseImplementedTrait(57-68)packages/core/cairo/src/erc20.ts (1)
getInitialSupply(239-270)packages/core/cairo_alpha/src/error.ts (1)
OptionsError(3-7)
packages/core/cairo/src/erc20.ts (1)
packages/core/cairo_alpha/src/erc20.ts (1)
getInitialSupply(238-269)
🪛 LanguageTool
packages/common/CHANGELOG.md
[grammar] ~4-~4: There might be a mistake here.
Context: # Changelog ## 0.1.1 (2025-09-02) - Cairo: Add decimals to ERC20 scheme ([#6...
(QB_NEW_EN)
packages/mcp/CHANGELOG.md
[grammar] ~3-~3: There might be a mistake here.
Context: # Changelog ## 0.1.1 (2025-09-02) - Cairo: Support decimals value added to E...
(QB_NEW_EN)
⏰ 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). (4)
- GitHub Check: build (solidity, default)
- GitHub Check: validate-cairo-alpha
- GitHub Check: validate-cairo
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (15)
packages/common/package.json (1)
3-3: Version bump looks correctMatches new 0.1.1 changelog entry. No concerns.
packages/core/cairo/CHANGELOG.md (2)
3-6: LGTM on entry and versioning2.1.0 for ERC20 decimals customization aligns with our minor bump policy.
3-6: No changes needed; alpha changelog already updated
Thepackages/core/cairo_alpha/CHANGELOG.mdUnreleased section already includes “Support ERC20 decimals customization” under line 5, matching the stable release note.packages/core/cairo_alpha/src/erc20.ts (4)
1-1: Import looks correct.Needed for the new immutable-config injection helpers.
19-33: Defaults include decimals = '18' — ensure withDefaults also applies it.You added a default, but
withDefaultsdoesn’t currently propagate it (see separate comment with a patch).
75-81: Good: decimals parsed as u8 and threaded through base/premint.This addresses prior “hardcoded 18” issues.
189-205: Trait path confirmed
ERC20Component::ImmutableConfigwithERC20ImmutableConfigandDECIMALS: u8is used consistently in core code and snapshot tests; no changes needed.packages/mcp/src/cairo/tools/erc20.test.ts (1)
33-39: LGTM: tests now pass decimals through the tool.Covers the basic path with explicit decimals.
packages/mcp/src/cairo/schemas.ts (1)
50-50: Makedecimalsoptional and validate as u8- decimals: z.string().describe(cairoERC20Descriptions.decimals), + decimals: z + .string() + .regex(/^\d+$/, 'Must be an integer string') + .refine(v => { + const n = Number(v); + return Number.isInteger(n) && n >= 0 && n <= 255; + }, 'Must be between 0 and 255') + .optional() + .describe(cairoERC20Descriptions.decimals),If you intend to keep
decimalsrequired, confirm that CLI defaults and core/cairo generators handle it and document this breaking change.packages/mcp/src/cairo/tools/erc20.ts (1)
16-16: The directorypackages/mcp/src/cairo/tools/erc20.tsimportserc20Schemafrom../schemas, but there is noschemasdirectory underpackages/mcp/src/cairo. Please verify the correct location or filename oferc20Schemabefore applying the suggested default fordecimals.packages/core/cairo/src/erc20.ts (5)
22-22: LGTM: sensible default.Defaulting ERC20 decimals to '18' matches ecosystem expectations.
42-42: LGTM: explicitdecimalsoption on the public interface.Clear API signal and aligns with defaults.
80-82: LGTM: premint computed with configured decimals.Passing the parsed
decimalsfixes the mis-minting bug when decimals ≠ 18.
190-206: LGTM: DECIMALS via immutable config trait.Straightforward and avoids relying on removed DefaultConfig.
239-270: LGTM with a minor edge-case note.
getInitialSupplyguards precision and pathological.repeatsizes; works with u8-bound decimals.Optional: add tests for inputs like ".", "0.", ".0", and "000.000" to assert normalization to "0" and ensure zero-mint is skipped after the above change.
Hey @ericglau! Could you take a look at this alert? |
|
@SocketSecurity ignore npm/undici@6.21.3 |
|
@SocketSecurity ignore npm/prettier@3.6.2 |
ericglau
left a comment
ericglau
left a comment
There was a problem hiding this comment.
LGTM, thanks.
I've pushed a few further changes:
- I reverted the changelog/package.json changes, since the changeset will cause those to be updated during release. Also removed it from alpha's changelog, since that will eventually be merged to stable (which both have these same changes).
- Changed
decimalsto be optional in the MCP server.
| name: ['MyToken'], | ||
| symbol: ['MTK'], | ||
| decimals: ['6', '18'], | ||
| decimals: ['18'], |
There was a problem hiding this comment.
I'm curious why the '6' case was removed here for cairo stable, but it is still being tested for cairo alpha.
There was a problem hiding this comment.
Cairo compiler is not very efficient and one of the main problems we faces when testing the generated test project is exponential compilation time. That's why we had to split test project generation and compilation instead of compiling altogether.
The recent changes in the 2.12 version of Cairo have seriously improved compilation time. As a result ERC20 test project of ~400 contracts (when 6 and 18 used for decimals) compiles fine with 2.12.x and fails due to timeout with 2.11.4. Keeping only the decimals value of 18 reduces the number of contract by 2 times for the compilation to succeed
3 participants
Summary by Sourcery
Enable customization of token decimals by adding a new decimals parameter everywhere (builder, printer, generator, tests, and UI), defaulting to 18, and enforce u8 bounds in tests.
New Features:
Enhancements:
Tests: