Auto-detect Core asset vs collection for plugin add/update - #129
Auto-detect Core asset vs collection for plugin add/update#129MarkSackerberg wants to merge 2 commits into
Conversation
When adding or updating plugins, resolve the address as an asset first and fall back to a collection instead of failing with "Unable to fetch asset". The --collection flag remains as an explicit override. Co-authored-by: MarkSackerberg <MarkSackerberg@users.noreply.github.com>
WalkthroughCore plugin add and update commands now share account resolution for asset and collection addresses. Transaction sending also supports configurable compute-unit limits and prepended priority-fee instructions, with collection auto-detection covered by tests. ChangesCore plugin account resolution
Transaction compute-budget configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CorePluginsCommand
participant resolveCoreAccount
participant CoreAsset
participant CoreCollection
participant PluginBatchOperation
CLI->>CorePluginsCommand: add or update plugins
CorePluginsCommand->>resolveCoreAccount: resolve target address
resolveCoreAccount->>CoreAsset: fetch asset
resolveCoreAccount->>CoreCollection: fetch collection if needed
resolveCoreAccount-->>CorePluginsCommand: return account type and collectionId
CorePluginsCommand->>PluginBatchOperation: submit add or update batch
PluginBatchOperation-->>CLI: report command result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/commands/core/core.plugins.test.ts`:
- Around line 43-61: Add resolver-level tests covering a valid but absent
address, an asset supplied with forceCollection, and an RPC rejection, alongside
the existing collection auto-detection test. Exercise the resolver’s explicit
override and error paths and assert their documented outcomes, using the
resolver symbols and test setup already present in the surrounding cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e3927d2-84a9-4dba-af59-a5e7442fece1
📒 Files selected for processing (4)
src/commands/core/plugins/add.tssrc/commands/core/plugins/update.tssrc/lib/core/fetch/resolveCoreAccount.tstest/commands/core/core.plugins.test.ts
| it('adds a plugin to a collection without --collection by auto-detecting', async function() { | ||
| this.timeout(30000) | ||
| const { collectionId } = await createCoreCollection() | ||
|
|
||
| const addInput = [ | ||
| 'core', | ||
| 'plugins', | ||
| 'add', | ||
| collectionId, | ||
| 'test-files/plugins.json', | ||
| ] | ||
|
|
||
| const { stderr: addStderr, code: addCode } = await runCli(addInput) | ||
| const cleanAddStderr = stripAnsi(addStderr) | ||
|
|
||
| expect(addCode).to.equal(0) | ||
| expect(cleanAddStderr).to.contain('Resolved as collection') | ||
| expect(cleanAddStderr).to.contain('Successfully added') | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the resolver’s negative branches.
Add resolver-level tests for a valid absent address, an asset passed with forceCollection, and an RPC rejection. The new integration cases only verify successful collection auto-detection, leaving the explicit override and clear-error contracts unguarded.
Also applies to: 99-125
🧰 Tools
🪛 ESLint
[error] 44-44: Invalid group length in numeric value.
(unicorn/numeric-separators-style)
[error] 55-55: Expected "code" to come before "stderr"
(perfectionist/sort-objects)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/commands/core/core.plugins.test.ts` around lines 43 - 61, Add
resolver-level tests covering a valid but absent address, an asset supplied with
forceCollection, and an RPC rejection, alongside the existing collection
auto-detection test. Exercise the resolver’s explicit override and error paths
and assert their documented outcomes, using the resolver symbols and test setup
already present in the surrounding cases.
Genesis account creation can exceed Solana's 200k default compute budget under Node 24 CI. Prepend setComputeUnitLimit (400k) in the shared send path so heavier Metaplex transactions stop failing flakily. Co-authored-by: MarkSackerberg <MarkSackerberg@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/umi/sendOptions.ts`:
- Around line 10-11: Reorder the computeUnitLimit property and its documentation
in the send options interface so they appear before priorityFee, satisfying the
perfectionist/sort-interfaces ordering rule without changing the property
definition.
In `@src/lib/umi/sendTransaction.ts`:
- Around line 12-14: Avoid using DEFAULT_COMPUTE_UNIT_LIMIT as the implicit
priority-fee budget in sendTransaction when computeUnitLimit is omitted. Use a
tighter transaction-specific compute-unit estimate, or require an explicit
computeUnitLimit whenever priorityFee is enabled, while preserving the existing
default only for transactions without priority fees.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d9ad036-33ae-4066-918f-020d285fee93
📒 Files selected for processing (2)
src/lib/umi/sendOptions.tssrc/lib/umi/sendTransaction.ts
| /** Override the default compute unit limit (400_000). */ | ||
| computeUnitLimit?: number | undefined |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move computeUnitLimit before priorityFee.
ESLint’s perfectionist/sort-interfaces rule requires the new property to precede priorityFee; move the documentation and field above line 9 to avoid the lint failure.
🧰 Tools
🪛 ESLint
[error] 11-11: Expected "computeUnitLimit" to come before "priorityFee"
(perfectionist/sort-interfaces)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/umi/sendOptions.ts` around lines 10 - 11, Reorder the
computeUnitLimit property and its documentation in the send options interface so
they appear before priorityFee, satisfying the perfectionist/sort-interfaces
ordering rule without changing the property definition.
Source: Linters/SAST tools
| /** Default above Solana's 200k so heavier Metaplex txs (e.g. Genesis create) don't flake. */ | ||
| const DEFAULT_COMPUTE_UNIT_LIMIT = 400_000 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching sendTransaction.ts:\n'
fd -a 'sendTransaction\.ts$' . || true
file="$(fd 'sendTransaction\.ts$' . | head -n 1 || true)"
if [ -n "$file" ]; then
printf '\nFile: %s\n' "$file"
wc -l "$file"
printf '\nRelevant file contents:\n'
cat -n "$file"
fi
printf '\nSearch for DEFAULT_COMPUTE_UNIT_LIMIT and priorityFee handling:\n'
rg -n "DEFAULT_COMPUTE_UNIT_LIMIT|computeUnitLimit|priorityFee|priorityFeeMicroLamports|computeBudget" .Repository: metaplex-foundation/cli
Length of output: 4048
🌐 Web query:
Solana priority fees computed based on requested compute unit limit documentation
💡 Result:
On the Solana network, the prioritization fee is calculated based on the requested compute unit (CU) limit, not the actual number of compute units consumed by the transaction execution [1][2][3]. The formula for calculating the prioritization fee is: priority_fee = ceil(compute_unit_price * compute_unit_limit / 1,000,000) In this formula: - compute_unit_price: The price in micro-lamports per compute unit, set using the SetComputeUnitPrice instruction [1][2]. - compute_unit_limit: The maximum number of compute units the transaction is allowed to consume, set using the SetComputeUnitLimit instruction [1][2]. - 1,000,000: The conversion factor (micro-lamports per lamport) [2][4]. - ceil: The result is rounded up to the nearest whole lamport [2][3]. Key Considerations: - Requested vs. Actual: Because the fee is charged based on the requested limit, setting a limit significantly higher than necessary will result in overpayment [1][2][5]. - Defaults: If a transaction does not explicitly include a SetComputeUnitLimit instruction, the network uses a default limit based on the number and type of instructions in the transaction (e.g., 200,000 CUs for each non-builtin instruction) [2][4][6]. - Optimization: To minimize fees while ensuring transaction success, it is recommended to simulate the transaction to determine the actual CU usage, then set the compute unit limit to that value plus a small margin of error [5][3].
Citations:
- 1: https://solana.com/docs/core/fees/compute-budget
- 2: https://solana.com/docs/core/fees/fee-structure
- 3: https://docs.chainstack.com/docs/solana-compute-budget
- 4: https://solana.com/docs/core/fees
- 5: https://github.com/solana-foundation/developer-content/blob/main/content/guides/advanced/how-to-request-optimal-compute.md
- 6: https://github.com/solana-foundation/developer-content/blob/main/docs/core/fees.md
Avoid using the 400k fallback as the priority-fee budget for every transaction.
Lines 37-46 apply DEFAULT_COMPUTE_UNIT_LIMIT whenever computeUnitLimit is omitted, and then charge priorityFee per requested CU. Solana bases priority fees on the requested CU limit, so lighter transactions can overpay; use a tighter per-transaction estimate or require an explicit limit when priority fees are enabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/umi/sendTransaction.ts` around lines 12 - 14, Avoid using
DEFAULT_COMPUTE_UNIT_LIMIT as the implicit priority-fee budget in
sendTransaction when computeUnitLimit is omitted. Use a tighter
transaction-specific compute-unit estimate, or require an explicit
computeUnitLimit whenever priorityFee is enabled, while preserving the existing
default only for transactions without priority fees.
Source: MCP tools
2 participants
Summary
mplx core plugins addandupdatepreviously assumed the address was an asset unless--collectionwas passed. Passing a collection address without the flag failed withUnable to fetch asset.This change introduces
resolveCoreAccount, which:--collectionas an explicit override (collection only)This mirrors the auto-detect pattern already used by
genesis bucket fetchwhen--typeis omitted (safeFetch*+ fallback).Also prepends a 400k compute unit limit on Genesis create only. Create was flaking on Node 24 CI with
Computational budget exceededat Solana's 200k default. A global CU limit was tried and rejected because it pushed near-full txs (e.g. add presale bucket) over the transaction size cap.Usage
Test plan
--collectionstill pass--collection(auto-detect)