[Stellar] Use constructor arguments in Stellar contracts #723
Conversation
i'm ignoring a couple warnings in a scaffold download test file, since i didn't touch anything related to those files
|
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 WalkthroughThe changes introduce a new Changes
Sequence DiagramsequenceDiagram
participant Builder as Contract Builder
participant GenCode as Code Generator
participant Print as Print Module
participant Deploy as Deploy Command
rect rgb(200, 220, 255)
Note over Builder,Deploy: New Constructor Argument Flow
Builder->>Builder: addConstructorArgument(arg: ConstructorArgument)
Builder->>GenCode: contract.constructorArgs: ConstructorArgument[]
GenCode->>Print: printConstructor(contract)
Print->>Deploy: printDeployCommand(contract.constructorArgs)
Deploy->>Print: deploy: string[] (CLI instructions)
Print->>Print: printFunction2(..., deploy)
Print->>Print: Inject deploy comments into constructor body
end
rect rgb(220, 255, 220)
Note over Builder,GenCode: Constructor Parameter Propagation
Builder->>Builder: addBase() adds name, symbol args
Builder->>Builder: addPremint() adds recipient, premint args
GenCode->>GenCode: mapConstructorArgs to Rust expressions
GenCode->>GenCode: Apply type-specific defaults (Address, String, i128)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Areas requiring extra attention:
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
All contributors have signed the CLA ✍️ ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/stellar/src/non-fungible.ts (1)
124-127: Consider parameterizing the URI value.The
uriconstructor argument uses a hard-coded placeholder value'www.mytoken.com', whilenameandsymbolare parameterized. For consistency and to fully realize the PR's objective of making values configurable at deployment, consider accepting auriparameter inaddBaseand passing it through from the options.Apply this diff to parameterize the URI:
-function addBase(c: ContractBuilder, name: string, symbol: string, pausable: boolean) { +function addBase(c: ContractBuilder, uri: string, name: string, symbol: string, pausable: boolean) { // Set metadata - c.addConstructorArgument({ name: 'uri', type: 'String', value: 'www.mytoken.com' }); + c.addConstructorArgument({ name: 'uri', type: 'String', value: uri }); c.addConstructorArgument({ name: 'name', type: 'String', value: name }); c.addConstructorArgument({ name: 'symbol', type: 'String', value: symbol }); c.addConstructorCode(`Base::set_metadata(e, uri, name, symbol);`);Then update the call at line 90:
- addBase(c, toByteArray(allOpts.name), toByteArray(allOpts.symbol), allOpts.pausable); + addBase(c, toByteArray('www.mytoken.com'), toByteArray(allOpts.name), toByteArray(allOpts.symbol), allOpts.pausable);packages/core/stellar/src/print.ts (1)
381-403: Consider shell escaping for constructor argument values.At line 394,
arg.valueis concatenated directly without shell escaping. Values containing spaces or special characters (like"Custom $ Token"shown in test snapshots at lines 726, 924, 1158) could produce invalid or unsafe shell commands if users copy-paste these comments.Consider wrapping values in quotes or applying proper shell escaping for robustness.
Example approach:
function printDeployCommand(args: ConstructorArgument[]): string[] { if (args.length === 0) { return []; } const cmd = []; cmd.push('// deploy this smart contract with the Stellar CLI:'); cmd.push('//'); cmd.push('// stellar contract deploy \\'); cmd.push('// --wasm path/to/file.wasm \\'); cmd.push('// -- \\'); args.map((arg, i) => { if (arg.value) { - let formattedArg = `// --${arg.name} ${arg.value}`; + // Quote values that contain spaces or special characters + const needsQuoting = /[\s"'$\\]/.test(arg.value); + const quotedValue = needsQuoting ? `"${arg.value.replace(/"/g, '\\"')}"` : arg.value; + let formattedArg = `// --${arg.name} ${quotedValue}`; if (i !== args.length - 1) { formattedArg += ' \\'; } cmd.push(formattedArg); } }); return cmd; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/core/stellar/src/fungible.test.ts.snapis excluded by!**/*.snappackages/core/stellar/src/non-fungible.test.ts.snapis excluded by!**/*.snappackages/core/stellar/src/stablecoin.test.ts.snapis excluded by!**/*.snappackages/core/stellar/src/zip-rust.compile.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
packages/core/stellar/README.md(1 hunks)packages/core/stellar/src/contract.ts(4 hunks)packages/core/stellar/src/fungible.test.ts.md(12 hunks)packages/core/stellar/src/fungible.ts(2 hunks)packages/core/stellar/src/non-fungible.test.ts.md(14 hunks)packages/core/stellar/src/non-fungible.ts(1 hunks)packages/core/stellar/src/print.ts(6 hunks)packages/core/stellar/src/set-access-control.ts(3 hunks)packages/core/stellar/src/stablecoin.test.ts.md(16 hunks)packages/core/stellar/src/zip-rust.compile.test.ts.md(2 hunks)packages/core/stellar/src/zip-shared.test.ts(3 hunks)packages/core/stellar/src/zip-shared.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T20:50:43.101Z
Learnt from: CoveMB
Repo: OpenZeppelin/contracts-wizard PR: 644
File: packages/ui/api/services/open-ai.ts:43-56
Timestamp: 2025-09-12T20:50:43.101Z
Learning: In the OpenZeppelin contracts-wizard project, the current streaming implementation in packages/ui/api/services/open-ai.ts intentionally handles text deltas and function_calls with different formats (raw text chunks vs JSON objects). While this can cause JSON.parse issues, harmonizing the response format is considered outside scope of dependency updates as it requires UI refactoring to handle unified streaming responses.
Applied to files:
packages/core/stellar/src/zip-shared.test.ts
🧬 Code graph analysis (2)
packages/core/stellar/src/zip-shared.test.ts (1)
packages/core/stellar/src/zip-shared.ts (1)
printRustNameTest(46-67)
packages/core/stellar/src/print.ts (2)
packages/core/stellar/src/utils/format-lines.ts (1)
Lines(1-1)packages/core/stellar/src/contract.ts (1)
ConstructorArgument(74-76)
⏰ 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). (1)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (30)
packages/core/stellar/README.md (1)
16-16: LGTM!The addition of "stablecoin" to the list of supported contract types is appropriate and aligns with the broader changes in the PR.
packages/core/stellar/src/set-access-control.ts (3)
35-35: LGTM!The addition of the
valuefield with a descriptive placeholder'<owner address>'provides helpful guidance for users during deployment.
53-53: LGTM!Consistent use of the
valuefield with placeholder'<admin address>'aligns with the ownable pattern.
96-96: LGTM!The dynamic placeholder
<${role} address>is a good pattern for role-based access control, making it clear what value should be provided.packages/core/stellar/src/zip-shared.test.ts (2)
137-137: LGTM!Typo correction from "bellow" to "below" improves code quality.
202-230: LGTM!The new test case for single String constructor argument provides good coverage for the expanded constructor argument handling. The test validates that String-type arguments are properly formatted in the generated test scaffolding.
packages/core/stellar/src/fungible.ts (2)
92-94: LGTM!The refactoring moves metadata initialization from hard-coded values to constructor arguments, enabling better contract reusability. The implementation correctly uses the parameters passed to
addBase.
203-205: LGTM!The premint handling now uses constructor arguments with appropriate placeholder values for
recipientand the calculatedpremintAbsoluteforpremint. The simplification of theBase::mintcall to use the constructor argument names improves readability.packages/core/stellar/src/zip-shared.ts (3)
11-13: LGTM!Helper function for formatting Rust String literals is well-scoped and reusable.
58-60: LGTM!The refactoring to use
getConstructorArgsinstead ofgetAddressArgsproperly handles all constructor argument types, not just Address types. This improves the test scaffolding generation.
23-44: <!-- [scratchpad_start]
[task verification_stellar_types 2/2 COMPLETE]
[observations]
- Argument type in Stellar (contract.ts line 69-72): { name: string; type?: string }
- ConstructorArgument extends Argument (line 74+)
- getConstructorArgs handles only: 'Address', 'String', 'i128'
- Test file shows getAddressArgs (different function) also silently filters unsupported types
- Actual Stellar usage: ONLY 'Address', 'String', 'i128' used across all Stellar contracts
- No instances of unsupported types found in Stellar codebase
- Default case in getConstructorArgs CAN theoretically return empty string if type not in switch
[analysis]
- Default case is reachable only if a type value outside {Address, String, i128} is used
- Currently: NO unsupported types in use → default case never hits
- Potentially: Risk if new Stellar types added in future (u64, bool, etc.)
- Codebase pattern: getAddressArgs also silently filters, suggesting established approach
- Current behavior: Works correctly for all actual use cases but lacks validation
[pending NONE - analysis complete]
[done]
- Confirmed no unsupported types currently used in Stellar
- Confirmed default case exists but not triggered in practice
- Review concern is theoretically valid but not actively causing issues
</scratchpad_end] -->No unsupported constructor argument types are currently used in Stellar contracts.
The review comment's concern is theoretical: all constructor arguments in the Stellar codebase use only 'Address', 'String', or 'i128' types, so the default case that returns an empty string (line 40) is never reached in practice.
However, the underlying point about defensive programming is valid. If a new Stellar type were introduced but not added to the switch statement, the empty string would silently be filtered out, potentially resulting in generated Rust code with fewer arguments than expected. This could be addressed by adding explicit error handling for unsupported types rather than relying on filtering empty strings.
The codebase already follows a similar pattern elsewhere (e.g.,
getAddressArgsalso silently filters unmatched types), suggesting this is an established approach rather than an oversight.packages/core/stellar/src/zip-rust.compile.test.ts.md (3)
25-33: LGTM!The addition of the deploy command comment in the generated constructor is valuable documentation that shows users exactly how to deploy the contract with the Stellar CLI, including all required arguments.
34-36: LGTM!The constructor signature correctly reflects the parameterization of metadata (name, symbol) and minting parameters (recipient, premint), enabling contract reusability as described in the PR objectives.
67-67: LGTM!The test setup correctly passes all four constructor arguments in the expected order, validating the new constructor signature.
packages/core/stellar/src/contract.ts (4)
74-76: LGTM!The new
ConstructorArgumentinterface extendsArgumentwith an optionalvaluefield, enabling constructor arguments to carry default or placeholder values. This is a clean, backward-compatible extension that supports the PR's objectives.
10-10: LGTM!Updating the
constructorArgstype fromArgument[]toConstructorArgument[]properly reflects the new capability while maintaining backward compatibility.
86-86: LGTM!The
ContractBuilderproperty type update is consistent with the interface change.
248-248: LGTM!The method signature correctly uses the new
ConstructorArgumenttype, enabling callers to pass arguments with optional values.packages/core/stellar/src/print.ts (3)
251-251: LGTM!Correctly passes
undefinedfor thedeployparameter when printing regular functions, since deployment commands are only relevant for constructors.
281-303: LGTM!The constructor printing logic correctly generates deployment commands via
printDeployCommandand passes them toprintFunction2for injection into the generated constructor.
307-326: LGTM!The signature expansion and deployment command injection logic is correct. Deploy commands are conditionally injected only for constructors, maintaining clean output for regular functions.
packages/core/stellar/src/non-fungible.test.ts.md (3)
7-43: LGTM!The basic non-fungible token test correctly demonstrates the updated constructor pattern with parameterized metadata (uri, name, symbol) and helpful deployment guidance comments.
91-165: LGTM!The pausable non-fungible token test correctly extends the base pattern to include ownership configuration. The deploy command appropriately uses a placeholder for the owner address since it's a deployment-time decision.
899-990: Test correctly validates complex name handling.This test demonstrates that the system handles token names with spaces and special characters. The deploy command comment at line 924 shows the name value without shell escaping, which relates to the escaping concern flagged in the
print.tsreview.packages/core/stellar/src/stablecoin.test.ts.md (3)
7-42: LGTM!The basic stablecoin test correctly demonstrates the parameterized constructor with name and symbol. The hardcoded 18 decimals at line 32 is appropriate for stablecoins.
256-294: LGTM!The preminted stablecoin test correctly demonstrates the constructor extension with recipient and premint parameters. The deploy command at line 281 appropriately shows the configured premint value.
862-996: LGTM!The comprehensive stablecoin test correctly demonstrates the most complex configuration with multiple roles. The constructor properly initializes metadata, minting, admin, and all role assignments using
grant_role_no_authat lines 910-912, which is appropriate for constructor-time initialization.packages/core/stellar/src/fungible.test.ts.md (3)
7-42: LGTM!The basic fungible token test correctly demonstrates the parameterized constructor pattern with name and symbol parameters.
296-331: LGTM - Smart optimization for zero premint.This test correctly demonstrates that when premint is zero, the constructor omits the recipient and premint parameters entirely. This keeps the constructor interface minimal and avoids unnecessary parameters.
574-698: LGTM - Comprehensive role-based configuration.This test correctly validates the most complex fungible token setup with multiple roles and upgradeability. The constructor signature at lines 608-618 is appropriately formatted across multiple lines for readability given the number of parameters.
|
I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement |
ElliotFriend
changed the title
…zard into feat/stellar-use-constructor
| return constructorArg.value | ||
| ? stringFromStr(constructorArg.value) | ||
| : constructorArg.name === 'name' | ||
| ? stringFromStr('MyToken') |
There was a problem hiding this comment.
In wich conditions would we want to return those hardcoded value?
Should we make it that only
return constructorArg.value
? stringFromStr(constructorArg.value)
is needed (maybe having default value upstream?
There was a problem hiding this comment.
The situation here is essentially: use value if provided and giving a default/placeholder symbol/name/other if it's not.
Having a default would probably be a good approach, though. Since the value in the constructor arg type is optional, I wasn't really sure how to give it a default. I can make it a required parameter, if that would be better?
3 participants
Stellar supports
__constructorfunctions in a smart contract, but most of the wizard contracts still hard-code the token name and symbol in the constructor function. this makes sharing the contract executables between multiple tokens more difficult, and will lead to more fees paid long-term.This PR aims to put all the hard-coded bits of these wizard contracts into the constructor arguments. It also provides a helpful deploy command in the outputted Rust code, instructing the user how they can deploy the contract with their configured name, symbol, etc. for all the constructor arguments required.
I started in on adding some tests to cover the new functionality, but wanted to get this PR out there for a sanity check first. I can add more tests, if it seems like i'm moving in the right direction.
If there's any changes that need to be made, I'm definitely happy to do so. Just let me know 😁