Closed
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThis PR introduces a suite of CLI commands and utilities for executing operations on MPL Core Assets. It adds six subcommands under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 7
🤖 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/commands/core/asset/execute/index.ts`:
- Around line 14-16: The run() method of CoreAssetExecute currently destructures
unused vars (args, flags) from await this.parse(CoreAssetExecute); either remove
the unused destructuring entirely and leave run() empty (for a parent/subcommand
entry), or explicitly show help by calling the command help helper (e.g., await
this.parse(CoreAssetExecute) without destructuring and then call this._help() or
this.help()) so args and flags are not unused; update the run() implementation
in CoreAssetExecute accordingly.
In `@src/commands/core/asset/execute/raw.ts`:
- Around line 42-53: The readStdin() helper can hang if --stdin is used without
piped input; update the readStdin method to first check process.stdin.isTTY and
immediately reject (or throw) with a clear message advising to pipe data or use
a different flag, and additionally implement a configurable timeout (e.g.,
5–10s) that rejects if no data arrives; reference the readStdin function and the
Promise resolution/rejection handlers so you add the TTY check before attaching
'data'/'end' listeners and start a timer that clears on 'end' and rejects on
timeout.
In `@src/commands/core/asset/execute/transfer-token.ts`:
- Around line 22-26: The amount flag can be negative because Flags.integer
allows negatives; add a positive-value validation early in this command (e.g.,
in the command's run method) to ensure the parsed amount > 0 before attempting
the transfer. Locate the static override flags declaration and the command's
entrypoint (run/execute) in transfer-token.ts, check the value of the parsed
amount flag (amount) and if it's <= 0 call this.error or throw a clear CLI error
indicating the amount must be a positive integer so the transfer logic never
runs with non-positive amounts.
- Around line 47-70: Replace the non-idempotent call to createAssociatedToken
with createIdempotentAssociatedToken so creating the destination ATA won't fail
if it already exists: change the creation of createAtaIx to call
createIdempotentAssociatedToken(umi, { mint: mintPubkey, owner:
destinationPubkey }) (and update imports to pull createIdempotentAssociatedToken
from mpl-toolbox and remove or replace the old createAssociatedToken import)
while keeping the rest of the flow (calling createAtaIx.getInstructions(), then
transferTokensIx.getInstructions()) unchanged.
In `@test/commands/core/core.execute.test.ts`:
- Line 19: Replace the hard-coded sleep expression "await new Promise(resolve =>
setTimeout(resolve, 10000))" (and the similar occurrences) with condition-based
waiting: implement a small retry/poll loop or use an existing test helper (e.g.,
waitFor / retryUntil) that repeatedly checks the real condition relevant to the
preceding action (for example, poll signer balance via getBalance until it
matches expected, or poll for the token account to exist) and resolves as soon
as the condition is met or a timeout is reached; update the tests that currently
use the fixed sleep at the mentioned locations to call this wait helper and
assert the condition instead of sleeping.
- Around line 61-65: The four tests repeat the same sequence of running the CLI
and extracting/asserting the signer PDA; create a shared helper (e.g.,
getSignerPdaFromCli or fetchSignerPda) that calls
runCli(['core','asset','execute','signer', assetId]), combines stdout/stderr,
runs stripAnsi and extractSignerPda, and returns the parsed signerPda (or
asserts truthy inside the helper); then replace the duplicated blocks in
core.execute.test.ts with calls to this helper (update each test to call
getSignerPdaFromCli(assetId) and assert or rely on the helper's assertion) and
remove the repeated code using the existing symbols runCli, stripAnsi,
extractSignerPda, and signerPda.
In `@test/commands/core/executehelpers.ts`:
- Line 20: The KEYPAIR_PATH constant uses process.cwd(), which makes tests
dependent on the current working directory; update KEYPAIR_PATH in
executehelpers.ts to build the path relative to the test file (use __dirname or
path.resolve with a relative path to the test-files directory) so the path is
stable regardless of where tests are run; change the initializer for
KEYPAIR_PATH to derive from __dirname (e.g., resolve/join from __dirname to the
test-files/key.json) while keeping the constant name KEYPAIR_PATH.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecb5e7af-1719-4a68-97d7-da8a83a9f81e
📒 Files selected for processing (10)
src/commands/core/asset/execute/index.tssrc/commands/core/asset/execute/raw.tssrc/commands/core/asset/execute/signer.tssrc/commands/core/asset/execute/transfer-asset.tssrc/commands/core/asset/execute/transfer-sol.tssrc/commands/core/asset/execute/transfer-token.tssrc/lib/execute/deserializeInstruction.tstest/commands/core/core.execute.test.tstest/commands/core/executehelpers.tstest/lib/deserializeInstruction.test.ts
Comment on lines
+14
to
+16
| public async run(): Promise<void> { | ||
| const {args, flags} = await this.parse(CoreAssetExecute) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused parsed variables in parent command.
The args and flags variables are parsed but never used. For a parent command that serves as an entry point to subcommands, the run() method body can be empty or simply display help.
♻️ Suggested simplification
public async run(): Promise<void> {
- const {args, flags} = await this.parse(CoreAssetExecute)
+ await this.parse(CoreAssetExecute)
+ // Parent command - subcommands handle actual execution
}📝 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.
Suggested change
| public async run(): Promise<void> { | |
| const {args, flags} = await this.parse(CoreAssetExecute) | |
| } | |
| public async run(): Promise<void> { | |
| await this.parse(CoreAssetExecute) | |
| // Parent command - subcommands handle actual execution | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/core/asset/execute/index.ts` around lines 14 - 16, The run()
method of CoreAssetExecute currently destructures unused vars (args, flags) from
await this.parse(CoreAssetExecute); either remove the unused destructuring
entirely and leave run() empty (for a parent/subcommand entry), or explicitly
show help by calling the command help helper (e.g., await
this.parse(CoreAssetExecute) without destructuring and then call this._help() or
this.help()) so args and flags are not unused; update the run() implementation
in CoreAssetExecute accordingly.
Comment on lines
+42
to
+53
| private async readStdin(): Promise<string[]> { | ||
| return new Promise((resolve, reject) => { | ||
| let data = '' | ||
| process.stdin.setEncoding('utf8') | ||
| process.stdin.on('data', (chunk) => { data += chunk }) | ||
| process.stdin.on('end', () => { | ||
| const lines = data.split('\n').map(l => l.trim()).filter(l => l.length > 0) | ||
| resolve(lines) | ||
| }) | ||
| process.stdin.on('error', reject) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a TTY check or timeout for stdin reading.
If a user invokes --stdin without piping data (e.g., runs the command interactively), readStdin() will hang indefinitely waiting for input. Consider checking if stdin is a TTY and providing guidance, or adding a timeout.
♻️ Suggested improvement
private async readStdin(): Promise<string[]> {
+ if (process.stdin.isTTY) {
+ this.error('No input piped to stdin. Use: echo "<base64>" | mplx core asset execute raw <assetId> --stdin')
+ }
+
return new Promise((resolve, reject) => {
let data = ''
process.stdin.setEncoding('utf8')
process.stdin.on('data', (chunk) => { data += chunk })
process.stdin.on('end', () => {
const lines = data.split('\n').map(l => l.trim()).filter(l => l.length > 0)
resolve(lines)
})
process.stdin.on('error', reject)
})
}📝 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.
Suggested change
| private async readStdin(): Promise<string[]> { | |
| return new Promise((resolve, reject) => { | |
| let data = '' | |
| process.stdin.setEncoding('utf8') | |
| process.stdin.on('data', (chunk) => { data += chunk }) | |
| process.stdin.on('end', () => { | |
| const lines = data.split('\n').map(l => l.trim()).filter(l => l.length > 0) | |
| resolve(lines) | |
| }) | |
| process.stdin.on('error', reject) | |
| }) | |
| } | |
| private async readStdin(): Promise<string[]> { | |
| if (process.stdin.isTTY) { | |
| this.error('No input piped to stdin. Use: echo "<base64>" | mplx core asset execute raw <assetId> --stdin') | |
| } | |
| return new Promise((resolve, reject) => { | |
| let data = '' | |
| process.stdin.setEncoding('utf8') | |
| process.stdin.on('data', (chunk) => { data += chunk }) | |
| process.stdin.on('end', () => { | |
| const lines = data.split('\n').map(l => l.trim()).filter(l => l.length > 0) | |
| resolve(lines) | |
| }) | |
| process.stdin.on('error', reject) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/core/asset/execute/raw.ts` around lines 42 - 53, The readStdin()
helper can hang if --stdin is used without piped input; update the readStdin
method to first check process.stdin.isTTY and immediately reject (or throw) with
a clear message advising to pipe data or use a different flag, and additionally
implement a configurable timeout (e.g., 5–10s) that rejects if no data arrives;
reference the readStdin function and the Promise resolution/rejection handlers
so you add the TTY check before attaching 'data'/'end' listeners and start a
timer that clears on 'end' and rejects on timeout.
Comment on lines
+22
to
+26
| static override flags = { | ||
| mint: Flags.string({ description: 'Token mint address', required: true }), | ||
| amount: Flags.integer({ description: 'Amount to transfer in smallest unit (e.g., lamports for wrapped SOL)', required: true }), | ||
| destination: Flags.string({ description: 'Destination wallet address', required: true }), | ||
| } |
Contributor
There was a problem hiding this comment.
Add validation for positive amount.
Flags.integer allows negative values. Similar to transfer-sol.ts which validates amountInSol > 0, this command should validate that the amount is positive before attempting the transfer.
🛡️ Proposed fix
public async run(): Promise<unknown> {
const { args, flags } = await this.parse(ExecuteTransferToken)
const { umi, explorer, chain } = this.context
+ if (flags.amount <= 0) {
+ this.error('Amount must be a positive number')
+ }
+
const spinner = ora('Fetching asset...').start()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/core/asset/execute/transfer-token.ts` around lines 22 - 26, The
amount flag can be negative because Flags.integer allows negatives; add a
positive-value validation early in this command (e.g., in the command's run
method) to ensure the parsed amount > 0 before attempting the transfer. Locate
the static override flags declaration and the command's entrypoint (run/execute)
in transfer-token.ts, check the value of the parsed amount flag (amount) and if
it's <= 0 call this.error or throw a clear CLI error indicating the amount must
be a positive integer so the transfer logic never runs with non-positive
amounts.
Comment on lines
+47
to
+70
| // Create the destination token account if it doesn't exist, then transfer | ||
| const createAtaIx = createAssociatedToken(umi, { | ||
| mint: mintPubkey, | ||
| owner: destinationPubkey, | ||
| }) | ||
|
|
||
| const transferTokensIx = transferTokens(umi, { | ||
| source: findAssociatedTokenPda(umi, { | ||
| mint: mintPubkey, | ||
| owner: assetSignerPda, | ||
| }), | ||
| destination: findAssociatedTokenPda(umi, { | ||
| mint: mintPubkey, | ||
| owner: destinationPubkey, | ||
| }), | ||
| authority: createNoopSigner(assetSignerPda), | ||
| amount: flags.amount, | ||
| }) | ||
|
|
||
| // Collect all instructions as a flat array for the execute wrapper | ||
| const instructions = [ | ||
| ...createAtaIx.getInstructions(), | ||
| ...transferTokensIx.getInstructions(), | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does mpl-toolbox createAssociatedToken fail if the account already exists?
💡 Result:
Yes, mpl-toolbox's createAssociatedToken fails if the associated token account already exists. The library provides both createAssociatedToken (standard SPL Associated Token Account creation, which errors if the account exists) and createIdempotentAssociatedToken (idempotent version that succeeds if it exists). Official docs distinguish them and show createAssociatedToken example without checks, while createTokenIfMissing is for general token accounts. SPL standard create fails on existing account.
Citations:
- 1: https://mpl-toolbox.typedoc.metaplex.com/functions/createIdempotentAssociatedToken.html
- 2: https://mpl-toolbox.typedoc.metaplex.com/
- 3: https://developers.metaplex.com/umi/toolbox/token-managment
- 4: https://solana-labs.github.io/solana-program-library/token/js/functions/createAssociatedTokenAccountIdempotentInstruction.html
- 5: https://mpl-toolbox.typedoc.metaplex.com/types/CreateAssociatedTokenInstructionAccounts.html
- 6: https://developers.metaplex.com/dev-tools/umi/toolbox/token-managment
- 7: https://developers.metaplex.com/toolbox
Replace createAssociatedToken with createIdempotentAssociatedToken.
createAssociatedToken fails if the associated token account already exists. Use createIdempotentAssociatedToken from mpl-toolbox instead, which succeeds whether the account exists or not. This prevents transaction failures when transferring to wallets that already have the token account.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/core/asset/execute/transfer-token.ts` around lines 47 - 70,
Replace the non-idempotent call to createAssociatedToken with
createIdempotentAssociatedToken so creating the destination ATA won't fail if it
already exists: change the creation of createAtaIx to call
createIdempotentAssociatedToken(umi, { mint: mintPubkey, owner:
destinationPubkey }) (and update imports to pull createIdempotentAssociatedToken
from mpl-toolbox and remove or replace the old createAssociatedToken import)
while keeping the rest of the flow (calling createAtaIx.getInstructions(), then
transferTokensIx.getInstructions()) unchanged.
|
|
||
| before(async () => { | ||
| await runCli(['toolbox', 'sol', 'airdrop', '100', 'TESTfCYwTPxME2cAnPcKvvF5xdPah3PY7naYQEP2kkx']) | ||
| await new Promise(resolve => setTimeout(resolve, 10000)) |
Contributor
There was a problem hiding this comment.
Replace fixed sleeps with condition-based waiting to reduce flakiness.
Hard-coded delays make these integration tests timing-sensitive and slower than needed.
💡 Proposed pattern
+const waitFor = async (
+ predicate: () => Promise<boolean>,
+ timeoutMs = 20_000,
+ intervalMs = 500,
+) => {
+ const start = Date.now()
+ while (Date.now() - start < timeoutMs) {
+ if (await predicate()) return
+ await new Promise(resolve => setTimeout(resolve, intervalMs))
+ }
+ throw new Error('Timed out waiting for chain state')
+}Then replace each fixed sleep with a state check relevant to the preceding action (e.g., signer balance observed, token account observed, etc.).
Also applies to: 69-69, 112-112, 198-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/core/core.execute.test.ts` at line 19, Replace the hard-coded
sleep expression "await new Promise(resolve => setTimeout(resolve, 10000))" (and
the similar occurrences) with condition-based waiting: implement a small
retry/poll loop or use an existing test helper (e.g., waitFor / retryUntil) that
repeatedly checks the real condition relevant to the preceding action (for
example, poll signer balance via getBalance until it matches expected, or poll
for the token account to exist) and resolves as soon as the condition is met or
a timeout is reached; update the tests that currently use the fixed sleep at the
mentioned locations to call this wait helper and assert the condition instead of
sleeping.
Comment on lines
+61
to
+65
| const { stdout: signerOut, stderr: signerErr } = await runCli([ | ||
| 'core', 'asset', 'execute', 'signer', assetId | ||
| ]) | ||
| const signerPda = extractSignerPda(stripAnsi(signerOut) + stripAnsi(signerErr)) | ||
| expect(signerPda).to.be.ok |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract signer-PDA lookup into a shared helper to remove duplication.
The same lookup/parse/assert sequence is repeated in four tests.
♻️ Proposed refactor
+const getSignerPda = async (assetId: string): Promise<string> => {
+ const { stdout, stderr } = await runCli(['core', 'asset', 'execute', 'signer', assetId])
+ const signerPda = extractSignerPda(stripAnsi(stdout) + stripAnsi(stderr))
+ if (!signerPda) throw new Error(`Could not extract signer PDA for asset ${assetId}`)
+ return signerPda
+}Then replace each repeated block with:
-const { stdout: signerOut, stderr: signerErr } = await runCli([
- 'core', 'asset', 'execute', 'signer', assetId
-])
-const signerPda = extractSignerPda(stripAnsi(signerOut) + stripAnsi(signerErr))
-expect(signerPda).to.be.ok
+const signerPda = await getSignerPda(assetId)Also applies to: 104-108, 134-138, 190-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/core/core.execute.test.ts` around lines 61 - 65, The four tests
repeat the same sequence of running the CLI and extracting/asserting the signer
PDA; create a shared helper (e.g., getSignerPdaFromCli or fetchSignerPda) that
calls runCli(['core','asset','execute','signer', assetId]), combines
stdout/stderr, runs stripAnsi and extractSignerPda, and returns the parsed
signerPda (or asserts truthy inside the helper); then replace the duplicated
blocks in core.execute.test.ts with calls to this helper (update each test to
call getSignerPdaFromCli(assetId) and assert or rely on the helper's assertion)
and remove the repeated code using the existing symbols runCli, stripAnsi,
extractSignerPda, and signerPda.
| import path from 'node:path' | ||
|
|
||
| const TEST_RPC = 'http://127.0.0.1:8899' | ||
| const KEYPAIR_PATH = path.join(process.cwd(), 'test-files', 'key.json') |
Contributor
There was a problem hiding this comment.
Make test keypair path independent of current working directory.
Using process.cwd() on Line 20 can break when tests are invoked from another directory.
💡 Proposed fix
import fs from 'node:fs'
-import path from 'node:path'
@@
-const KEYPAIR_PATH = path.join(process.cwd(), 'test-files', 'key.json')
+const KEYPAIR_PATH = new URL('../../../test-files/key.json', import.meta.url)📝 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.
Suggested change
| const KEYPAIR_PATH = path.join(process.cwd(), 'test-files', 'key.json') | |
| const KEYPAIR_PATH = new URL('../../../test-files/key.json', import.meta.url) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/core/executehelpers.ts` at line 20, The KEYPAIR_PATH constant
uses process.cwd(), which makes tests dependent on the current working
directory; update KEYPAIR_PATH in executehelpers.ts to build the path relative
to the test file (use __dirname or path.resolve with a relative path to the
test-files directory) so the path is stable regardless of where tests are run;
change the initializer for KEYPAIR_PATH to derive from __dirname (e.g.,
resolve/join from __dirname to the test-files/key.json) while keeping the
constant name KEYPAIR_PATH.
Contributor
Author
|
Closing again - there's a better approach |
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
1 participant
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.
Adding mplx core asset execute commands: