[Stellar] security contact as contract metadata #679
Conversation
Co-authored-by: Eric Lau <ericglau@outlook.com>
|
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 WalkthroughReplaces the security contact tag with a generalized contract metadata system across Stellar core, printing, and info-setting. Updates interfaces, builder APIs, and rendering to emit contractmeta!(key="...", val="...") entries. Tests and snapshots are adjusted accordingly. UI tooltip text is updated. Adds a patch changeset for @openzeppelin/wizard-stellar. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI (Wizard)
participant Core as setInfo(…)
participant Builder as ContractBuilder
participant Printer as printContract
User->>UI: Enter security contact (email/URL)
UI->>Core: setInfo(builder, { securityContact, license })
Core->>Builder: addContractMetadata({ key: "contact", value: securityContact })
Core->>Builder: license = license (optional)
note over Builder: Builder.metadata updated (Map<key,value>)
UI->>Printer: printContract(contract)
Printer->>Printer: printMetadata(contract.metadata)
Printer-->>UI: contract source with contractmeta!(key="contact", val="...")
note right of Printer: No securityContact tag path\nMetadata drives emission
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (2)
packages/core/stellar/src/set-info.ts (1)
12-16: Consider a more explicit metadata key and trim inputKey 'contact' is generic. 'security-contact' reads clearer and avoids collisions; trimming prevents empty/whitespace-only entries.
Apply this diff:
-export function setInfo(c: ContractBuilder, { securityContact, license }: Info): void { - if (securityContact) c.addContractMetadata({ key: 'contact', value: securityContact }); +export function setInfo(c: ContractBuilder, { securityContact, license }: Info): void { + const sc = securityContact?.trim(); + if (sc) c.addContractMetadata({ key: 'security-contact', value: sc });If you adopt this, update tests/snapshots and any UI copy that shows the key.
packages/core/stellar/src/contract.ts (1)
270-274: Harden addContractMetadata against empty keys/valuesGuard against empty keys and normalize values to reduce surprises.
Apply this diff:
- addContractMetadata(metadata: { key: string; value: string }[] | { key: string; value: string }) { - (Array.isArray(metadata) ? metadata : [metadata]).forEach(({ key, value }) => this.metadata.set(key, value)); - - if (this.metadata.size > 0) this.addUseClause('soroban_sdk', 'contractmeta'); - } + addContractMetadata(metadata: { key: string; value: string }[] | { key: string; value: string }) { + const items = Array.isArray(metadata) ? metadata : [metadata]; + for (const { key, value } of items) { + const k = key?.trim(); + if (!k) continue; + this.metadata.set(k, (value ?? '').toString()); + } + if (this.metadata.size > 0) this.addUseClause('soroban_sdk', 'contractmeta'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/core/stellar/src/contract.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
.changeset/silent-bags-repair.md(1 hunks)packages/core/stellar/src/contract.test.ts(1 hunks)packages/core/stellar/src/contract.test.ts.md(1 hunks)packages/core/stellar/src/contract.ts(3 hunks)packages/core/stellar/src/fungible.compile.test.ts(1 hunks)packages/core/stellar/src/print.ts(2 hunks)packages/core/stellar/src/set-info.ts(1 hunks)packages/ui/src/stellar/InfoSection.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
.changeset/silent-bags-repair.md
🧬 Code graph analysis (4)
packages/core/stellar/src/contract.test.ts (2)
packages/core/stellar/src/contract.ts (1)
ContractBuilder(74-275)packages/core/stellar/src/print.ts (1)
printContract(20-40)
packages/core/stellar/src/fungible.compile.test.ts (2)
packages/core/stellar/src/utils/compile-test.ts (1)
runRustCompilationTest(40-69)packages/core/stellar/src/fungible.ts (1)
buildFungible(57-88)
packages/core/stellar/src/set-info.ts (1)
packages/core/stellar/src/contract.ts (1)
ContractBuilder(74-275)
packages/core/stellar/src/print.ts (1)
packages/core/stellar/src/contract.ts (1)
Contract(3-17)
⏰ 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: build (solidity, default)
- GitHub Check: build (stellar, compile)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
.changeset/silent-bags-repair.md (1)
1-5: Changeset LGTMPatch bump and summary look appropriate for the scope.
packages/core/stellar/src/fungible.compile.test.ts (1)
293-313: Good coverage for metadata pathTest adds a realistic opts.info.securityContact scenario for compilation.
packages/core/stellar/src/print.ts (1)
31-31: Correct placement of metadata emissionPrinting metadata immediately after use clauses is correct (macro is imported before use).
packages/core/stellar/src/contract.test.ts (2)
92-96: LGTMSingle metadata case covered.
98-106: LGTMMulti-metadata and composition with docs covered.
Optionally add a snapshot for escaping to guard against regressions introduced by special chars in metadata. Example test to add:
test('contract metadata escaping', t => { const Foo = new ContractBuilder('Foo'); Foo.addContractMetadata([ { key: 'a"b', value: 'c\\d' }, { key: 'line', value: 'x\ny' }, ]); t.snapshot(printContract(Foo)); });packages/core/stellar/src/contract.test.ts.md (3)
158-173: Snapshot correctnessMacro import precedes contractmeta! and formatting looks consistent.
174-190: Snapshot correctnessMultiple metadata entries emitted in declared order; import present.
191-209: Snapshot correctness with documentationDocs and metadata combine cleanly; ordering is sensible.
packages/core/stellar/src/contract.ts (2)
4-4: Interface extension LGTMContract.metadata added with appropriate type.
77-77: Builder initialization LGTMMap initialized eagerly; fine.
| function printMetadata(contract: Contract) { | ||
| return Array.from(contract.metadata.entries()).map(([key, value]) => `contractmeta!(key="${key}", val="${value}");`); | ||
| } |
There was a problem hiding this comment.
Escape metadata strings to avoid invalid Rust when values contain quotes/backslashes/newlines
Unescaped user-supplied key/value can break codegen (e.g., value with a double quote).
Apply this diff:
-function printMetadata(contract: Contract) {
- return Array.from(contract.metadata.entries()).map(([key, value]) => `contractmeta!(key="${key}", val="${value}");`);
-}
+function printMetadata(contract: Contract) {
+ const esc = (s: string) =>
+ s
+ .replace(/\\/g, '\\\\')
+ .replace(/"/g, '\\"')
+ .replace(/\n/g, '\\n')
+ .replace(/\r/g, '\\r')
+ .replace(/\t/g, '\\t');
+ return Array.from(contract.metadata.entries()).map(
+ ([key, value]) => `contractmeta!(key="${esc(key)}", val="${esc(value)}");`,
+ );
+}Consider adding a unit test that includes quotes/newlines to lock this in (see suggestion on test file).
📝 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.
| function printMetadata(contract: Contract) { | |
| return Array.from(contract.metadata.entries()).map(([key, value]) => `contractmeta!(key="${key}", val="${value}");`); | |
| } | |
| function printMetadata(contract: Contract) { | |
| const esc = (s: string) => | |
| s | |
| .replace(/\\/g, '\\\\') | |
| .replace(/"/g, '\\"') | |
| .replace(/\n/g, '\\n') | |
| .replace(/\r/g, '\\r') | |
| .replace(/\t/g, '\\t'); | |
| return Array.from(contract.metadata.entries()).map( | |
| ([key, value]) => `contractmeta!(key="${esc(key)}", val="${esc(value)}");`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/core/stellar/src/print.ts around lines 370 to 372, the metadata
key/value are emitted raw and not escaped, which can produce invalid Rust code
when values contain quotes, backslashes, newlines, etc.; update printMetadata to
escape metadata strings for Rust string literals (at minimum replace backslash
with \\ and double quote with \", convert newlines to \n and tabs to \t, and
handle carriage returns) before interpolating into the contractmeta!(...)
invocation so generated Rust remains valid; add or update a unit test that
passes keys/values containing quotes, backslashes and newlines to ensure
escaping is correct and future changes don't regress.
There was a problem hiding this comment.
@CoveMB We should sanitize the value when adding metadata, since that is user-provided. We can use toByteArray from packages/core/stellar/src/utils/convert-strings.ts, as that is what we use for other string literals.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| ␊ | ||
| use soroban_sdk::contractmeta;␊ | ||
| ␊ | ||
| contractmeta!(key="contact", val="security@example.com");␊ |
There was a problem hiding this comment.
I've noticed in the EVM communities it is common to have a regular contact and a security contact. Would it be beneficial to do that in this ecosystem as well? We don't have to add the general contact now, but it might be a good idea to differentiate the security contact now so as to make space for including a general contact in the future.
There was a problem hiding this comment.
Noted, that make sense, security_contact then?
| if (securityContact) { | ||
| c.addSecurityTag(securityContact); | ||
| } | ||
| export function setInfo(c: ContractBuilder, { securityContact, license }: Info): void { |
There was a problem hiding this comment.
We should add at least 1 snapshot test that covers going through this setInfo function. Currently, the snapshot tests in contract.test.ts only test addContractMetadata directly, while the compilation test in fungible.compile.test.ts does not include a snapshot.
| function printMetadata(contract: Contract) { | ||
| return Array.from(contract.metadata.entries()).map(([key, value]) => `contractmeta!(key="${key}", val="${value}");`); | ||
| } |
There was a problem hiding this comment.
@CoveMB We should sanitize the value when adding metadata, since that is user-provided. We can use toByteArray from packages/core/stellar/src/utils/convert-strings.ts, as that is what we use for other string literals.
Fixes #675
Add ability for the Contract model to store metadata
Use this metadata to store security contact