Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds an asset-signer test mode: a Mocha root hook creates/funds an asset-signer PDA and writes a temp CLI config; Changes
Sequence DiagramsequenceDiagram
participant Mocha as Mocha Root Hook
participant FS as File System
participant Umi as Umi Client
participant RPC as RPC / Blockchain
participant CLI as Test CLI Runner
Mocha->>FS: load test keypair
Mocha->>Umi: build client (mplCore & mplToolbox)
Umi->>Umi: register keypair identity
Umi->>RPC: createCoreAsset()
RPC-->>Umi: assetId
Umi->>Umi: derive asset-signer PDA
Umi->>RPC: transfer SOL to PDA
RPC-->>Umi: confirm transfer
Mocha->>FS: write temp CLI config (RPC, keypair, vault asset-signer)
Mocha->>CLI: setAssetSignerConfig(configPath)
CLI->>CLI: runCli() reads isAssetSignerMode()
alt asset-signer mode
CLI->>RPC: spawn CLI with `-c config` (execute via PDA)
else normal mode
CLI->>RPC: spawn CLI with `-k keypair`
end
Mocha->>FS: afterAll: delete temp config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/commands/bg/bg.tree.create.test.ts (2)
18-59:⚠️ Potential issue | 🟠 MajorMissing asset-signer mode skip guards in tree creation tests.
These
itblocks usecreateBubblegumTreehelper but lack theif (process.env.MPLX_TEST_WALLET_MODE === 'asset-signer') return this.skip()guard present in other test files. Since tree creation allocates large accounts (CPI-incompatible), these tests will fail in asset-signer mode.🐛 Proposed fix - add skip guards
it('creates a basic Bubblegum tree with default configuration', async function () { + if (process.env.MPLX_TEST_WALLET_MODE === 'asset-signer') return this.skip() const { treeAddress, signature } = await createBubblegumTree() expect(treeAddress).to.match(/^[a-zA-Z0-9]{32,44}$/) expect(signature).to.match(/^[a-zA-Z0-9]{32,}$/) }) it('creates a tree with custom depth and buffer size', async function () { + if (process.env.MPLX_TEST_WALLET_MODE === 'asset-signer') return this.skip() const { treeAddress, signature } = await createBubblegumTree({Apply similar fix to all
itblocks in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/bg/bg.tree.create.test.ts` around lines 18 - 59, Each test that calls createBubblegumTree must bail out when running in asset-signer mode to avoid allocating large/CPI-incompatible accounts; at the top of every it(...) block (the ones using createBubblegumTree, e.g., "creates a basic Bubblegum tree...", "creates a tree with custom depth and buffer size", "creates a public tree", and "creates a tree with a name for storage") add the guard: if (process.env.MPLX_TEST_WALLET_MODE === 'asset-signer') return this.skip(); so the tests skip in asset-signer mode.
100-137:⚠️ Potential issue | 🟠 MajorMissing skip guards for remaining tree tests.
The tests at lines 100-137 (
validates tree name formatandprevents duplicate tree names) also usecreateBubblegumTreehelper and need the asset-signer skip guard.🐛 Proposed fix
it('validates tree name format', async function () { + if (process.env.MPLX_TEST_WALLET_MODE === 'asset-signer') return this.skip() const validName = `test-tree-${Date.now()}` const { treeAddress } = await createBubblegumTree({it('prevents duplicate tree names on same network', async function () { + if (process.env.MPLX_TEST_WALLET_MODE === 'asset-signer') return this.skip() const treeName = `unique-tree-${Date.now()}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/bg/bg.tree.create.test.ts` around lines 100 - 137, Add the same asset-signer skip guard to these two tests that use createBubblegumTree so they are skipped when the asset signer isn't available: at the start of the "validates tree name format" and "prevents duplicate tree names on same network" it blocks, call the existing skip helper (e.g., skipIfNoAssetSigner() or if (!hasAssetSigner()) this.skip()) before invoking createBubblegumTree so the test early-exits when the asset-signer is missing.test/commands/cm/cm.create.test.ts (1)
34-45:⚠️ Potential issue | 🟠 MajorKeep the unit tests free of suite-level chain setup.
In asset-signer mode the three on-chain cases immediately skip, but this outer
beforestill performs an airdrop and waits 10 seconds. That makes the remaininghasGuardstests depend on validator setup even though the matrix documents them as pure unit tests. Guard this hook or move it under the on-chain cases.Suggested minimal fix
describe('cm create commands', function () { before(async () => { + if (process.env.MPLX_TEST_WALLET_MODE === 'asset-signer') return const { stdout, stderr, code } = await runCliDirect( ["toolbox", 'sol', "airdrop", "100", "TESTfCYwTPxME2cAnPcKvvF5xdPah3PY7naYQEP2kkx"] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/cm/cm.create.test.ts` around lines 34 - 45, The suite-level before hook in the "cm create commands" describe block runs an airdrop via runCliDirect and waits 10s unconditionally which couples unit-only tests (like the hasGuards cases) to on-chain setup; either move this airdrop/wait into the on-chain test cases or guard the before hook so it only runs when executing on-chain tests (e.g., check the same condition used to skip the three on-chain cases or an environment flag such as asset-signer mode) and leave the unit tests untouched; update the before hook around runCliDirect and the setTimeout so it only executes when that condition is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/commands/bg/bg.nft.create.test.ts`:
- Around line 9-22: The before hook always performs an airdrop and calls
createBubblegumTree (assigning testTree) even when running in asset-signer mode;
wrap the airdrop and createBubblegumTree call in a conditional that checks the
asset-signer mode (e.g., process.env.ASSET_SIGNER or an existing helper like
isAssetSignerMode()) so that when asset-signer mode is enabled the test setup
skips the runCliDirect airdrop and createBubblegumTree calls and avoids spending
SOL and time, but still retains any other setup steps and the testTree
assignment only when the tree is actually created.
In `@test/commands/core/core.asset-signer.test.ts`:
- Around line 84-90: The test currently writes a config that sets the
asset-signer wallet payer to 'owner' which masks whether the CLI -p override is
honored; update the test in core.asset-signer.test.ts to explicitly verify the
override payer is used by (a) recording the balance of the dedicated override
keypair (the key at KEYPAIR_PATH you pass via -p) before the transfer, invoking
the CLI transfer that uses -p, then asserting that that keypair's balance
decreased after the transfer (use the same balance-check helper used elsewhere
in tests), OR alternatively modify the config written to make the default payer
(owner) unusable (e.g. remove/fund owner with insufficient balance) so the test
will fail if the CLI ignores -p; target the fs.writeFileSync block that writes
rpcUrl, keypair, activeWallet, wallets and the test that asserts the transfer
success string (around the transfer invocation) to add the before/after balance
assertion.
In `@test/runCli.ts`:
- Around line 53-57: runCli currently falls back to keypair mode when
isAssetSignerMode() is true but assetSignerConfigPath is undefined, hiding
setup-ordering bugs; update runCli to fail fast: if isAssetSignerMode() is true
and assetSignerConfigPath is falsy, immediately reject (or throw) with a clear
error mentioning asset signer mode and missing assetSignerConfigPath instead of
building args with '-k'; keep runCliDirect as the intended keypair path and
reference isAssetSignerMode(), assetSignerConfigPath and runCliDirect in the
error message to guide correct usage.
In `@test/WALLET_MODES.md`:
- Around line 109-124: Add a blank line after each H3 heading (the lines
starting "### CPI Limitations", "### Authority Mismatch", and "### What IS
Tested in Asset-Signer Mode") to satisfy MD022, and hyphenate the phrase
"asset-signer specific" to "asset-signer-specific" in the paragraph under the
last heading so the term is grammatically correct; update those three headings
and the phrase accordingly in WALLET_MODES.md.
---
Outside diff comments:
In `@test/commands/bg/bg.tree.create.test.ts`:
- Around line 18-59: Each test that calls createBubblegumTree must bail out when
running in asset-signer mode to avoid allocating large/CPI-incompatible
accounts; at the top of every it(...) block (the ones using createBubblegumTree,
e.g., "creates a basic Bubblegum tree...", "creates a tree with custom depth and
buffer size", "creates a public tree", and "creates a tree with a name for
storage") add the guard: if (process.env.MPLX_TEST_WALLET_MODE ===
'asset-signer') return this.skip(); so the tests skip in asset-signer mode.
- Around line 100-137: Add the same asset-signer skip guard to these two tests
that use createBubblegumTree so they are skipped when the asset signer isn't
available: at the start of the "validates tree name format" and "prevents
duplicate tree names on same network" it blocks, call the existing skip helper
(e.g., skipIfNoAssetSigner() or if (!hasAssetSigner()) this.skip()) before
invoking createBubblegumTree so the test early-exits when the asset-signer is
missing.
In `@test/commands/cm/cm.create.test.ts`:
- Around line 34-45: The suite-level before hook in the "cm create commands"
describe block runs an airdrop via runCliDirect and waits 10s unconditionally
which couples unit-only tests (like the hasGuards cases) to on-chain setup;
either move this airdrop/wait into the on-chain test cases or guard the before
hook so it only runs when executing on-chain tests (e.g., check the same
condition used to skip the three on-chain cases or an environment flag such as
asset-signer mode) and leave the unit tests untouched; update the before hook
around runCliDirect and the setTimeout so it only executes when that condition
is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac77270b-3214-4fef-9a6e-551609ec6f1b
📒 Files selected for processing (21)
.mocharc.jsonpackage.jsontest/WALLET_MODES.mdtest/commands/bg/bg.integration.test.tstest/commands/bg/bg.nft.create.test.tstest/commands/bg/bg.nft.transfer.test.tstest/commands/bg/bg.tree.create.test.tstest/commands/bg/bghelpers.tstest/commands/cm/cm.create.test.tstest/commands/cm/cm.full.test.tstest/commands/cm/cm.insert.test.tstest/commands/cm/cm.withdraw.test.tstest/commands/core/core.asset-signer.test.tstest/commands/distro/distro.deposit.test.tstest/commands/distro/distro.withdraw.test.tstest/commands/genesis/genesis.integration.test.tstest/commands/genesis/genesis.launch.test.tstest/commands/genesis/genesis.presale.test.tstest/commands/genesis/genesis.withdraw.test.tstest/runCli.tstest/setup.asset-signer.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/commands/core/core.asset-signer.test.ts`:
- Around line 143-145: The test fails because balanceAfter.basisPoints and
balanceBefore.basisPoints are bigints while Chai's lessThan expects numbers;
update the assertion in the test (around the variables balanceAfter,
balanceBefore, payerPubkey retrieved via umi.rpc.getBalance) to use a compatible
comparison — either convert to numbers with Number(balanceAfter.basisPoints) and
Number(balanceBefore.basisPoints) inside expect(...).to.be.lessThan(...) or
perform a direct bigint comparison (e.g., expect(balanceAfter.basisPoints <
balanceBefore.basisPoints).to.be.true) so TypeScript no longer complains.
- Around line 44-47: The test defines a duplicate extractTreeAddress function;
remove this local implementation and import the shared extractTreeAddress helper
from the bghelpers module instead, using the shared function
(extractTreeAddress) in place of the local one so you get support for multiple
output formats; update any call sites in this test (core.asset-signer.test.ts)
to match the helper's signature and run tests to confirm no behavior/regression
issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4eb3b17-257e-476c-ab63-2a8ea76546f9
📒 Files selected for processing (4)
test/WALLET_MODES.mdtest/commands/bg/bg.nft.create.test.tstest/commands/core/core.asset-signer.test.tstest/runCli.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/asset-signer-wallets.md`:
- Line 123: The current line is ambiguous about SOL wrapping; update the
sentence to explicitly state that this limitation concerns creating/funding
wrapped SOL (wSOL) token accounts via the Token Program and that calling
transferSol in a CPI (cross-program invocation) context will fail to create or
fund a wSOL token account; reference the Token Program and the transferSol
helper so readers know this is about creating/funding wSOL in CPI rather than
general SOL transfers.
- Line 96: Clarify the "(public trees)" note next to "Bubblegum: `nft create`,
`nft transfer`, `nft burn`, `collection create`" by explaining that creating a
new Bubblegum tree requires using runCliDirect due to CPI account-allocation
limits (see CPI Limitations section), while performing `nft create` (and
transfers/burns) against pre-existing public trees works with asset-signer
wallets; update the line to include this brief distinction or add a
parenthetical/note referencing the CPI Limitations section for details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 211aff50-6cbd-4d97-9f33-ccf09995d5db
📒 Files selected for processing (2)
docs/asset-signer-wallets.mdtest/commands/core/core.asset-signer.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/asset-signer-wallets.md`:
- Line 92: The docs currently list the Toolbox SOL operations "wrap" and
"unwrap" as fully transparent but elsewhere note that wSOL wrapping fails in
CPI; update both the "Toolbox SOL" entry and the later description so they are
consistent: change the "wrap"/"unwrap" visibility from fully transparent to
"partially transparent" (or add a caveat) in the Toolbox SOL line and add a
short note next to the wrap/unwrap descriptions explaining that wSOL wrapping
cannot be performed inside CPIs and will fail, referencing the same wording used
in the other section to keep phrasing consistent (look for the "Toolbox SOL"
heading and the wrap/unwrap discussion to edit).
- Line 87: Replace the overbroad sentence "All CLI commands work with
asset-signer wallets. The transaction wrapping happens transparently in the send
layer." with a qualified statement that acknowledges exceptions—e.g., "Most CLI
commands work with asset-signer wallets; however, some CPI cases are not
supported and require manual handling." —and add a direct pointer or inline link
to the existing "Unsupported CPI cases" section (the documented limitations
around CPI) so readers know to consult lines describing unsupported cases (the
section currently documenting unsupported CPI scenarios).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 157df667-b18a-4198-b6dc-e11e3dd1b71e
📒 Files selected for processing (1)
docs/asset-signer-wallets.md
|
|
||
| ## Supported Commands | ||
|
|
||
| All CLI commands work with asset-signer wallets. The transaction wrapping happens transparently in the send layer. |
Contributor
There was a problem hiding this comment.
Overbroad support statement conflicts with documented limitations.
Line 87 says all commands work, but Lines 118–125 explicitly document unsupported CPI cases. Reword this to avoid promising universal compatibility.
Suggested doc fix
-All CLI commands work with asset-signer wallets. The transaction wrapping happens transparently in the send layer.
+Most CLI commands work with asset-signer wallets. Transaction wrapping happens transparently in the send layer, with exceptions listed in [CPI Limitations](`#cpi-limitations`).📝 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
| All CLI commands work with asset-signer wallets. The transaction wrapping happens transparently in the send layer. | |
| Most CLI commands work with asset-signer wallets. Transaction wrapping happens transparently in the send layer, with exceptions listed in [CPI Limitations](`#cpi-limitations`). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/asset-signer-wallets.md` at line 87, Replace the overbroad sentence "All
CLI commands work with asset-signer wallets. The transaction wrapping happens
transparently in the send layer." with a qualified statement that acknowledges
exceptions—e.g., "Most CLI commands work with asset-signer wallets; however,
some CPI cases are not supported and require manual handling." —and add a direct
pointer or inline link to the existing "Unsupported CPI cases" section (the
documented limitations around CPI) so readers know to consult lines describing
unsupported cases (the section currently documenting unsupported CPI scenarios).
| ### Fully Transparent (no special handling needed) | ||
|
|
||
| - **Core**: `asset create`, `asset transfer`, `asset burn`, `asset update`, `collection create` | ||
| - **Toolbox SOL**: `balance`, `transfer`, `wrap`, `unwrap` |
Contributor
There was a problem hiding this comment.
wrap/unwrap listed as transparent contradicts SOL wrapping limitation.
Line 92 includes wrap/unwrap as fully transparent, but Line 123 says wSOL wrapping fails in CPI. Keep these sections consistent to prevent false expectations.
Suggested doc fix
-- **Toolbox SOL**: `balance`, `transfer`, `wrap`, `unwrap`
+- **Toolbox SOL**: `balance`, `transfer`- **SOL wrapping** — creating wrapped SOL (wSOL) token accounts fails in CPI context
+ **SOL wrapping** — `wrap`/`unwrap` flows that create or fund wSOL token accounts fail in CPI contextAlso applies to: 123-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/asset-signer-wallets.md` at line 92, The docs currently list the Toolbox
SOL operations "wrap" and "unwrap" as fully transparent but elsewhere note that
wSOL wrapping fails in CPI; update both the "Toolbox SOL" entry and the later
description so they are consistent: change the "wrap"/"unwrap" visibility from
fully transparent to "partially transparent" (or add a caveat) in the Toolbox
SOL line and add a short note next to the wrap/unwrap descriptions explaining
that wSOL wrapping cannot be performed inside CPIs and will fail, referencing
the same wording used in the other section to keep phrasing consistent (look for
the "Toolbox SOL" heading and the wrap/unwrap discussion to edit).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/commands/core/core.asset-signer.test.ts (1)
51-52:⚠️ Potential issue | 🟠 MajorThe
-poverride is still masked by the default config payer.Because this config leaves
vault.payerasowner, the transfer can still pass if the CLI ignores-pand charges the owner instead. Please assert that the dedicated payer’s balance decreases, or make the default payer unusable, so this test actually guards the override path.🔎 One way to make the test actually verify the override payer
let payerKeypairPath: string +let payerAddress: string let publicTreeAddress: string @@ // Generate and fund a separate fee payer keypair 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 { stdout, stderr, code } = await runCliWithConfig( + const umi = createUmi(TEST_RPC) + const balanceBefore = await umi.rpc.getBalance(publicKey(payerAddress)) + const { stdout, stderr } = await runCliWithConfig( ['toolbox', 'sol', 'transfer', '0.01', ownerAddress, '-p', payerKeypairPath], configPath, ) + const balanceAfter = await umi.rpc.getBalance(publicKey(payerAddress)) const output = stripAnsi(stdout) + stripAnsi(stderr) - expect(code).to.equal(0) expect(output).to.contain('SOL transferred successfully') + expect(balanceAfter.basisPoints < balanceBefore.basisPoints).to.be.true })Also applies to: 78-88, 91-98, 120-129
🤖 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 51 - 52, The test currently lets the default vault.payer (owner) be charged, so fix the test that uses payerKeypairPath and publicTreeAddress to ensure the CLI -p override is exercised by either (a) asserting the dedicated payer's balance decreases after the transfer (capture its pre- and post-balance and compare), or (b) make the default payer unusable for this scenario (e.g., transfer funds out or set its balance to zero) before invoking the command so only the override payer can pay; update all related test blocks that use payerKeypairPath/publicTreeAddress (including the other failing blocks) to follow the same pattern and assert the dedicated payer was billed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/commands/core/core.asset-signer.test.ts`:
- Around line 97-111: The fixed 2s sleeps after umi.rpc.airdrop and after
creating the tree make tests slow and flaky; replace them with readiness checks:
after umi.rpc.airdrop(newKp.publicKey, ...) poll the chain (e.g., via
umi.connection.getBalance or a waitForBalance helper) until the expected
lamports are present for newKp.publicKey instead of setTimeout, and after
running runCliDirect(['bg','tree','create',...]) use a polling check (e.g., call
getAccountInfo/getProgramAccounts or a waitForTree helper) against the tree
address returned by extractTreeAddress(stdout+stderr) until the tree account
exists/has expected state before assigning publicTreeAddress; keep timeouts and
retry intervals configurable and fail with a clear error if readiness isn't
reached in a reasonable timeout.
---
Duplicate comments:
In `@test/commands/core/core.asset-signer.test.ts`:
- Around line 51-52: The test currently lets the default vault.payer (owner) be
charged, so fix the test that uses payerKeypairPath and publicTreeAddress to
ensure the CLI -p override is exercised by either (a) asserting the dedicated
payer's balance decreases after the transfer (capture its pre- and post-balance
and compare), or (b) make the default payer unusable for this scenario (e.g.,
transfer funds out or set its balance to zero) before invoking the command so
only the override payer can pay; update all related test blocks that use
payerKeypairPath/publicTreeAddress (including the other failing blocks) to
follow the same pattern and assert the dedicated payer was billed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee5223c6-dff0-45be-9192-f07283889e8f
📒 Files selected for processing (1)
test/commands/core/core.asset-signer.test.ts
Comment on lines
+97
to
+111
| await umi.rpc.airdrop(newKp.publicKey, { basisPoints: 2_000_000_000n, identifier: 'SOL', decimals: 9 }) | ||
| await new Promise(resolve => setTimeout(resolve, 2000)) | ||
|
|
||
| // Create a public tree (anyone can mint) for bg mint test | ||
| const { stdout, stderr } = await runCliDirect([ | ||
| 'bg', 'tree', 'create', | ||
| '--maxDepth', '3', | ||
| '--maxBufferSize', '8', | ||
| '--canopyDepth', '1', | ||
| '--public', | ||
| ]) | ||
| const treeAddr = extractTreeAddress(stripAnsi(stdout + '\n' + stderr)) | ||
| if (!treeAddr) throw new Error('Could not create public tree') | ||
| publicTreeAddress = treeAddr | ||
| await new Promise(resolve => setTimeout(resolve, 2000)) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace the new fixed sleeps with readiness checks.
The 2s waits after funding the payer and creating the tree slow every asset-signer run and still leave the suite timing-sensitive on slower CI/RPC. Waiting on observable chain state here will be faster and less flaky.
🤖 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 97 - 111, The
fixed 2s sleeps after umi.rpc.airdrop and after creating the tree make tests
slow and flaky; replace them with readiness checks: after
umi.rpc.airdrop(newKp.publicKey, ...) poll the chain (e.g., via
umi.connection.getBalance or a waitForBalance helper) until the expected
lamports are present for newKp.publicKey instead of setTimeout, and after
running runCliDirect(['bg','tree','create',...]) use a polling check (e.g., call
getAccountInfo/getProgramAccounts or a waitForTree helper) against the tree
address returned by extractTreeAddress(stdout+stderr) until the tree account
exists/has expected state before assigning publicTreeAddress; keep timeouts and
retry intervals configurable and fail with a clear error if readiness isn't
reached in a reasonable timeout.
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.
Test Wallet Modes
The test suite runs in two wallet modes to verify all commands work with both normal keypairs and asset-signer wallets.
In asset-signer mode, a root hook (
test/setup.asset-signer.ts) creates a signing asset, funds its PDA, and writes a temporary config.runClithen uses-c <config>instead of-k <keypair>, so all transactions are wrapped in MPL Coreexecute().Infrastructure that can't be created via execute CPI (large account allocations) uses
runCliDirect, which always uses the normal keypair.Test Coverage by Wallet Mode
Legend:
describe.skip(not related to asset-signer)Core Commands
Asset-Signer Specific
-pToolbox
Token Metadata
Bubblegum
runCliDirectinternally (CPI limitation)describe.skipdescribe.skipdescribe.skipCandy Machine
Genesis
runCliDirectfor SOL wraprunCliDirectfor SOL wrapDistribution
Lib (Unit Tests)
Why Some Tests Skip in Asset-Signer Mode
CPI Limitations
These operations allocate large accounts, which fails when wrapped in
execute():The
createBubblegumTreehelper and CM creation in test setup userunCliDirectto bypass this.Authority Mismatch
Resources created with
runCliDirect(normal wallet) have the wallet as authority. In asset-signer mode, commands run as the PDA, which isn't the authority. This affects:What IS Tested in Asset-Signer Mode
The asset-signer specific test creates a public tree (anyone can mint) and verifies the PDA can mint a cNFT into it. This confirms bubblegum minting works through execute CPI when authority isn't an issue.