feat(cli): implement manual claim-ownership for Solana#839
Open
evan-gray wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughA new "claim-ownership" subcommand has been added to the Solana CLI module. The implementation validates the network, loads deployment configuration, derives the new owner from the signer, constructs a claimOwnership instruction, wraps it in a Solana transaction, signs and sends it, then logs the transaction hash with error handling. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as Solana CLI
participant Config as Deployment Config
participant Signer as Signer Handler
participant TxBuilder as Transaction Builder
participant SolanaNet as Solana Network
User->>CLI: invoke claim-ownership
CLI->>CLI: validate network & chain
CLI->>Config: load deployment config
Config-->>CLI: config loaded
CLI->>Signer: derive new owner from signer
Signer-->>CLI: owner derived
CLI->>TxBuilder: construct claimOwnership instruction
TxBuilder-->>CLI: instruction created
CLI->>CLI: wrap in Solana Transaction
CLI->>CLI: sign transaction
CLI->>SolanaNet: send signed transaction
SolanaNet-->>CLI: transaction hash
CLI->>CLI: log transaction hash
CLI-->>User: ownership transfer initiated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
📝 Coding Plan for PR comments
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}
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 `@cli/src/commands/solana.ts`:
- Around line 409-460: The claim-ownership path advertises the shared
"signer-type" option but then calls getSigner() which will hard-fail for Solana
ledger signers; add a preflight check immediately before calling getSigner() in
the claim-ownership handler that inspects signerType (from argv["signer-type"])
and the chain/platform (use chain or chainToPlatform(chain)) and if signerType
=== "ledger" and the platform is "Solana" print a clear error (e.g. "Ledger
signer not supported for Solana claim-ownership") and exit(1); this prevents
invoking getSigner() with an unsupported ledger type while keeping the shared
option shape.
- Around line 415-453: The handler currently reads the deployment via
loadConfig(path) but then uses argv["network"] and argv["chain"] when calling
pullChainConfig, which can pair a manager address from the deployment with the
wrong RPC and also skips validateChain; change the logic to derive the network
from the loaded deployments (use deployments.network or equivalent field) and
pass that into validateChain(...) and pullChainConfig(...) instead of
argv["network"], ensuring the manager address (created from
deployments.chains[chain]) is used with the matching deployment network; update
references around loadConfig, deployments, argv["network"], validateChain, and
pullChainConfig to reflect this single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a188e889-835c-4aa9-93cf-b4ae062381c8
📒 Files selected for processing (1)
cli/src/commands/solana.ts
Comment on lines
+409
to
+460
| .option("signer-type", options.signerType) | ||
| .example( | ||
| "$0 solana claim-ownership --chain Solana --network Mainnet", | ||
| "Claim ownership of the Solana NTT program" | ||
| ), | ||
| async (argv: any) => { | ||
| const path = argv["path"]; | ||
| const deployments: Config = loadConfig(path); | ||
| const chain: Chain = argv["chain"]; | ||
| const network = argv["network"]; | ||
| const signerType = argv["signer-type"] as SignerType; | ||
|
|
||
| if (!isNetwork(network)) { | ||
| console.error("Invalid network"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (chainToPlatform(chain) !== "Solana") { | ||
| console.error( | ||
| `claim-ownership is only supported on Solana chains, got ${chain}` | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const chainConfig = deployments.chains[chain]; | ||
| if (!chainConfig) { | ||
| console.error( | ||
| `Chain ${chain} not found in deployment configuration` | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| console.log(colors.blue("🔑 Manual claimOwnership Operation")); | ||
| console.log(`Chain: ${colors.yellow(chain)}`); | ||
|
|
||
| try { | ||
| const manager = { | ||
| chain, | ||
| address: toUniversal(chain, chainConfig.manager), | ||
| }; | ||
| const [, ctx, ntt] = await pullChainConfig( | ||
| network, | ||
| manager, | ||
| overrides | ||
| ); | ||
|
|
||
| const solanaNtt = ntt as SolanaNtt< | ||
| typeof ctx.config.network, | ||
| SolanaChains | ||
| >; | ||
|
|
||
| const signer = await getSigner(ctx, signerType); |
There was a problem hiding this comment.
Don't advertise ledger here if Solana immediately rejects it.
claim-ownership is Solana-only, but it reuses the generic signer option and then calls getSigner(), which hard-fails for Solana ledger signers. One of the exposed CLI paths is therefore unusable.
Suggested fix
- .option("signer-type", options.signerType)
+ .option("signer-type", {
+ ...options.signerType,
+ choices: ["privateKey"] as const,
+ default: "privateKey",
+ })If you want to keep the shared option shape, add an explicit preflight check before getSigner() instead.
📝 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
| .option("signer-type", options.signerType) | |
| .example( | |
| "$0 solana claim-ownership --chain Solana --network Mainnet", | |
| "Claim ownership of the Solana NTT program" | |
| ), | |
| async (argv: any) => { | |
| const path = argv["path"]; | |
| const deployments: Config = loadConfig(path); | |
| const chain: Chain = argv["chain"]; | |
| const network = argv["network"]; | |
| const signerType = argv["signer-type"] as SignerType; | |
| if (!isNetwork(network)) { | |
| console.error("Invalid network"); | |
| process.exit(1); | |
| } | |
| if (chainToPlatform(chain) !== "Solana") { | |
| console.error( | |
| `claim-ownership is only supported on Solana chains, got ${chain}` | |
| ); | |
| process.exit(1); | |
| } | |
| const chainConfig = deployments.chains[chain]; | |
| if (!chainConfig) { | |
| console.error( | |
| `Chain ${chain} not found in deployment configuration` | |
| ); | |
| process.exit(1); | |
| } | |
| console.log(colors.blue("🔑 Manual claimOwnership Operation")); | |
| console.log(`Chain: ${colors.yellow(chain)}`); | |
| try { | |
| const manager = { | |
| chain, | |
| address: toUniversal(chain, chainConfig.manager), | |
| }; | |
| const [, ctx, ntt] = await pullChainConfig( | |
| network, | |
| manager, | |
| overrides | |
| ); | |
| const solanaNtt = ntt as SolanaNtt< | |
| typeof ctx.config.network, | |
| SolanaChains | |
| >; | |
| const signer = await getSigner(ctx, signerType); | |
| .option("signer-type", { | |
| ...options.signerType, | |
| choices: ["privateKey"] as const, | |
| default: "privateKey", | |
| }) | |
| .example( | |
| "$0 solana claim-ownership --chain Solana --network Mainnet", | |
| "Claim ownership of the Solana NTT program" | |
| ), | |
| async (argv: any) => { | |
| const path = argv["path"]; | |
| const deployments: Config = loadConfig(path); | |
| const chain: Chain = argv["chain"]; | |
| const network = argv["network"]; | |
| const signerType = argv["signer-type"] as SignerType; | |
| if (!isNetwork(network)) { | |
| console.error("Invalid network"); | |
| process.exit(1); | |
| } | |
| if (chainToPlatform(chain) !== "Solana") { | |
| console.error( | |
| `claim-ownership is only supported on Solana chains, got ${chain}` | |
| ); | |
| process.exit(1); | |
| } | |
| const chainConfig = deployments.chains[chain]; | |
| if (!chainConfig) { | |
| console.error( | |
| `Chain ${chain} not found in deployment configuration` | |
| ); | |
| process.exit(1); | |
| } | |
| console.log(colors.blue("🔑 Manual claimOwnership Operation")); | |
| console.log(`Chain: ${colors.yellow(chain)}`); | |
| try { | |
| const manager = { | |
| chain, | |
| address: toUniversal(chain, chainConfig.manager), | |
| }; | |
| const [, ctx, ntt] = await pullChainConfig( | |
| network, | |
| manager, | |
| overrides | |
| ); | |
| const solanaNtt = ntt as SolanaNtt< | |
| typeof ctx.config.network, | |
| SolanaChains | |
| >; | |
| const signer = await getSigner(ctx, signerType); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/solana.ts` around lines 409 - 460, The claim-ownership path
advertises the shared "signer-type" option but then calls getSigner() which will
hard-fail for Solana ledger signers; add a preflight check immediately before
calling getSigner() in the claim-ownership handler that inspects signerType
(from argv["signer-type"]) and the chain/platform (use chain or
chainToPlatform(chain)) and if signerType === "ledger" and the platform is
"Solana" print a clear error (e.g. "Ledger signer not supported for Solana
claim-ownership") and exit(1); this prevents invoking getSigner() with an
unsupported ledger type while keeping the shared option shape.
Comment on lines
+415
to
+453
| const path = argv["path"]; | ||
| const deployments: Config = loadConfig(path); | ||
| const chain: Chain = argv["chain"]; | ||
| const network = argv["network"]; | ||
| const signerType = argv["signer-type"] as SignerType; | ||
|
|
||
| if (!isNetwork(network)) { | ||
| console.error("Invalid network"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (chainToPlatform(chain) !== "Solana") { | ||
| console.error( | ||
| `claim-ownership is only supported on Solana chains, got ${chain}` | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const chainConfig = deployments.chains[chain]; | ||
| if (!chainConfig) { | ||
| console.error( | ||
| `Chain ${chain} not found in deployment configuration` | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| console.log(colors.blue("🔑 Manual claimOwnership Operation")); | ||
| console.log(`Chain: ${colors.yellow(chain)}`); | ||
|
|
||
| try { | ||
| const manager = { | ||
| chain, | ||
| address: toUniversal(chain, chainConfig.manager), | ||
| }; | ||
| const [, ctx, ntt] = await pullChainConfig( | ||
| network, | ||
| manager, | ||
| overrides | ||
| ); |
There was a problem hiding this comment.
Use the deployment file's network as the source of truth.
This handler loads deployments, but then executes against argv["network"]. If those differ, you can pair the manager address from one environment with RPC/config from another and attempt the ownership claim on the wrong cluster. Also, this path skips validateChain(network, chain).
Suggested fix
const path = argv["path"];
const deployments: Config = loadConfig(path);
const chain: Chain = argv["chain"];
- const network = argv["network"];
+ const requestedNetwork = argv["network"];
+ const deploymentNetwork = deployments.network as Network;
const signerType = argv["signer-type"] as SignerType;
+ const network = requestedNetwork ?? deploymentNetwork;
if (!isNetwork(network)) {
console.error("Invalid network");
process.exit(1);
}
+
+ if (
+ requestedNetwork !== undefined &&
+ requestedNetwork !== deploymentNetwork
+ ) {
+ console.error(
+ `Deployment config is for ${deploymentNetwork}, but --network was ${requestedNetwork}`
+ );
+ process.exit(1);
+ }
if (chainToPlatform(chain) !== "Solana") {
console.error(
`claim-ownership is only supported on Solana chains, got ${chain}`
);
process.exit(1);
}
+
+ validateChain(network, chain);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/commands/solana.ts` around lines 415 - 453, The handler currently
reads the deployment via loadConfig(path) but then uses argv["network"] and
argv["chain"] when calling pullChainConfig, which can pair a manager address
from the deployment with the wrong RPC and also skips validateChain; change the
logic to derive the network from the loaded deployments (use deployments.network
or equivalent field) and pass that into validateChain(...) and
pullChainConfig(...) instead of argv["network"], ensuring the manager address
(created from deployments.chains[chain]) is used with the matching deployment
network; update references around loadConfig, deployments, argv["network"],
validateChain, and pullChainConfig to reflect this single source of truth.
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.
Summary by CodeRabbit