Add candy machine guard management commands#119
Open
MarkSackerberg wants to merge 4 commits intomainfrom
Open
Conversation
Add cm guard update, remove, and delete commands: - cm guard update: update guards from cm-config.json or via interactive wizard - cm guard remove: unwrap candy guard (return mint authority to CM authority) - cm guard delete: permanently delete candy guard account and reclaim rent Includes tests for all commands covering success and error paths.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a guard management subsystem to the Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as "cm guard update"
participant Config as "cm-config.json"
participant Blockchain as "Solana / Metaplex"
participant TxHandler as "Transaction Handler"
User->>CLI: invoke command (--wizard?)
CLI->>Config: read or write guard config
Config-->>CLI: return/write config
CLI->>Blockchain: fetch candy machine account
Blockchain-->>CLI: machine state
CLI->>Blockchain: fetch candy guard account
Blockchain-->>CLI: guard state
CLI->>TxHandler: build updateCandyGuard tx
TxHandler->>Blockchain: submit tx
Blockchain-->>TxHandler: confirm
TxHandler-->>CLI: success
CLI->>User: log completion & summary
sequenceDiagram
participant User
participant CLI as "cm guard remove"
participant Config as "cm-config.json"
participant Blockchain as "Solana / Metaplex"
participant TxHandler as "Transaction Handler"
User->>CLI: invoke command (--address or config)
CLI->>Config: resolve candy machine address (optional)
Config-->>CLI: address
CLI->>Blockchain: fetch candy machine
Blockchain-->>CLI: machine + mintAuthority
CLI->>Blockchain: fetch candy guard (derived)
Blockchain-->>CLI: guard verified
alt user confirmation required
CLI->>User: prompt (type yes-remove)
User-->>CLI: confirm
end
CLI->>TxHandler: build unwrap tx
TxHandler->>Blockchain: submit tx
Blockchain-->>TxHandler: confirm
TxHandler-->>CLI: success
CLI->>User: log completion (rent returned)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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/cm/guard/delete.ts`:
- Line 45: Validate the address format before calling publicKey(): check
flags.address with isPublicKey(flags.address) (or wrap publicKey(flags.address)
in a try/catch) and throw or log a specific "invalid address format" error when
it fails, instead of letting the later lookup produce "does not exist or is not
a valid candy guard"; update the code paths around candyGuardAddress and any
call sites to publicKey() so they only receive a validated PublicKey instance or
a handled error.
- Around line 50-56: The catch-all error handling around fetchCandyGuard masks
operational errors; update the try/catch in the delete command to inspect the
thrown error from fetchCandyGuard and only treat explicit "account not found"
errors (e.g., AccountNotFoundError or similar error.name/message from
fetchCandyGuard) by calling verifySpinner.fail and this.error with the "does not
exist or is not a valid candy guard" message; for all other errors, call
verifySpinner.fail and rethrow or surface the original error message (e.g.,
this.error(error.message)) so network/RPC/deserialization/base58 errors are
visible. Target symbols: fetchCandyGuard, verifySpinner, candyGuardAddress, and
this.error.
In `@src/commands/cm/guard/remove.ts`:
- Around line 71-87: The authority-only short-circuit is inside the same
try/catch that handles fetch errors, so calling this.error('This candy machine
does not have a candy guard attached...') gets caught and rewrapped; move the
authority-only check (the comparison candyMachine.mintAuthority ===
candyMachine.authority and the fetchSpinner.fail/this.error call) outside the
try/catch that wraps fetchCandyGuard/related fetches: first fetch and assign
candyMachine within a try, then after the try perform the equality check and
short-circuit if true, and only then set candyGuardAddress and call
fetchCandyGuard (in its own try/catch) so the catch that logs "Failed to fetch
candy machine or candy guard" never swallows the authority-only path.
In `@src/commands/cm/guard/update.ts`:
- Around line 275-277: The code converts user input to a number using
Number(numGroupsPrompt) without validation, which can yield NaN, negatives, or
fractions and silently skip the for loop; update the prompt for numGroupsPrompt
to validate input (e.g., use a validation function that enforces positive
integers like /^\d+$/) and after parsing check that numGroups is a finite
integer >= 1 before entering the loop (reject or reprompt on invalid input),
referencing the numGroupsPrompt variable and the numGroups/for (let i = 0; i <
numGroups; i++) loop to locate where to add validation and error handling.
- Around line 278-287: Extract the duplicated "6" into a shared constant (e.g.
MAX_GROUP_LABEL_LENGTH) exported from the cm library module and replace the
hardcoded 6 in the update wizard's prompt/validation (the input call validating
groupName and its message) with that constant; import MAX_GROUP_LABEL_LENGTH
into src/commands/cm/guard/update.ts and use it in the prompt message and
validation function, and also update the cm create wizard to import and use the
same constant so both flows (the input in update's groupName validation and the
create wizard) share the single source of truth.
- Around line 125-142: The current try/catch in update.ts relies on matching the
thrown message substring "authority-only" to short-circuit, which is dead
because the earlier this.error message doesn't contain that substring; instead,
move the check that candyMachine.mintAuthority === candyMachine.authority out of
the try block (or perform it immediately after fetching candyMachine but before
the try/catch that calls fetchCandyGuard), call fetchSpinner.fail and this.error
there to exit early, and only run the candyGuardAddress assignment and await
fetchCandyGuard(umi, publicKey(candyGuardAddress)) inside the try; this removes
the need to special-case error.message in the catch and prevents double-failing
the spinner (references: candyMachine.mintAuthority, candyMachine.authority,
candyGuardAddress, fetchCandyGuard, fetchSpinner, this.error).
- Around line 235-238: The input prompts use validate: () => true which provides
no validation; update the input(...) calls that set globalGuardsPrompt and the
similar group/global prompts to enforce allowed values (e.g., for yes/no prompts
implement validate: v => ['y','n','q'].includes(String(v).toLowerCase()) and
return an error string when invalid) and normalize the answer (toLowerCase())
before branching; for the groups prompt validate numeric input (e.g., v =>
/^\d+$/.test(String(v)) or allow empty) or remove the validate option entirely
if you intentionally accept any value—ensure the code that reads
globalGuardsPrompt, groupGuardsPrompt, etc., uses the validated/normalized value
so arbitrary text no longer silently skips sections.
- Around line 197-206: The local function checkAbort should be hoisted to a
class method or module-level helper so it can call the oclif exit hook instead
of terminating the process; change its signature from checkAbort(val: any) to
checkAbort(val: unknown) and update the logic to handle string and array cases
(string.trim().toLowerCase() === 'q' and Array.isArray(val) &&
val.includes('Quit')), replacing process.exit(0) with this.exit(0) when
implemented as a class method (or accept a Command-like context and call
ctx.exit(0) if made a helper), and keep the abort log but prefer this.log(...)
when available on the Command instance.
- Around line 101-103: The current validation that checks guardConfig and groups
runs after the wizard branch writes the updated config; move that check to run
before the code path that persists the config (the write/save call executed for
the --wizard branch) so that if (!guardConfig || Object.keys(guardConfig).length
=== 0) && (!groups || groups.length === 0) you call this.error(...) and return
before any save occurs; ensure the validation sits above the wizard handling (or
immediately before the saveConfig/writeConfig invocation) to prevent mutating
cm-config.json when aborting.
In `@test/commands/cm/cm.guard.test.ts`:
- Around line 141-148: The test is brittle because it regex-parses fetchStdout
from runCli(["cm","fetch", cmId]) to get mintAuthority; instead call and parse
the output of the command that already returns the guard address (runCli with
the "cm guard remove" invocation), e.g. capture the stdout from
runCli(["cm","guard","remove", cmId, ...]) and JSON.parse it to read the
candyGuardAddress property (or candyMachineAddress) rather than matching
fetchStdout with /"mintAuthority":\s*"([^"]+)"/; update the test to remove
reliance on cm fetch output and use the structured return value (variables:
fetchStdout, candyGuardAddress, runCli, "cm guard remove") accordingly.
- Around line 210-220: The test expects an 'authority-only' error but the
current remove command throws "Failed: This candy machine does not have a candy
guard attached. Nothing to remove."; update the error text emitted by the remove
command implementation in remove.ts (the code path executed by the CLI
subcommand handling "cm guard remove") so the thrown Error message includes the
token 'authority-only' (e.g., prepend or append "authority-only" to the existing
message), or alternatively change the assertion in the test (cm.guard.test.ts)
to match the actual message returned by runCli when calling
["cm","guard","remove","--address",cmId,"--force"]. Ensure you modify the
specific throw/new Error location in remove.ts referenced in the test flow so
runCli observes the updated message.
🪄 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
Run ID: 8856e88a-a344-412a-b852-4839c264957d
📒 Files selected for processing (6)
src/commands/cm/guard/delete.tssrc/commands/cm/guard/index.tssrc/commands/cm/guard/remove.tssrc/commands/cm/guard/update.tssrc/commands/cm/index.tstest/commands/cm/cm.guard.test.ts
- Validate candy guard address format in delete before lookup - Restructure authority-only short-circuit in remove/update so errors are not swallowed and rewrapped by the fetch try/catch - Move wizard config save after validation so aborted runs do not leave a mutated cm-config.json on disk - Replace process.exit with this.exit in the wizard abort helper - Enforce y/n/q and positive-integer validation on wizard prompts - Extract MAX_GROUP_LABEL_LENGTH constant to cm-utils - Parse candy guard address from remove stderr in tests instead of regexing cm fetch JSON output
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/cm/guard/delete.ts (1)
57-62:⚠️ Potential issue | 🟠 MajorHandle deserialization errors for existing non-Candy-Guard accounts.
Line 59 only catches
AccountNotFoundError, butfetchCandyGuardthrows a deserialization error (e.g.,CgDeserializationErrorError) when the account exists but is not a valid Candy Guard (such as the System Program used in tests). This causes the error to fall through to the generic "Failed to fetch candy guard" message instead of the intended "does not exist or is not a valid candy guard" message.The recommended solution is to use
safeFetchCandyGuardinstead, which returns null for both missing and invalid accounts, eliminating the need for error classification:- const candyGuard = await fetchCandyGuard(this.context.umi, candyGuardAddress) + const candyGuard = await safeFetchCandyGuard(this.context.umi, candyGuardAddress) + if (!candyGuard) { + this.error(`The account at ${candyGuardAddress} does not exist or is not a valid candy guard.`) + }Alternatively, if
fetchCandyGuardmust be retained, catch the deserialization error explicitly alongsideAccountNotFoundError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/cm/guard/delete.ts` around lines 57 - 62, The catch block currently only handles AccountNotFoundError and falls back to a generic message when fetchCandyGuard throws deserialization errors (e.g., CgDeserializationErrorError); replace the use of fetchCandyGuard with safeFetchCandyGuard (which returns null for missing/invalid accounts) or, if keeping fetchCandyGuard, update the catch to also detect the deserialization error type (CgDeserializationErrorError) and treat it the same as AccountNotFoundError so verifySpinner.fail('Candy guard not found') and this.error(`The account at ${candyGuardAddress} does not exist or is not a valid candy guard.`) are used for both cases instead of the generic failure message.
🤖 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/cm/cm.guard.test.ts`:
- Around line 167-178: The test uses the System Program address (111...111)
which is an existing account and causes fetchCandyGuard to throw a type-mismatch
instead of the expected "does not exist" path; update the test that calls
runCli(["cm","guard","delete",...]) to supply a truly uninitialized address by
generating a fresh keypair and using its base58 public key (e.g., use
generateSigner(umi) and its publicKey.toBase58()), or if Umi isn't available,
produce a valid random base58 string via the UMI utility for a new keypair; this
ensures the delete.ts code exercises the AccountNotFoundError "does not exist"
branch rather than the deserialization error.
---
Duplicate comments:
In `@src/commands/cm/guard/delete.ts`:
- Around line 57-62: The catch block currently only handles AccountNotFoundError
and falls back to a generic message when fetchCandyGuard throws deserialization
errors (e.g., CgDeserializationErrorError); replace the use of fetchCandyGuard
with safeFetchCandyGuard (which returns null for missing/invalid accounts) or,
if keeping fetchCandyGuard, update the catch to also detect the deserialization
error type (CgDeserializationErrorError) and treat it the same as
AccountNotFoundError so verifySpinner.fail('Candy guard not found') and
this.error(`The account at ${candyGuardAddress} does not exist or is not a valid
candy guard.`) are used for both cases instead of the generic failure message.
🪄 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
Run ID: c3b187d1-6022-44f6-9a2a-2e7eba70846e
📒 Files selected for processing (5)
src/commands/cm/guard/delete.tssrc/commands/cm/guard/remove.tssrc/commands/cm/guard/update.tssrc/lib/cm/cm-utils.tstest/commands/cm/cm.guard.test.ts
The System Program address is an existing account, so fetchCandyGuard raises a deserialization/type-mismatch error rather than AccountNotFoundError. Generate a fresh keypair instead so the test exercises the intended "does not exist" branch.
Oclif wraps long error messages across lines with a ` › ` continuation marker, so "does not exist" in the rendered message ends up split as "does \n › not exist". Strip the continuation markers before the substring assertion.
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.
Add cm guard update, remove, and delete commands:
Includes tests for all commands covering success and error paths.