Merged
Conversation
Tweaking asset signer methodology
Contributor
Summary by CodeRabbit
WalkthroughThis PR adds asset-signer wallet support, switches many commands to use UMI identity instead of the context signer for authority/owner roles, centralizes transaction send/confirm via a helper, and adds instruction serialization plus new CLI commands and tests. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client/CLI
participant Context as createContext
participant UMI as UMI Instance
participant Plugin as Asset-Signer Plugin
participant SendConfirm as umiSendAndConfirmTransaction
participant Wrap as wrapForAssetSigner
participant RPC as RPC/Blockchain
CLI->>Context: createContext(... activeWallet)
Context->>Plugin: assetSignerPlugin(state)
Plugin->>UMI: install()
CLI->>SendConfirm: umiSendAndConfirmTransaction(umi, tx)
SendConfirm->>Plugin: getAssetSigner(umi)
alt asset-signer active
Plugin-->>SendConfirm: AssetSignerState
SendConfirm->>Wrap: wrapForAssetSigner(umi, tx, assetSigner,...)
Wrap->>RPC: fetchAsset(...)
RPC-->>Wrap: asset data
Wrap-->>SendConfirm: wrapped tx
else standard wallet
Plugin-->>SendConfirm: undefined
end
SendConfirm->>UMI: umiSendTransaction(tx)
UMI->>RPC: sendTransaction(...)
RPC-->>SendConfirm: transaction + signature
SendConfirm-->>CLI: { transaction, signature }
sequenceDiagram
participant User as User/CLI
participant RawCmd as toolbox raw
participant Deserialize as deserializeInstruction
participant Builder as TransactionBuilder
participant Identity as umi.identity
participant SendConfirm as umiSendAndConfirmTransaction
participant RPC as RPC
User->>RawCmd: toolbox raw --instruction base64...
RawCmd->>Deserialize: deserializeInstruction(base64)
Deserialize-->>RawCmd: Instruction {programId, keys, data}
RawCmd->>Builder: transactionBuilder().add(instruction)*n
Builder->>Identity: set signers (umi.identity)
RawCmd->>SendConfirm: umiSendAndConfirmTransaction(umi, tx)
SendConfirm->>RPC: sendTransaction(signed tx)
RPC-->>SendConfirm: signature
SendConfirm-->>RawCmd: {transaction, signature}
RawCmd-->>User: success + explorer URL
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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/config/wallets/set.ts`:
- Around line 65-72: The non-asset-signer branch writes selectedWallet.path to
config.keypair but TypeScript allows selectedWallet.path to be undefined; add a
defensive guard in the block (the else handling for selectedWallet.type in the
set wallet command) to verify selectedWallet.path is a string before assigning
to config.keypair—if missing, surface an error (throw, return, or log and abort)
instead of writing undefined, and only delete config.activeWallet after a
successful assignment; reference selectedWallet.path and config.keypair in your
change.
In `@src/commands/core/asset/execute/index.ts`:
- Around line 10-12: The run() method in CoreAssetExecute currently parses args
and flags via "const {args, flags} = await this.parse(CoreAssetExecute)" but
never uses them; either remove the unused destructuring or implement the
intended behavior: if CoreAssetExecute is only a topic router for subcommands,
delete the parse call and make run() an empty method (or remove run() entirely),
otherwise implement the command logic using the parsed "args" and "flags" (or
pass them to the subcommand handler). Locate the run() method in the
CoreAssetExecute class and apply the appropriate change to eliminate the unused
variables.
In `@src/commands/toolbox/raw.ts`:
- Around line 83-86: The reduce that builds tx (instructions.reduce -> tx using
transactionBuilder()) currently hard-codes bytesCreatedOnChain: 0 which assumes
raw instructions never create accounts; add a brief inline comment by the
bytesCreatedOnChain: 0 entry documenting this assumption and its risk, and
modify the surrounding function (the raw command that constructs tx) to accept
an optional parameter or flag (e.g., bytesCreatedOnChainOverride) so callers can
supply a non-zero value when creating accounts; update the tx construction to
use that parameter (fallback to 0) and validate/describe it in the command/help
text.
In `@src/lib/Context.ts`:
- Around line 197-209: The code calls createSignerFromPath(ownerPath)
unconditionally which will throw if ownerPath is undefined for non-transaction
contexts; update the logic so that createSignerFromPath is only invoked when a
resolved ownerPath exists or when isTransactionContext requires a signer: i.e.,
keep the existing throw for the case when isTransactionContext is true and
ownerPath is missing, but otherwise avoid calling createSignerFromPath and do
not set realWalletSigner/signer; move the realWalletSigner = await
createSignerFromPath(ownerPath) and signer = realWalletSigner lines inside the
branch that verifies ownerPath (or inside the isTransactionContext branch after
the check), referencing ownerPath, isTransactionContext, createSignerFromPath,
realWalletSigner, signer, activeWallet and config.keypair.
In `@test/commands/core/core.asset-signer.test.ts`:
- Around line 101-105: Remove the hardcoded 2-second delay after the
transferSol(...).sendAndConfirm(umi) call: either delete the await new
Promise(resolve => setTimeout(resolve, 2000)) line (since sendAndConfirm already
awaits confirmation) or, if the delay is actually required for RPC propagation
flakiness, replace it with a targeted retry/wait mechanism and add an
explanatory comment next to transferSol/sendAndConfirm indicating why extra
waiting is necessary; references: transferSol and sendAndConfirm in
core.asset-signer.test.ts.
- Around line 127-128: The test's assertion currently only checks the first 4
chars of signerPda which is too weak; update the assertion in the test (the
expect(...) that references signerPda.slice(0, 4)) to assert a longer substring
or the full PDA (e.g., use signerPda or signerPda.slice(0, N) with N
substantially larger) so the output validation uniquely matches the PDA address
rather than accidental text.
In `@test/commands/toolbox/toolbox.raw.test.ts`:
- Line 8: The stripAnsi helper is duplicated; extract the inline const stripAnsi
= (str: string) => str.replace(/\u001b\[\d+m/g, '') into a shared test utility
(e.g., export a function named stripAnsi from a new or existing test helpers
module) and update tests that declare it (including toolbox.raw.test.ts and
core.execute.test.ts) to import and use that shared stripAnsi to remove
duplication and keep a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ef94545-16ff-43cd-a874-dd37c7918f2d
📒 Files selected for processing (52)
src/commands/bg/collection/create.tssrc/commands/bg/nft/burn.tssrc/commands/bg/nft/create.tssrc/commands/bg/nft/transfer.tssrc/commands/cm/create.tssrc/commands/cm/withdraw.tssrc/commands/config/wallets/add.tssrc/commands/config/wallets/list.tssrc/commands/config/wallets/new.tssrc/commands/config/wallets/set.tssrc/commands/core/asset/execute/index.tssrc/commands/core/asset/execute/info.tssrc/commands/core/asset/transfer.tssrc/commands/core/asset/update.tssrc/commands/core/collection/create.tssrc/commands/distro/create.tssrc/commands/distro/deposit.tssrc/commands/distro/withdraw.tssrc/commands/genesis/bucket/add-launch-pool.tssrc/commands/genesis/bucket/add-presale.tssrc/commands/genesis/bucket/add-unlocked.tssrc/commands/genesis/claim-unlocked.tssrc/commands/genesis/claim.tssrc/commands/genesis/create.tssrc/commands/genesis/deposit.tssrc/commands/genesis/finalize.tssrc/commands/genesis/launch/create.tssrc/commands/genesis/launch/register.tssrc/commands/genesis/presale/claim.tssrc/commands/genesis/presale/deposit.tssrc/commands/genesis/revoke.tssrc/commands/genesis/withdraw.tssrc/commands/tm/create.tssrc/commands/tm/transfer.tssrc/commands/tm/update.tssrc/commands/toolbox/raw.tssrc/commands/toolbox/sol/airdrop.tssrc/commands/toolbox/token/add-metadata.tssrc/commands/toolbox/token/create.tssrc/commands/toolbox/token/mint.tssrc/commands/toolbox/token/transfer.tssrc/lib/Context.tssrc/lib/execute/deserializeInstruction.tssrc/lib/umi/assetSignerPlugin.tssrc/lib/umi/sendAndConfirm.tssrc/lib/umi/sendTransaction.tssrc/lib/umi/wrapForAssetSigner.tstest/commands/core/core.asset-signer.test.tstest/commands/core/core.execute.test.tstest/commands/toolbox/toolbox.raw.test.tstest/lib/deserializeInstruction.test.tstest/runCli.ts
Comment on lines
+65
to
+72
| if (selectedWallet.type === 'asset-signer') { | ||
| // For asset-signer wallets, set activeWallet name instead of keypair | ||
| config.activeWallet = selectedWallet.name | ||
| } else { | ||
| // For file/ledger wallets, set keypair path and clear any active asset-signer | ||
| config.keypair = selectedWallet.path | ||
| delete config.activeWallet | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a guard for path existence.
For non-asset-signer wallets, selectedWallet.path is set to config.keypair. While the control flow ensures only file wallets reach this branch (which should have path), TypeScript doesn't narrow selectedWallet.path from string | undefined. This could write undefined to config.keypair if wallet data is malformed.
🛡️ Defensive check suggestion
} else {
// For file/ledger wallets, set keypair path and clear any active asset-signer
+ if (!selectedWallet.path) {
+ this.error('Wallet path is missing')
+ }
config.keypair = selectedWallet.path
delete config.activeWallet
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/config/wallets/set.ts` around lines 65 - 72, The
non-asset-signer branch writes selectedWallet.path to config.keypair but
TypeScript allows selectedWallet.path to be undefined; add a defensive guard in
the block (the else handling for selectedWallet.type in the set wallet command)
to verify selectedWallet.path is a string before assigning to config.keypair—if
missing, surface an error (throw, return, or log and abort) instead of writing
undefined, and only delete config.activeWallet after a successful assignment;
reference selectedWallet.path and config.keypair in your change.
Comment on lines
+83
to
+86
| const tx = instructions.reduce( | ||
| (builder, ix) => builder.add({ instruction: ix, signers: [umi.identity], bytesCreatedOnChain: 0 }), | ||
| transactionBuilder(), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the bytesCreatedOnChain: 0 assumption.
Setting bytesCreatedOnChain: 0 assumes raw instructions don't create accounts. If instructions do create accounts, the transaction might fail due to insufficient compute/priority fee estimation. Consider adding a comment or allowing users to specify this value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/toolbox/raw.ts` around lines 83 - 86, The reduce that builds tx
(instructions.reduce -> tx using transactionBuilder()) currently hard-codes
bytesCreatedOnChain: 0 which assumes raw instructions never create accounts; add
a brief inline comment by the bytesCreatedOnChain: 0 entry documenting this
assumption and its risk, and modify the surrounding function (the raw command
that constructs tx) to accept an optional parameter or flag (e.g.,
bytesCreatedOnChainOverride) so callers can supply a non-zero value when
creating accounts; update the tx construction to use that parameter (fallback to
0) and validate/describe it in the command/help text.
src/lib/Context.ts
Outdated
Comment on lines
+197
to
+209
| if (!ownerPath) { | ||
| ownerPath = config.keypair | ||
| } | ||
|
|
||
| if (!ownerPath && isTransactionContext) { | ||
| throw new Error( | ||
| `Asset-signer wallet '${activeWallet.name}' could not resolve an owner wallet.\n` + | ||
| `Ensure the asset owner is a saved wallet, or set a default keypair in your config.` | ||
| ) | ||
| } | ||
|
|
||
| realWalletSigner = await createSignerFromPath(ownerPath) | ||
| signer = realWalletSigner |
Contributor
There was a problem hiding this comment.
Potential error for non-transaction contexts without configured keypair.
When isAssetSigner is true but isTransactionContext is false and no owner path can be resolved, createSignerFromPath(ownerPath) on line 208 will be called with undefined, throwing an error. This could affect info/read-only commands that don't need a real signer.
Consider wrapping the signer creation in the transaction context check:
- realWalletSigner = await createSignerFromPath(ownerPath)
- signer = realWalletSigner
+ if (isTransactionContext || ownerPath) {
+ realWalletSigner = await createSignerFromPath(ownerPath)
+ signer = realWalletSigner
+ } else {
+ signer = createNoopSigner()
+ }📝 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
| if (!ownerPath) { | |
| ownerPath = config.keypair | |
| } | |
| if (!ownerPath && isTransactionContext) { | |
| throw new Error( | |
| `Asset-signer wallet '${activeWallet.name}' could not resolve an owner wallet.\n` + | |
| `Ensure the asset owner is a saved wallet, or set a default keypair in your config.` | |
| ) | |
| } | |
| realWalletSigner = await createSignerFromPath(ownerPath) | |
| signer = realWalletSigner | |
| if (!ownerPath) { | |
| ownerPath = config.keypair | |
| } | |
| if (!ownerPath && isTransactionContext) { | |
| throw new Error( | |
| `Asset-signer wallet '${activeWallet.name}' could not resolve an owner wallet.\n` + | |
| `Ensure the asset owner is a saved wallet, or set a default keypair in your config.` | |
| ) | |
| } | |
| if (isTransactionContext || ownerPath) { | |
| realWalletSigner = await createSignerFromPath(ownerPath) | |
| signer = realWalletSigner | |
| } else { | |
| signer = createNoopSigner() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/Context.ts` around lines 197 - 209, The code calls
createSignerFromPath(ownerPath) unconditionally which will throw if ownerPath is
undefined for non-transaction contexts; update the logic so that
createSignerFromPath is only invoked when a resolved ownerPath exists or when
isTransactionContext requires a signer: i.e., keep the existing throw for the
case when isTransactionContext is true and ownerPath is missing, but otherwise
avoid calling createSignerFromPath and do not set realWalletSigner/signer; move
the realWalletSigner = await createSignerFromPath(ownerPath) and signer =
realWalletSigner lines inside the branch that verifies ownerPath (or inside the
isTransactionContext branch after the check), referencing ownerPath,
isTransactionContext, createSignerFromPath, realWalletSigner, signer,
activeWallet and config.keypair.
Comment on lines
+127
to
+128
| // Should show the PDA address, not the wallet address | ||
| expect(output).to.contain(signerPda.slice(0, 4)) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Weak address assertion could match unintended strings.
Checking only the first 4 characters of signerPda could match other addresses or text in the output. Consider checking more characters or the full address for stronger validation.
- expect(output).to.contain(signerPda.slice(0, 4))
+ expect(output).to.contain(signerPda)📝 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
| // Should show the PDA address, not the wallet address | |
| expect(output).to.contain(signerPda.slice(0, 4)) | |
| // Should show the PDA address, not the wallet address | |
| expect(output).to.contain(signerPda) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/core/core.asset-signer.test.ts` around lines 127 - 128, The
test's assertion currently only checks the first 4 chars of signerPda which is
too weak; update the assertion in the test (the expect(...) that references
signerPda.slice(0, 4)) to assert a longer substring or the full PDA (e.g., use
signerPda or signerPda.slice(0, N) with N substantially larger) so the output
validation uniquely matches the PDA address rather than accidental text.
| import { keypairIdentity, publicKey } from '@metaplex-foundation/umi' | ||
| import fs from 'node:fs' | ||
|
|
||
| const stripAnsi = (str: string) => str.replace(/\u001b\[\d+m/g, '') |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared stripAnsi helper.
The stripAnsi function is duplicated across test files (also in core.execute.test.ts via import from corehelpers). Consider consolidating into a shared test utility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/toolbox/toolbox.raw.test.ts` at line 8, The stripAnsi helper is
duplicated; extract the inline const stripAnsi = (str: string) =>
str.replace(/\u001b\[\d+m/g, '') into a shared test utility (e.g., export a
function named stripAnsi from a new or existing test helpers module) and update
tests that declare it (including toolbox.raw.test.ts and core.execute.test.ts)
to import and use that shared stripAnsi to remove duplication and keep a single
source of truth.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/Context.ts (1)
225-258:⚠️ Potential issue | 🟠 MajorKeep
Context.payeraligned with the resolved fee payer.Line 227 always returns
signerascontext.payerin asset-signer mode, but Lines 253-258 may install a different payer fromconfig.payer/-p. That splits the source of truth for fee payer selection and makes any remainingcontext.payerconsumers behave as if the override never happened. Reuse the same resolved signer for both the returned context andassetSignerPlugin().💡 Suggested fix
- const payer = isAssetSigner ? signer : (payerPath ? await createSignerFromPath(payerPath) : signer) + let payer = signer + if (!isAssetSigner) { + payer = payerPath ? await createSignerFromPath(payerPath) : signer + } else if (realWalletSigner) { + payer = payerPath ? await createSignerFromPath(payerPath) : realWalletSigner + } ... - if (assetSigner && realWalletSigner) { - // Resolve fee payer: -p flag overrides, otherwise the owner pays. - const feePayer = payerPath - ? await createSignerFromPath(payerPath) - : realWalletSigner - umi.use(assetSignerPlugin({ info: assetSigner, authority: realWalletSigner, payer: feePayer })) + if (assetSigner && realWalletSigner) { + umi.use(assetSignerPlugin({ info: assetSigner, authority: realWalletSigner, payer })) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/Context.ts` around lines 225 - 258, The Context.payer currently remains set to signer for asset-signer mode while assetSignerPlugin may later use a different fee payer; update the payer resolution so the same signer is used for both Context.payer and the plugin: compute feePayer when assetSigner && realWalletSigner (using createSignerFromPath(payerPath) or realWalletSigner), then assign that feePayer to the payer variable used earlier (instead of leaving it as signer) before calling umi.use(assetSignerPlugin(...)); ensure references to signerIdentity/signerPayer and assetSignerPlugin({ authority: realWalletSigner, payer: feePayer }) remain consistent so Context.payer matches the installed plugin payer.
🤖 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/toolbox/raw.ts`:
- Around line 83-86: The code currently hard-codes signers: [umi.identity] when
building the transaction (instructions, transactionBuilder(), umi.identity),
which will submit instructions that require other signer keys and fail; before
reducing into the transaction builder, iterate the deserialized instructions and
for each instruction inspect its account metas (or account keys) for any account
meta with isSigner === true whose pubkey is not umi.identity.publicKey (or
equivalent identity key) and reject/throw a clear error explaining unsupported
multi-signer instructions; do not attempt to synthesize missing Signer objects
because the serialized format drops them—validate and abort early instead of
building the transaction.
In `@test/commands/core/core.asset-signer.test.ts`:
- Around line 168-180: Before invoking the CLI in the '-p' test, record the
lamport balances for the owner keypair (mainKp.publicKey) and the override payer
(newKp.publicKey) using the test UMI instance (created via createUmi and
assigned to umi); run the CLI, then re-fetch both balances and assert that the
override payer (payerKeypairPath / newKp) decreased by the transaction fee while
the owner (mainKp) did not pay the fee (or changed only by any expected amount);
update the test around the CLI invocation in core.asset-signer.test.ts to use
umi.rpc.getBalance (or the existing UMI balance API) to capture pre/post
balances and add assertions comparing the delta to prove the fee came from
payerKeypairPath.
In `@test/commands/toolbox/toolbox.raw.test.ts`:
- Around line 22-59: Add an integration test that runs the 'toolbox raw' command
through the asset-signer execution path: create a test case similar to the
existing "executes a raw SOL transfer instruction" but configure the
environment/CLI to use an asset-signer wallet (e.g., write a temporary
asset-signer config or set the env var/flag your CLI uses to detect an
asset-signer), then call runCli(['toolbox','raw','--instruction', serialized])
and assert the command succeeds (code 0) and prints "Executed 1 instruction(s)"
and "Signature:". Use the same helpers/values (serializeInstruction, runCli,
walletAddress, publicKey) so the test exercises the execute-wrapper path
introduced for asset-signer wallets.
---
Outside diff comments:
In `@src/lib/Context.ts`:
- Around line 225-258: The Context.payer currently remains set to signer for
asset-signer mode while assetSignerPlugin may later use a different fee payer;
update the payer resolution so the same signer is used for both Context.payer
and the plugin: compute feePayer when assetSigner && realWalletSigner (using
createSignerFromPath(payerPath) or realWalletSigner), then assign that feePayer
to the payer variable used earlier (instead of leaving it as signer) before
calling umi.use(assetSignerPlugin(...)); ensure references to
signerIdentity/signerPayer and assetSignerPlugin({ authority: realWalletSigner,
payer: feePayer }) remain consistent so Context.payer matches the installed
plugin payer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4719915-de0b-4e38-93e5-d453ae312b35
📒 Files selected for processing (6)
src/commands/config/wallets/set.tssrc/commands/core/asset/execute/index.tssrc/commands/toolbox/raw.tssrc/lib/Context.tstest/commands/core/core.asset-signer.test.tstest/commands/toolbox/toolbox.raw.test.ts
Comment on lines
+83
to
+86
| // bytesCreatedOnChain is 0 since we can't infer account creation from raw instructions | ||
| const tx = instructions.reduce( | ||
| (builder, ix) => builder.add({ instruction: ix, signers: [umi.identity], bytesCreatedOnChain: 0 }), | ||
| transactionBuilder(), |
Contributor
There was a problem hiding this comment.
Reject raw instructions that need signer keys you don't have.
Line 85 hard-codes signers: [umi.identity]. If a deserialized instruction marks any other pubkey as isSigner, the command will still submit it without the required signature and fail on-chain. The serialized format also drops extra Signer objects, so this should be validated before building the transaction.
🛡️ Guard unsupported multi-signer instructions before sending
spinner.text = `Executing ${instructions.length} instruction(s)...`
+ const unsupportedSignerIndex = instructions.findIndex(ix =>
+ ix.keys.some(
+ key =>
+ key.isSigner &&
+ key.pubkey.toString() !== umi.identity.publicKey.toString(),
+ ),
+ )
+ if (unsupportedSignerIndex !== -1) {
+ this.error(
+ `Instruction ${unsupportedSignerIndex + 1} requires a signer other than the active wallet.`,
+ )
+ }
+
// bytesCreatedOnChain is 0 since we can't infer account creation from raw instructions
const tx = instructions.reduce(
(builder, ix) => builder.add({ instruction: ix, signers: [umi.identity], bytesCreatedOnChain: 0 }),
transactionBuilder(),
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/toolbox/raw.ts` around lines 83 - 86, The code currently
hard-codes signers: [umi.identity] when building the transaction (instructions,
transactionBuilder(), umi.identity), which will submit instructions that require
other signer keys and fail; before reducing into the transaction builder,
iterate the deserialized instructions and for each instruction inspect its
account metas (or account keys) for any account meta with isSigner === true
whose pubkey is not umi.identity.publicKey (or equivalent identity key) and
reject/throw a clear error explaining unsupported multi-signer instructions; do
not attempt to synthesize missing Signer objects because the serialized format
drops them—validate and abort early instead of building the transaction.
Comment on lines
+168
to
+180
| describe('separate fee payer via -p', () => { | ||
| let payerKeypairPath: string | ||
|
|
||
| before(async function () { | ||
| // Generate and fund a second keypair | ||
| const umi = createUmi(TEST_RPC).use(mplCore()).use(mplToolbox()) | ||
| const keypairData = JSON.parse(fs.readFileSync(KEYPAIR_PATH, 'utf-8')) | ||
| const mainKp = umi.eddsa.createKeypairFromSecretKey(new Uint8Array(keypairData)) | ||
| umi.use(keypairIdentity(mainKp)) | ||
|
|
||
| const newKp = generateSigner(umi) | ||
| payerKeypairPath = path.join(os.tmpdir(), `mplx-test-payer-${Date.now()}.json`) | ||
| fs.writeFileSync(payerKeypairPath, JSON.stringify(Array.from(newKp.secretKey))) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the -p case prove who paid the fee.
Lines 188-195 still pass if the CLI silently falls back to the owner/default payer, because the assertion only checks the success message. Capture owner and override-payer balances around the call so the test proves fees came from payerKeypairPath.
🧪 Example assertion that validates the override
describe('separate fee payer via -p', () => {
let payerKeypairPath: string
+ let payerAddress: string
before(async function () {
// Generate and fund a second keypair
const umi = createUmi(TEST_RPC).use(mplCore()).use(mplToolbox())
const keypairData = JSON.parse(fs.readFileSync(KEYPAIR_PATH, 'utf-8'))
const mainKp = umi.eddsa.createKeypairFromSecretKey(new Uint8Array(keypairData))
umi.use(keypairIdentity(mainKp))
const newKp = generateSigner(umi)
+ payerAddress = newKp.publicKey.toString()
payerKeypairPath = path.join(os.tmpdir(), `mplx-test-payer-${Date.now()}.json`)
fs.writeFileSync(payerKeypairPath, JSON.stringify(Array.from(newKp.secretKey)))
tempFiles.push(payerKeypairPath)
...
it('transfers SOL from PDA with a different wallet paying fees', async function () {
+ const umi = createUmi(TEST_RPC)
+ const ownerBefore = await umi.rpc.getBalance(publicKey(ownerAddress))
+ const payerBefore = await umi.rpc.getBalance(publicKey(payerAddress))
const { stdout, stderr, code } = await runCliWithConfig(
['toolbox', 'sol', 'transfer', '0.01', ownerAddress, '-p', payerKeypairPath],
configPath,
)
const output = stripAnsi(stdout) + stripAnsi(stderr)
expect(code).to.equal(0)
expect(output).to.contain('SOL transferred successfully')
+
+ const ownerAfter = await umi.rpc.getBalance(publicKey(ownerAddress))
+ const payerAfter = await umi.rpc.getBalance(publicKey(payerAddress))
+ expect(ownerAfter.basisPoints - ownerBefore.basisPoints).to.equal(10_000_000n)
+ expect(payerAfter.basisPoints < payerBefore.basisPoints).to.equal(true)
})
})Also applies to: 187-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/core/core.asset-signer.test.ts` around lines 168 - 180, Before
invoking the CLI in the '-p' test, record the lamport balances for the owner
keypair (mainKp.publicKey) and the override payer (newKp.publicKey) using the
test UMI instance (created via createUmi and assigned to umi); run the CLI, then
re-fetch both balances and assert that the override payer (payerKeypairPath /
newKp) decreased by the transaction fee while the owner (mainKp) did not pay the
fee (or changed only by any expected amount); update the test around the CLI
invocation in core.asset-signer.test.ts to use umi.rpc.getBalance (or the
existing UMI balance API) to capture pre/post balances and add assertions
comparing the delta to prove the fee came from payerKeypairPath.
Comment on lines
+22
to
+59
| it('executes a raw SOL transfer instruction', async function () { | ||
| const destination = 'TESTfCYwTPxME2cAnPcKvvF5xdPah3PY7naYQEP2kkx' | ||
| const lamports = 10000n | ||
|
|
||
| const data = new Uint8Array(12) | ||
| const view = new DataView(data.buffer) | ||
| view.setUint32(0, 2, true) | ||
| view.setBigUint64(4, lamports, true) | ||
|
|
||
| const instruction = { | ||
| programId: publicKey('11111111111111111111111111111111'), | ||
| keys: [ | ||
| { pubkey: publicKey(walletAddress), isSigner: true, isWritable: true }, | ||
| { pubkey: publicKey(destination), isSigner: false, isWritable: true }, | ||
| ], | ||
| data, | ||
| } | ||
|
|
||
| const serialized = serializeInstruction(instruction) | ||
|
|
||
| const { stdout, stderr, code } = await runCli([ | ||
| 'toolbox', 'raw', '--instruction', serialized | ||
| ]) | ||
|
|
||
| const output = stripAnsi(stdout) + stripAnsi(stderr) | ||
| expect(code).to.equal(0) | ||
| expect(output).to.contain('Executed 1 instruction(s)') | ||
| expect(output).to.contain('Signature:') | ||
| }) | ||
|
|
||
| it('fails when no instructions are provided', async function () { | ||
| try { | ||
| await runCli(['toolbox', 'raw']) | ||
| expect.fail('Expected command to fail without instructions') | ||
| } catch (error: any) { | ||
| expect(error.message).to.contain('You must provide instructions via --instruction or --stdin') | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cover the asset-signer execute path in this suite.
These tests only exercise toolbox raw with a plain keypair. The new behavior in this PR is the execute-wrapper path when an asset-signer wallet is active, and a regression there would still pass this file. Please add one integration case that runs the command through an asset-signer config.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/commands/toolbox/toolbox.raw.test.ts` around lines 22 - 59, Add an
integration test that runs the 'toolbox raw' command through the asset-signer
execution path: create a test case similar to the existing "executes a raw SOL
transfer instruction" but configure the environment/CLI to use an asset-signer
wallet (e.g., write a temporary asset-signer config or set the env var/flag your
CLI uses to detect an asset-signer), then call
runCli(['toolbox','raw','--instruction', serialized]) and assert the command
succeeds (code 0) and prints "Executed 1 instruction(s)" and "Signature:". Use
the same helpers/values (serializeInstruction, runCli, walletAddress, publicKey)
so the test exercises the execute-wrapper path introduced for asset-signer
wallets.
blockiosaurus
approved these changes
Mar 19, 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.
Adds asset-signer wallets — register an MPL Core asset's signer PDA as a wallet, and all CLI commands transparently operate through the PDA via the on-chain execute instruction
.sendAndConfirm(umi)
How it works
Register an asset's PDA as a wallet
mplx config wallets add vault --asset
Switch to it — every command now acts as the PDA
mplx config wallets set vault
These all work transparently through the PDA
mplx toolbox sol balance # shows PDA balance
mplx toolbox sol transfer 0.01 # transfers SOL from PDA
mplx toolbox token transfer 100 # transfers tokens from PDA
mplx core asset create --name "NFT" --uri "..." # PDA is the
creator/authority
mplx core asset transfer # transfers PDA-owned
assets
Override back to normal wallet or Override fee payer (asset owner still signs execute, different wallet pays fees)
mplx toolbox sol balance -k /path/to/wallet.json
mplx toolbox sol transfer 0.01 -p /path/to/other-wallet.json
Architecture
Umi plugin (assetSignerPlugin) stores asset-signer state on the umi instance:
Send layer (umiSendAndConfirmTransaction) reads the plugin state and wraps transactions in execute() automatically. setFeePayer() overrides the transaction fee payer to the real wallet before signing.
Inner signer preservation: generated keypairs (e.g., new asset signers from core asset create) are collected from the inner transaction and attached to the execute wrapper so they sign the outer transaction.
Commands added