Merged
Conversation
Contributor
Summary by CodeRabbit
WalkthroughSignature string conversion was moved from the CLI command to the asset-creation utility. The CLI command Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/core/create/createAssetFromArgs.ts (1)
28-33:⚠️ Potential issue | 🟡 MinorHandle null and string signature fields from response.
res.transaction.signaturecan beTransactionSignature | null | stringperUmiTransactionResponse, but the cast masks these cases. The code will throw at runtime if the signature isnullor a base58 string. Add a guard before conversion.🛡️ Suggested guard + normalization
- return { - asset: transaction.asset, - signature: txSignatureToString(res.transaction.signature as TransactionSignature), - } + const signature = res.transaction.signature + if (!signature) { + throw new Error('Missing transaction signature in confirmation response') + } + const signatureString = + typeof signature === 'string' + ? signature + : txSignatureToString(signature as TransactionSignature) + + return { + asset: transaction.asset, + signature: signatureString, + }src/commands/core/asset/create.ts (1)
218-287:⚠️ Potential issue | 🟠 MajorUse
Promise<unknown>return type andthis.logto preserve oclif's JSON output handling.
TransactionCommandenables JSON output (enableJsonFlag = true), and switching toconsole.logwithPromise<void>bypasses oclif's output serialization. This breaks--jsonflag support and removes structured output needed by tests and CI. Other transaction commands in the same directory (e.g.,burn.ts) correctly usePromise<unknown>and return results.🔧 Suggested fix
- public async run(): Promise<void> { + public async run(): Promise<unknown> {Replace all three instances of
console.log(this.formatAssetResult(result, explorer))with:- console.log(this.formatAssetResult(result, explorer)) + this.log(this.formatAssetResult(result, explorer)) + return result
🤖 Fix all issues with AI agents
In `@src/commands/core/asset/create.ts`:
- Around line 209-214: formatAssetResult currently types its parameter as any
which hides the contract that result.asset and result.signature are strings;
replace the any with a concrete type (either derive the return type from
createAssetFromArgs or introduce a small interface like AssetCreationResult with
asset: string and signature: string) and update the function signature to use
that type so TypeScript enforces that result.signature is a string when calling
generateExplorerUrl; ensure any other callers of formatAssetResult are updated
if needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/commands/core/asset/create.ts`:
- Line 256: Replace direct console output with OCLIF's logger: change all calls
using console.log(this.formatAssetResult(result, explorer)) to
this.log(this.formatAssetResult(result, explorer)) (and any other console.log
usages in this file related to asset creation). Locate the calls around the
asset creation flow that invoke formatAssetResult and ensure they use this.log
so output flags (e.g., --json), test stdout capture, and redirection are
respected.
- Line 6: Remove PublicKey from the import list on the top-of-file import that
currently reads "import { generateSigner, PublicKey, Umi } ..." and update the
formatAssetResult function signature to use the imported AssetCreationResult
type instead of its inline type; ensure you import AssetCreationResult (from its
module) if not already present so formatAssetResult references
AssetCreationResult and the unused PublicKey import can be deleted.
In `@src/lib/core/create/createAssetFromArgs.ts`:
- Around line 8-11: The AssetCreationResult type declares asset as a string but
the function createAssetFromArgs doesn't have an explicit return type and
returns transaction.asset which is likely a PublicKey; update the contract by
either (A) changing AssetCreationResult.asset to the actual type (e.g.,
PublicKey) and add an explicit return annotation on createAssetFromArgs to :
Promise<AssetCreationResult> (import the PublicKey type where needed), or (B)
convert transaction.asset to a string with .toString() before returning and keep
AssetCreationResult.asset as string, and annotate createAssetFromArgs' return
type accordingly so TypeScript enforces the shape. Ensure you reference
transaction.asset in the fix and adjust the consumer types (e.g., in create.ts)
to match the chosen representation.
- Line 37: res.transaction.signature is being cast to TransactionSignature
without null-checking before passing to txSignatureToString; update the
createAssetFromArgs logic so you explicitly handle the possible types
(TransactionSignature | null | string) returned by umiSendAndConfirmTransaction:
check res.transaction.signature for null and throw or return a clear error if
null, or if it can be a string pass it directly to txSignatureToString (or
extend txSignatureToString to accept the union), and replace the unsafe cast on
res.transaction.signature with a safe branch that ensures a non-null
TransactionSignature is provided to txSignatureToString.
Contributor
|
are you planning to address the code rabbit stuff? At least some of them seem to make sense. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/commands/core/asset/create.ts (3)
209-216:formatAssetResultcorrectly typed — LGTM.
result: AssetCreationResultreplaces the priorany, enforcing that bothresult.assetandresult.signatureare strings.generateExplorerUrlreceives astringas expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/core/asset/create.ts` around lines 209 - 216, No change required: formatAssetResult already uses the correctly typed parameter AssetCreationResult and constructs the string using result.asset and result.signature; leave the formatAssetResult(result: AssetCreationResult, explorer: ExplorerType) implementation as-is and ensure generateExplorerUrl is called with the signature string as shown.
218-288:run()correctly returnsvoidand usesthis.logthroughout — LGTM.All three code paths (wizard, file-based, and direct flags) now use
this.log, resolving the previously flaggedconsole.logissue.Promise<void>correctly reflects that results are surfaced via side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/core/asset/create.ts` around lines 218 - 288, The run() method correctly returns Promise<void> and consistently uses this.log instead of console.log across all branches (wizard, file-based, direct flags), so no code changes are needed; leave the run() signature as Promise<void>, keep the this.log usages, and ensure functions referenced (createAndUploadMetadata, createAssetPrompt, handleFileBasedCreation, createAssetFromArgs, formatAssetResult, getPluginData, generateSigner) remain unchanged.
8-8: Import correctly updated — LGTM.
AssetCreationResultis imported from the utility, and the now-unnecessaryPublicKeyimport was removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/core/asset/create.ts` at line 8, The import was correctly updated in create.ts: ensure the old PublicKey import is fully removed and AssetCreationResult remains imported from createAssetFromArgs (check the import statement referencing createAssetFromArgs and the AssetCreationResult type); no further changes required besides confirming there are no other unused PublicKey imports in this module or related modules used by createAssetFromArgs.src/lib/core/create/createAssetFromArgs.ts (2)
35-38: Null guard for signature — LGTM.The explicit null check and descriptive error message correctly address the previously flagged missing guard before passing to
txSignatureToString.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/create/createAssetFromArgs.ts` around lines 35 - 38, The null check for res.transaction.signature is correct but ensure callers use the non-null value: keep the explicit guard in createAssetFromArgs (the signature constant and the if (signature === null) throw ...) before passing the value to txSignatureToString; this ensures txSignatureToString is only called with a non-null signature and preserves the descriptive error message "Transaction signature is null — transaction may not have been confirmed".
8-11: Addressed prior type-safety concerns — LGTM.The explicit
Promise<AssetCreationResult>annotation on the function (line 22) and.toString()call on line 41 resolve the previously flaggedPublicKey/stringmismatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/create/createAssetFromArgs.ts` around lines 8 - 11, The review shows the type-safety issue for AssetCreationResult has been resolved (keep the exported interface AssetCreationResult, the explicit Promise<AssetCreationResult> return on createAssetFromArgs, and the .toString() conversion), but there’s a duplicated review marker; remove the duplicate review/comment token ([duplicate_comment] or redundant approval note) from the PR metadata/comments so only a single approval remains while leaving the code symbols AssetCreationResult, createAssetFromArgs, and the .toString() usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/core/create/createAssetFromArgs.ts`:
- Line 41: In createAssetFromArgs, don't silently default transaction.asset to
an empty string; instead detect a missing/falsy transaction.asset and throw a
descriptive Error (e.g., "Missing transaction.asset") so callers (and
formatAssetResult) don't receive an empty asset value; update the object
construction that currently uses transaction.asset ?
transaction.asset.toString() : '' to validate transaction.asset first, call
transaction.asset.toString() when present, and throw when it's absent.
---
Duplicate comments:
In `@src/commands/core/asset/create.ts`:
- Around line 209-216: No change required: formatAssetResult already uses the
correctly typed parameter AssetCreationResult and constructs the string using
result.asset and result.signature; leave the formatAssetResult(result:
AssetCreationResult, explorer: ExplorerType) implementation as-is and ensure
generateExplorerUrl is called with the signature string as shown.
- Around line 218-288: The run() method correctly returns Promise<void> and
consistently uses this.log instead of console.log across all branches (wizard,
file-based, direct flags), so no code changes are needed; leave the run()
signature as Promise<void>, keep the this.log usages, and ensure functions
referenced (createAndUploadMetadata, createAssetPrompt, handleFileBasedCreation,
createAssetFromArgs, formatAssetResult, getPluginData, generateSigner) remain
unchanged.
- Line 8: The import was correctly updated in create.ts: ensure the old
PublicKey import is fully removed and AssetCreationResult remains imported from
createAssetFromArgs (check the import statement referencing createAssetFromArgs
and the AssetCreationResult type); no further changes required besides
confirming there are no other unused PublicKey imports in this module or related
modules used by createAssetFromArgs.
In `@src/lib/core/create/createAssetFromArgs.ts`:
- Around line 35-38: The null check for res.transaction.signature is correct but
ensure callers use the non-null value: keep the explicit guard in
createAssetFromArgs (the signature constant and the if (signature === null)
throw ...) before passing the value to txSignatureToString; this ensures
txSignatureToString is only called with a non-null signature and preserves the
descriptive error message "Transaction signature is null — transaction may not
have been confirmed".
- Around line 8-11: The review shows the type-safety issue for
AssetCreationResult has been resolved (keep the exported interface
AssetCreationResult, the explicit Promise<AssetCreationResult> return on
createAssetFromArgs, and the .toString() conversion), but there’s a duplicated
review marker; remove the duplicate review/comment token ([duplicate_comment] or
redundant approval note) from the PR metadata/comments so only a single approval
remains while leaving the code symbols AssetCreationResult, createAssetFromArgs,
and the .toString() usage unchanged.
# Conflicts: # pnpm-lock.yaml
MarkSackerberg
approved these changes
Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.